Opened 7 years ago

Closed 7 years ago

#7515 closed defect (fixed)

Better definition for ADDRMAP events

Reported by: atagar Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client spec
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The ADDRMAP specification is making me twitch for a few reasons...

  • It doesn't say what the events do. My guess from seeing a few is that it simply indicates DNS resolution. However, the spec doesn't make this clear and it allows hostnames to be resolved to other hostnames (I'm guessing that is a mistake).
  • The errors are undefined. An 'XXXX' is pretty bad - I hope we can at least say what the error code can include.
  • The spec summary has some mistakes. For instance, it doen't list GMTExpiry as being optional (despite saying so below), and it should make it clear that 'error' and 'GMTExpiry' are key/value entries. My first thought when I saw this event was "WTF? They have an optional positional argument?!? Gah!"

Patch is available in my 'addrmap' branch...
https://gitweb.torproject.org/user/atagar/torspec.git/commitdiff/0168f7d58aaf4d8d840407f1b3420218ee2d3eeb

Child Tickets

Change History (5)

comment:1 Changed 7 years ago by atagar

Status: newneeds_review

comment:2 in reply to:  description Changed 7 years ago by rransom

Replying to atagar:

The ADDRMAP specification is making me twitch for a few reasons...

  • It doesn't say what the events do. My guess from seeing a few is that it simply indicates DNS resolution. However, the spec doesn't make this clear and it allows hostnames to be resolved to other hostnames (I'm guessing that is a mistake).

The TrackHostExits option should cause Tor to emit hostname-to-hostname ADDRMAP events.

  • The errors are undefined. An 'XXXX' is pretty bad - I hope we can at least say what the error code can include.

Unless you really need a list of all possible errors, it's better to underspecify than to write a spec that forbids useful future extensions or that is incompatible with Tor's actual behaviour.

  • The spec summary has some mistakes. For instance, it doen't list GMTExpiry as being optional (despite saying so below), and it should make it clear that 'error' and 'GMTExpiry' are key/value entries. My first thought when I saw this event was "WTF? They have an optional positional argument?!? Gah!"

Duplicate of #6829.

comment:3 Changed 7 years ago by atagar

The TrackHostExits option should cause Tor to emit hostname-to-hostname ADDRMAP events.

Ahh, good to know - thanks.

Unless you really need a list of all possible errors, it's better to underspecify than to write a spec that forbids useful future extensions or that is incompatible with Tor's actual behaviour.

It should at least say the characters that are allowed. '1*(ALNUM / "_")' seems like it should be broad enough - if tor includes something beyond that then I'd definitely like to see that in the spec.

Duplicate of #6829.

Good point. I'm surprised that ticket has lasted so long considering that it's a simple spec fix.

comment:4 Changed 7 years ago by nickm

Keywords: tor-client spec added
Milestone: Tor: 0.2.4.x-final

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

ok, I think I adapted it to be about right. Fixed in 93574d1bcabb5d05ca0dcec4f1f3204c091d2673

Note: See TracTickets for help on using tickets.