#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 13 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 13 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 13 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 13 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 13 months ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:6 Changed 13 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 13 months ago by nickm

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

comment:9 Changed 13 months ago by asn

Reviewer: mikeperry

comment:10 Changed 13 months ago by rl1987

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

comment:11 Changed 13 months ago by mikeperry

Status: needs_reviewmerge_ready

lgtm!

comment:12 Changed 13 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

merged!

Note: See TracTickets for help on using tickets.