The Tor Project issueshttps://gitlab.torproject.org/groups/tpo/-/issues2022-02-25T13:32:57Zhttps://gitlab.torproject.org/tpo/core/arti/-/issues/318Tests that run Arti shouldn't use default configuration or directories2022-02-25T13:32:57ZNick MathewsonTests that run Arti shouldn't use default configuration or directoriesSome of our tests work by running Arti as an application. We need to make sure that all of these tests override the default configuration path and storage directories used by `arti`.
To override the default configuration path, use a di...Some of our tests work by running Arti as an application. We need to make sure that all of these tests override the default configuration path and storage directories used by `arti`.
To override the default configuration path, use a different configuration file by passing "-c [path/to/file.toml]" on the command line. We should override the default configuration path because the developer might have some settings in their own `arti.toml` file that would interfere with the operation of the test.
To override the default directories, just make sure that we specify at least one configuration file that contains a non-default `[storage]` section. We should override the default storage paths because we don't want to contaminate the developer's own cache and state path with (say) all the relays from a chutney network.Arti 0.1.0 release: Okay for experimental embeddingNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/arti/-/issues/310nightly fails in CI because of clippy::needless_borrow2022-02-23T13:15:15ZIan Jacksoniwj@torproject.orgnightly fails in CI because of clippy::needless_borrowFor example,
```
warning: this expression borrows a value the compiler would automatically borrow
--> crates/tor-persist/src/fs.rs:152:36
|
152 | std::fs::write(&fname_tmp, (&output).as_bytes())?;
| ...For example,
```
warning: this expression borrows a value the compiler would automatically borrow
--> crates/tor-persist/src/fs.rs:152:36
|
152 | std::fs::write(&fname_tmp, (&output).as_bytes())?;
| ^^^^^^^^^ help: change this to: `output`
|
```
```
warning: this expression borrows a value the compiler would automatically borrow
--> crates/tor-llcrypto/src/pk/ed25519.rs:78:9
|
78 | (&pk).into()
| ^^^^^ help: change this to: `pk`
|
```
Some of these are correct. For example, it seems to me that the first case could be just `write(&.., &output)`.
Others are not; for example, taking the compiler's suggestion for the second one results in a compile error. This seems to be a bug in nightly, which I have just filed: https://github.com/rust-lang/rust/issues/93509Arti 0.1.0 release: Okay for experimental embeddingIan Jacksoniwj@torproject.orgIan Jacksoniwj@torproject.orghttps://gitlab.torproject.org/tpo/core/arti/-/issues/309maint and tests scripts should not have implementation language in filename2022-02-18T16:12:41ZIan Jacksoniwj@torproject.orgmaint and tests scripts should not have implementation language in filenameIMO putting `.sh` or `.py` or whatever on the end of the filename for an executable script is an antipattern.
Putting the language a script is written in, into the filename, means that if you later decide that it ought to be rewritten i...IMO putting `.sh` or `.py` or whatever on the end of the filename for an executable script is an antipattern.
Putting the language a script is written in, into the filename, means that if you later decide that it ought to be rewritten in Python or whatever, you have to edit all call sites, trip up humans' finger macros, etc. Many projects end up this way with scripts called `yada.sh` that are written in something else.
The language a script is written in can be discovered from the `#!`.
```
zealot:arti> git-ls-files | grep '\.py$' | xargs ls -l | grep rwx
-rwxrwxr-x 1 ian ian 3120 Jan 21 11:58 maint/add_warning.py
-rwxrwxr-x 1 ian ian 2534 Jan 5 10:12 maint/check_toposort.py
-rwxrwxr-x 1 ian ian 582 Jan 5 10:12 maint/list_crates.py
-rwxrwxr-x 1 ian ian 3298 Jan 5 10:12 maint/postprocess_coverage.py
zealot:arti> git-ls-files | grep '\.sh$' | xargs ls -l | grep rwx
-rwxrwxr-x 1 ian ian 694 Jan 5 10:12 maint/binary_size.sh
-rwxrwxr-x 1 ian ian 3048 Jan 5 10:12 maint/cargo_audit.sh
-rwxrwxr-x 1 ian ian 322 Jan 21 16:17 maint/changed_crates.sh
-rwxrwxr-x 1 ian ian 1696 Jan 5 10:12 maint/check_licenses.sh
-rwxrwxr-x 1 ian ian 2315 Jan 21 16:17 maint/coverage.sh
-rwxrwxr-x 1 ian ian 488 Jan 5 10:12 maint/docker_reproducible_build.sh
-rwxrwxr-x 1 ian ian 638 Jan 5 10:12 maint/fuzz_it_all.sh
-rwxrwxr-x 1 ian ian 165 Jan 5 10:12 maint/readmes.sh
-rwxrwxr-x 1 ian ian 4076 Jan 5 10:12 maint/reproducible_build.sh
-rwxrwxr-x 1 ian ian 1022 Jan 5 10:12 maint/thanks.sh
-rwxrwxr-x 1 ian ian 3943 Jan 28 17:29 maint/with_coverage.sh
-rwxrwxr-x 1 ian ian 1344 Jan 5 10:12 tests/chutney/arti-bench.sh
-rwxrwxr-x 1 ian ian 1321 Jan 28 17:29 tests/chutney/setup.sh
-rwxrwxr-x 1 ian ian 733 Jan 5 10:12 tests/chutney/teardown.sh
zealot:arti>
```Arti 0.1.0 release: Okay for experimental embeddingIan Jacksoniwj@torproject.orgIan Jacksoniwj@torproject.orghttps://gitlab.torproject.org/tpo/core/arti/-/issues/291Broken link(s) in docs depending on features2022-01-20T18:10:25ZIan Jacksoniwj@torproject.orgBroken link(s) in docs depending on featuresOn my system file:///volatile/rustcargo/Rustup/Arti/arti/target/doc/arti_client/index.html has this:
> by calling `[tor_rtcompat::async_std::current_runtime]`, which
i.e. a busted link.
This is probably only present when you're buildi...On my system file:///volatile/rustcargo/Rustup/Arti/arti/target/doc/arti_client/index.html has this:
> by calling `[tor_rtcompat::async_std::current_runtime]`, which
i.e. a busted link.
This is probably only present when you're building the docs with --all-features. but ff you build docs *without* --all-features you get more non-link stuff and some compiler warnings about them....Arti 0.1.0 release: Okay for experimental embeddingIan Jacksoniwj@torproject.orgIan Jacksoniwj@torproject.orghttps://gitlab.torproject.org/tpo/core/arti/-/issues/268"Manual" blocking/removing of Tor nodes2022-02-28T18:51:40Zwouter lueks"Manual" blocking/removing of Tor nodesWe've been looking into using Arti as a Tor library for mobile apps. In our settings, the amount of daily data sent (kilobytes) is very small compared to retrieving the consensus (megabytes). To reduce mobile bandwidth load, we made some...We've been looking into using Arti as a Tor library for mobile apps. In our settings, the amount of daily data sent (kilobytes) is very small compared to retrieving the consensus (megabytes). To reduce mobile bandwidth load, we made some modifications to Arti to run with a smaller consensus that is updated less frequently. See [LightArti](https://github.com/c4dt/lightarti-rest) and Issue #267.
As a result of using a smaller long-term consensus, it sometimes contains nodes that are no longer reachable. To limit the number of nodes that will go offline, LightArti aims to pick stable nodes. This is not always enough to ensure reliable path establishment when the consensus is older than a few days. Therefore LightArti also has functionality to provide an additional blocklist of nodes that should not be used.
To facilitate this, we modified our forked Arti to allow access to the list of nodes in the (side-loaded) consensus/directory and to remove unavailable nodes from it. We achieved this by modifying the module `tor-netdoc` in the following manner:
* The field `consensus` of the structure `UnvalidatedConsensus` and the field `routers` of the structure `Consensus` are set as public to be able to remove churned routers appearing over time.
You can find a (rough) diff wrt an older version of arti [here](https://github.com/c4dt/arti/pull/2).
Ideally we would prefer to extend stock Arti to provided by Arti that enables making these modifications. For example:
* Adding the ability to retrieve, modify and set the list of nodes in `netdir`; or
* Adding the ability to directly remove nodes from the consensusArti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/262Use write_all and flush for socks replies2021-12-15T14:27:13ZNick MathewsonUse write_all and flush for socks repliesThe current socks proxy code is error-prone when it writes replies. It does something like this:
```
let reply =
request.reply(tor_socksproto::SocksStatus::GENERAL_FAILURE, No...The current socks proxy code is error-prone when it writes replies. It does something like this:
```
let reply =
request.reply(tor_socksproto::SocksStatus::GENERAL_FAILURE, None);
socks_w
.write(&reply[..])
.await
.context("Couldn't write SOCKS reply")?;
```
But there are two issues: the `write` function doesn't actually have to write all of the data it's given: instead, it is allowed to write only some of the data. So we should call [`write_all`](https://docs.rs/futures/0.3.18/futures/io/trait.AsyncWriteExt.html#method.write_all) instead.
Also, even after `write_all` is called, we don't have a guarantee that the data will really be sent on the network. To get that, we need to call `flush()` or `close()`. (We should call `close()` on errors and `flush()` on success reporting.)
This logic is complicated enough that we should probably have a new function in proxy.rs that does it all for us, to avoid duplicated code.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/259Handle RELAY_TRUNCATED cells less incorrectly2021-12-15T15:51:54ZNick MathewsonHandle RELAY_TRUNCATED cells less incorrectlyWhen Arti gets a TRUNCATED cell now, it says: "Received Truncated cell that we can't handle, terminating circuit!"
I wrote it that way based on the incorrect impression that Tor doesn't send TRUNCATED cells. But it turns out, Tor does!...When Arti gets a TRUNCATED cell now, it says: "Received Truncated cell that we can't handle, terminating circuit!"
I wrote it that way based on the incorrect impression that Tor doesn't send TRUNCATED cells. But it turns out, Tor does! It's in src/core/or/command.c, in the function `command_process_destroy_cell`.
Now, Tor doesn't actually do anything _special_ with TRUNCATED cells: instead it just decides to tear down the circuit. (See `circuit_truncated()` in src/core/or/circuitbuild.c.) But we should at least make the following changes:
* Report the reason for the circuit failure that comes from the TRUNCATED cell.
* Not treat the cell as 'unhandled'.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/250Integrate test coverage tracking with gitlab CI2022-01-27T12:56:23ZNick MathewsonIntegrate test coverage tracking with gitlab CIIf we did a coverage run as part of our CI, and then gave that coverage run to gitlab in cobertura format, supposedly gitlab could track the coverage over time and tell people when their patches were missing test coverage.
At least, so ...If we did a coverage run as part of our CI, and then gave that coverage run to gitlab in cobertura format, supposedly gitlab could track the coverage over time and tell people when their patches were missing test coverage.
At least, so I'm told. :sweat_smile:
We should double check that grcov works on our gitlab CI runners. (Tarpaulin didn't, when last we tried, due to some kernel compatibility issue.)
[Gitlab's documentation](https://docs.gitlab.com/ee/user/project/merge_requests/test_coverage_visualization.html) may be helpful here.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/245Follow-up from "Build circuits preemptively": Make things configurable.2021-12-07T15:04:43ZNick MathewsonFollow-up from "Build circuits preemptively": Make things configurable.The following discussions from !154 should be addressed:
- [ ] @nickm started a [discussion](https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/154#note_2763510):
> It would be good if this were configurable. (Is it par...The following discussions from !154 should be addressed:
- [ ] @nickm started a [discussion](https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/154#note_2763510):
> It would be good if this were configurable. (Is it part of the specification?)
- [ ] @nickm started a [discussion](https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/154#note_2763513):
> Yeah, these should probably be part of the configuration. But let's put that in after we have the configuration refactored.
- [ ] @nickm started a [discussion](https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/154#note_2763518):
> This duration should probably be configurable at some point too.
- [ ] @nickm started a [discussion](https://gitlab.torproject.org/tpo/core/arti/-/merge_requests/154#note_2763520):
> Maybe this parallelism should be configurable too, down the road.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/236Update to a newer version of simple_asn1, once available.2021-11-14T23:29:17ZNick MathewsonUpdate to a newer version of simple_asn1, once available.There's a [panic bug](https://github.com/acw/simple_asn1/issues/27) in simple_asn1 version 0.6.0; I've submitted [a patch](https://github.com/acw/simple_asn1/pull/28). We should upgrade once a new version is out.
(Should we also submit ...There's a [panic bug](https://github.com/acw/simple_asn1/issues/27) in simple_asn1 version 0.6.0; I've submitted [a patch](https://github.com/acw/simple_asn1/pull/28). We should upgrade once a new version is out.
(Should we also submit a RustSec advisory?)Arti 0.1.0 release: Okay for experimental embeddingNick MathewsonNick Mathewsonhttps://gitlab.torproject.org/tpo/core/arti/-/issues/229Measured test coverage 50% for arti, arti-client.2021-12-01T15:23:15ZNick MathewsonMeasured test coverage 50% for arti, arti-client.Right now we have no way to measure coverage for our integration tests (see #227), and no great way other than integration tests to check the behavior of the `arti` and `arti-client` crates.
We should address these problems from both di...Right now we have no way to measure coverage for our integration tests (see #227), and no great way other than integration tests to check the behavior of the `arti` and `arti-client` crates.
We should address these problems from both directions at once: Improving integration tests (and testing their coverage), and refactoring where reasonable to test our higher-level crates.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/227Coverage support for integration tests2021-12-01T15:11:18ZNick MathewsonCoverage support for integration testsWe should come up with a method for running our integration tests under coverage, so we can see how well we're testing our higher-level modules.
This might require rewriting some of our integration tests in rust? They could launch a ch...We should come up with a method for running our integration tests under coverage, so we can see how well we're testing our higher-level modules.
This might require rewriting some of our integration tests in rust? They could launch a chutney network in rust, then call functions in arti-client or invoke arti manually.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/212Ship more example programs with Arti 0.1.02022-02-24T19:39:47ZNick MathewsonShip more example programs with Arti 0.1.0In #164, we brainstormed a bunch of example programs that we might right in order to show people how to use Arti. We should write and ship more of those with Arti 0.1.0.In #164, we brainstormed a bunch of example programs that we might right in order to show people how to use Arti. We should write and ship more of those with Arti 0.1.0.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/181Inspect and adjust mutex usage in all crates2021-11-16T17:44:12ZNick MathewsonInspect and adjust mutex usage in all cratesWhen I was first programming Arti, I didn't fully understand the difference between [`futures::lock::Mutex`](https://docs.rs/futures/0.3.17/futures/lock/struct.MutexLockFuture.html) and [`std::sync::Mutex`](https://doc.rust-lang.org/std/...When I was first programming Arti, I didn't fully understand the difference between [`futures::lock::Mutex`](https://docs.rs/futures/0.3.17/futures/lock/struct.MutexLockFuture.html) and [`std::sync::Mutex`](https://doc.rust-lang.org/std/sync/struct.Mutex.html). In particular, I believed that I used the futures-aware mutex in a lot of places where it wasn't necessary.
(The practical difference, as I now understand it, is that you need to use `futures::lock::Mutex` anywhere that you might `await` when holding a lock. It is okay to use the regular `std::sync::Mutex`, even in an `async` function so long as you never `await` while holding the lock. The contemplated Rust [`must_not_await`](https://github.com/rust-lang/rust/issues/83310) feature will make correct usage easier to enforce here.)
It would be a good idea for someone to have a careful look at everyplace in our code that uses either `Mutex`, and see if it needs to be replaced with the other.Arti 0.1.0 release: Okay for experimental embeddingetaetahttps://gitlab.torproject.org/tpo/core/arti/-/issues/83Improved lockfile support to prevent concurrent modification of state2022-01-10T14:27:49ZNick MathewsonImproved lockfile support to prevent concurrent modification of stateWe should do smarter about multiple processes using the same state files at the same time.
~~Right now we use advisory locking via the `fslock` crate, which does not yet always behave right with a single process opening the same file mo...We should do smarter about multiple processes using the same state files at the same time.
~~Right now we use advisory locking via the `fslock` crate, which does not yet always behave right with a single process opening the same file more than once. But see [this PR](https://github.com/brunoczim/fslock/pull/3).~~ [This issue was fixed.]
After that is fixed, we should make the state code smarter about lock collisions. I'd prefer to be in a situation where failing to get the lock just puts us into a "read-only" state, where we just take the other process's word about the current guards and circuit timeouts.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/78Integration-test Arti in a chutney network2022-02-04T13:27:53ZNick MathewsonIntegration-test Arti in a chutney networkFor reliability, it would be good to have a script that can integration-test an arti client on a chutney network. It would be cool to have a 'flood mode' that we can use to benchmark the code when it's under load too.
Chutney already g...For reliability, it would be good to have a script that can integration-test an arti client on a chutney network. It would be cool to have a 'flood mode' that we can use to benchmark the code when it's under load too.
Chutney already generates an "arti.toml" file as part of its configuration process that can be used to tell an Arti instance how to use that chutney network.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/55Declare a somewhat-stable API for using a Tor client2022-01-12T15:25:40ZNick MathewsonDeclare a somewhat-stable API for using a Tor clientWe should have a "tor client" object that you can spin up and use to make connections. It doesn't need to be a forever API, but it would be a good idea to try to have it working from the start.We should have a "tor client" object that you can spin up and use to make connections. It doesn't need to be a forever API, but it would be a good idea to try to have it working from the start.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/41chanmgr: Discard channels after sufficient disuse2022-02-09T15:54:42ZNick Mathewsonchanmgr: Discard channels after sufficient disuseRight now, tor-chanmgr doesn't actually drop channels for being too old or unused; we should fix that. (Fortunately, relays will close them for us.)
Here's the general approach:
# 1. Backend: tracking channel disuse
1. For every `tor...Right now, tor-chanmgr doesn't actually drop channels for being too old or unused; we should fix that. (Fortunately, relays will close them for us.)
Here's the general approach:
# 1. Backend: tracking channel disuse
1. For every `tor_proto::Channel`, we should keep a timestamp that tells us when the channel was last "used". A channel counts as being "used" whenever it has at least one circuit on it; it only becomes unused when it has no circuits.
We'll probably want to keep track of when a channel is used inside the `tor_proto::channel` code, since that's the code that sees when circuits are actually attached or removed to a channel. For the purposes of this ticket, only `CircEnt::Open` and `CircEnt::Opening` circuits count; `CircEnt::DestroySent` circuits don't count.
These timestamps can use the mechanism from `tor_proto::util::ts` (or something like it) to ensure that they are as cheap as possible. (Or they might want to hold a `coarsetime::Instant` if no atomic behavior is needed. But if we make that choice, we should add a new struct to wrap `coarsetime::Instant` to make it easily replaceable with another cheap-timestamp mechanism.)
These timestamps should be exposed as a method from Channel, something like this:
```
/// If the channel is not in use, return the amount of time it has had with no circuits.
///
/// Return None if the channel is currently in use.
pub fn duration_unused_at(&self, now: Instant) -> Option<Duration> { ... }
```
# 2. Discarding disused channels
In tor_changmgr, we'll want code that drops channels that have been disused for too long. This code should probably be in several parts. First, we'll need a function with a signature something like this:
```
/// Expire all channels that have been unused for too long.
///
/// Return a Duration until the next time at which a channel _could_ expire.
fn expire_channels(&self, now: Instant) -> Duration { ... }
```
Then we'll need a background task with a loop like this:
```
loop {
let delay = if let Some(cm) = Weak::upgrade(&cm) {
cm.expire_channels(runtime.now())
} else {
return; // channel manager is closed.
}
runtime.sleep(delay).await; // This will sometimes be an underestimate, but it's no big deal; we just sleep some more.
}
```
And finally we'll need some way to configure the actual "maximum unused duration" for a channel. This should probably be a Duration associated with the channel in ChannelState::Open; for consistency, it should be chosen at random when the channel is built, from the range 180..270 seconds.
(The lower bound of 180 seconds means that if we have no unused channels, the `expire_channels()` call can always safety return 180 seconds.)Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/core/arti/-/issues/33Support RESOLVE and RESOLVE_PTR commands2021-07-27T18:03:30ZNick MathewsonSupport RESOLVE and RESOLVE_PTR commandsThese two commands are socks extensions. Arti parses them, but doesn't actually implement them. Further, there is backend support for sending RELAY_RESOLVE commands. All this will need is some connecting together.These two commands are socks extensions. Arti parses them, but doesn't actually implement them. Further, there is backend support for sending RELAY_RESOLVE commands. All this will need is some connecting together.Arti 0.1.0 release: Okay for experimental embeddinghttps://gitlab.torproject.org/tpo/anti-censorship/rdsys/-/issues/13Monitor rdsys using monit2022-02-22T15:11:37ZPhilipp Winterphw@torproject.orgMonitor rdsys using monitLet's create monitoring targets for rdsys once it's deployed. Here's what's worth monitoring:
1. Rdsys's distributors, e.g. HTTPS and Salmon. Each of these distributors is a separate process and can die independently.
2. ~~Rdsys's backen...Let's create monitoring targets for rdsys once it's deployed. Here's what's worth monitoring:
1. Rdsys's distributors, e.g. HTTPS and Salmon. Each of these distributors is a separate process and can die independently.
2. ~~Rdsys's backend. Its registration API is open to the public and would be a great monitoring target.~~Deploy RDSYS alongside BridgeDBmeskiomeskio@torproject.orgmeskiomeskio@torproject.org