Opened 3 years ago

Closed 3 years ago

#19958 closed enhancement (implemented)

Implement proposal 264 (protocol versioning)

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201608, review-group-9
Cc: Actual Points: 2
Parent ID: #15055 Points: 2
Reviewer: dgoulet Sponsor:

Description

This is not 100% strictly necessary for #15055 to work, but every time we change a protocol, we will wish that we had included this feature.

Mostly implemented in a fit of C while I was on vacation.

Child Tickets

Change History (23)

comment:1 Changed 3 years ago by nickm

Keywords: TorCoreTeam201608 added
Owner: set to nickm
Points: 2
Status: newaccepted

comment:2 Changed 3 years ago by nickm

Actual Points: 2
Status: acceptedneeds_review
Type: defectenhancement

See branch "protover" in my public repository. I have also updated proposal 264, and created a new fun proposal 272.

comment:3 Changed 3 years ago by nickm

Now instead see "protover_v2" in my public repository. It is rebased onto master, with a couple of tweaks to avoid conflicts with #19163.

comment:4 Changed 3 years ago by nickm

Keywords: review-group-8 added

comment:5 Changed 3 years ago by nickm

Ping? I really need review on this. I want this branch to go in before we add any more protocol features, so more things may block on it than you know.

comment:6 Changed 3 years ago by dgoulet

Priority: MediumHigh
Reviewer: dgoulet

Oh... I've already looked at the code and read the spec here but failed to give back comments. First thing Monday morning! Taking this one as a Reviewer.

comment:7 Changed 3 years ago by dgoulet

Status: needs_reviewneeds_revision

Review at https://gitlab.com/dgoulet/tor/merge_requests/11

So far, actual-review-point: 0.3

Comment on the proposal:

  • HSMid is both for RP and IP. Taking proposal 224 where we'll have a new version protocol for introduction point and directory. However, the RP don't need a protocol bump. So if clients start looking at HSMid to pick an RP, it's weird to me that they would be OK with version 1 for RP but version 2 for IP. Do you think it's fine to do that? or should we have different values for IP and RP?
  • About hidden service. We are currently implementing version 3 of the protocol where v0-1 are obsolete and 2 is the current one. Yet, this makes it that HSDir and HSMid are currently at version 1. I think we should have them in the proposal and enforce version 2 for both on the relay side? So, with prop224, we'll make the recommended version be HSDir=2-3 but required should be HSDir=0-3 because technically relay still support those versions and with 224 we planned to remove the support. So when we deploy 224, we can switch in the consensus to HSDir=2-3 and voila. Does that makes sense?

comment:8 Changed 3 years ago by nickm

I think separate values for IP and RP is fine. So, HSIntroPoint, HSRendPoint?

WRT the other ones, could you attach a patch to the proposal so I understand what you're suggestiong?

It doesn't make sense for "recommended" to be more narrow than "required" -- "required" means "exit if you don't have these" and "recommended" means "warn if you don't have these".

comment:9 in reply to:  8 Changed 3 years ago by dgoulet

Replying to nickm:

I think separate values for IP and RP is fine. So, HSIntroPoint, HSRendPoint?

That sounds good to me. We could also shrink those to save "bytes" I guess with like HSip and HSrp but no strong opinion here.

WRT the other ones, could you attach a patch to the proposal so I understand what you're suggestiong?

Sure will try to do that asap.

It doesn't make sense for "recommended" to be more narrow than "required" -- "required" means "exit if you don't have these" and "recommended" means "warn if you don't have these".

I think I got that correctly. In this particular case, v0 and v1 are "obsolete" that is no tor uses it but relay still can handle it... so we recommend "2 and 3" but require "0 to 3" because we don't want to cut v0 and v1 yet. That was my reasoning but might be really bad...

However, if this comes from the consensus I can't see any harm of doing so rather than abruptly ripping off v0 and v1 from tor and receiving a 404 error when you try v1 for instance on an HSDir leading to "version discovery".

Version 0, edited 3 years ago by dgoulet (next)

comment:10 Changed 3 years ago by dgoulet

See branch ticket19958_01 with top commit as an attempt at breaking down the HS with the current protocol versions.

comment:11 Changed 3 years ago by nickm

Okay, I believe I've responded to your review items.

I am okay with *changing* these versions, but have a look at this paragraph:

   Because all relays currently on the network are 0.2.4.19 or later, we
   can require 0.2.4.19, and use 0.2.4.19 as the minimal version so we
   we don't need to do code archaeology to determine how many
   no-longer-relevant versions of each protocol once existed.

I like the idea of keeping the versions in sync, but there's not a point to giving names for versions that only old clients use.

What do you think of my torspec changes in dgoulet_ticket19958_01 ? (Based on your branch)

comment:12 in reply to:  11 Changed 3 years ago by dgoulet

Replying to nickm:

What do you think of my torspec changes in dgoulet_ticket19958_01 ? (Based on your branch)

I like it!

comment:13 Changed 3 years ago by nickm

Merged the spec branch to torspec.

comment:14 Changed 3 years ago by nickm

I've updated my protover_v2 branch to match the updated spec.

comment:15 Changed 3 years ago by asn

Style suggestion: Perhaps we can functionify all the new code of format_networkstatus_vote() into a get_supported_protocols_str() function? I think that might make that function cleaner.

comment:16 Changed 3 years ago by nickm

asn: done! (in abcb6489e1bfd95591d945bb62b52d6aceb8f39a)

comment:17 Changed 3 years ago by nickm

Keywords: review-group-9 added; review-group-8 removed

comment:18 Changed 3 years ago by isis

Status: needs_revisionneeds_review

comment:19 Changed 3 years ago by isis

Status: needs_reviewneeds_revision

I've read through Nick's code twice now, and found no major problems beyond what dgoulet had already found. I still think we should strive to keep the "symmetry" in networkstatus lines… other than that everything looks good to me.

Nick and I on IRC spoke briefly, and Nick is worried that DirAuths might accidentally vote to shut themselves down due to missing required subprotocols. I agree that this is worrisome, but it should be an easy fix.

comment:20 Changed 3 years ago by nickm

Status: needs_revisionneeds_review

I've added a few more commits:

  • Removing version_known, per isis's suggestion. 3d7260b700d636
  • Renaming "proto " to "pr " in vote and consensus documents. 04dd51807d93
  • Double-checking dirauth vote lines. 8fe26b40d5cc69

comment:21 Changed 3 years ago by nickm

In d881250ff68a7bb I've updated prop264 to know about "pr".

comment:22 Changed 3 years ago by dgoulet

I've pushed latest from nickm here: https://gitlab.com/dgoulet/tor/merge_requests/11

I went over the latest new commits, all lgtm except one little detail in a comment. Leaving this ticket in needs_review for now as others might want to look at it.

comment:23 Changed 3 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Squashing and merging.

Note: See TracTickets for help on using tickets.