Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#12932 closed defect (fixed)

Transport Key-Value pairs should be space separated

Reported by: sysrqb Owned by: isis
Priority: Immediate Milestone:
Component: Circumvention/BridgeDB Version:
Severity: Keywords: pt, bridgedb-dist, easy, bridgedb-0.2.4, isis2014Q3Q4
Cc: yawning, isis Actual Points:
Parent ID: #12130 Points:
Reviewer: Sponsor:

Description

pt-spec says that the options should be space-separated:

 2.1.0.1. Bridge torrc lines

    Bridge lines specify how Tor should connect to a bridge. The Bridge
    line format is:
 
       Bridge [<transport>] <address>:<port> [<id-fingerprint>] [<k>=<v>] [<k>=<v>] [<k>=<v>]
 
    The PT-specific parts of this format are the [transport] and [k=v]
    values.
 
    <transport> is the name of the PT that MUST be used when connecting to
    the bridge, and the <k>=<v> values are PT-specific parameters that
    MUST be passed to the PT when connecting to the bridge (this MAY
    include keys, passwords or other PT configuration options) as
    specified in [CLIENTPARAMS].

we comma-separate them:

        args = ",".join(["%s=%s" % (k, v) for k, v in self.argdict.items()])

Child Tickets

Change History (7)

comment:1 Changed 5 years ago by yawning

Cc: yawning added
Parent ID: #12130

comment:2 Changed 5 years ago by yawning

Yes, the pt-spec has each pluggable transport (server) passing these back in a comma separated format, and yes, as far as I am aware little-t tor doesn't convert that to something that's actually directly usable by the client (which requires them to be space separated).

As much as it sucks to have BridgeDB being the one doing the conversion here, it's the expedient thing to do (the alternative would be to make the changes in tor, which would put people who want to run obfs4 bridges in the position of "well, you need Really Bleeding Edge tor, compile it from source").

As it stands this blocks obfs4 deployment, so we should come to a conclusion on the Right Thing to do.

comment:3 Changed 5 years ago by isis

Cc: isis added
Keywords: pt bridgedb-dist easy added
Priority: normalblocker
Status: newaccepted

Okay. Since this is blocking the deployment of obfs4, I'm changing the priority to "blocker", and tagging it "easy" since it's a one-character patch in bridgedb.Bridges.PluggableTransport.getTransportLine() (though probably some unittest changes too).

comment:4 Changed 5 years ago by isis

Status: acceptedneeds_review

I have fix for this in my fix/12932-pt-args-spaces branch which fixes this.


In addition to the one-char bug pointed out above, there were an additional four bugs in the legacy parser, bridgedb.Bridges.parseExtraInfoFile() which I am about to deprecate:

       # get the transport line
       if ID and line.startswith("transport "):
           fields = line[10:].split()
           # [ arglist ] field, optional
           if len(fields) >= 3:
               arglist = fields[2:]               # BUGS 1 and 2
               # parse arglist [k=v,...k=v] as argdict {k:v,...,k:v} 
               argdict = {}
               for arg in arglist:
                   try: k,v = arg.split('=')      # BUG 3
                   except ValueError: continue    # BUG 4
                   argdict[k] = v
                   logging.debug("  Parsing Argument: %s: %s", k, v)

BUGS:

  1. This assumes the PT arguments are space-separated in the extrainfo descriptor. They are not; they are comma-separated.
  2. This would result in parsing the entire, comma-separated group of PT arguments into:
    {"key1": "a,key2=b,key3=c"}
    
  3. This would produce a ValueError, because there's more than one '=' character. (Meaning that the whole set of arguments would be discarded due to Bug #4.)
  4. The whole set of arguments gets discarded, without even so much as a log message, if there was more than one argument.


These bugs additionally have been fixed, and this portion of the legacy parser, bridgedb.Bridges.parseExtraInfoFile() now looks like this:

       # get the transport line
       if ID and line.startswith("transport "):
           fields = line[10:].split()

           if len(fields) >= 3:
               argdict = {}
               allargs = ','.join(fields[2:])
               for arg in allargs.split(','):
                   try:
                       k, v = arg.split('=')
                   except ValueError:
                       logging.warn("  Couldn't parse K=V from PT arg: %r" % arg)
                   else:
                       logging.debug("  Parsed PT Argument: %s: %s" % (k, v))
                       argdict[k] = v


These are all bug fixes on a single commit, 4300329a30f3b6aa3e390b140193dd50faa6e03f, from #4568. And I'm still deprecating the entire function anyway (for #9380) because the rest of it is likely just as full of bugs.


The regression test for this requires additional mocking of transport obfs4 lines in the bridge-extrainfo descriptors. This has been added in Leekspin-1.1.0, and BridgeDB's version has been bumped to the newest version.

comment:5 Changed 5 years ago by isis

Keywords: bridgedb-0.2.4 added
Resolution: fixed
Status: needs_reviewclosed

All tests pass when obfs4 is enabled, and those tests are skipped for now while obfs4 is disabled.

This is merged for 0.2.4.

comment:6 Changed 5 years ago by isis

Keywords: Isis2014Q3Q4 added

comment:7 Changed 5 years ago by isis

Keywords: isis2014Q3Q4 added; Isis2014Q3Q4 removed
Note: See TracTickets for help on using tickets.