Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#3589 closed enhancement (implemented)

Advertise bridge pluggable transports using extra-info lines

Reported by: asn Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: pt tor-bridge
Cc: karsten, aagbsn Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We should implement the "Advertising bridge methods" section of proposal 180.

Child Tickets

TicketTypeStatusOwnerSummary
#5396defectclosedUpdate torspec wrt transports in extra-info descriptors

Attachments (1)

0001-Note-that-bridges-never-give-out-extra-info-docs.patch (892 bytes) - added by karsten 7 years ago.
Patch to dir-spec.txt

Download all attachments as: .zip

Change History (30)

comment:1 Changed 8 years ago by asn

Parent ID: #3591

comment:2 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-final

This may be doable with a small amount of code. If so, we could try do it in Dec for 0.2.3.

comment:3 Changed 8 years ago by asn

Karsten told me that 'extra-info descriptors' are mainly used for statistics, third-party information, and generally things that clients should not be very interested at.
Do we want to put the transport information in 'extra-info descriptors' or in regular relay descriptors? Clients fetching bridge descriptors that use transports will probably almost always be interested in the transports.

Karsten also told me that bridgedb doesn't even support parsing of extra-info descriptors at the moment.

I don't have strong opinions on this, since I know almost nothing about the directory format design decisions.

comment:4 Changed 8 years ago by asn

Cc: karsten added

(Also, CCing Karsten.)

comment:5 Changed 8 years ago by karsten

Cc: aagbsn added

So, I talked to Aaron about this a few days ago. He thinks it doesn't really matter where bridges include this information, though extra-info descriptors would require slightly more work.

Here's another reason why it might make more sense to add pluggable transports information to server descriptors. Clients download server descriptors from bridges, but not their extra-info descriptors. Clients could learn about other transports of a bridge and store that information in their state file. And if one of their transports gets blocked they might try another transport. Most of this is probably not supported right now, but putting the information in the right descriptors might facilitate such a design in the future.

comment:6 Changed 8 years ago by nickm

Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final
Type: defectenhancement

It's a feature; can't go in 0.2.3.x at this point.

At this point, I'm thinking that the information might actually need to go in both places: stuff that only the authority/bridgedb should know about needs to go in extrainfo; stuff that the client needs to know about *might* belong in the descriptor. Though I can imagine exceptions there. It might be that knowing one transport to reach a bridge doesn't entitle you to know all of them, for instance.

comment:7 in reply to:  6 ; Changed 7 years ago by karsten

Replying to nickm:

At this point, I'm thinking that the information might actually need to go in both places: stuff that only the authority/bridgedb should know about needs to go in extrainfo; stuff that the client needs to know about *might* belong in the descriptor. Though I can imagine exceptions there. It might be that knowing one transport to reach a bridge doesn't entitle you to know all of them, for instance.

Oh, I hadn't thought of the client not knowing a bridge's extra-info descriptor as a security property.

