Opened 5 months ago

Closed 4 months ago

Last modified 6 weeks ago

#21329 closed defect (fixed)

onions/detached and onions/current should return empty lists instead of errors

Reported by: meejah Owned by:
Priority: Medium Milestone: Tor: 0.3.1.x-final
Component: Core Tor/Stem Version: Tor: 0.2.7.5
Severity: Normal Keywords: controller
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

If you do a GETINFO query on onions/detached or onions/current and there aren't any, a 551 error is returned. Most other things just return an empty list/value in this case and I think these should too.

One consequence of this is that if you ask for onions/current with some other keys and there are no current onions, the whole query will fail.

So GETINFO onions/current should return onions/current= instead of an error.

For example you can try carml cmd getinfo orconn-status onions/current to see this behavior.

Child Tickets

Change History (18)

comment:1 Changed 5 months ago by nickm

  • Milestone set to Tor: unspecified

Makes sense to me; a patch would be welcome.

comment:3 Changed 5 months ago by dgoulet

  • Milestone changed from Tor: unspecified to Tor: 0.3.1.x-final
  • Status changed from new to needs_review

comment:4 Changed 5 months ago by nickm

  • Status changed from needs_review to merge_ready

Looks correct to me!

comment:5 Changed 4 months ago by nickm

  • Keywords review-group-16 added

comment:6 Changed 4 months ago by nickm

  • Keywords review-group-16 removed
  • Resolution set to implemented
  • Status changed from merge_ready to closed

merged!

comment:7 Changed 4 months ago by atagar

  • Resolution implemented deleted
  • Status changed from closed to reopened

Gonna reopen. Recent push for this broke Stem's integ tests...

======================================================================
FAIL: test_without_ephemeral_hidden_services
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/runner.py", line 126, in wrapped
    return func(self, *args, **kwargs)
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/runner.py", line 143, in wrapped
    return func(self, *args, **kwargs)
  File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/integ/control/controller.py", line 569, in test_without_ephemeral_hidden_services
    self.assertEqual([], controller.list_ephemeral_hidden_services())
AssertionError: Lists differ: [] != ['']

Second list contains 1 additional elements.
First extra element 0:


- []
+ ['']

comment:8 Changed 4 months ago by meejah

Looking quickly at Stem's code, I think this is an integration test bug.

That is, I think list_ephemeral_hidden_services should check if it got an empty answer from Tor (similar to other GETINFO empty replies).

That said, I also don't see how that test ever passed since it appears it would re-raise the 552/protocol-error exception that 'getinfo onions/current' does without the patch in this ticket.

comment:9 Changed 4 months ago by atagar

Thanks, I'll dig a little more on my end to figure out exactly what about tor's wire response changed and which side the bug lays on. Might not be able to get to this until after the weekend (visiting family, and can no longer build new tor versions on my netbook since it's pretty out of date).

comment:10 Changed 4 months ago by nickm

  • Component changed from Core Tor to Core Tor/Tor

comment:11 Changed 4 months ago by atagar

Ok, took a quick peek. Change is the following...

Before:

>>> GETINFO onions/current
551 No onion services of the specified type.
After:

>>> GETINFO onions/current
250-onions/current=
250 OK

Both responses are valid according to the spec and it doesn't specify what's done when no hidden services exist. Stem specifically looks for the old 551 response, hence the discrepancy.

https://gitweb.torproject.org/stem.git/tree/stem/control.py#n2732

I wouldn't mind changing stem to recognize the new behavior but this tor change may break folks who presently call list_ephemeral_hidden_services(). So it's a tradeoff - is this behavior change worth that breakage.

Up to you, Nick.

comment:12 Changed 4 months ago by teor

Maybe the stem list processing could be tweaked?

(That is, why is there a semantic difference between an empty list, and a list containing an empty string? Neither is a valid ephemeral onion service.)

comment:13 Changed 4 months ago by atagar

Hi teor. I'm more than happy to change Stem to recognize the new behavior. The point I was raising is that existing Stem releases will have an issue.

Honestly this is a pretty minor bug. It's an edge case for ephemeral hidden services so I don't see this biting many people. I'd be happy to go either way (change tor and risk a minor breakage for stem users, or keep tor as-is).

Nick: Any preference?

Last edited 4 months ago by atagar (previous) (diff)

comment:14 Changed 4 months ago by meejah

atagar: this breaks multi-key GETINFO queries, though; all other "empty things" in the control protocol return an empty-list like that.

So, if you do GETINFO info/names onions/current then you'll get a 551 and the whole thing fails (instead of, for all other cases, getting two answers)...

comment:15 Changed 4 months ago by atagar

  • Component changed from Core Tor/Tor to Core Tor/Stem

Gotcha. Personally I find that a compelling reason to go with this change. As mentioned before it's very much an edge case for Stem users and fixing this sounds worth it to me.

Reassigning this to myself to tweak Stem to handle the new responses. Thanks meejah!

comment:16 Changed 4 months ago by atagar

  • Resolution set to fixed
  • Status changed from reopened to closed

comment:17 Changed 6 weeks ago by meejah

  • Version changed from Tor: 0.2.9.9 to Tor: 0.2.7.5

comment:18 Changed 6 weeks ago by meejah

I believe this bug first existed in 0.2.7.5 and commit 915c7438a77edfaf3103b69cb494a4f069a79a0c

Note: See TracTickets for help on using tickets.