Opened 9 months ago

Closed 7 months ago

#20077 closed defect (fixed)

Make is_sensitive_dir_purpose and purpose_needs_anonymity consistent

Reported by: teor Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: refactor, 030-proposed, TorCoreTeam201610
Cc: Actual Points:
Parent ID: Points: 1
Reviewer: teor Sponsor:

Description

I'm not sure why these both exist, but it's very confusing.

Child Tickets

Change History (17)

comment:1 Changed 8 months ago by chelseakomlo

See changes here: https://github.com/chelseakomlo/tor_patches/commit/c6d0255eebdafe7b635c6c762c8c16a5a45b5a2f

In doing this ticket, I found that:

  1. The parameter router_purpose for purpose_needs_anonymity did not change the logic flow. In every case, unless the dir_purpose matched specific cases, the function would return true.
  1. The cases in is_sensitive_dir_purpose are compatible with purpose_needs_anonymity, so these two functions could easily be merged.The logic is in purpose_needs_anonymity is inverted- purpose_needs_anonymity returns true by default, unless in the specified cases, whereas is_sensitive_dir_purpose returned false unless certain cases were met.
  1. I struggled to write a clean/simple comment for purpose_needs_anonymity is accurate, please review!
  1. I wrote the unit tests to have names which describe their intended behavior. In each test, it states the expected behavior, under the given condition. However, because of column length, the wording for the test names is abbreviated, please let me know if the naming is confusing.

comment:2 Changed 8 months ago by teor

  • Keywords 030-proposed TorCoreTeam201610 added
  • Reviewer set to teor
  • Status changed from new to needs_revision

Hi, thanks for this patch!

In general, it looks great, and I would really like to get rid of is_sensitive_dir_purpose. But I think we need to keep router_purpose in purpose_needs_anonymity.
Otherwise, we change the logic from:

  • if you're a bridge, do everything anonymously,
  • otherwise, do some things non-anonymously,
  • otherwise, do everything else anonymously,

to:

  • regardless of whether you're a bridge or not,
    • do some things non-anonymously,
    • otherwise, do everything else anonymously,

comment:3 Changed 8 months ago by nickm

  • Milestone changed from Tor: 0.2.??? to Tor: 0.3.0.x-final

comment:4 Changed 8 months ago by chelseakomlo

Hey! It isn't clear what cases should be tested for router_purpose in purpose_needs_anonymity.

In the previous code, purpose_needs_anonymity returned true by default, and the only router_purpose case that was tested would return also true. So removing this case doesn't (from what I can see!) change the logic flow.

See here: https://github.com/chelseakomlo/tor_patches/commit/c6d0255eebdafe7b635c6c762c8c16a5a45b5a2f#diff-c56fd972333216da3bb1852bcc89f57dL131

Are there a list of cases where a router_purpose should _not_ use an anonymous connection?

comment:5 Changed 8 months ago by chelseakomlo

Ok, sorry, I understand. ROUTER_PURPOSE_BRIDGE overrides the other dir_purpose cases.

Thanks! Will add that back in!

Last edited 8 months ago by chelseakomlo (previous) (diff)

comment:6 Changed 7 months ago by chelseakomlo

Ok! Here are the latest changes.

https://github.com/chelseakomlo/tor_patches/commit/1e65a752e612858a6bcccbaaa7d4d554f5bed227

I had one question when doing this:

Outside of directory.c, this functionality is used in two other places, neither of which test whether the router purpose is a bridge (as is done throughout directory.c).

Specifically, this change feels slightly dirty: https://github.com/chelseakomlo/tor_patches/commit/1e65a752e612858a6bcccbaaa7d4d554f5bed227#diff-0798d3d17392dc5c15f3f58a5fc6b29aR2392

But I'm not sure if making two interfaces for this (one specifically for callers who only want to check the dir_purpose) is overkill.

Last edited 7 months ago by chelseakomlo (previous) (diff)

comment:7 Changed 7 months ago by chelseakomlo

This refactor also assumes that the same conditions met by purpose_needs_anonymity also hold for is_sensitive_dir_purpose. is_sensitive_dir_purpose was more restrictive though.

comment:8 Changed 7 months ago by nickm

This looks good to me; I'm going to merge it into 0.3.0.

I'm thinking of converting the big "if" into a switch statement too. I'll open a separate ticket for that.

comment:9 Changed 7 months ago by nickm

  • Status changed from needs_revision to merge_ready

comment:10 Changed 7 months ago by chelseakomlo

Sounds great. I can make the change to use a switch statement here if you don't want to open a separate ticket.

comment:11 Changed 7 months ago by nickm

Sure; just add it in a separate commit?

comment:12 Changed 7 months ago by chelseakomlo

Sounds good, I'll do that later today.

comment:13 Changed 7 months ago by chelseakomlo

comment:14 Changed 7 months ago by teor

Thanks for both refactoring commits.

One minor nitpick on the switch refactor:
Some compilers will complain if a switch statement doesn't have a default: case. I'm not sure whether we have that warning turned on by default, but let's be nice to people who do.

comment:16 Changed 7 months ago by nickm

merged to master! (which is now 0.3.0). Thanks!

comment:17 Changed 7 months ago by nickm

  • Resolution set to fixed
  • Status changed from merge_ready to closed
Note: See TracTickets for help on using tickets.