Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#10395 closed enhancement (implemented)

Tor's consensus lists Torbrowser updates

Reported by: StrangeCharm Owned by: mikeperry
Priority: High Milestone: Tor: 0.2.6.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: pantheon chronos prop227 nickm-patch unfrozen
Cc: mcs, brade, nickm, gk, michael, adrelanos@… Actual Points:
Parent ID: #10393 Points:
Reviewer: Sponsor:

Description

Tor's consensus lists not only recommended versions of Torbrowser, but also the version numbers, names, and hashes of incremental updates.

Child Tickets

Change History (37)

comment:2 Changed 4 years ago by nickm

Keywords: prop227 added
Milestone: Chronos: phase twoTor: 0.2.6.x-final
Priority: normalmajor

comment:3 Changed 4 years ago by nickm

Keywords: nickm-patch added
Status: newneeds_review

I've done a possible implementation of proposal 227 as branch "prop227" in my public repo. Still needs tests and documentation and a changes file.

comment:4 Changed 4 years ago by nickm

Now it has some initial tests.

comment:5 Changed 4 years ago by gk

Cc: gk added

comment:6 Changed 4 years ago by nickm

Cc: gk removed

And now it actually runs its voting algorithm when I run it inside chutney.

Latest version is in a branch called "prop227_v2".

comment:7 Changed 4 years ago by gk

Cc: gk added

I still like to be on Cc here. :)

comment:8 in reply to:  7 Changed 4 years ago by nickm

Replying to gk:

I still like to be on Cc here. :)

Whoops; sorry. I think that was one of those concurrent-edit glitches. :/

comment:9 Changed 4 years ago by michael

Cc: michael added

comment:10 in reply to:  6 ; Changed 4 years ago by mcs

Replying to nickm:

Latest version is in a branch called "prop227_v2".

I do not see that branch when I look at https://gitweb.torproject.org/nickm/tor.git/refs/heads
Am I looking at the wrong repo or did you forget to push it or ?

comment:11 Changed 4 years ago by mikeperry

Nick - when I initially wrote this proposal, I assumed that the full consensus was available from the control port. It looks like 'getinfo ns/all' doesn't actually give us any of the consensus headers where this field may appear.

I am wondering if it would be possible to export this 'package' field to the control port via a getinfo command, so we can extract it without having to parse the entire consensus? I suppose an event could also work.

comment:12 in reply to:  10 ; Changed 4 years ago by nickm

Replying to mcs:

Replying to nickm:

Latest version is in a branch called "prop227_v2".

I do not see that branch when I look at https://gitweb.torproject.org/nickm/tor.git/refs/heads
Am I looking at the wrong repo or did you forget to push it or ?

You are right; it appears I forgot to push it. I pushed the thing on my desktop named "prop227_v2". Hope it was right. So sorry :(

comment:13 in reply to:  11 Changed 4 years ago by nickm

Replying to mikeperry:

Nick - when I initially wrote this proposal, I assumed that the full consensus was available from the control port. It looks like 'getinfo ns/all' doesn't actually give us any of the consensus headers where this field may appear.

I am wondering if it would be possible to export this 'package' field to the control port via a getinfo command, so we can extract it without having to parse the entire consensus? I suppose an event could also work.

A getinfo command would not be hard.

comment:14 in reply to:  12 Changed 4 years ago by mcs

Replying to nickm:

You are right; it appears I forgot to push it. I pushed the thing on my desktop named "prop227_v2". Hope it was right. So sorry :(

No problem. Thanks for your work on this feature. Kathy and I reviewed the prop227_v2 code as best we could (we are not very familiar with the tor code, so we may have overlooked something). It looks good to us. We have just a couple of minor comments:

  • There is a typo in the comment just before validate_recommended_package_line (recommened_packages should be recommended_packages). Actually, it might be better to replace that with RecommendedPackages to match the torrc option. Also, unless it is not your practice to do so, it would be helpful to include a comment there that defines the grammar for the line.
  • In or.h, replace "pacakges" with "packages" in a comment.

We also have a couple of comments on Proposal 227 itself:

  • The consensus method, currently listed as (TBD), can be filled in with 19.
  • There is no definition for DIGESTTYPE. Or is that defined in another spec.? We are assuming that it has the same definition as DIGESTVAL.

comment:15 Changed 4 years ago by mcs

Kathy and also want to raise some Prop 227 issues that are specific to Tor Browser's intended usage (for Mike and GK to ponder):

  • The proposal mentions a "proposed update timestamp" which is checked against the consensus timestamp. Where will that timestamp come from? It seems like it will need to be a field within the JSON document that the consensus points to, but if so, the steps described are slightly out of order (the JSON file would need to be downloaded and its hash verified against the consensus before the timestamp could be retrieved and checked).
  • If we do not include an OS/platform name in the consensus package names, then we will always need to release Tor Browser for all platforms at the same time. This might be OK, but it is worth thinking about whether we want to use one package name (e.g., tbb-stable or maybe just tb-stable) or 3 package names (e.g., tb-stable-linux, tb-stable-win, tb-stable-mac).

comment:16 Changed 4 years ago by nickm

I think that having the os/platform name in the package name is essential.

comment:17 in reply to:  11 ; Changed 4 years ago by nickm

I've added a new commit to the branch to clean up those comments. Thanks!

comment:18 Changed 4 years ago by nickm

I've added another commit to implement the GETINFO command, I hope.

comment:19 in reply to:  17 ; Changed 4 years ago by mcs

Replying to nickm:

I've added a new commit to the branch to clean up those comments. Thanks!

Looks good except (and I hate to mention this), there is a typo ("RecommenedPackages" should be "RecommendedPackages").

comment:20 in reply to:  19 Changed 4 years ago by gk

Replying to mcs:

Replying to nickm:

I've added a new commit to the branch to clean up those comments. Thanks!

Looks good except (and I hate to mention this), there is a typo ("RecommenedPackages" should be "RecommendedPackages").

And don't miss s/Information abour/Information about/ ;)

EDIT: Maybe you did not mean "about", though...

Last edited 4 years ago by gk (previous) (diff)

comment:21 Changed 4 years ago by mcs

Replying to nickm:

I've added another commit to implement the GETINFO command, I hope.

A few comments:

  • There is a small typo in src/or/control.c (replace "abour" with "about").
  • I think the strcmpstart() call that you added in the following code should just be strcmp():
       } else if (!strcmpstart(question, "ns/purpose/")) {
         *answer = networkstatus_getinfo_by_purpose(question+11, time(NULL));
         return *answer ? 0 : -1;
    +  } else if (!strcmpstart(question, "consensus/packages")) {
    +    const networkstatus_t *ns = networkstatus_get_latest_consensus();
    +    if (ns->package_lines)
    +      *answer = smartlist_join_strings(ns->package_lines, "\n", 1, NULL);
    +    return *answer ? 0 : -1;
       } else {
         return 0;
       }
    

I am not sure how to test this myself to see that things look from the client side of the control port, but I trust you that it is correct.

One more thing we made need though: a way to retrieve the "valid-after" date of the consensus. I think we need this to implement this portion of prop 227:

If the current consensus timestamp is not yet more recent than
the proposed update timestamp, the updater will delay installing
the package until a consensus timestamp that is more recent than
the update timestamp has been obtained by the Tor client.

At least I think "current consensus timestamp" probably refers to the "valid-after" timestamp.

comment:22 Changed 4 years ago by nickm

Thanks! Typos fixed; added a consensus/valid-after GETINFO. Better?

comment:23 in reply to:  22 Changed 4 years ago by gk

Replying to nickm:

Thanks! Typos fixed; added a consensus/valid-after GETINFO. Better?

The changes look good to me. The prop227 implementation looks good to me, too. Two nits and one question:

1) DIGESTTYPE is now specified in prop227. You might want to add that to the "The grammar is" comment in dirserv.c, too.

2) s/Skip digestname=digestval/Skip digesttype=digestval/ <- might be a bit more consistent

3) validate_recommended_package_line() in dirserv.c allows digest type and value constructs like
"=foobar sha256=3c179f46ca77069a6a0bac70212a9b3b838b2f66129cb52d568837fc79d8fcc7" or
"= = sha256=3c179f46ca77069a6a0bac70212a9b3b838b2f66129cb52d568837fc79d8fcc7" if I see that correctly. Is that intentional and if so what is the reasoning behind it? I am wondering if that is even contrary to the spec as I am inclined to interpret "any number of characters" as implying "at least one character". But maybe I am mistaken.

comment:24 in reply to:  15 Changed 4 years ago by gk

Replying to mcs:

Kathy and also want to raise some Prop 227 issues that are specific to Tor Browser's intended usage (for Mike and GK to ponder):

  • The proposal mentions a "proposed update timestamp" which is checked against the consensus timestamp. Where will that timestamp come from? It seems like it will need to be a field within the JSON document that the consensus points to, but if so, the steps described are slightly out of order (the JSON file would need to be downloaded and its hash verified against the consensus before the timestamp could be retrieved and checked).

