Opened 2 years ago

Closed 22 months ago

Last modified 15 months ago

#16069 closed defect (fixed)

ipv4 + ipv6 exit : v6 policy is displayed twice, v4 isn't displayed

Reported by: toralf Owned by:
Priority: Very High Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: 026-backport, ipv6, PostFreeze027, 2016-bug-retrospective
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I do have 2 different exit policies for ipv6 (just 6 rules) and ipv4 (reduced exit policy) but the status page shows the ipv6 rule set for ipv4 too :

https://atlas.torproject.org/#details/F1BE15429B3CE696D6807F4D4A58B1BFEC45C822

Child Tickets

Attachments (1)

torrc (4.5 KB) - added by toralf 2 years ago.
torrc

Download all attachments as: .zip

Change History (51)

comment:1 Changed 2 years ago by karsten

Good catch. But it's not a bug. Here's why:

Your relay writes its full IPv4 exit policy to its server descriptor, but it only includes a summary of its IPv6 exit policy in the same document. The reason is that normal clients don't care about the details anymore but only use the summary these days. If an exit node rejects a connection they thought was permitted, they'll try somewhere else. In theory, relays could switch to only including an IPv4 exit policy summary in their server descriptors, but nobody wrote that code yet. In this case it seems like both summaries are the same, even if the detailed exit policies differ.

So, there's no way how Atlas could display the long form of your IPv6 exit policy. But can you think of a way to make this more intuitive for Atlas users? Should we throw out the full exit policy entirely?

Changed 2 years ago by toralf

torrc

comment:2 Changed 2 years ago by toralf

Erm, the current behaviour shows in this example a wrong "IPv4 Exit Policy Summary" and an incomplete "Exit Policy" (torrc attached).

So better show nothing than wrong/incomplete values IMO.

comment:3 Changed 2 years ago by karsten

  • Component changed from Onionoo to Tor

If that's really the torrc that your relay currently uses, then there's possibly a bug in Tor. Here's a recent server descriptor published by your relay:

router zwiebeltoralf 5.9.158.75 443 0 0
or-address [2a01:4f8:190:514a::2]:443
platform Tor 0.2.6.7 on Linux
protocols Link 1 2 Circuit 1
published 2015-05-18 14:48:49
fingerprint F1BE 1542 9B3C E696 D680 7F4D 4A58 B1BF EC45 C822
uptime 0
bandwidth 8388608 41943040 3236864
extra-info-digest 29047D780FB5E5774B44D359C24FA68A16FDCF77
onion-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBALyeOrYKA4kdObL4DFWgEOV9OCnJRSOkGHGNyYGZOTiFWwE7lgfmZm2w
fQcGwo/Auv/vOHBbifYjdO3z/Mdhx96imieloV4a9+Q69i6f9lK/r0VjlsxudIvN
mVYqd145RV+d5mv45WLS+H1D6+XrkR/+LIBYT11/maEV8ICWHlBvAgMBAAE=
-----END RSA PUBLIC KEY-----
signing-key
-----BEGIN RSA PUBLIC KEY-----
MIGJAoGBANdI7BYS2UyCQ+2jVJnTMKyvYm41U4tAsE2+XD6CrmKF+Mm6ePyan9uw
spmfPcG9l5vVNUC2npldyxhbbExTskzhw4BGDhDbSPZGnclC3AmCuJUz+Gj4FbC4
x+2m83XAUFUzCI9R1tHkEYPEThJiapl7hN/iUEvcX9l3B8bX61zfAgMBAAE=
-----END RSA PUBLIC KEY-----
hidden-service-dir
contact replace k with c : kontakt @ zwiebeltoralf . de
ntor-onion-key OUk2GV0LkdUTNTuETpte/eqIKp4tfmF383e/C0uCehs=
reject 0.0.0.0/8:*
reject 169.254.0.0/16:*
reject 127.0.0.0/8:*
reject 192.168.0.0/16:*
reject 10.0.0.0/8:*
reject 172.16.0.0/12:*
reject 5.9.158.75:*
accept *:443
accept *:989-995
accept *:6660-6669
accept *:6679
accept *:6697
reject *:*
ipv6-policy accept 443,989-995,6660-6669,6679,6697
router-signature
-----BEGIN SIGNATURE-----
CbsQ3/HNBJJLRmm4yE+KIyQW46U92Mn1z7wPZW79FUdxbvGpAC1LrdWT3VTWAGyV
+wDKd9iFyJqLLFyLj5QdjPfeomRrC91fGeYKOMUHI62t039WhBj0sWoVpafLWQzt
gQgJswW/DGEN5SGjYmTAAjQbX+B5jz0K7Zn4s87fx60=
-----END SIGNATURE-----

