ReachableAddresses adds an extra reject *:* on every SAVECONF

To fix this issue, we can do two things:

1. We should add the reject : to a copy of the list after it has been parsed in parse_reachable_addresses() using append_exit_policy_string(), rather than adding it to the option itself in options_validate().

2. We might also want to call exit_policy_remove_redundancies() on the parsed policy, so that long policies with redundancies are handled more efficiently. This is only likely to ever matter on busy hidden services.

changed milestone to %Tor: 0.3.5.x-final in legacy/trac

added 035-triaged-in-20180711 in Legacy / Trac annoyance in Legacy / Trac component::core tor/tor in Legacy / Trac configuration in Legacy / Trac milestone::Tor: 0.3.5.x-final in Legacy / Trac owner::neel in Legacy / Trac points::0.5 in Legacy / Trac priority::medium in Legacy / Trac resolution::fixed in Legacy / Trac reviewer::mikeperry in Legacy / Trac saveconf in Legacy / Trac severity::normal in Legacy / Trac status::closed in Legacy / Trac tor-client in Legacy / Trac torrc in Legacy / Trac type::defect in Legacy / Trac labels

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

Trac:
Keywords: N/A deleted, tor-03-unspecified-201612 added
Milestone: Tor: 0.3.??? to Tor: unspecified

Remove an old triaging keyword.

Trac:
Keywords: tor-03-unspecified-201612 deleted, N/A added

Trac:
Keywords: N/A deleted, saveconf, torrc, annoyance, configuration added

Trac:
Cc: N/A to neel@neelc.org
Status: new to assigned
Owner: N/A to neel

My GitHub branch is here: https://github.com/neelchauhan/tor/tree/b20874a

It seems to pass all CI tests except for Build 37.9 which fails on a changed apt-get package name (https://travis-ci.org/neelchauhan/tor/jobs/385463554). This explains why Travis CI marks this as "failed" (https://travis-ci.org/neelchauhan/tor/builds/385463545?utm_source=github_status&utm_medium=notification). I don't keep track of Debian, sorry (I use and develop on FreeBSD).

This is a bugfix, but it's not essential for 0.3.4, and may have unexpected consequences. So I've assigned it to 0.3.5.

I would like to make sure this patch is tested with stem - I think atagar (or a nyx user?) reported this bug originally.

Trac:
Milestone: Tor: unspecified to Tor: 0.3.5.x-final
Status: assigned to needs_review

Trac:
Reviewer: N/A to mikeperry

Trac:
Keywords: N/A deleted, 035-triaged-in-20180711 added

This change does not appear to append the reject line if the user uses ReachableORAddresses?

It looks like the code in parse_reachable_addresses() has the property that ReachableAddresses can set either/both OR and DIR addresses if those options are not set, but if they are set, they take precidence over ReachableAddresses. I'm guessing we want this append logic to follow the same behavior?

Also maybe this means it should be part of parse_reachable_addresses() and not policies_parse_from_options() for that reason?

Trac:
Cc: neel@neelc.org to neel@neelc.org, atagar
Status: needs_review to needs_revision

Good catch. My updated PR is here: https://github.com/torproject/tor/pull/245

Trac:
Status: needs_revision to needs_review

I am sorry for the delay here.

Technically, you should not be appending the reject to the or and dir policy if the user only set one of those options and not the other.

However, I think this is OK because it is simple and extra rejects at the end can't hurt in this case (unlike the situation we had before, where they were piling up in the actual config vars). Better simple than we miss an edge case, I think.

Setting this to merge ready!

Trac:
Status: needs_review to merge_ready

Before I merge -- do we have any tests for this code?

There are no tests right now, but I can write them if you want it.

I'd like a test to make sure that the line gets added at the appropriate time.

Also somebody should test manually to make sure that the "reject" really happens -- that is, if you list a bunch of 'accept' reachable addresses, then other addresses should not be accepted.

I have written tests and pushed them. Same PR as last one. Everything passes.

Merged!

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

This is a bugfix on commit 249b72f5 in 0.1.1.5-alpha.

closed

changed time estimate to 4h

moved from legacy/trac#20874 (moved)

added Bug label and removed 1 deleted label

added Client label and removed 1 deleted label