Good question. Even after re-reading section 3.1 a couple of times it is still not easy to extract all the necessary things. I think we need to rethink/clarify that section at least partly (for instance, that the Tor Browser is dowloading the JSON file again and is using that one (How does it e.g. know it got the one the Tor client downloaded earlier to check whether the hash is correct?) seems a bit weird. At least the hash should be checked again. Probably the JSON file should get downloaded only once anyway...). But as far as I can see there are no additional tor things needed.

  • If we do not include an OS/platform name in the consensus package names, then we will always need to release Tor Browser for all platforms at the same time. This might be OK, but it is worth thinking about whether we want to use one package name (e.g., tbb-stable or maybe just tb-stable) or 3 package names (e.g., tb-stable-linux, tb-stable-win, tb-stable-mac).

This is a good point. I am leaning to Nick's idea to even include the full OS/platform portion. IIRC in the past we had at least one occasion where we shipped only new bundles for 64 bit Linux. That was related to a compiler bug: #10195. If we think that's too much overhead then at least the OS should get added. We may want to add a Tor Browser for Android one day and I can imagine that these bundles are different enough that we (at least in the beginning) have to release bugfixes much more frequent than for Linux/Mac/Windows bundles.

comment:25 in reply to:  22 Changed 4 years ago by mcs

Replying to nickm:

Thanks! Typos fixed; added a consensus/valid-after GETINFO. Better?

Looks good. I agree with the issues raised by gk in comment:23

I compiled a tor from your prop227_v2 branch. Although there are no package lines in the consensus (so I cannot test the package feature very well), things seem to work OK (e.g., I can do getinfo consensus/valid-after and get back the timestamp). One question: when I do this:

getinfo consensus/packages

I get this:

551 Internal error

Maybe an empty string should be returned instead of an error? E.g.,

250-consensus/packages=
250 OK

comment:26 Changed 4 years ago by nickm

Keywords: unfrozen added

comment:27 Changed 4 years ago by nickm

Branch "prop227_v2" is updated.

Proposal is updated to say "one or more".

Thanks!

comment:28 Changed 4 years ago by gk

Looks good to me now, thanks.

comment:29 Changed 4 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Thanks! I've merged the branch into master.

comment:30 Changed 4 years ago by proper

Cc: adrelanos@… added
Resolution: implemented
Status: closedreopened

Since you implemented getinfo consensus/valid-after (great!) could you, while you're still somewhat at it, I hope, please also implement getinfo consensus/fresh-until and getinfo consensus/valid-until for completeness sake?

This is useful data.

Tails' tordate parses the consensus file:
https://labs.riseup.net/code/projects/tails/repository/revisions/1fe898d6c68a0cb5bede96bb1b13efdd099a1b3b/entry/config/chroot_local-includes/etc/NetworkManager/dispatcher.d/20-time.sh#L129

So does Whonix's anondate:
https://github.com/Whonix/anon-shared-helper-scripts/blob/master/usr/lib/anon-shared-helper-scripts/anondate

But parsing that file is ugly. ControlPort command would be better.

comment:31 in reply to:  30 Changed 4 years ago by mcs

Replying to proper:

Since you implemented getinfo consensus/valid-after (great!) could you, while you're still somewhat at it, I hope, please also implement getinfo consensus/fresh-until and getinfo consensus/valid-until for completeness sake?

Did you look at the patch? I am pretty sure Nick added support for all three timestamps (he is always thinking ahead and anticipating our needs). See:

https://gitweb.torproject.org/tor.git/tree/src/or/networkstatus.c#n1923

comment:32 Changed 4 years ago by nickm

Resolution: implemented
Status: reopenedclosed

Yes indeed, those are indeed implemented.

comment:33 Changed 4 years ago by proper

Resolution: implemented
Status: closedreopened

Awesome!

Can you add these also to control-spec.txt please?

And are these read from verified and/or unverified consensus? In case those are also read from unverified consensus, any way to determine that using ControlPort?

comment:34 Changed 4 years ago by nickm

Resolution: implemented
Status: reopenedclosed

done

comment:35 Changed 4 years ago by proper

Very good description.

What however is not yet documented in control-spec.txt is consensus/packages.

comment:36 Changed 4 years ago by nickm

Opening new ticket to merge 227 into spec.

comment:37 Changed 4 years ago by proper

For reference:

Opening new ticket to merge 227 into spec.

Done by nickm. See #14896.

Note: See TracTickets for help on using tickets.