Changing component to Tor, because if this is really a bug in Tor, fixing that would be more important than making Atlas' interface more intuitive.

comment:4 Changed 2 years ago by toralf

IMO it there is an issue - the output of my script http://www.zwiebeltoralf.de/pub/info.py shows a lot of used ports of bitcoins and so which are not opened in ipv6.

comment:5 Changed 2 years ago by atagar

Oh interesting. Another related ticket (the spec around this needs clarification): #16103

comment:6 Changed 2 years ago by nickm

  • Keywords 026-backport ipv6 added
  • Milestone set to Tor: 0.2.7.x-final

comment:7 Changed 2 years ago by nickm

  • Keywords PostFreeze027 added

I'd merge patches for these for 0.2.7 if they come in on time. In some cases, that will require figuring out an as-yet-unsolved bugs.

comment:8 Changed 23 months ago by nickm

  • Priority changed from normal to major

comment:9 Changed 23 months ago by nickm

  • Priority changed from major to critical

I think this is maybe a bug, or maybe a documentation error? Either way, it's really-need-to-fix IMO.

comment:10 Changed 23 months ago by teor

As far as I'm aware, the current tor code treats accept and accept6 identically, and treats reject and reject6 identically.

When an accept or accept6 line has an address "*", it's treated as "all IPv4 and IPv6 addresses". For example, "accept *:443" and "accept6 *:443" both mean "accept connections to all IPv4 and IPv6 addresses on port 443".

So all ExitPolicy lines in the attached torrc past line 39 are being ignored:
"ExitPolicy reject6 *:*"

However, if you specify "accept6 *6:443" you will get the behaviour you're looking for: "accept connections to all IPv6 addresses on port 443".

comment:11 Changed 23 months ago by nickm_mobile

Ugh. I think that accept6/reject6 shpuld either get a warning, or start meaning ipv6 only. And the manpage has to get fixed too. Possibly the sample torrc as well.

comment:12 Changed 23 months ago by teor

Ideally, I'd like to see the following:

  • accept6/reject6 means IPv6 only, accept6 *:* means all IPv6 addresses, warn on IPv4 address or *4

In addition, we could:

  • introduce accept4/reject4, which mean IPv4 only, accept4 *:* means all IPv4 addresses, warn on IPv6 address or *6
  • deprecate accept/reject, which mean IPv4 and IPv6, accept *:* means all IPv4 and IPv6 addresses, warn on all uses and recommend user moves to accept4/accept6/reject4/reject6

I think this provides a sensible migration path and I'm happy to code it (I'm familiar with the code in question).

What do you think, nickm?
Which parts would you want in 027? Would you want any of it in 026?

comment:13 Changed 23 months ago by teor

I've given this some further thought today. In summary:

  • The current accept/accept6 scheme is confusing, as accept6 looks like it should mean all IPv6 addresses only.
  • My first suggestion of changing accept6 to mean IPv6 addresses, without changing the meaning of accept (both IPv4 and IPv6), just seems to add to the confusion.
  • I no longer like the semantics of changing accept to mean IPv4 addresses, as it changes the meaning of accept *:* and reject *:*, which is the last entry in many ExitPolicy blocks. This could cause IPv6 Exits to have many more open ports, a serious issue.

I am now if favour of the following scheme: (based on my second suggestion above)

  • accept/reject don't change semantics, they mean both IPv4 and IPv6 addresses.
    • This preserves the semantics of accept *:* and reject *:*, which are vital to the correct operation of many ExitPolicy blocks.
    • Warn that any torrc ExitPolicy entries after an accept *:* or reject *:* are ignored. (This would perhaps have assisted toralf in discovering and resolving their issues at config time.)
    • Warn that any accept/reject *:N entries cover both IPv4 and IPv6.
  • accept4/reject4 mean IPv4 addresses only. (Using with *:N means IPv4 only.)
    • Using accept4/reject4 with IPv6 addresses or *6:N is an error or a warning. It has no effect.
    • Info about new behaviour for consistency with accept6/reject6?
  • accept6/reject6 mean IPv6 addresses only. (Using with *:N means IPv6 only.)
    • Using accept6/reject6 with IPv4 addresses or *4:N is an error or a warning. It has no effect.
    • Warn or info about changed behaviour.

