Opened 2 weeks ago

Closed 7 days 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:


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...

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 2 weeks 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) {
 169       return 1;
 171       return 1;
 173       return 0;
 175       return 0; // XXX: bug?
 177       return 1;
 179       return 1;
 180   }
 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);
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 2 weeks 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 2 weeks ago by atagar

Stem change pushed to treat 552s as 'not a relay':

comment:4 Changed 2 weeks 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...

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 2 weeks ago by rl1987

Owner: set to rl1987
Status: newaccepted

comment:6 Changed 2 weeks 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 days ago by nickm

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

comment:9 Changed 12 days ago by asn

Reviewer: mikeperry

comment:10 Changed 10 days ago by rl1987

Aforementioned tweaks to tor code:

comment:11 Changed 8 days ago by mikeperry

Status: needs_reviewmerge_ready


comment:12 Changed 7 days ago by nickm

Resolution: implemented
Status: merge_readyclosed


Note: See TracTickets for help on using tickets.