Re: [tor-bugs] #28655 [Circumvention/BridgeDB]: If a bridge supports obfs4, don't give out its other flavors

[Tor Bug Tracker & Wiki]

#28655: If a bridge supports obfs4, don't give out its other flavors
+---
 Reporter:  arma|  Owner:  phw
 Type:  defect  | Status:  closed
 Priority:  High|  Milestone:
Component:  Circumvention/BridgeDB  |Version:
 Severity:  Normal  | Resolution:  fixed
 Keywords:  bridgedb|  Actual Points:  5
Parent ID:  | Points:  2
 Reviewer:  |Sponsor:  Sponsor19
+---
Changes (by phw):

 * status:  merge_ready => closed
 * resolution:   => fixed
 * actualpoints:   => 5


Comment:

 Thanks for the review, sysrqb. I squashed the commits into a single commit
 and
 
[https://gitweb.torproject.org/bridgedb.git/commit/?id=f279fa084f19a56bf5fce9a8a575e16c8abe9dad
 merged it into develop and master]. I also updated the config file in our
 [https://gitweb.torproject.org/project/bridges/bridgedb-
 admin.git/commit/?id=ea23c7529cd33ce7f4fd89c475c3060ebdb3f292 bridgedb-
 admin] repository -- and [https://gitweb.torproject.org/project/bridges
 /bridgedb-admin.git/commit/?id=640cc71bf382a1ef7059c919498cea0b0a567086
 cleaned up a few local changes], so polyanthum is in sync with the master
 branch again.

 I also pulled the patch on polyanthum and restarted BridgeDB, so we should
 be done with this ticket.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #28655 [Circumvention/BridgeDB]: If a bridge supports obfs4, don't give out its other flavors

[Tor Bug Tracker & Wiki]

#28655: If a bridge supports obfs4, don't give out its other flavors
+-
 Reporter:  arma|  Owner:  phw
 Type:  defect  | Status:  merge_ready
 Priority:  High|  Milestone:
Component:  Circumvention/BridgeDB  |Version:
 Severity:  Normal  | Resolution:
 Keywords:  bridgedb|  Actual Points:
Parent ID:  | Points:  2
 Reviewer:  |Sponsor:  Sponsor19
+-
Changes (by sysrqb):

 * status:  needs_review => merge_ready


Comment:

 Replying to [comment:18 phw]:
 > Replying to [comment:17 sysrqb]:
 > > - I agree extending `SUPPORTED_TRANSPORTS` or creating a new `list`
 config option like `PROBING_RESISTANT_TRANSPORTS` is a good idea. Hard-
 coding the list of probing resistant PTs in one place is not great, but
 hard-coding them in two places is asking for bugs :)
 >
 > I created a separate `PROBING_RESISTANT_TRANSPORTS` in bridgedb.conf,
 right under `SUPPORTED_TRANSPORTS`. I was a bit undecided if this is
 something we should expose in the BridgeDB config because it's not meant
 to be configurable unless you really know what you're doing. That said, I
 agree that a separate config options seems to be the cleanest solution.

 Yeah, I agree with you about this not being a bridgedb-specific attribute.
 But, bridgedb must learn this somehow, so either "hard-coding the list in
 a (constant) variable" or "adding this as a config option" seem like the
 best choices. I don't have a strong opinion on which is better, I simply
 didn't like seeing the list of PTs hard-coded in multiple places :)

 >
 > > For the leekspin patch, I think it looks good. My only concern is in
 the
 
[https://gitweb.torproject.org/user/phw/leekspin.git/tree/leekspin/util.py?id=3bc9c660e8df80fe89693c8e4fad38955011bf20#n65
 description] of the new argument. It says `m` out of `n`, but it's not
 immediately obvious what `m` is here. `n` is an actual argument (`-n`,
 `--descriptors`), but `m` is not a valid argument. Replacing `` with
 `` would make it more readable, or somehow note `m` **is** `xp`: "make
 `` (`xp`) out of all ``".
 >
 > Good point, also fixed.
 >

 Great, looks good. I think the commits can be squashed before merging.

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #28655 [Circumvention/BridgeDB]: If a bridge supports obfs4, don't give out its other flavors

[Tor Bug Tracker & Wiki]

#28655: If a bridge supports obfs4, don't give out its other flavors
+--
 Reporter:  arma|  Owner:  phw
 Type:  defect  | Status:  needs_review
 Priority:  High|  Milestone:
Component:  Circumvention/BridgeDB  |Version:
 Severity:  Normal  | Resolution:
 Keywords:  bridgedb|  Actual Points:
Parent ID:  | Points:  2
 Reviewer:  |Sponsor:  Sponsor19
+--
Changes (by phw):

 * status:  needs_revision => needs_review


Comment:

 Replying to [comment:17 sysrqb]:
 > -
 