This has the following impacts:

  • accept6/reject6 lines change behaviour to only affect IPv6. This could change the number of open IPv4 or IPv6 ports on existing Exits. However, this is likely to be closer to actual operator intent.
  • counter-intuitive combinations are now flagged using warnings or errors.

We may also need to make similar changes to the default exit policy in-code, manpage, sample torrc, and torspec.

comment:14 Changed 23 months ago by nickm

Hm. This sounds pretty comprehensive and sensible. How much of this do you think it makes sense to do for 0.2.7, seeing that I really want to stop fixing non-showstopper non-regression issues this week?

comment:15 Changed 23 months ago by teor

We can't make the changes I suggested above.

I just read router_add_exit_policy, which already enforces the following conditions when parsing Exit relay descriptors:

  • accept/reject must not be followed by an IPv6 address
  • accept6/reject6 must not be followed by an IPv4 address

This descriptor-parsing code is already deployed to the authorities, thousands of relays, and millions of clients.

So I suggest a matching set of changes on the torrc parsing side:

  • accept6/reject6 must not be followed by an IPv4 address, including *4
    • if this happens, warn and ignore the ExitPolicy entry
  • accept6/reject6 *:* produces an accept6/reject6 IPv6 wildcard address only
    • this is changed behaviour, but it's probably what the operator expected
    • info about changed behaviour?
  • accept/reject must not be followed by an IPv6 address, including *6
    • if this happens, warn and ignore the ExitPolicy entry
  • accept/reject *:* produces an accept/reject wildcard IPv4 address only
    • info/warn about changed behaviour?)

This last change is consistent with the other changes, and resolves toralf's issue. But it might confuse operators who end their policies in accept/reject *:* and expect it to catch IPv4 and IPv6. That said, the existing order dependency between IPv4 and IPv6 lines is how we got into this mess, and we need to kill it.

comment:16 Changed 23 months ago by teor

