[tor-bugs] #5463 [BridgeDB]: BridgeDB must GPG-sign outgoing mails

[Tor Bug Tracker & Wiki]

#5463: BridgeDB must GPG-sign outgoing mails
-----------------------------+----------------------------
     Reporter:  rransom      |      Owner:  isis
         Type:  enhancement  |     Status:  closed
     Priority:  normal       |  Milestone:
    Component:  BridgeDB     |    Version:
   Resolution:  fixed        |   Keywords:  bridgegb-email
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+----------------------------
Changes (by isis):

 * status:  needs_review => closed
 * resolution:   => fixed


Comment:

 Replying to [comment:21 sysrqb]:
 > nice job, isis.

 Thanks for the review!

 > I really think those should be scrubbed except for when safe logging is
 disabled, unless there's a good reason?

 Safelogging is automagic now! :D (see #9199)

 > - lib/bridgedb/Dist.py:419 - need to scrub email address
 > - lib/bridgedb/Dist.py:426 - need to scrub email address
 > - lib/bridgedb/Dist.py:428 - need to scrub email address
 > - lib/bridgedb/Dist.py:433 - need to scrub email address

 Automagic, as mentioned!

 > - lib/bridgedb/bridgerequest.py:29 - s/bridges/bridges'/s

 Thanks, fixed!

 > - Bridges.isBlocked() is buggy. It returns true if any of a bridges
 ipaddr:port pair are not known to be blocked in the country. 1) When the
 filter returns true, we don't know which pair, 2) we should somehow mark
 the bridge as unhealthy and only return it if there aren't other options
 >

 Yeah, none of the blocking check code currently works. I believe there is
 a ticket for this...

 > - lib/bridgedb/email/request.py - missing file header?

 Doh. Added!

 > - determineBridgeRequestOptions(): the presence of "get" defines an
 email as valid? I don't have a significantly better answer except that
 maybe only set it when a valid command is found? Not much is gained by
 this, though.

 Yeah... so I mostly added that because I assumed that other subclasses of
 `bridgedb.bridgerequest.BridgeRequest` would need a way to check that the
 request was valid. Because `get` was the only thing common to all request
 strings, it was what I chose to determine if the person/robot was able to
 follow basic instruction... not really necessary, and if it turns out to
 be a problem then we can just automatically set `isValid(True)`.

 > - EmailBridgeRequest:withoutBlockInCountry(): Should we support multiple
 country codes per line? space and/or comma separated?

 But why would you be traveling between multiple countries which block Tor
 bridges so fast that you need bridges which support both countries?

 > - Should our response inform the user that we only used the last
 transport specified in their email request?

 Perhaps. But if they were doing things like what's in the
 `test_createResponseBody_bridges_obfsobfswebzipv6()` unittest, then they
 are probably trying to mess with us somehow. That, or get multiple bridge
 types simultaneously. If someone wants to wreck theirs and the bridges'
 anonymity by probing the internet with every PT type on the wiki everytime
 they fire up tor or TBB, then they can slowly accumulate the collection of
 different PTs they need to do it over the course of a day or so.

 I guess what I mean is: "I'd rather not help/inform/enable users who
 appear to be somewhat-abusing BridgeDB in to destroy their anonymity and
 burn bridges as fast as possible."

 Also, in general, I'm for less wall-of-text emails, as someone who
 currently can't handle reading/answering the amount of email they receive.

 > - determineBridgeRequestOptions(): There's no harm in setting
 `request.isValid(True)` and `request.wantsKey(True)` prior to raises the
 exception, but is there a reason for doing this?

 Another somewhat awkward API design choice in this case. I assumed that
 normally, a distributor would want to set `wantsKey(True)`, and then
 include the keys with whatever response it gives later. However, since we
 don't currently support `PGP/MIME` and/or `MIME/multipart`, we can't
 attach the key, and it has to occupy the entire body of the email (ugh,
 gross).

 Later, if someone hacks in MIME support, the `EmailRequestedKey` exception
 should be rather unnecessary, and we could just tack the key on as an
 attachment to whatever else we're sending.

 I'm not sure if any other distributors in the future will want a
 `wantsKey()` method.

 > - lib/bridgedb/email/server.py:183 - Do we want to unconditionally log
 the client's email address?
 > - lib/bridgedb/email/server.py:636 - unconditionally logging client
 email address

 Automagick magick! :)

 > - lib/bridgedb/email/server.py:464 - s/param/var/
 > - MailMessage:getClientAddress() - Missing docstring
 > - MailMessage:getRecipient() - incoming - DOCDOC, and s/param/var/

 Fixed, thanks again.

 > - While the email server is getting an upgrade, should we also bump it
 to use the ESMTPFactory? We won't see any benefit from this, but I wonder
 if there's a reason for us to continue using SMTP.

 Hmm. Not sure? I thought that no mail servers actually support full ESMTP,
 and there's risk with not being able to talk to them, but we should only
 be talking to the local Postfix, IIRC.

 Though, did you have a specific extension in mind that would help with
 something?

 > - lib/bridgedb/email/server.py:316,328,332 - trailing white space after
 read(), seek(), tell()
 >
 > - lib/bridgedb/email/server.py:732 - The schedule was originally going
 to be used to partition the hash-rings so that bridgedb can rotate through
 the bridges over a given period, (obviously) it's not doing this right
 now. If we had higher bridge churn then this would be more useful.

 Yeah... I still keep wondering if we should re-enable that.

 I ended up using a schedule for implementing the gimp-captcha timeouts in
 #11215 though, so they're still useful.

 > - lib/bridgedb/strings.py:116 - s/the word the word/the word/ and
 s/tranlate/translate/

 Robert Ransom gave me a patch for this one, and it's already merged.
 Thanks, both of you!


 I'll respond to the rest of this tomorrow:

 > - lib/bridgedb/email/templates.py:33
 >
 > Is there a benefit to
 > {{{
 >         while not len(command) >= 25:
 > }}}
 > rather than
 > {{{
 >         while len(command) < 25:
 > }}}
 > ?
 >
 > - template.gettext(strings.HOWTO_TBB[1]) %
 strings.EMAIL_SPRINTF[\"HOWTO_TBB1\"] - adds an extra space between the
 words. "the %s Tor"
 >

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/5463#comment:24>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online