shared-rand-current-value violates spec

changed milestone to %Tor: 0.3.0.x-final

added component::core tor/tor milestone::Tor: 0.3.0.x-final owner::asn points::0.1 priority::high resolution::fixed severity::major status::closed type::defect labels

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

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.

Trac:
Milestone: N/A to Tor: 0.3.0.x-final

I can take this.

Trac:
Points: N/A to 0.1
Owner: N/A to asn
Status: new to assigned

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?

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.

Hello, please see bug21059 in my torspec repo: https://gitweb.torproject.org/user/asn/torspec.git/commit/?h=bug21059&id=54007cd9596fe3a0230a27b54ac4fcad94ab2cac

Trac:
Status: assigned to needs_review

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.

Trac:
Status: needs_review to needs_revision

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.

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.

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

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

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.

Trac:
Status: needs_revision to needs_review

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

Trac:
Status: needs_review to merge_ready

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.

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.

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!

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

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

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.

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

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

Trac:
Status: closed to reopened
Resolution: fixed to N/A

Trac:
Status: reopened to needs_review

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.

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.

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.

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.

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

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

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 :)

Trac:
Resolution: N/A to fixed
Status: needs_review to closed

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?

Trac:
Status: closed to reopened
Resolution: fixed to N/A

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?

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.

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.

Trac:
Resolution: N/A to fixed
Status: reopened to closed

closed

changed time estimate to 48m

moved to tpo/core/tor#21059 (closed)

shared-rand-current-value violates spec

Child items ...

Activity