The fixes in comment 15 are required to get policies_parse_exit_policy_internal working as intended. Currently, the ipv6_exit parameter is ignored if the last line is accept *:*. (Which can't be the intention, surely.)

I'll need to add IPv6 entries to DEFAULT_EXIT_POLICY in policies.c as well.
(Previous versions of tor would have parsed it and used it to cover both IPv4 and IPv6.)

And if local_address should take an array of local IPv4 and IPv6 addresses, not just a single IPv4 address. Split off into #17027. (This is a potential security issue, as it allows connections to local ports on an exit.)

comment:17 Changed 23 months ago by teor

My branch bug16069-exit-policy-inconsistent on https://github.com/teor2345/tor.git has the following changes:

  • Fix up some comments in the exit policy
  • Warn when parsing torrcs if we have an IPv4 address on an accept6/reject6, or an IPv6 address on an accept/reject
  • Modify accept/reject * to mean IPv4 only, rather than IPv4 and IPv6, and various consequential changes, including documentation and unit tests. Add a changes file.

I'm not so sure about the final commit, is it too risky for 027?
(But it's what we need to do to avoid confusion over the meaning of accept/reject *)

comment:18 Changed 23 months ago by teor

  • Status changed from new to needs_review

comment:19 Changed 23 months ago by teor

I've added some fixups to branch bug16069-exit-policy-inconsistent on ​https://github.com/teor2345/tor.git

For the fixes in comment 15:

  • made further updates to the manpage, updated the minimal and sample torrcs. (I've updated torrc.minimal.in-staging, do I need to copy it across to torrc.minimal.in?)
  • added an assertion to tor_addr_parse_mask_ports for the new flags.

For the comment fixes:

  • Removed a trailing space in a comment

comment:20 follow-up: Changed 23 months ago by nickm

Design notes:

I think that changing the meaning of accept/reject * to mean IPv4 only is too risky for 0.2.7. I think we could safely have those continue to mean what they mean today.

I think it's fine to do a NOTICE when * means "IPv4 and IPv6".

I think accept6 * should mean "accept *6".

Code notes:

It seems like the TAPMP_IPV[46]_ONLY options won't actually stop any addresses that *don't* begin with a star. That seems wrong. I would expect TAPMP_IPV4_ONLY to reject [FE80::]/16:80, for example.

comment:21 Changed 23 months ago by nickm

  • Status changed from needs_review to needs_revision

comment:22 in reply to: ↑ 20 ; follow-up: Changed 23 months ago by teor

Replying to nickm:

Design notes:

I think that changing the meaning of accept/reject * to mean IPv4 only is too risky for 0.2.7. I think we could safely have those continue to mean what they mean today.

I agree. I just couldn't work out how to fix this issue, and maintain the old behaviour. I like your solution.

I think it's fine to do a NOTICE when * means "IPv4 and IPv6".

I think accept6 * should mean "accept *6".

So the full specification would be:

  • accept/reject * means IPv4 and IPv6 with NOTICE
  • accept/reject IPv4 or *4 means IPv4
  • accept/reject IPv6 or *6 means IPv6
  • accept6/reject6 * means IPv6 only (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv4 or *4 means ignore with WARN? (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv6 or *6 means IPv6 (existing behaviour)


Code notes:

It seems like the TAPMP_IPV[46]_ONLY options won't actually stop any addresses that *don't* begin with a star. That seems wrong. I would expect TAPMP_IPV4_ONLY to reject [FE80::]/16:80, for example.

The TAPMP_IPV[46]_ONLY code only controls what * gets expanded into.

The code in router_parse_addr_policy rejects address family mismatches, regardless of whether they came from a * or a specific address, and regardless of whether they came from a torrc or a descriptor.

comment:23 in reply to: ↑ 22 ; follow-up: Changed 23 months ago by nickm

Replying to teor:

Replying to nickm:

[...]

I think it's fine to do a NOTICE when * means "IPv4 and IPv6".

I think accept6 * should mean "accept *6".

So the full specification would be:

  • accept/reject * means IPv4 and IPv6 with NOTICE
  • accept/reject IPv4 or *4 means IPv4
  • accept/reject IPv6 or *6 means IPv6
  • accept6/reject6 * means IPv6 only (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv4 or *4 means ignore with WARN? (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv6 or *6 means IPv6 (existing behaviour)

Yes, that looks good!

Code notes:

It seems like the TAPMP_IPV[46]_ONLY options won't actually stop any addresses that *don't* begin with a star. That seems wrong. I would expect TAPMP_IPV4_ONLY to reject [FE80::]/16:80, for example.

The TAPMP_IPV[46]_ONLY code only controls what * gets expanded into.

In that case probably the option should be called TAPMP_STAR_IPV[46]_ONLY or something, and the documentation should explain that it only applies to * expansions?

comment:24 in reply to: ↑ 23 Changed 23 months ago by teor

Replying to nickm:

Replying to teor:

Replying to nickm:

[...]

I think it's fine to do a NOTICE when * means "IPv4 and IPv6".

I think accept6 * should mean "accept *6".

So the full specification would be:

  • accept/reject * means IPv4 and IPv6 with NOTICE
  • accept/reject IPv4 or *4 means IPv4
  • accept/reject IPv6 or *6 means IPv6
  • accept6/reject6 * means IPv6 only (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv4 or *4 means ignore with WARN? (changed behaviour, but no-one expected it to mean IPv4)
  • accept6/reject6 IPv6 or *6 means IPv6 (existing behaviour)

Yes, that looks good!

There's one implication that it's worth being aware of:

torrc exit policies will be more lenient than descriptor exit policies:

  • accept/reject * gets expanded into accept/reject *4, accept6/reject6 *6
  • accept/reject IPv6 or *6 gets transformed into accept6/reject6 IPv6 or *6
  • accept6/reject6 * gets transformed into accept6/reject6 *6
  • accept6/reject6 IPv4 or *4 gets ignored

So there may be some confusion if people compare their torrc and exit policies.

But any descriptor policy can be copied into a torrc and it will parse and mean the same thing. (This is a highly desirable property.)

Code notes:

It seems like the TAPMP_IPV[46]_ONLY options won't actually stop any addresses that *don't* begin with a star. That seems wrong. I would expect TAPMP_IPV4_ONLY to reject [FE80::]/16:80, for example.

The TAPMP_IPV[46]_ONLY code only controls what * gets expanded into.

In that case probably the option should be called TAPMP_STAR_IPV[46]_ONLY or something, and the documentation should explain that it only applies to * expansions?

Done! Of course, we'll only be using the IPv6 variant in this patch.

comment:25 Changed 23 months ago by teor

Oops, descriptors don't use *4 and *6.
They just use *.
So that's why we warn on accept/reject *, because it means IPv4 & IPv6 in a torrc, but IPv4 in a descriptor.

comment:26 Changed 23 months ago by teor

Note: we should also modify the behaviour of private for consistency:

  • accept/reject private means both IPv4 and IPv6 addresses
  • accept6/reject6 private means IPv6 addresses only

And you're out of luck if you want to only block IPv4 private addresses.

comment:27 Changed 23 months ago by teor

Actually, that's more complicated than I'm comfortable with.
Let's leave the behaviour of private unchanged (both IPv4 and IPv6), add a warning, and document it.

comment:28 Changed 23 months ago by teor

  • Status changed from needs_revision to needs_review

Please see my branch bug16069-exit-policy-inconsistent-v2.

Commits:

  • Fix comments that are incomplete or incorrect
  • Add warnings / notices and ignore accept6/reject6 IPv4 (with unit test)
  • Make accept6/reject6 produce IPv6 wildcards only (with unit test)

comment:29 follow-up: Changed 23 months ago by nickm

okay, thoughts!

+      log_notice(LD_GENERAL,
+                 "accept/reject * expands into rules which apply to all IPv4 "
+                 "and IPv6 addresses.");

Maybe this should:

  • mention the actual policy that's getting extended?
  • tell the user what to do if they only wanted it to apply to IPv4?
  • not happen once per policy line per hup. :)
  • Give the user some way to avoid the message if they really did mean "all ipv4 and ipv6".
+    if (flags & TAPMP_STAR_IPV4_ONLY) {
+      family = AF_INET;
+      tor_addr_from_ipv4h(addr_out, 0);

This doesn't match the documentation, which says that TAPMP_STAR_IPV4_ONLY has no effect unless TAPMP_EXTENDED_STAR is also set.

-                                              EXIT_POLICY_IPV6_ENABLED |
+                                              ~EXIT_POLICY_IPV6_ENABLED |
                                               EXIT_POLICY_ADD_DEFAULT,0));

Should this bitwise "or" be an "and"?

comment:30 in reply to: ↑ 29 ; follow-up: Changed 23 months ago by teor

Replying to nickm:

okay, thoughts!

+      log_notice(LD_GENERAL,
+                 "accept/reject * expands into rules which apply to all IPv4 "
+                 "and IPv6 addresses.");

Maybe this should:

  • mention the actual policy that's getting extended?

Yes, I'll fix both instances where a similar message is used (torrc parsing and descriptor parsing) so they have the actual policy.

  • tell the user what to do if they only wanted it to apply to IPv4?
  • not happen once per policy line per hup. :)
  • Give the user some way to avoid the message if they really did mean "all ipv4 and ipv6".

Hmm, yes, this is a complex one to fix. I suggest a split solution:

  • downgrading the severity of the every-line-every-hup message to INFO or DEBUG
  • telling the user to use *4 for IPv4 or *6 for IPv6

Then creating another message that is NOTICE that only occurs once per torrc parse when:

  • the policy has an accept *:* or reject *:* line
  • other lines occur after that line (and will be ignored)

We can then create a message telling the user:

  • that lines after accept/reject *:* are being ignored
  • to use *4 for IPv4 or *6 for IPv6 or put accept/reject *:* at the end of the policy to silence this notice

For toralf's torrc and similar torrcs:

  • this patch makes the IPv6 section would now be IPv6 only
  • the accept/reject section would only apply to IPv4 due to ExitPolicy reject6 *:* at the end of the accept6/reject6 section
  • no warnings would be issued, as reject *:* occurs last

If someone goes against the advice to end with accept/reject *:*, and puts accept6/reject6/accept/reject after an accept/reject *:*, then they will get this NOTICE once on every torrc parse.

+    if (flags & TAPMP_STAR_IPV4_ONLY) {
+      family = AF_INET;
+      tor_addr_from_ipv4h(addr_out, 0);

This doesn't match the documentation, which says that TAPMP_STAR_IPV4_ONLY has no effect unless TAPMP_EXTENDED_STAR is also set.

Yes, the conditionals are nested incorrectly here. I'll fix it.

There's no need for it to be active in other contexts, and in a late change, I prefer to limit the scope of new flags.

-                                              EXIT_POLICY_IPV6_ENABLED |
+                                              ~EXIT_POLICY_IPV6_ENABLED |
                                               EXIT_POLICY_ADD_DEFAULT,0));

Should this bitwise "or" be an "and"?

Uh, no, the line with the bitwise "not" should have been deleted entirely.
The bitwise "not" clears the flag, but at the cost of setting every other flag.
I think I'll split this test and do it once with the flag, and once without.

Thanks for picking these up!

comment:31 Changed 23 months ago by teor

Split off #17056 - Do we need ExitPolicy private[4|6]:... ?
Because private means IPv4 and IPv6, even when preceeded by accept6/reject6.

comment:32 in reply to: ↑ 30 Changed 23 months ago by teor

Replying to teor:

Replying to nickm:

okay, thoughts!

+      log_notice(LD_GENERAL,
+                 "accept/reject * expands into rules which apply to all IPv4 "
+                 "and IPv6 addresses.");

Maybe this should:

...

  • tell the user what to do if they only wanted it to apply to IPv4?
  • not happen once per policy line per hup. :)
  • Give the user some way to avoid the message if they really did mean "all ipv4 and ipv6".

Hmm, yes, this is a complex one to fix. I suggest a split solution:

  • downgrading the severity of the every-line-every-hup message to INFO or DEBUG
  • telling the user to use *4 for IPv4 or *6 for IPv6

Then creating another message that is NOTICE that only occurs once per torrc parse when:

  • the policy has an accept *:* or reject *:* line
  • other lines occur after that line (and will be ignored)

We can then create a message telling the user:

  • that lines after accept/reject *:* are being ignored
  • to use *4 for IPv4 or *6 for IPv6 or put accept/reject *:* at the end of the policy to silence this notice

For toralf's torrc and similar torrcs:

  • this patch makes the IPv6 section would now be IPv6 only
  • the accept/reject section would only apply to IPv4 due to ExitPolicy reject6 *:* at the end of the accept6/reject6 section
  • no warnings would be issued, as reject *:* occurs last

If someone goes against the advice to end with accept/reject *:*, and puts accept6/reject6/accept/reject after an accept/reject *:*, then they will get this NOTICE once on every torrc parse.

Now that I think about it, any ExitPolicy lines after accept/reject *:* are almost certainly a misconfiguration. Should we elevate them to WARN?

(Note we won't WARN on policies of the form accept *:N,reject *6:N,accept *:*. I think this is ok, as it's unclear if they are intentional or not, and deciding whether they are or not is non-trivial. We could NOTICE/INFO when rules override each other, but this is often intentional. Maybe we coulod issue a single NOTICE with the resultant policy per torrc parse?)

comment:33 Changed 23 months ago by teor

Please see my branch bug16069-exit-policy-inconsistent-v3.

It fixes the issues identified above, and also correctly ignores just a single policy line on accept6/reject6 IPv4:... (before, it ignored the entire policy). I added a new flag is_ignored to addr_policy_t to skip a single policy, and updated the unit tests for it.

Commits in this branch are:

  • Fix comments that are incomplete or incorrect
  • Add warnings / notices and ignore accept6/reject6 IPv4 (with unit test)
  • Add a warning for redundant ExitPolicy lines after accept/reject *:* and variants
  • Make accept6/reject6 produce IPv6 wildcards only (with unit test)

comment:34 Changed 23 months ago by teor

These unit tests also pass on my Linux box.

comment:35 Changed 23 months ago by teor

Added a fixup commit that frees the policy lists used in the unit tests.

comment:36 Changed 23 months ago by teor

This branch may require manual changes to merge correctly with #17027: this branch adds calls to functions that #17027 adds arguments to.

comment:37 Changed 23 months ago by teor

Ugh, I disliked the way I added int is_ignored:1 to addr_policy_t. It was an ugly design.

Instead, I added int *malformed_list to the arguments of router_parse_addr_policy_item_from_string. It is set to 1 if the entire list should be ignored, and 0 if just the current item should be ignored.

I also found a lurking NULL-dereference bug in my additions to router_parse_addr_policy_item_from_string and fixed it.

See my branch bug16069-exit-policy-inconsistent-v4.

comment:38 Changed 23 months ago by teor

Pushed a fixup for the unit test in later commits, they needed the extra argument to router_parse_addr_policy_item_from_string.

Unit tests pass on OS X and Linux.

comment:39 follow-up: Changed 23 months ago by nickm

Better and better.

Only two questions here:

  • Are you completely sure this doesn't affect how we parse descriptors at all?
  • How verbose are these logs going to be?

comment:40 in reply to: ↑ 39 Changed 23 months ago by teor

Replying to nickm:

Better and better.

Only two questions here:

  • Are you completely sure this doesn't affect how we parse descriptors at all?

Descriptors are parsed by router_add_exit_policy:

  • The only modifications to this function itself are comments and a log message. (The log message was ambiguous, I have pushed a fixup.)
  • router_add_exit_policy calls router_parse_addr_policy(tok, 0), whereas the torrc parser router_parse_addr_policy_item_from_string calls router_parse_addr_policy(tok, TAPMP_EXTENDED_STAR).
  • All my changes are conditional on TAPMP_EXTENDED_STAR being set, so they don't affect the router_add_exit_policy codepath. I have pushed a fixup to ensure that we don't even set any extra flags unless TAPMP_EXTENDED_STAR is set.

Routersets are parsed by routerset_parse:

  • routerset_parse has been modified to use malformed_list. The code used to reject the entire list on errors, and that behaviour has been preserved.
  • I've added code that ignores the line if malformed_list is 0. It's never executed in the current implementation, because routerset_parse never uses accept6/reject6, and therefore can't get accept6/reject6 IPv4. (But it's there in case we decide to ignore other types of policy lines in future.)
  • How verbose are these logs going to be?

For serious misconfiguration errors where the operator isn't getting the exit policy they expect:

  • One warning-level message per HUP if there are any redundant torrc lines after an accept/reject *:* line (or equivalent). The message identifies the first redundant line. (We advise operators to finish exit policies with accept/reject *:* as the final entry, and no warning is issued in this case, as there are no redundant lines.)
  • One warning-level message per HUP per accept6/reject6 line with an IPv4 address (or *4). We ignore these lines, so we let the operator know they're not getting what they expect. (Should we only warn on the first of these?)
  • One warning-level message per HUP per accept6/reject6 line with a private alias. We include both IPv4 and IPv6 addresses when processing these lines, so we let the operator know they're not getting what they expect. (Should we only warn on the first of these?)

For potentislly unexpected policy outcomes:

  • One info-level message per accept/reject *:<port> line per HUP. There are typically many of these lines per exit policy. (Should this be at debug level? Should we only log on the first of these?)

comment:41 Changed 23 months ago by teor

Pushed a fixup that eliminates the final ~EXIT_POLICY_IPV6_ENABLED (there were two) and reverts the corresponding test result change in bug16069-exit-policy-inconsistent-v4.

Please use my bug16069-bug17027 branch if you want to merge both #16069 and #17027. It resolves some merge conflicts and function argument changes, and includes the above fixup.

comment:42 Changed 23 months ago by teor

  • Resolution set to fixed
  • Status changed from needs_review to closed

Merged

comment:43 Changed 22 months ago by teor

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopened for possible backport to 026

comment:44 Changed 22 months ago by teor

These sorts of issues could be avoided if operators put their IPv6 policy first with accept6/reject6, and end it with accept6/reject6 *:*, then their IPv4 policy with accept/reject, then end with accept/reject *:*. Or, alternately, to have a single IPV4 and IPv6 policy, with accept/reject, ending in accept/reject *:*.

We could update the manual with this advice.

nickm, what do you think?

comment:45 Changed 22 months ago by nickm

I'd be glad to take a manual patch!

comment:46 Changed 22 months ago by teor

Please see my branch ipv6-exitpolicy-docs.
I left the docs a little broken after #16069 and #17027, with some formatting issues and a bad merge of the #17027 manpage patch. This branch also fixes these issues.

comment:47 Changed 22 months ago by teor

  • Status changed from reopened to needs_review

comment:48 Changed 22 months ago by nickm

  • Resolution set to fixed
  • Status changed from needs_review to closed

merged!

comment:49 Changed 15 months ago by nickm

  • Keywords 2016-bug-retrospective added

Marking for bug retrospective based on Priority.

comment:50 Changed 15 months ago by nickm

Mark more tickets for bug retrospective based on hand-review of changelogs from 0.2.5 onwards.

Note: See TracTickets for help on using tickets.