Opened 6 years ago

Closed 6 years ago

#9715 closed enhancement (fixed)

update tor spec for hidden service descriptor asynchronous events

Reported by: dave2008 Owned by:
Priority: Medium Milestone:
Component: Core Tor/Tor Version:
Severity: Keywords: tor-spec
Cc: zackw@… Actual Points:
Parent ID: #8510 Points:
Reviewer: Sponsor:

Child Tickets

Change History (11)

comment:1 Changed 6 years ago by dave2008

Parent ID: #8510

comment:2 Changed 6 years ago by zwol

Cc: zackw@… added

comment:3 Changed 6 years ago by asn

Torspec review:

  • You forgot to add your new event names in the 3.4. SETEVENTS section.
  • Talking about event names, are you sure you want to name your events REQUESTED and RECEIVED? Maybe we should add a namespace to them and call them HS_DESC_REQUESTED and HS_DESC_RECEIVED or something like that.
  • What happens when a request fails? Do we have an event for that, or how is it indicated?

comment:4 Changed 6 years ago by asn

[cont]

  • Also, why is DescriptorID optional? When does it appear and when not? Looking at the code, it only appears on the REQUESTED event. Why?

Thanks for the patch!

comment:5 Changed 6 years ago by dave2008

LOL, I made a big typo in my commit, it should be

"650" "HS_DESC" SP Action SP HSAddress SP AuthType SP HsDir [SP DescriptorID]

I will fix that :)

I will add the failed event as well.

The reason I made DescriptorID optional is when the client received the descriptor, the descriptorID is already lost, and I have to recompute it. This computation result is only used in this async event and seems like an extra work that has not benefit the tor connection at all. So I choose to ignore it, do you think it makes sense?

comment:6 Changed 6 years ago by nickm

Merged; thanks!

comment:7 Changed 6 years ago by dave2008

Resolution: fixed
Status: newclosed

comment:8 Changed 6 years ago by asn

Resolution: fixed
Status: closedreopened

Hey Dave,

I'm looked at your unit tests and I noticed:

650 HS_DESC REQUESTED ajhb7kljbiru65qo 0 $AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=TestDir at 1.2.3.4 b3oeducbhjmbqmgw2i3jtz4fekkrinwj

It seems that Tor long names (as produced by format_node_description()) allow spaces in them, hence the $AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=TestDir at 1.2.3.4.

Doesn't this mess up the parsing? Since now the controller thinks that DescriptorID is actually the string at instead of b3oeducbhjmbqmgw2i3jtz4fekkrinwj.

comment:9 Changed 6 years ago by atagar

It seems that Tor long names (as produced by format_node_description()) allow spaces in them

Tor might, but the spec doesn't. From the controlspec...

LongName   = Fingerprint [ ( "=" / "~" ) Nickname ]

Fingerprint = "$" 40*HEXDIG
NicknameChar = "a"-"z" / "A"-"Z" / "0" - "9"
Nickname = 1*19 NicknameChar

Indeed, things would be pretty tricky to parse if spaces were allowed.

comment:10 Changed 6 years ago by dave2008

You are right, I used the wrong function here. I should probably use node_get_verbose_nickname instead. I am updating it now. Sorry for the confusion.

comment:11 Changed 6 years ago by nickm

Resolution: fixed
Status: reopenedclosed

Merging the #8510 fix under the impression that it fixes this ticket too. Let me know (and reopen this ticket) if it doesn't.

Note: See TracTickets for help on using tickets.