#25423: Treat 'ExitRelay 0' as a reject-all policy ---------------------------+-------------------------- Reporter: atagar | Owner: dmr Type: defect | Status: assigned Priority: Medium | Milestone: Component: Core Tor/Stem | Version: Severity: Normal | Resolution: Keywords: | Actual Points: Parent ID: | Points: Reviewer: | Sponsor: ---------------------------+--------------------------
Comment (by dmr): I've pushed a few commits for the issue here: `g...@github.com:dmr-x/stem.git` branch: `25423-get-exit-policy` revrange: `2018f3fd2642f43aeab8e1e0d2142a6a4905e651..0f797ee5de4a7e25d307203d65a044ca1b9e9534` web link: https://github.com/dmr-x/stem/tree/25423-get-exit-policy These commits should address the issue almost entirely. However, in prepping these to submit, I saw that atagar recently addressed #25739. The changes are very similar (cool!) but overlap imperfectly (if they were identical, I'd be a bit freaked out!). I rebased my changes to //right before// the commits for #25739, but I'd need to spend some time to merge the changes together. (I can do that - it'll just likely not be for a week or so.) I'd like to provide some info in this Trac ticket. Namely: I'd like to go over the changes I pushed, as well as some rationale and what I think might remain with the issue after changes are merged. For reference, I tested against `Tor version 0.3.2.10 (git- 0edaa32732ec8930).` ==== 1. Failure for `GETINFO exit-policy/full` to return info. I noticed in my testing that `GETINFO exit-policy/full` does not always return info. In that case, I've always seen this: {{{ >>> GETINFO exit-policy/full 551 router_get_my_routerinfo returned NULL }}} I've noticed two circumstances in which the exit-policy is unqueryable: * tor is configured as a relay, and it //has not yet learned its address// (i.e. `GETINFO address` also fails) * tor is configured as a client The former is transient and doesn't typically last long, but //could// for instance last a while if the relay doesn't have network access. The latter is persistent, i.e. the client-only configuration ALWAYS returns this: {{{ >>> GETINFO exit-policy/full 551 router_get_my_routerinfo returned NULL }}} `Controller.get_exit_policy()` has the `default` parameter. Without specifying a default, `get_exit_policy()` will raise an exception in these cases. So `Controller.get_exit_policy(None)` is handy to avoid an exception, as I've seen `nyx` and other parts of `stem` do. But since `get_exit_policy()` previously would ~always return something, and is now changing to a form that is less stable - and for clients always fails - consumers of this updated API have to be weary. As mentioned, I checked `nyx`'s code - luckily it always uses `default=None`, so that shouldn't cause any problems. Nonetheless, we can't be sure who all is using `stem` - maybe we need to note this possibility in the changelog? Or maybe we should change the functionality, so that consumers of `controller.get_exit_policy()` don't have this problem / have to wait? We could potentially do a `get_conf('ORPort')` and return `None` proactively if tor is running as a client (i.e. we don't have an `ORPort` configured). ==== 2. Potential tor bug: waiting for address in non-exit relays Of particular note: The behavior of returning NULL / waiting for knowing an address happens even in non-exit relays, i.e. even when the exit policy should be fully known as `reject *:*` beforehand by the config alone. atagar: Perhaps we should file this as a bug in tor? ==== 3. Fixed cache-invalidation bugs Two commits I pushed are related to existing cache-invalidation bugs, but I ran into them when testing for cache invalidation related to the changes. The crux was: `exitpolicy` instead of `exit_policy` cache key, and `exitpolicy` compared with (non-`lower()`ed) `ExitPolicy`. Since there weren't any tests (unit or integ) for the cache invalidation here, I added regression tests. I ran into a further bug related to cache invalidation and still have it to file, but it's largely orthogonal, and it's not as straightforward of a fix, so I didn't just go fix it. ==== 4. Multiple configuration changes could cause our cache to be invalid As alluded to above, I had to edit the cache invalidation anyway for this change. All of these torrc options, if changed, could invalidate our cache: (code snippet) {{{ CONFIG_OPTIONS_AFFECTING_EXIT_POLICY = ( 'ExitRelay', 'ExitPolicy', 'ExitPolicyRejectPrivate', 'ExitPolicyRejectLocalInterfaces', 'IPv6Exit', ) }}} See the corresponding commit. ==== 5. Additional event that can invalidate our cache If our relay's address changes, then our ExitPolicy may change (due to the default rule of rejecting its own address). In looking at the [[https://gitweb.torproject.org/torspec.git/tree /control- spec.txt?id=d4a64fbf5aaba383638d9f3c70bd2951f8c5ad89#n2539|control spec]], it looks like this can be done by registering for a `StatusEvent` and handling `status_type==StatusType.SERVER` with action `EXTERNAL_ADDRESS`. Changing address is a bit unlikely to happen, so I'll file that as a separate ticket. But it is an outstanding "bug", if you will. I didn't have time to get to addressing it yet. (Pun not //originally// intended, but now I kinda like it.) ==== 6. Further potential to change exit policy In reviewing the tor `man` page, I saw this torrc option: {{{ ServerDNSTestAddresses hostname,hostname,... When we're detecting DNS hijacking, make sure that these valid addresses aren't getting redirected. If they are, then our DNS is completely useless, and we'll reset our exit policy to "reject *:*". This option only affects name lookups that your server does on behalf of clients. (Default: "www.google.com, www.mit.edu, www.yahoo.com, www.slashdot.org") }}} To note here is that `tor` may change our exit policy underneath us. While again this should be rare, I don't know if there's anything stem can listen to, in order to have a correct exit policy if this happens. I looked at bit at the [[https://gitweb.torproject.org/torspec.git/tree /control- spec.txt?id=d4a64fbf5aaba383638d9f3c70bd2951f8c5ad89#n2626|control spec]] - perhaps `tor` would emit this event: `SERVER_STATUS` / `DNS_USELESS` action? If it isn't... perhaps we need another event in the control spec for any time tor's exit policy changes? (That would also cover the address- changing case) ==== 7. Test function `compare_exit_policy()` is probably unnecessary (kept in) I cannot think of a configuration case where we shouldn't be able to predict whether the exit policy has the relay's address in it, or not. I left this in the test code (refactored into a function) because I was being conservative, but I wanted to call it out since it may now be unnecessary. It could be a remnant from the previous behavior that built the exit policy based on `get_info('address', None)` and other pieces of data, so if tor didn't know its address stem wouldn't add that into the exit policy. atagar, do you think we can remove this? ==== 8. Probably unnecessary use of `with self._msg_lock` (removed) I spent a deal of time working through why the `with self._msg_lock:` line existed in `get_exit_policy()` and believe that it was because the old implementation sent multiple messages and that we'd ideally want them to represent as atomic of a state as possible, so we'd lock-out others from sending messages during that time. In the new implementation, we only send a GETINFO message and block on its response. I don't think we need the line for the lock anymore, since that comes as part of `msg()`. I removed it in my commits, but atagar I see you still have it in the commits you pushed for #25739 - I think it's probably unnecessary but it would be great to get your opinion on this, too. ==== 9. Potentially deprecate `ExitPolicy.get_config_policy()` (no change made) In `stem`, `Controller.get_exit_policy()` was the only consumer of `exit_policy.get_config_policy()` (and now there isn't any). Perhaps we should deprecate the latter? `nyx` doesn't use it either. I'm not sure an external consumer of our API would need/want to use this. -- Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25423#comment:6> Tor Bug Tracker & Wiki <https://trac.torproject.org/> The Tor Project: anonymity online
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs