Opened 8 months ago

Closed 7 months ago

#21059 closed defect (fixed)

shared-rand-current-value violates spec

Reported by: atagar Owned by: asn
Priority: High Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Major Keywords:
Cc: Actual Points:
Parent ID: Points: 0.1
Reviewer: Sponsor:

Description

Hi Nick. Shared randomness just made it into the consensus and it's making Stem's validator squawk. Trouble is that it's in the wrong position in the header. According to the spec...

The preamble contains the following items.  They MUST occur in the
order given here:

However, shared-rand-current-value appears last...

% wget http://128.31.0.39:9131/tor/status-vote/current/consensus
% less consensus
...
required-client-protocols Cons=1-2 Desc=1-2 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=4 LinkAuth=1 Microdesc=1-2 Relay=2
required-relay-protocols Cons=1 Desc=1 DirCache=1 HSDir=1 HSIntro=3 HSRend=1 Link=3-4 LinkAuth=1 Microdesc=1 Relay=1-2
params CircuitPriorityHalflifeMsec=30000 NumDirectoryGuards=3 NumEntryGuards=1 NumNTorsPerTAP=100 Support022HiddenServices=0 UseNTorHandshake=1 UseOptimisticData=1 bwauthpid=1 cbttestfreq=60 pb_disablepct=0 usecreatefast=0
shared-rand-current-value 6 nwr/w2uC49g/ZxbGpshs0w2nqWWqCsrHWzQhkuelAgA=
dir-source dannenberg 0232AF901C31A04EE9848595AF9BB7620D4C5B2E dannenberg.torauth.de 193.23.244.244 80 443
contact Andreas Lehner
...

Think I'll mark this as 'high' since it's a new regression in the live consensuses. Gonna hazard to guess we'll need to update the spec rather than tor since this has already made it into a release?

Kinda unfortunate since it means it won't live with the other shared randomness attributes...

Child Tickets

Change History (32)

comment:1 Changed 8 months ago by arma

Yeah, I think we loosen the spec to let them appear in whatever order, and then we put them in the order we want (I guess in a future consensus method if it's different from what we do now).

comment:2 Changed 8 months ago by nickm

Milestone: Tor: 0.3.0.x-final

Well, the spec sort of has that looseness, I think. Clients are required to accept the header and footer lines in whatever order they occur, and that's fine. The issue is that authorities must generate them in the same order, or else they won't reach the same consensus document.

IMO, let's just change the spec.

comment:3 Changed 8 months ago by asn

Owner: set to asn
Points: 0.1
Status: newassigned

I can take this.

comment:4 Changed 8 months ago by asn

BTW, what's the right way to fix the spec here? Should we mention in the spec that preamble items can appear in whatever order, but all authorities need to generate them in a consistent way?

comment:5 Changed 8 months ago by nickm

We should say that anybody parsing a consensus or a vote MUST accept them in any order, but that authorities building a consensus MUST produce them in a specific order, and then we specify that specific order.

comment:6 Changed 8 months ago by asn

Status: assignedneeds_review

comment:7 Changed 8 months ago by atagar

We should say that anybody parsing a consensus or a vote MUST accept them in any order, but that authorities building a consensus MUST produce them in a specific order, and then we specify that specific order.

For what it's worth this doesn't change things for Stem. Without validation Stem is quite happy with the consensus - it's just validation squawking because... well, it's wrong.

Without these validation checks we wouldn't know about this issue at all, so I'm inclined to day 'everything working as intended on the stem and spec front'. We can fix this with a tor change that moves the property to the right spot, and await that release.

Concerning DocTor, I can make a temporary tweak so it ignores this particular issue until the fix is live.

comment:8 Changed 8 months ago by atagar

Status: needs_reviewneeds_revision

comment:9 Changed 8 months ago by dgoulet

Wait, why don't we simply move the shared-rand-current-value value in the spec to the right place where the tor code actually puts it? That way we have both code and spec aligned.

comment:10 Changed 8 months ago by atagar

That would certainly be the quickest resolution (and what I originally thought we were gonna go with). Little nicer if the field moved though. There's a bunch of shared-random fields and this should live with them.

Clients besides Stem's validator don't care about this so there's no big rush. We can await a tor fix.

comment:11 Changed 8 months ago by atagar

Stem validation around this is now suspended. DocTor is happy.

comment:12 Changed 8 months ago by atagar

For what it's worth shared-rand-previous-value lines have the same issue.

comment:13 in reply to:  9 Changed 8 months ago by asn

Status: needs_revisionneeds_review

Replying to dgoulet:

Wait, why don't we simply move the shared-rand-current-value value in the spec to the right place where the tor code actually puts it? That way we have both code and spec aligned.

OK, I made a patch for this approach as well, since people seem to like it better.
Please find it at branch bug21059_v2:
https://gitweb.torproject.org/user/asn/torspec.git/commit/?h=bug21059_v2

Let me know if this is better.

comment:14 Changed 8 months ago by nickm

Status: needs_reviewmerge_ready

looks like an improvement to me. Atagar, how does this look to you?

comment:15 Changed 8 months ago by dgoulet

I'm happy with this. I don't see an argument for changing the code instead of the spec for something that simple to fix.

comment:16 Changed 8 months ago by atagar

If tor indeed places all four fields after params then I'd be happy with this. Might be wise to check a vote to ensure it's the case for shared-rand-participate and shared-rand-commit.

comment:17 Changed 8 months ago by atagar

Nope, the proposed spec patch evidently has the wrong position for the shared randomness parameters that appear in votes. We also misorderd the new 'recommended-*-protocols' parameters.

Giving this a shot in my bug21059 branch. How does this look?

Current vote I'm seeing doesn't have any legacy-dir-key lines so I'm unsure where shared randomness is in reference to those. Can someone confirm shared-random comes after legacy-dir-key? Can we finally drop legacy-dir-key from the spec?

Thanks!

comment:18 Changed 8 months ago by atagar

For what it's worth I'll be cutting a stem hotfix release for this so it doesn't have a sad validator. But first I'm waiting for this spec change to be reviewed. Clearly it's error prone enough that it needs a second pair of eyes or two. ;)

comment:19 Changed 8 months ago by atagar

Ah. Turns out Stem doesn't properly parse shared randomness since it's misdocumented as being in the wrong section in the spec.

https://trac.torproject.org/projects/tor/ticket/21102

comment:20 Changed 8 months ago by atagar

Resolution: fixed
Status: merge_readyclosed

Oh for the love of... flag-thresholds is also in the wrong spot. That one was added way back in 2013 but in Stem I coded against tor's actual behavior rather than the spec causing it to go under the radar. Fixed.

Spec ordering with regard to this is clearly error prone enough that I'm gonna change this from a MUST clause to SHOULD. Changes pushed...

https://gitweb.torproject.org/torspec.git/commit/?id=578477a6af99ec1e4a501e267a563730519ffc0f

As such Stem will no longer check the ordering of these fields. Tor will drift further from the spec here once we drop these checks but I don't think this is a detail any of us are gonna lose sleep over. :)

Feel free to reopen if you'd care to go a different course with this.

comment:21 Changed 8 months ago by teor

Resolution: fixed
Status: closedreopened

I think we should specify the actual requirements on consensus generation per consensus method, and then make everything else ignore order.

See my branch bug21059 on https://github.com/teor2345/torspec.git

comment:22 Changed 8 months ago by teor

Status: reopenedneeds_review

comment:23 Changed 8 months ago by atagar

Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.

I see two options...

  • Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
  • Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.

Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.

comment:24 in reply to:  23 ; Changed 8 months ago by teor

Replying to atagar:

Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.

I see two options...

  • Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
  • Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.

Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.

I think we agree here.

I also think my patch does what you want, with the extra constraint that the arbitrary order of consensus lines may vary between consensus methods, but must be consistent for a particular consensus method:

-   The preamble contains the following items.  They SHOULD occur in the
-   order given here:
+   The preamble contains the following items.
+
+   Each consensus method specifies an order in which these items MUST occur.
+   (A change in order requires a new consensus method.)  In a vote, these
+   items SHOULD occur in the same order.
+
+   When parsing the consensus and votes, these items MUST be accepted in any
+   order.

comment:25 Changed 8 months ago by atagar

Hmmmm. The sentence that reads...

"Each consensus method specifies an order in which these items MUST occur"

... strikes me as the same as what we previously had...

"The preamble contains the following items. They MUST occur in the order given here:"

When I read 'each consensus method specifies an order' I think 'ok, so this is the order'. My change (simply changing MUST to SHOULD) drops any strong assurance that what follows is the actual ordering.

That said, I don't care too strongly about this. This patch feels like a lateral move to me. If Nick prefers it I don't have a strong objection to merging it.

comment:26 in reply to:  24 Changed 8 months ago by asn

Replying to teor:

Replying to atagar:

Hi teor, are you sure you want to go this route? The problem in this ticket isn't that clients are unable to accept misordered fields. Stem can still read the consensus just fine. Rather, the trouble is that its descriptor validator was telling us that tor's actual behavior doesn't match the spec.

I see two options...

  • Fix the ordering in the spec, and be careful in the future that we don't keep making this mistake.
  • Loosen the requirement from MUST to SHOULD so it's no big whoop if tor's order matches the spec or not.

Considering that I found three separate instances of these fields being misordered I don't think this is a detail we want to keep contending with.

I think we agree here.

I also think my patch does what you want, with the extra constraint that the arbitrary order of consensus lines may vary between consensus methods, but must be consistent for a particular consensus method:

Hey, why do you think this consensus method consistency rule is important? I feel it makes the text confusing.

Personally, given the inconsistencies that atagar noted, I think the patches in comment:6 and comment:20 look good to me.

comment:27 Changed 8 months ago by atagar

Cut a Stem 1.5.4 hotfix release that drops this validation...

https://gitweb.torproject.org/stem.git/commit/?id=d561974

comment:28 Changed 7 months ago by asn

Resolution: fixed
Status: needs_reviewclosed

Hello, to finally resolve this ticket I merged the branch from comment:6 which specifies that the preamble MUST be accepted in whatever order by clients, but authorities MUST be consistent about it (so that consensus can be reached).

I will close this ticket. Feel free to reopen if you think something is wrong.

Thanks :)

