Opened 5 months ago

Closed 4 months ago

#25852 closed defect (fixed)

GETINFO exit-policy for tor client should return 551

Reported by: dmr Owned by:
Priority: Medium Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, tor-client, fast-fix
Cc: Actual Points:
Parent ID: Points:
Reviewer: dgoulet Sponsor:

Description

In #25423 it was noticed that...

[...] the client-only configuration ALWAYS returns this:

>>> GETINFO exit-policy/full
551 router_get_my_routerinfo returned NULL

The control spec didn't specify behavior for this case. For GETINFO it states:

The server sends a 551 or 552 error on failure.

And more generically describes 551 and 552 as:

     551 Internal error
               [Something went wrong inside Tor, so that the client's
                request couldn't be fulfilled.]

     552 Unrecognized entity
               [A configuration key, a stream ID, circuit ID, event,
                mentioned in the command did not actually exist.]

Both 551 and 552 are a bit odd for this behavior, but the spec doesn't have a code that indicates "You asked for everything ok, but I'm not operating in that mode right now" (i.e. N/A).

However, the spec does indicate the following for GETINFO fingerprint:

    "fingerprint" -- the contents of the fingerprint file that Tor
      writes as a relay, or a 551 if we're not a relay currently.
      (Added in 0.1.2.3-alpha)

In discussion with dgoulet and atagar over IRC, we agreed it made the most sense to follow this precedent.

dgoulet reports that GETINFO fingerprint currently returns:

551 Not running in server mode

The spec notes "Unless specified to have specific contents, the human-readable messages in error replies should not be relied upon to match those in this document.", so this info is mentioned to facilitate consistency.

Child Tickets

Change History (15)

comment:1 Changed 5 months ago by dmr

Corresponding ticket for behavior in stem: #25853

comment:3 Changed 5 months ago by nickm

Milestone: Tor: 0.3.4.x-final

comment:4 Changed 5 months ago by dgoulet

Keywords: fast-fix added
Reviewer: dgoulet

comment:5 in reply to:  2 Changed 5 months ago by dgoulet

Status: needs_reviewneeds_revision

Replying to rl1987:

Hmmmm so I'm wondering. So router_get_my_routerinfo() does return NULL if we aren't in server_mode() but it also returns NULL if the rebuild failed.

So we could be in "server mode" but just unable to create a descriptor for some reasons... Thus what I propose is that if !server_mode() then we should be sending back "Not running in server mode" but if we are but router_get_my_routerinfo() returns NULL, we should send back "Internal error" (which is what getinfo by default will return if the errmsg value is still NULL).

lgtm;

comment:6 Changed 5 months ago by atagar

Hi David, from a user perspective could we enumerate all the ways this can fail? Presently I'm aware of two...

  1. We're not a relay (no ORPort configured).
  2. We haven't yet been able to resolve our externally facing address.

If we could have distinct error codes and descriptive messages for each way this GETINFO command can fail that would be ideal.

comment:7 in reply to:  6 Changed 5 months ago by dgoulet

Replying to atagar:

Hi David, from a user perspective could we enumerate all the ways this can fail? Presently I'm aware of two...

  1. We're not a relay (no ORPort configured).
  2. We haven't yet been able to resolve our externally facing address.

Any error path in router_rebuild_descriptor() basically. I see the resolve issue you pointed out, then unable to digest a key (unlikely), then we created a descriptor that we weren't able to parse. But more could arise in the future with code change.

So what we need is probably a way to know what was the error if it ever failed. I see two ways of doing it, maybe one is probably better than the other,

  1. Make router_get_my_routerinfo() return an error code with a NULL descriptor. And then with that error code, we could get a human readable string.
  1. Have a global variable that keeps the error message explaining what happened if something happened. And then the control port asks for this, it could look at that value if a NULL value is returned to learn what happened (considering we are in server_mode()).
  1. <insert better idea> :)

If we could have distinct error codes and descriptive messages for each way this GETINFO command can fail that would be ideal.

Agree. Better informing the user is always better. But it gets a bit more complicated if you try to enumerate all possible internal errors that tor can have. So I think, for some errors, "Internal error" is fine and then the tor logs should have the deets :).

comment:8 Changed 5 months ago by atagar

Gotcha, thanks David. I'll leave this in your capable hands but for what it's worth the reason I bring this up is largely about cacheability. When 'GETINFO exit-policy/full' returns something I can safely cache it until...

  1. A SETCONF changes our ExitPolicy.
  2. Our externally facing address changes.

Tor provides events for those so once I get my hands on a policy I can skip future GETINFO calls (Nyx does a lot of this to lessen its load on tor).

In the case of error responses I'm curious about cacheability too. In particular if it's transient (ie. I should retry) or not. Both the error cases above are less 'error' than 'no exit policy until X' so I can cache that. So if we can differentiate them from other (possibly transient) errors that would be helpful.

comment:10 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

comment:12 Changed 5 months ago by dgoulet

Status: needs_reviewneeds_revision

comment:13 Changed 5 months ago by rl1987

Status: needs_revisionneeds_review

comment:14 Changed 4 months ago by dgoulet

Status: needs_reviewmerge_ready

Ok I think this lgtm; There could be some syntax/naming changes that nickm might want but overall this is good imo.

We can tweak things after if needed as well.

comment:15 Changed 4 months ago by nickm

Resolution: fixed
Status: merge_readyclosed

Okay, merged! The only change I wanted here was a9ef335c1b220ad632aad1d75f671a2fcd2b0863 -- we shouldn't have two functions that do the same thing, unless one is implemented by calling the other.

Note: See TracTickets for help on using tickets.