Are we sure there's no way for a client to learn a bridge's extra-info descriptor? (I didn't find one, but I may have missed something.)

By the way, a client can easily look up a bridge's sanitized extra-info descriptor, and we're not sanitizing much of those. Is that problematic for existing extra-info descriptor contents? We might want to discuss sanitizing pluggable transport lines in extra-info descriptors in the future. (Right now, unknown lines are simply dropped in the sanitizing process.)

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

Replying to karsten:

Oh, I hadn't thought of the client not knowing a bridge's extra-info descriptor as a security property.

I had.

Are we sure there's no way for a client to learn a bridge's extra-info descriptor? (I didn't find one, but I may have missed something.)

There shouldn't be; IIRC, the bridge authority deliberately doesn't answer requests for extainfos, and bridges don't answer questions for their own extrainfo.

(We should double-check that, I guess)

comment:9 in reply to:  8 ; Changed 7 years ago by asn

Replying to nickm:

Replying to karsten:

Oh, I hadn't thought of the client not knowing a bridge's extra-info descriptor as a security property.

I had.

Are we sure there's no way for a client to learn a bridge's extra-info descriptor? (I didn't find one, but I may have missed something.)

There shouldn't be; IIRC, the bridge authority deliberately doesn't answer requests for extainfos, and bridges don't answer questions for their own extrainfo.

(We should double-check that, I guess)

Maybe it's this protection in directory_handle_command_get():

  if (!strcmpstart(url,"/tor/server/") ||
      (!options->BridgeAuthoritativeDir &&
       !options->BridgeRelay && !strcmpstart(url,"/tor/extra/"))) {

(Is this property of extra-info descriptors documented on the spec? I didn't manage to find it.)


So, as I understand it, we like bridges writing their transports in their extra-info descriptors. That leaves bridgedb responsible for managing the transport information for each bridge.

I don't have strong opinions on the subject, and maybe taking the paranoid route is the way to go.


As far as an implementation plan is concerned, we should first implement the method lines in extra-info descriptors (as detailed by 180). Then we should implement DECLARE and method-info lines, which allow bridges to store method information in their router descriptors.

comment:10 Changed 7 years ago by asn

Parent ID: #3591#4685

comment:11 in reply to:  9 ; Changed 7 years ago by karsten

Replying to asn:

(Is this property of extra-info descriptors documented on the spec? I didn't manage to find it.)

See the attached patch to dir-spec.txt.

I don't have strong opinions on the subject, and maybe taking the paranoid route is the way to go.

Not necessarily. You could leave the design decision open by allowing transport information in both server and extra-info descriptors. BridgeDB could support both designs, and the transport developers could decide whether clients should be able to learn about their transport from other transports or via BridgeDB only.

We still need to discuss how to sanitize bridge descriptors with pluggable transport lines. I'm going to open a new ticket for that.

Changed 7 years ago by karsten

Patch to dir-spec.txt

comment:12 in reply to:  11 Changed 7 years ago by karsten

Replying to karsten:

We still need to discuss how to sanitize bridge descriptors with pluggable transport lines. I'm going to open a new ticket for that.

Please see #4957.

comment:13 Changed 7 years ago by asn

Nick, I'm looking at how and when descriptors are generated. Managed proxies are configured step-by-step in run_scheduled_events(), and sometimes they require one or two ticks to get finalized. Meanwhile, router_rebuild_descriptor() and its callers are called from many codepaths, and I'm worrying that we might generate a descriptor before all managed proxies are fully configured (which means that the descriptor won't list all the transports we support).

In my tests, the bridge always builds a descriptor after managed proxies are configured [0], but I'm not confident that this will always be true.

Do you think it's acceptable to check if router.c:desc_routerinfo is non-NULL, right after all managed proxies are configured (using pt_proxies_configuration_pending() in run_scheduled_events())? If we already had generated a descriptor (that is, desc_routerinfo is non-NULL), we should generate a new one (now that all transports are registered) and re-publish it.

[0]:

Breakpoint 1, extrainfo_dump_to_string (s_out=0x83df7a8, extrainfo=0x83df7a8, ident_key=0x81c0280) at router.c:2242
2242	{
(gdb) bt
#0  extrainfo_dump_to_string (s_out=0x83df7a8, extrainfo=0x83df7a8, ident_key=0x81c0280) at router.c:2242
#1  0x080812a2 in router_rebuild_descriptor (force=0) at router.c:1629
#2  0x080817d3 in router_get_my_routerinfo () at router.c:1408
#3  0x080eadd5 in directory_conn_is_self_reachability_test (conn=0xb68011a8) at directory.c:636
#4  0x080f3578 in connection_dir_client_reached_eof (conn=<optimized out>) at directory.c:1979
#5  0x080f506e in connection_dir_reached_eof (conn=0xb68011a8) at directory.c:2256
#6  0x080d0423 in connection_reached_eof (conn=0xb68011a8) at connection.c:3872
#7  connection_handle_read_impl (conn=0xb68011a8) at connection.c:2685
#8  connection_handle_read (conn=0xb68011a8) at connection.c:2697
#9  0x08051b14 in conn_read_callback (fd=-1, event=2, _conn=0xb68011a8) at main.c:702
#10 0x0017c0c9 in event_base_loop () from /usr/lib/libevent-2.0.so.5
#11 0x08052a24 in do_main_loop () at main.c:1924
#12 0x080541e5 in tor_main (argc=3, argv=0xbffff454) at main.c:2619
#13 0x0804f1cb in main (argc=3, argv=0xbffff454) at tor_main.c:30

comment:14 Changed 7 years ago by nickm

asn: I think what you actually want to do to decide whether to make a new descriptor is to call mark_my_descriptor_dirty() whenever the managed proxy information is changed: options_transition_affects_descriptor might be a good place to do that.

comment:15 Changed 7 years ago by asn

Status: newneeds_review

Please see bug3589 in https://git.gitorious.org/mytor/mytor.git.

comment:16 in reply to:  14 ; Changed 7 years ago by rransom

Replying to nickm:

asn: I think what you actually want to do to decide whether to make a new descriptor is to call mark_my_descriptor_dirty() whenever the managed proxy information is changed: options_transition_affects_descriptor might be a good place to do that.

But the bridge authority should reject a new descriptor if nothing in the descriptor itself has changed significantly enough. (The bridge authority will decide whether to accept or reject a new descriptor before it has seen the extra-info document.)

comment:17 in reply to:  16 Changed 7 years ago by nickm

Replying to rransom:

Replying to nickm:

asn: I think what you actually want to do to decide whether to make a new descriptor is to call mark_my_descriptor_dirty() whenever the managed proxy information is changed: options_transition_affects_descriptor might be a good place to do that.

But the bridge authority should reject a new descriptor if nothing in the descriptor itself has changed significantly enough. (The bridge authority will decide whether to accept or reject a new descriptor before it has seen the extra-info document.)

Ow, that's right. Perhaps the bridge authority doesn't need to take so much pains to reject "not very changed" descriptors, since nobody is cacheing them. Otherwise, we'll need to do something like suppressing publication until we have configured (or not configured) our transports. Perhaps that's a good idea in any case.

comment:18 in reply to:  15 ; Changed 7 years ago by karsten

Replying to asn:

Please see bug3589 in https://git.gitorious.org/mytor/mytor.git.

From reading the code, bridges would include lines like the following:

method obfs2 12.34.56.78:9012
method obfs3 21.43.65.87:9021

Should we rename "method" to something more meaningful, e.g., "pluggable-transport" or just "transport"?

And is there a patch for dir-spec.txt?

comment:19 in reply to:  18 Changed 7 years ago by asn

Status: needs_reviewneeds_revision

Replying to karsten:

Replying to asn:

Please see bug3589 in https://git.gitorious.org/mytor/mytor.git.

From reading the code, bridges would include lines like the following:

method obfs2 12.34.56.78:9012
method obfs3 21.43.65.87:9021

Should we rename "method" to something more meaningful, e.g., "pluggable-transport" or just "transport"?

Sounds like a good idea. I'll provide a patch for tor.git and torspec.git tomorrow.

And is there a patch for dir-spec.txt?

No. I should make one of those too!

comment:20 Changed 7 years ago by asn

Status: needs_revisionneeds_review

I updated branch bug3589 to rename "method" to "transport".

I also made #5396 for the torspec side of this.

comment:21 Changed 7 years ago by karsten

Parent ID: #4685

Removing parent ticket relationship to #4685 in order to close it.

comment:22 Changed 7 years ago by nickm

Status: needs_reviewneeds_revision

Reviewing...

I don't like including transports.h in circuitbuild.h. Presumably that's just for the transport_t declaration. If so the right fix is to do a forward-declaration of struct transport_t in circuitbuild.h, and have the functions take that instead.

Looks like there's now a bogus newline before proxy_prepare_for_restart.

In the future, please don't re-order functions within a file except in a commit that does nothing but re-order functions. No need to change this time though.

I'd be more comfortable if pt_configure_remaining_proxies would only call mark_my_descriptor_dirty when there were previously unconfigured proxies.

The documentation for the addr field in transport_t doesn't give me enough information to say whether t->addr is right thing to stick in a transport line.

Is this tested?

No changes file.

comment:23 Changed 7 years ago by nickm

Also doesn't pass "make check-spaces"

comment:24 Changed 7 years ago by asn

Note to self: Also change the file-level documentation wrt to the new behavior of managed_proxy_t.transports.

comment:25 Changed 7 years ago by asn

Keywords: pt added

comment:26 Changed 7 years ago by asn

Status: needs_revisionneeds_review

Pushed update that hopes to address Nick's comments in branch bug3589. Check again!

comment:27 Changed 7 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks more okay now; merging to master.

comment:28 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:29 Changed 7 years ago by nickm

Component: Tor BridgeTor
Note: See TracTickets for help on using tickets.