comment:29 Changed 7 months ago by atagar

Resolution: fixed
Status: closedreopened

Hi George, gonna reopen. I argued against your patch a while ago and I still disagree with it. If you have the spec say MUST then this order is something we should validate. See my earlier comments regarding why you probably don't want this headache.

Mind reverting your patch?

comment:30 in reply to:  29 Changed 7 months ago by asn

Replying to atagar:

Hi George, gonna reopen. I argued against your patch a while ago and I still disagree with it. If you have the spec say MUST then this order is something we should validate. See my earlier comments regarding why you probably don't want this headache.

Mind reverting your patch?

Hey atagar, sorry about that. I think I had not understood your concerns.

FWIW I'm also fine with your patch in comment:20. How would you feel if I rebased your comment:20 patch to current torspec master, and pushed that instead?

comment:31 Changed 7 months ago by atagar

Hi George. Actually, my change from comment:20 was already pushed (though by changing it back to MUST this reverted it). How in particular are you inclined toward combining these changes?

comment:24 has the reason I'm leaning toward making this SHOULD. However, if you'd care to do an audit that the spec's order is now accurate and ensure we're really careful not to keep making mistakes on this we can go ahead and make it a MUST clause.

comment:32 in reply to:  31 Changed 7 months ago by asn

Resolution: fixed
Status: reopenedclosed

Replying to atagar:

Hi George. Actually, my change from comment:20 was already pushed (though by changing it back to MUST this reverted it). How in particular are you inclined toward combining these changes?

comment:24 has the reason I'm leaning toward making this SHOULD. However, if you'd care to do an audit that the spec's order is now accurate and ensure we're really careful not to keep making mistakes on this we can go ahead and make it a MUST clause.

Hey Damian,

sorry about that. I reverted my commit, and now strict ordering is SHOULD.

I'm closing this ticket again, given that we didn't hear from teor. I'm personally OK with the current state of the spec.

Note: See TracTickets for help on using tickets.