Opened 5 years ago

Closed 5 years ago

#6257 closed defect (fixed)

Bump to @type bridge-extra-info 1.1

Reported by: karsten Owned by: atagar
Priority: Medium Milestone:
Component: Core Tor/Stem Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

#4957 adds a new transport line to sanitized bridge extra-info descriptors, e.g.,

transport obfs2
transport obfs3

I think we'll have to bump the version number for these descriptors from 1.0 to 1.1. Even though 1.0 parsers won't understand this line, they shouldn't break. Therefore the new version should be 1.1, not 2.0.

However, both metrics-lib and stem look for "1.0" as the version number and won't parse 1.1 descriptors at all. See:

stem/stem/descriptor/__init__.py:  elif first_line in ("@type bridge-extra-info 1.0"):

I'll update metrics-lib very soon. Once stem is updated, too, I'll make metrics-db produce 1.1 descriptors.

Child Tickets

Change History (10)

comment:1 follow-up: Changed 5 years ago by atagar

Thanks. I noticed that in your commit ba74915 you simply replaced 1.0 with 1.1. Shouldn't we still accept 1.0 descriptors?

comment:2 follow-up: Changed 5 years ago by atagar

  • Status changed from new to needs_information

Change made...
https://gitweb.torproject.org/stem.git/commitdiff/c35c8a6cbb3659841f79159512c24a08a5e1d622

Though I'm not clear on this new field's attributes. Is it a simple only-appears-once string field? Is it a space separated listing? Does it appear multiple times to provide a list of values?

comment:3 in reply to: ↑ 1 Changed 5 years ago by karsten

Replying to atagar:

Thanks. I noticed that in your commit ba74915 you simply replaced 1.0 with 1.1. Shouldn't we still accept 1.0 descriptors?

That's the metrics-db commit which is about writing descriptors, not parsing them. metrics-web, which parses descriptors, still accepts 1.0 descriptors.

comment:4 in reply to: ↑ 2 Changed 5 years ago by karsten

Replying to atagar:

Change made...
https://gitweb.torproject.org/stem.git/commitdiff/c35c8a6cbb3659841f79159512c24a08a5e1d622

In that commit, doesn't the following line only accept versions 1.0 and 1.1?

+    elif desc_type == "bridge-extra-info" and major_version == 1 and minor_version in (0, 1):

Shouldn't stem also accept versions 1.2 and higher? It won't necessarily understand the new keywords, but it should parse all the parts that it understands.

Though I'm not clear on this new field's attributes. Is it a simple only-appears-once string field? Is it a space separated listing? Does it appear multiple times to provide a list of values?

See the new transport keyword in dir-spec.txt:

    "transport" transportname address:port [arglist] NL
        [Any number.]

        Signals that the router supports the 'transportname' pluggable
        transport in IP address 'address' and TCP port 'port'.

For sanitized bridge descriptors we scrub the part after transportname as explained in sanitizing step 5 on the formats page. transportname is a single string, but there can be any number of transport lines.

Can I deploy the changed metrics-db code to produce 1.1 bridge extra-infos descriptors then?

comment:5 Changed 5 years ago by atagar

  • Status changed from needs_information to needs_revision

Can I deploy the changed metrics-db code to produce 1.1 bridge extra-infos descriptors then?

Feel free. Stem doesn't yet have any consumers so it doesn't need to block descriptor changes yet.

I'll try to make the remaining fixes that you mentioned this next week.

comment:6 follow-up: Changed 5 years ago by atagar

  • Status changed from needs_revision to needs_information

Shouldn't stem also accept versions 1.2 and higher? It won't necessarily understand the new keywords, but it should parse all the parts that it understands.

Fixed...
https://gitweb.torproject.org/stem.git/commitdiff/d52e934c24c9d327e277eb60bfee060697cb43ec

See the new transport keyword in dir-spec.txt...

Hmm, this is kinda a weird field then that the spec says...
"transport" transportname address:port [arglist]

but will only appear in practice as...
"transport" transportname

Is there any reason that stem should have the ability to parse the former? If so, then can I rely on transportname being unique so I can make this a {transportname => address:port [arglist]} dictionary?

For now I'm taking the lazy option and just pretending that they only appear as the simpler scrubbed version...
https://gitweb.torproject.org/stem.git/commitdiff/a0f980c996587ef112379891339d463bda3d487a

comment:7 in reply to: ↑ 6 Changed 5 years ago by asn

  • Status changed from needs_information to new

Replying to atagar:

Shouldn't stem also accept versions 1.2 and higher? It won't necessarily understand the new keywords, but it should parse all the parts that it understands.

Fixed...
https://gitweb.torproject.org/stem.git/commitdiff/d52e934c24c9d327e277eb60bfee060697cb43ec

See the new transport keyword in dir-spec.txt...

Hmm, this is kinda a weird field then that the spec says...
"transport" transportname address:port [arglist]

but will only appear in practice as...
"transport" transportname

Is there any reason that stem should have the ability to parse the former? If so, then can I rely on transportname being unique so I can make this a {transportname => address:port [arglist]} dictionary?

Yes, at the moment, transportname is supposed to be the unique identifier of a pluggable transport used by a bridge.

That is, if a bridge publishes more than one transport lines with the same transportname, it is either evil or a future version of tor where we have decided that this should be allowed (I don't see this happening atm).

comment:8 follow-up: Changed 5 years ago by atagar

or a future version of tor where we have decided that this should be allowed

Um... so you *do* want to allow it? It would be nice if the spec required the transportname to be unique (it would let me make things nicer for users). But if the answer is "it's unique for now but might change" then that translates to "it's not unique" since that would let a future Tor change break us.

comment:9 in reply to: ↑ 8 Changed 5 years ago by asn

Replying to atagar:

or a future version of tor where we have decided that this should be allowed

Um... so you *do* want to allow it? It would be nice if the spec required the transportname to be unique (it would let me make things nicer for users). But if the answer is "it's unique for now but might change" then that translates to "it's not unique" since that would let a future Tor change break us.

OK, sorry if I gave the impression that this will change in the future.

Let me rephrase as: "The transportname is a unique identifier for each ClientTransportPlugin line.". I also checked the 180 spec again to make sure that this won't be problematic in the future.

Also, FWIW, the tor code is using this property of the transportname to match ClientTransportPlugin lines with Bridge lines.

comment:10 Changed 5 years ago by atagar

  • Resolution set to fixed
  • Status changed from new to closed

Filed a ticket to clarify the spec...
https://trac.torproject.org/6550

... and made a change to parse the non-scrubbed version of transport lines...
https://gitweb.torproject.org/stem.git/commitdiff/ac5be8cee5f58517c09217fd6860f7ef483dacc2

Resolving. Feel free to reopen if I missed something!

Note: See TracTickets for help on using tickets.