[https://gitweb.torproject.org/user/phw/bridgedb.git/tree/bridgedb/test/test_https_distributor.py?h=bug28655=ad034c358a38dcea98948d4b2c50e0758f15ce13#n197
 Reusing]  `bridges` is a little confusing (used first as a method
 parameter), can you use a different variable name?

 Yes, good point.

 > - I agree extending `SUPPORTED_TRANSPORTS` or creating a new `list`
 config option like `PROBING_RESISTANT_TRANSPORTS` is a good idea. Hard-
 coding the list of probing resistant PTs in one place is not great, but
 hard-coding them in two places is asking for bugs :)

 I created a separate `PROBING_RESISTANT_TRANSPORTS` in bridgedb.conf,
 right under `SUPPORTED_TRANSPORTS`. I was a bit undecided if this is
 something we should expose in the BridgeDB config because it's not meant
 to be configurable unless you really know what you're doing. That said, I
 agree that a separate config options seems to be the cleanest solution.

 > For the leekspin patch, I think it looks good. My only concern is in the
 
[https://gitweb.torproject.org/user/phw/leekspin.git/tree/leekspin/util.py?id=3bc9c660e8df80fe89693c8e4fad38955011bf20#n65
 description] of the new argument. It says `m` out of `n`, but it's not
 immediately obvious what `m` is here. `n` is an actual argument (`-n`,
 `--descriptors`), but `m` is not a valid argument. Replacing `` with
 `` would make it more readable, or somehow note `m` **is** `xp`: "make
 `` (`xp`) out of all ``".

 Good point, also fixed.

 The latest commit in my branch addresses your review:
 https://gitweb.torproject.org/user/phw/bridgedb.git/log/?h=bug28655
 And here's the leekspin fix:
 
https://gitweb.torproject.org/user/phw/leekspin.git/commit/?id=d34c804cd0f01af5206833e62c0dedec8565b235

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs

Re: [tor-bugs] #28655 [Circumvention/BridgeDB]: If a bridge supports obfs4, don't give out its other flavors

[Tor Bug Tracker & Wiki]

#28655: If a bridge supports obfs4, don't give out its other flavors
+
 Reporter:  arma|  Owner:  phw
 Type:  defect  | Status:  needs_revision
 Priority:  High|  Milestone:
Component:  Circumvention/BridgeDB  |Version:
 Severity:  Normal  | Resolution:
 Keywords:  bridgedb|  Actual Points:
Parent ID:  | Points:  2
 Reviewer:  |Sponsor:  Sponsor19
+
Changes (by sysrqb):

 * status:  needs_review => needs_revision


Comment:

 Sorry this took so long to review. It looks good! I only have a few small
 nitpicks.

 -
 
[https://gitweb.torproject.org/user/phw/bridgedb.git/tree/bridgedb/test/test_https_distributor.py?h=bug28655=ad034c358a38dcea98948d4b2c50e0758f15ce13#n197
 Reusing]  `bridges` is a little confusing (used first as a method
 parameter), can you use a different variable name?
 - Ugh,
 
[https://gitweb.torproject.org/user/phw/bridgedb.git/tree/bridgedb/filters.py?h=bug28655=ad034c358a38dcea98948d4b2c50e0758f15ce13#n157
 python]. Sure, I guess that's correct. (no changes needed)
 - I agree extending `SUPPORTED_TRANSPORTS` or creating a new `list` config
 option like `PROBING_RESISTANT_TRANSPORTS` is a good idea. Hard-coding the
 list of probing resistant PTs in one place is not great, but hard-coding
 them in two places is asking for bugs :)

 Overall, I think the bridgedb patch is good. I worry about making
 `SUPPORTED_TRANSPORTS` more complex. I think a simple `str` -> `boolean`
 mapping is easy to understand. The two ways I see doing this are not as
 obvious:

 `dict` -> `tuple` as `str` -> (`boolean`, `boolean`)
 `dict` -> `dict` -> `boolean` as `str` -> `str` ({supported, probing
 resistant}) -> `boolean`)

 Maybe you had another idea for this, as well :) but with these options, I
 think adding a new config variable would be more readable.

 For the leekspin patch, I think it looks good. My only concern is in the
 
[https://gitweb.torproject.org/user/phw/leekspin.git/tree/leekspin/util.py?id=3bc9c660e8df80fe89693c8e4fad38955011bf20#n65
 description] of the new argument. It says `m` out of `n`, but it's not
 immediately obvious what `m` is here. `n` is an actual argument (`-n`,
 `--descriptors`), but `m` is not a valid argument. Replacing `` with
 `` would make it more readable, or somehow note `m` **is** `xp`: "make
 `` (`xp`) out of all ``".

--
Ticket URL: 
Tor Bug Tracker & Wiki 
The Tor Project: anonymity online
___
tor-bugs mailing list
tor-bugs@lists.torproject.org
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs