Opened 7 months ago

Closed 6 months ago

#30365 closed task (implemented)

prop289: Update tor-spec.txt with authenticated SENDME spec

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.4.1.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: tor-spec, prop289, sendme, 041-should
Cc: Actual Points: 0.2
Parent ID: #26288 Points: 0.2
Reviewer: arma Sponsor: SponsorV

Description

Basically either merge proposal 289 into tor-spec.txt or create sendme-spec.txt which should then be referenced within tor-spec.txt.

Child Tickets

Change History (13)

comment:1 Changed 6 months ago by nickm

Keywords: 041-should added

comment:2 Changed 6 months ago by dgoulet

Actual Points: 0.2
Sponsor: SponsorV
Status: assignedneeds_review
Type: defecttask

Branch: ticket30365_01.

This merges the details into tor-spec.txt and dir-spec.txt. And closes the proposal.

comment:3 Changed 6 months ago by dgoulet

Reviewer: nickm

comment:4 Changed 6 months ago by nickm

Status: needs_reviewneeds_revision

This mostly lgtm but I would like Roger to have a look at it before we merge, since he's likeliest to remember anything that it gets wrong about the flow control algorithm.

There are a few points where I would like clarification:

1.

+   Note that these limits do NOT apply to cells that tor receives from one
+   host and relays to another. Circuit-level flow control is end-to-end so
+   both ends track these windows, never the middle points.

This text above is a little misleading. Because of our leaky pipe topology, every relay on the circuit has a pair of windows, and the OP has a pair of windows for every relay on the circuit. These windows do not apply to relayed cells, however, and a relay that is never used for streams will never decrement its window or cause the client to decrement a window.

2.

+   An OR or OP (depending of the stream direction), is willing to receive more
+   cells when its deliver window goes down below a full increment (100). For
+   example, if the window started at 1000, it should send a RELAY_SENDME when
+   it reaches 900.

Instead of saying "is willing" I'd suggest saying "sends a RELAY_SENDME cell to indicate that it is willing".

3.

+         The DIGEST is the rolling digest value from the cell that immediately
+         preceded this RELAY_SENDME. This value is matched on the other side
+         from the previous cell sent that the OR/OP must remember.

When you say "the cell that immediately preceded", let's clarify what kind of cell. I think you mean "the RELAY cell on the same circuit from the same sender that immediately preceded", but maybe you mean only RELAY_DATA?

4.
Does anything in this text say that if you get a RELAY_DATA cell when your deliver window is 0, you should kill the circuit? If not, it should.

5.
Also, a suggestion:

-Author: Rob Jansen, Roger Dingledine
+Author: Roger Dingledine, David Goulet, Rob Jansen

Sometimes people in academia try to send important social signals through author ordering. Please check with Rob and Roger before re-ordering the authors here.

comment:5 in reply to:  4 ; Changed 6 months ago by dgoulet

Status: needs_revisionneeds_review

Replying to nickm:

This mostly lgtm but I would like Roger to have a look at it before we merge, since he's likeliest to remember anything that it gets wrong about the flow control algorithm.

There are a few points where I would like clarification:

1.

+   Note that these limits do NOT apply to cells that tor receives from one
+   host and relays to another. Circuit-level flow control is end-to-end so
+   both ends track these windows, never the middle points.

This text above is a little misleading. Because of our leaky pipe topology, every relay on the circuit has a pair of windows, and the OP has a pair of windows for every relay on the circuit. These windows do not apply to relayed cells, however, and a relay that is never used for streams will never decrement its window or cause the client to decrement a window.

Good catch! I've actually re-used this paragraph your wrote which I think explains it great!

2.

+   An OR or OP (depending of the stream direction), is willing to receive more
+   cells when its deliver window goes down below a full increment (100). For
+   example, if the window started at 1000, it should send a RELAY_SENDME when
+   it reaches 900.

Instead of saying "is willing" I'd suggest saying "sends a RELAY_SENDME cell to indicate that it is willing".

Fixed.

3.

+         The DIGEST is the rolling digest value from the cell that immediately
+         preceded this RELAY_SENDME. This value is matched on the other side
+         from the previous cell sent that the OR/OP must remember.

When you say "the cell that immediately preceded", let's clarify what kind of cell. I think you mean "the RELAY cell on the same circuit from the same sender that immediately preceded", but maybe you mean only RELAY_DATA?

Yes, it is RELAY_DATA cell. I've clarified.

4.
Does anything in this text say that if you get a RELAY_DATA cell when your deliver window is 0, you should kill the circuit? If not, it should.

Added!

5.
Also, a suggestion:

-Author: Rob Jansen, Roger Dingledine
+Author: Roger Dingledine, David Goulet, Rob Jansen

Sometimes people in academia try to send important social signals through author ordering. Please check with Rob and Roger before re-ordering the authors here.

