Trac issueshttps://gitlab.torproject.org/legacy/trac/-/issues2020-06-13T17:27:21Zhttps://gitlab.torproject.org/legacy/trac/-/issues/26483Static check results in "local variable 'exc' is assigned to but never used"2020-06-13T17:27:21ZdmrStatic check results in "local variable 'exc' is assigned to but never used"[[updated to 2.0.0](https://github.com/PyCQA/pyflakes/commit/efddb93524db7949322181d5e14c32681779a6e8|pyflakes)] now additionally "[checks] for unused exception binding in `except:` block", causing the following lint errors:
```
STATIC C...[[updated to 2.0.0](https://github.com/PyCQA/pyflakes/commit/efddb93524db7949322181d5e14c32681779a6e8|pyflakes)] now additionally "[checks] for unused exception binding in `except:` block", causing the following lint errors:
```
STATIC CHECKS
* /path/to/stem/descriptor/reader.py
line 515 - local variable 'exc' is assigned to but never used | except TypeError as exc:
* /path/to/stem/util/system.py
line 1327 - local variable 'exc' is assigned to but never used | except CallTimeoutError as exc:
* /path/to/stem/interpreter/__init__.py
line 148 - local variable 'exc' is assigned to but never used | except (KeyboardInterrupt, stem.SocketClosed) as exc:
line 172 - local variable 'exc' is assigned to but never used | except stem.SocketClosed as exc:
line 184 - local variable 'exc' is assigned to but never used | except (KeyboardInterrupt, EOFError, stem.SocketClosed) as exc:
```
line numbers as of [[https://gitweb.torproject.org/stem.git/tree/stem/?id=3818cf41cae98ae7558f5002ef3a5152ede5b2fb|3818cf41cae98ae7558f5002ef3a5152ede5b2fb]]
Strangely, those only appear for py35 runs - I didn't see them in my py27 runs. Not sure why that is.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26228Clarify/determine specification for padding bytes, (formerly also PADDING cell)2020-06-13T15:28:22ZdmrClarify/determine specification for padding bytes, (formerly also PADDING cell)**EDIT: strikethrough content below is now covered in #26870 instead**
==== Background
I was trying to interpret the tor-spec for padding bytes, and ending up asking nickm for some clarification over IRC.
nickm suggested most of the cc'...**EDIT: strikethrough content below is now covered in #26870 instead**
==== Background
I was trying to interpret the tor-spec for padding bytes, and ending up asking nickm for some clarification over IRC.
nickm suggested most of the cc'd for the ticket - I added atagar, too.
==== Unclear areas
Here are the points that need clarification / specification:
* spec for padding bytes does not clearly say what senders `MUST` or `SHOULD` do, [[mentioning that padding is with 0 bytes](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n412|only)] or [[NUL bytes](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1480|(elsewhere))]
* spec for padding bytes does not say what receivers `MUST` or `SHOULD` do, when receiving non-zero bytes in the Cell (e.g. warn? ignore?)
* ~~spec is a bit inconsistent with `PADDING` cells ^^[1^^]^^[2^^]~~
==== Discussion: padding bytes
For the padding bytes that are not part of `PADDING` cells, nickm offered the following as a non-exhaustive set of possible forward-compatible options:
* "the [padding] bytes SHOULD be zero, and that implementations MUST ignore them"
* "The first 8 padding bytes MUST be zero; all subsequent padding bytes SHOULD be randomized. Implementations MUST ignore padding bytes"
* "All padding bytes should be randomized; implemenations MUST ignore unrecognized padding bytes"
... and mentioned that "[he doesn't] know enough of the argument in favor of randomization to have a very strong preference"
==== ~~Inconsistency: `PADDING` cell payload~~
~~(see bullet above)~~
~~These references highlight the inconsistency:~~
~~^^[1^^] `PADDING: Payload is unused.` per [[3 "Cell Packet format"](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n469|sec)].~~
~~ implies 0 bytes of payload, so the rest should be padded per that section~~
~~^^[2^^] `The contents of a PADDING, VPADDING, or DROP cell SHOULD be chosen randomly, and MUST be ignored.` per [[7.2 "Link padding"](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1723|sec)].~~
~~ implies the payload of a `PADDING` cell actually is the rest of the size of the cell, and that it SHOULD be chosen randomly~~
~~The `PADDING` cells were mentioned in IRC but not discussed.~~
~~I think a simple change to make the spec consistent between the two sections would be this:~~
~~`PADDING: Payload contains random data. (See Sec 7.2)`~~
~~However, given the other points here, is that correct?~~Tor: 0.3.5.x-finaldmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26870Spec: clarify inconsistency for [V]PADDING/DROP cell content vs. padding bytes2020-06-13T15:28:22ZdmrSpec: clarify inconsistency for [V]PADDING/DROP cell content vs. padding bytesThis ticket is split off from #26228.
Here's the relevant description from that ticket:
Replying to [dmr](#26228):
> [...]
> ==== Unclear areas
> Here are the points that need clarification / specification:
> * [...]
> * spec is a bit i...This ticket is split off from #26228.
Here's the relevant description from that ticket:
Replying to [dmr](#26228):
> [...]
> ==== Unclear areas
> Here are the points that need clarification / specification:
> * [...]
> * spec is a bit inconsistent with `PADDING` cells ^^[1^^]^^[2^^]
> [...]
>
> ==== Inconsistency: `PADDING` cell payload
> (see bullet above)
>
> These references highlight the inconsistency:
>
> ^^[1^^] `PADDING: Payload is unused.` per [[3 "Cell Packet format"](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n469|sec)].
> implies 0 bytes of payload, so the rest should be padded per that section
> ^^[2^^] `The contents of a PADDING, VPADDING, or DROP cell SHOULD be chosen randomly, and MUST be ignored.` per [[7.2 "Link padding"](https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt?id=f6e93d9751002d970614662c8173ff2fa5b7c193#n1723|sec)].
> implies the payload of a `PADDING` cell actually is the rest of the size of the cell, and that it SHOULD be chosen randomly
>
> The `PADDING` cells were mentioned in IRC but not discussed.
> I think a simple change to make the spec consistent between the two sections would be this:
> {{{
> PADDING: Payload contains random data. (See Sec 7.2)
> }}}
>
> However, given the other points here, is that correct?Tor: 0.3.5.x-finaldmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26860Spec: decryption order appears to be wrongly specified2020-06-13T15:28:19ZdmrSpec: decryption order appears to be wrongly specifiedcatalyst noted in `#tor-dev` that the decryption algorithm in 5.5:
* `won't work unless decryption is commutative (which it is for plain AES-CTR, but not for the proposal)`
* `doesn't look like it'll work to handle a relay cell arriving ...catalyst noted in `#tor-dev` that the decryption algorithm in 5.5:
* `won't work unless decryption is commutative (which it is for plain AES-CTR, but not for the proposal)`
* `doesn't look like it'll work to handle a relay cell arriving at the OP that originates from an intermediate node`
I had also noted the latter, so I postulated `is the order of that line backwards? that is... should it be "For I=1...N?"` and asked `what does little-t tor do?`
And asn noted:
> in relay_decrypt_cell() it does:
> {{{
> if (CIRCUIT_IS_ORIGIN(circ)) { /* We're at the beginning of the circuit.
> * We'll want to do layered decrypts. */
> crypt_path_t *thishop, *cpath = TO_ORIGIN_CIRCUIT(circ)->cpath;
> thishop = cpath;
> }}}
> i think this is I=1...N like you say
> (for 1 being the hop closest to the OP)
So it looks like this is an error in the spec, rather than a problem with `tor`.Tor: 0.3.5.x-finaldmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/25423Treat 'ExitRelay 0' as a reject-all policy2020-06-13T15:24:31ZDamian JohnsonTreat 'ExitRelay 0' as a reject-all policyOops! Most folks use 'ExitPolicy reject *:*' to flag their relay as being a non-exit, but turns out that tor has another option that does the same thing...
https://www.torproject.org/docs/tor-manual.html.en#ExitRelay
We should change t...Oops! Most folks use 'ExitPolicy reject *:*' to flag their relay as being a non-exit, but turns out that tor has another option that does the same thing...
https://www.torproject.org/docs/tor-manual.html.en#ExitRelay
We should change the Controller's get_exit_policy() method to provide a reject-all policy when this is set. Caught thanks to Gary on tor-relays...
https://lists.torproject.org/pipermail/tor-relays/2018-March/014741.htmldmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26226Plan/discuss stem.client architecture/design2020-06-13T07:45:49ZdmrPlan/discuss stem.client architecture/designIdeally stem.client will be as easily and broadly usable by the Tor and Python communities as possible.
Plan out some ideas and discuss.Ideally stem.client will be as easily and broadly usable by the Tor and Python communities as possible.
Plan out some ideas and discuss.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/25631Integration tests set up and publish descriptors for an exit relay2020-06-13T07:33:23ZdmrIntegration tests set up and publish descriptors for an exit relaySome support for talking with Tor's ORPort has recently made it to stem (see #18856).
Integration tests thus needed to set up a relay with ORPort in the torrc. 3fe2221ae2bad6d826c05fd0ce96e5c09a21fc01 did this, but the defaults for a re...Some support for talking with Tor's ORPort has recently made it to stem (see #18856).
Integration tests thus needed to set up a relay with ORPort in the torrc. 3fe2221ae2bad6d826c05fd0ce96e5c09a21fc01 did this, but the defaults for a relay are to publish descriptors and act as an exit. Both of these are undesirable for stem's integration tests.
The corresponding torrc options to disable these are:
* `ExitRelay 0`
* `PublishServerDescriptor 0`dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26431Document a threat model for stem.client2018-09-28T18:28:11ZdmrDocument a threat model for stem.clientIt would be beneficial to document the threat model that `stem.client` is trying to meet (and thereby, probably some of the use cases envisioned for `stem.client`).
From a network-fingerprint sense, it is unlikely that `stem.client` cou...It would be beneficial to document the threat model that `stem.client` is trying to meet (and thereby, probably some of the use cases envisioned for `stem.client`).
From a network-fingerprint sense, it is unlikely that `stem.client` could ever match the fingerprint that little-t `tor` does, since `stem.client` is a pure-Python implementation. Some side-channel behavior in particular is likely to be extremely difficult to align, and different Python implementations would make this even harder.
But how close should `stem.client` come, how closely should it track to `tor` development, and what should it take into account?
Some of this discussion //may// ripple into updating the [[https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt|tor-spec]] with some `SHOULD` statements.
In general, it's important to document the threat model so that consumers of `stem.client` can know what to expect, and whether they should use `tor` in a controlled fashion instead.
This threat model should become a living document that is maintained.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/27112Decouple payload processing from pop/unpack + tune abstraction layers2018-09-28T18:27:39ZdmrDecouple payload processing from pop/unpack + tune abstraction layersA few goals here:
- allow RELAY cells to be unpacked / popped without error, and passed to another level for decryption
- put the encryption/decryption logic in cell.py, but require the crypto state be managed in Connection/Circuit/Strea...A few goals here:
- allow RELAY cells to be unpacked / popped without error, and passed to another level for decryption
- put the encryption/decryption logic in cell.py, but require the crypto state be managed in Connection/Circuit/Stream layersdmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26227Review existing stem.client code2018-08-22T03:22:50ZdmrReview existing stem.client codeThe scope of this code review includes literally the code in `stem.client`, but also any supporting code or consumers of it, such as `stem.descriptor.remote`.The scope of this code review includes literally the code in `stem.client`, but also any supporting code or consumers of it, such as `stem.descriptor.remote`.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26420Testing - specify literal patterns instead of regex patterns2018-08-06T01:51:01ZdmrTesting - specify literal patterns instead of regex patterns~~w.r.t. [[https://gitweb.torproject.org/stem.git/commit/?id=7711050619af1a2f8ecf4aa87f774baa5f367b3b|7711050619af1a2f8ecf4aa87f774baa5f367b3b]], I was planning to file this ticket anyways, so might as well now for the discussion.~~
ata...~~w.r.t. [[https://gitweb.torproject.org/stem.git/commit/?id=7711050619af1a2f8ecf4aa87f774baa5f367b3b|7711050619af1a2f8ecf4aa87f774baa5f367b3b]], I was planning to file this ticket anyways, so might as well now for the discussion.~~
atagar linked [[StackOverflow answer](https://stackoverflow.com/questions/8672754/how-to-show-the-error-messages-caught-by-assertraises-in-unittest-in-python2-7/8673096#8673096|this)] in the commit message.
(I was a bit behind on filing this ticket, but already started doing the literal `re.escape()` bit in my test cases. Hence atagar's comment in the commit.)
Anyway, here's the ticket text I had started to prep - now //slightly// tweaked:
===
The testing codebase makes pretty extensive use of `unittest.TestCase.assertRaisesRegexp()`.
An example is [[https://gitweb.torproject.org/stem.git/tree/test/integ/client/connection.py?id=0192b29a4784465e5f69f11ced584a54644e4a90#n36|here]]:
```
def test_no_common_link_protocol(self):
"""
Connection without a commonly accepted link protocol version.
"""
for link_protocol in (1, 2, 6, 20):
self.assertRaisesRegexp(stem.SocketError, 'Unable to establish a common link protocol with 127.0.0.1:1113', Relay.connect, '127.0.0.1', test.runner.ORPORT, [link_protocol])
```
The second argument is treated as a regex pattern, so it will actually match more than intended - possibly leading to some false negatives (although unlikely in this example).
The use of `unittest.TestCase.assertRaisesRegexp()` without `re.escape()` for a literal expression is decently common - the use of it intended for a regex is fairly rare (I haven't come across a test yet that I can recall).
Having a "literal" check is possible in (at least) two ways:
```
with self.assertRaises(SomeException) as cm:
do_something(some_param)
self.assertEqual(str(cm.exception), expected_literal)
```
```
self.assertRaisesRegexp(SomeException, '^%s$' % re.escape(expected_literal), do_something, some_param)
```
Importantly, both of these check for //exact// string.
Much of the codebase doesn't use `re.escape()`, and where it does, I didn't see any `^` or `$` (although I didn't search exhaustively), so that could match on substrings, also allowing for subtle bugs.
So we may consider a helper class `StemTestCase(unittest.TestCase)` which adds an `assertRaisesLiteral()` method, to make it easier to do this. (Or some other means of adding that in.)
We could of course take the second option with `re.escape()`, but since a lot of the codebase already doesn't seem to do that, it's probably quite easy to forget, especially the `'^%s$' % ` part.
~~atagar: thoughts on these options? or leave things as-is / `wontfix`?~~
~~(Filing this as a //task// ticket, as it's probably a discussion point more than anything else. I'd expect from the edge cases, there //could// be some defects, some enhancements.)~~
See especially [[comment:2|atagar's comment about implementation thoughts]].dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26916Make tox config more useful/friendly for running multiple interpreters/versions2018-07-24T19:33:31ZdmrMake tox config more useful/friendly for running multiple interpreters/versionsRight now `tox` can be a bit of a pain for dev usage.
This ticket is intended to remove some of the hassle for multiple interpreters/versions, and make the default environments config a bit more useful.
I've identified a few improvement...Right now `tox` can be a bit of a pain for dev usage.
This ticket is intended to remove some of the hassle for multiple interpreters/versions, and make the default environments config a bit more useful.
I've identified a few improvements under this theme - these are better described in commits.
**atagar: I've got a PR coming soon**dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26811tox fails with pip 102018-07-16T21:51:52Zdmrtox fails with pip 10Trying to run `tox` in a fresh `stem` workspace (no `.tox` dir, which contains the virtualenvs) now fails.
Failure output with pip 10:
```
no such option: --allow-all-external
ERROR: InvocationError: 'path/to/pip install --allow-all-ext...Trying to run `tox` in a fresh `stem` workspace (no `.tox` dir, which contains the virtualenvs) now fails.
Failure output with pip 10:
```
no such option: --allow-all-external
ERROR: InvocationError: 'path/to/pip install --allow-all-external -e .'
```
Here's a deprecation note from a tox / pip 9 virtualenv:
```
py27 runtests: commands[0] | pip install --allow-all-external -e .
DEPRECATION: --allow-all-external has been deprecated and will be removed in the future. Due to changes in the repository protocol, it no longer has any effect.
```
With pip 10 (somewhat recently released), this `--allow-all-external` flag seemingly got removed.
**atagar: I'll have a patch for this shortly! **dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26766Cell unused content is ignored while packing2018-07-12T18:53:46ZdmrCell unused content is ignored while packingThe Cell `unused` attribute is populated when unpacking content, with any content that wasn't assigned to another attribute per the cell structure.
However, it currently isn't used for anything else^^*. It would make the most sense to a...The Cell `unused` attribute is populated when unpacking content, with any content that wasn't assigned to another attribute per the cell structure.
However, it currently isn't used for anything else^^*. It would make the most sense to allow such a Cell to be repacked to binary identicality, changing `_pack()` to allow this.
IRC log of discussion with atagar:
```
18:52 < dmr> atagar: (2) should `unused` be used in `_pack()`, prior to additional payload padding? I think it should, to allow repacking into something with the same binary value.
[...]
18:55 <+atagar> (2) agreed, it should
18:56 <+atagar> [...] Would you mind sending a patch for (2)?
[...]
19:08 <+atagar> dmr: Oh btw, if you could send a unit test along with (2) I'd appreciate it. Our tests should check that re-packing cells produces the same bytes we read.
[...]
19:27 < dmr> atagar: sounds good, and for (2) I was planning that very test set!
```
This ticket tracks the implementation change and the test changes. **I've got a pull request coming soon! **
^^* 'twas not intended, but I got quite a kick out of that. It's almost... well... unused. (Sigh.)dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26684NetinfoCell doesn't preserve unused content during unpacking2018-07-07T18:29:22ZdmrNetinfoCell doesn't preserve unused content during unpacking`NetinfoCell` currently doesn't unpack the `content` remainder (after addresses) correctly into the `unused` field, instead always populating it with `b''`.`NetinfoCell` currently doesn't unpack the `content` remainder (after addresses) correctly into the `unused` field, instead always populating it with `b''`.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26225Jenkins test failure: test_get_microdescriptors - test may be unreliable2018-06-24T23:10:36ZdmrJenkins test failure: test_get_microdescriptors - test may be unreliableA [[Jenkins test run (`Build #3443 (May 25, 2018 9:40:56 PM) `)](https://jenkins.torproject.org/job/stem-tor-ci/3443/console|recent)] indicates that our `test_get_microdescriptors` test may be a bit unreliable:
```
======================...A [[Jenkins test run (`Build #3443 (May 25, 2018 9:40:56 PM) `)](https://jenkins.torproject.org/job/stem-tor-ci/3443/console|recent)] indicates that our `test_get_microdescriptors` test may be a bit unreliable:
```
======================================================================
21:43:42 ERROR: test_get_microdescriptors
21:43:42 ----------------------------------------------------------------------
21:43:42 Traceback (most recent call last):
21:43:42 File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/require.py", line 58, in wrapped
21:43:42 return func(self, *args, **kwargs)
21:43:42 File "/srv/jenkins-workspace/workspace/stem-tor-ci/test/integ/control/controller.py", line 1164, in test_get_microdescriptors
21:43:42 for desc in controller.get_microdescriptors():
21:43:42 File "/srv/jenkins-workspace/workspace/stem-tor-ci/stem/control.py", line 488, in wrapped
21:43:42 for val in func(self, *args, **kwargs):
21:43:42 File "/srv/jenkins-workspace/workspace/stem-tor-ci/stem/control.py", line 1773, in get_microdescriptors
21:43:42 raise stem.OperationFailed(message = "Data directory doens't contain cached microescriptors (%s)" % cached_descriptor_path)
21:43:42 OperationFailed: Data directory doens't contain cached microescriptors (/srv/jenkins-workspace/workspace/stem-tor-ci/test/data/cached-microdescs)
```
Later test runs have passed, and nothing major has changed in the code.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/26421Include info about the --quiet / -q option in --help2018-06-24T21:52:16ZdmrInclude info about the --quiet / -q option in --helpI was originally going to file a ticket to //implement// the `--quiet` option, but in looking at the code, saw that it already exists.
It doesn't get displayed when we do `run_tests.py --help`, so make it do that.I was originally going to file a ticket to //implement// the `--quiet` option, but in looking at the code, saw that it already exists.
It doesn't get displayed when we do `run_tests.py --help`, so make it do that.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/25821Stem getconf cache doesn't clear for CONF_CHANGED events; probably should set...2018-05-21T16:32:02ZdmrStem getconf cache doesn't clear for CONF_CHANGED events; probably should set valueAs mentioned in #25423, I found a cache-invalidation bug in `stem.control.Controller`.
The tl;dr is that the `getconf` cache currently doesn't get cleared properly by the listener that intends to clear it, but //clearing// probably isn'...As mentioned in #25423, I found a cache-invalidation bug in `stem.control.Controller`.
The tl;dr is that the `getconf` cache currently doesn't get cleared properly by the listener that intends to clear it, but //clearing// probably isn't what we actually want to do.dmrdmrhttps://gitlab.torproject.org/legacy/trac/-/issues/25757Allow offline integ tests to be run in a no-network/firewalled state2018-04-10T16:24:35ZdmrAllow offline integ tests to be run in a no-network/firewalled stateStem's integration tests are run by e.g. `run_tests.py --all` or `tox`, and by default do not include the `ONLINE` target. (See `run_tests.py --help` for more info on running `ONLINE` tests.)
These remaining integration tests should be ...Stem's integration tests are run by e.g. `run_tests.py --all` or `tox`, and by default do not include the `ONLINE` target. (See `run_tests.py --help` for more info on running `ONLINE` tests.)
These remaining integration tests should be runnable offline; however, it appears that they require the `tor` process to bootstrap to 5% in order to run.
Waiting for 5% was introduced in commit `fbfa73a099d9645f18d9846420cbf0145065b11d` from 2011.
Empirically, I switched this to 0% and all these integ tests still worked fine.
In chatting with atagar over IRC, he wasn't aware of a reason offhand for waiting for 5%, and suggested we could change this since the tests still pass.dmrdmr