Opened 3 months ago

Closed 2 months ago

#27034 closed defect (implemented)

Clarification on 'GETINFO exit-policy/*'s valid 'non-transient internal errors'

Reported by: atagar Owned by: rl1987
Priority: Low Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: mikeperry Sponsor:

Description

Hi network team. I was just about to tweak Stem to treat 552 GETINFO response codes as "I'm not configured to be a relay" per...

https://trac.torproject.org/projects/tor/ticket/25853
https://trac.torproject.org/projects/tor/ticket/25852
https://gitweb.torproject.org/torspec.git/commit/?id=c5453a0

However, the spec addition says "Will send 552 error if the server is not running as nion router or if there's non-transient internal error." So I'm unsure what the response code actually indicates. If we *are* a relay that what are the scenarios where tor validly returns a 552?

For now in stem gonna pretend that part doesn't exist and 552s indicate 'not a relay'. :)

Child Tickets

Change History (12)

comment:1 Changed 3 months ago by rl1987

Hello atagar.

Yes currently it is safe to assume that 552 indicate "not an onion router". At this point we have the following in router.c:

 161 /** Return true if we expect given error to be transient.
 162  * Return false otherwise.
 163  */
 164 int
 165 routerinfo_err_is_transient(int err)
 166 {
 167   switch (err) {
 168     case TOR_ROUTERINFO_ERROR_NO_EXT_ADDR:
 169       return 1;
 170     case TOR_ROUTERINFO_ERROR_CANNOT_PARSE:
 171       return 1;
 172     case TOR_ROUTERINFO_ERROR_NOT_A_SERVER:
 173       return 0;
 174     case TOR_ROUTERINFO_ERROR_DIGEST_FAILED:
 175       return 0; // XXX: bug?
 176     case TOR_ROUTERINFO_ERROR_CANNOT_GENERATE:
 177       return 1;
 178     case TOR_ROUTERINFO_ERROR_DESC_REBUILDING:
 179       return 1;
 180   }
 181 
 182   return 0;
 183 }

The only way to get TOR_ROUTERINFO_ERROR_DIGEST_FAILED is to fail computing identity key digest:

2362   ri->identity_pkey = crypto_pk_dup_key(get_server_identity_key());
2363   if (crypto_pk_get_digest(ri->identity_pkey,
2364                            ri->cache_info.identity_digest)<0) {
2365     routerinfo_free(ri);
2366     return TOR_ROUTERINFO_ERROR_DIGEST_FAILED;
2367   }

This really should not happen, and if it does, it's probably a bug.

Let me know if anything else should be done about this.

comment:2 Changed 3 months ago by atagar

Thanks rl1987! In that case could we adjust the spec to drop "or if there's non-transient internal error" from that sentence?

comment:3 Changed 3 months ago by atagar

Stem change pushed to treat 552s as 'not a relay': https://gitweb.torproject.org/stem.git/commit/?id=542fa1f

comment:4 Changed 3 months ago by atagar

Oh wait, just realized GETINFO 552 responses are also used to indicate 'the getinfo parameter you requested does not exist'. I have another ticket where the network team asked for Stem to rely on 552 codes meaning *that* rather than parsing the message text to determine if tor is indicating that or something else...

https://trac.torproject.org/projects/tor/ticket/26112

We can't have it both ways. Tor either needs to provide unique error statuses for different issues or we callers will need to parse exception messages. :/

Gonna resolve the above ticket in favor of this since it's unactionable on our side.

comment:5 Changed 3 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:6 Changed 3 months ago by rl1987

Perhaps we should also tweak tor code to:

  • Print stacktrace when failed to compute digest (use BUG() macro).
  • Only respond with 552 if not a server.

comment:8 Changed 2 months ago by nickm

Milestone: Tor: 0.3.5.x-final
Status: acceptedneeds_review

comment:9 Changed 2 months ago by asn

Reviewer: mikeperry

comment:10 Changed 2 months ago by rl1987

Aforementioned tweaks to tor code: https://github.com/torproject/tor/pull/268

comment:11 Changed 2 months ago by mikeperry

Status: needs_reviewmerge_ready

lgtm!

comment:12 Changed 2 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.