Wow! that is true lol, ordering matters for them... Ok I went in alphabetical order, I'll just put my name at the end, will save us all the troubles :). For me, this is mostly useful to know "who to contact" if any questions ;).

See the fixup commit b8b6bb938f3238e5.

comment:6 Changed 6 months ago by nickm

Reviewer: nickmarma

LGTM now. Roger, can you have a look before this merges, if there's time?

comment:7 in reply to:  5 Changed 6 months ago by arma

Replying to dgoulet:

-Author: Rob Jansen, Roger Dingledine
+Author: Roger Dingledine, David Goulet, Rob Jansen

Sometimes people in academia try to send important social signals through author ordering. Please check with Rob and Roger before re-ordering the authors here.

Wow! that is true lol, ordering matters for them... Ok I went in alphabetical order, I'll just put my name at the end, will save us all the troubles :). For me, this is mostly useful to know "who to contact" if any questions ;).

This fixup still leaves me first and Rob second. I think I put Rob first because he thought of the idea (and wrote about it in his sniper attack paper). Let's leave him first rather than revise history right when we close the proposal. :)

comment:8 Changed 6 months ago by arma

+        "sendme_emit_min_version" -- Minimum SENDME version that can be sent.
+
+        "sendme_accept_min_version" -- Minimum SENDME version that is accepted.

the other entries say a minimum and maximum and default. that would be useful here too. especially since we will eventually change the default, and then we'll want to annotate here what versions had which default.

comment:9 Changed 6 months ago by arma

s/depending of the stream direction/depending on the stream direction/

comment:10 Changed 6 months ago by arma

s/teared down/torn down/g (three instances)

might also want s/precedes/precedes (triggers)/ and s/preceded/preceded (triggered)/

s/have its StreamID=0/has its StreamID=0/

I'm confused that the digest of a version 1 sendme cell has 20 bytes, but then there's some mention of 4 also? I think the 4 is because that's what how many bytes we actually put in a relay header? What if one side remembers 4 bytes, but then gets a v1 sendme with a DATA_LEN of 20? Should it compare just the first 4 bytes Or the last 4 bytes? As currently written, if the DATA_LEN in a v1 sendme is 6, then we're supposed to read 20 bytes, which is unintuitively more than the 6 that it said it included?

Should we include in the spec some mention of leaving some bytes unused in some relay_data cells, to ensure there is unpredictable data going through?

And lastly, what did we decide on how directory mirrors should handle serving consensus documents once sendme_accept_min_version moves to 1? Did we make some exception for that situation, so clients who only emit 0 can still get the consensus to learn that their supported protovers are too old? Might be good to put that in the spec if we decided to build it.

Thanks!

Version 1, edited 6 months ago by arma (previous) (next) (diff)

comment:11 Changed 6 months ago by dgoulet

This fixup still leaves me first and Rob second. I think I put Rob first because he thought of the idea (and wrote about it in his sniper attack paper). Let's leave him first rather than revise history right when we close the proposal. :)

Apologies! I've put back everyone in the right order. :S

the other entries say a minimum and maximum and default. that would be useful here too. especially since we will eventually change the default, and then we'll want to annotate here what versions had which default.

Good catch! I've also added the "First appeared:" ...

I'm confused that the digest of a version 1 sendme cell has 20 bytes, but then there's some mention of 4 also? I think the 4 is because that's what how many bytes we actually put in a relay header? What if one side remembers 4 bytes, but then gets a v1 sendme with a DATA_LEN of 20? Should it compare just the first 4 bytes? Or the last 4 bytes? As currently written, if the DATA_LEN in a v1 sendme is 6, then we're supposed to read 20 bytes, which is unintuitively more than the 6 that it said it included?

The original proposal said 4 bytes because that is the size of the rolling digest in a cell but at the OR/OP, we have access to the full digest so the full 20 bytes was used in the end.

I've corrected the paragraph that talks about 4 bytes. Now it is all about 20 bytes.

Should we include in the spec some mention of leaving some bytes unused in some relay_data cells, to ensure there is unpredictable data going through?

Yes! Added a sentence in the circuit-level section.

And lastly, what did we decide on how directory mirrors should handle serving consensus documents once sendme_accept_min_version moves to 1? Did we make some exception for that situation, so clients who only emit 0 can still get the consensus to learn that their supported protovers are too old? Might be good to put that in the spec if we decided to build it.

We did not go that way. The deployment plan in the prop289 should be enough and that is:

  1. Emit v1
  2. Protover goes to FlowCtrl=1
  3. Accept v1.

At step (2), old clients will be phased out. I created this *GREAT* wiki page ;) in order for us to remember that we need to do that for a tor release:

https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/ReleaseParameters

Created 3 fixup commits to fix all the above (3 because changes span over 3 files/commits :).

Thanks!

comment:12 Changed 6 months ago by arma

looks good to me!

comment:13 Changed 6 months ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Squashed and merged!

Note: See TracTickets for help on using tickets.