Make is_sensitive_dir_purpose and purpose_needs_anonymity consistent

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

changed milestone to %Tor: 0.3.0.x-final

added 030-proposed TorCoreTeam201610 component::core tor/tor milestone::Tor: 0.3.0.x-final points::1 priority::medium refactor resolution::fixed reviewer::teor severity::normal status::closed type::defect labels

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.

2. 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.

3. I struggled to write a clean/simple comment for purpose_needs_anonymity is accurate, please review!

4. 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.

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,

Trac:
Reviewer: N/A to teor
Status: new to needs_revision
Keywords: N/A deleted, 030-proposed, TorCoreTeam201610 added

Trac:
Milestone: Tor: 0.2.??? to Tor: 0.3.0.x-final

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?

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

Thanks! Will add that back in!

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.

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.

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.

Trac:
Status: needs_revision to merge_ready

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

Sure; just add it in a separate commit?

Sounds good, I'll do that later today.

Ok, here are both:

• Fix for #20077 (moved) https://github.com/chelseakomlo/tor_patches/commit/195ccce94e250a150e208f7a8fb9ba8375b6fe89

• Refactor to use switch statement https://github.com/chelseakomlo/tor_patches/commit/5442841d5f19d7cf6e53412e34dca2a60129deb6

Let me know if there are other small changes/cleanups here.

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.

Sure, I was wondering whether the default case would be needed.

Refactor to use switch statement: https://github.com/chelseakomlo/tor_patches/commit/471b0c5175521bb2bc49eb7e30d78f656a3a2843

Fix for #20077 (moved): https://github.com/chelseakomlo/tor_patches/commit/195ccce94e250a150e208f7a8fb9ba8375b6fe89

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

Trac:
Resolution: N/A to fixed
Status: merge_ready to closed

closed

changed time estimate to 8h

mentioned in issue #20410 (moved)

mentioned in issue #20620 (moved)

moved to tpo/core/tor#20077 (closed)

mentioned in issue tpo/core/tor#20410 (closed)