[tor-dev] Deprecating Tor Protocol Versions
Hi all, Nick and I were talking about how we remove legacy features in tor, and their corresponding subprotocol versions. Here is a list of the current subprotocol versions: https://gitweb.torproject.org/torspec.git/tree/tor-spec.txt#n2049 Here's a recent protocol version proposal, which deals with recommending and requiring new features: https://gitweb.torproject.org/torspec.git/tree/proposals/303-protover-removal-policy.txt But we don't have a similar proposal for removing support for older protocol versions from the tor codebase. For an example of what that proposal could look like, see our proposal for deprecating consensus methods: https://gitweb.torproject.org/torspec.git/tree/proposals/290-deprecate-consensus-methods.txt Here's the original conversation Nick and I had: https://github.com/torproject/tor/pull/1874#discussion_r423713579 But after reading our consensus methods deprecation proposal, I've changed my mind. I think that we should check for "protocol N, and any later version" by default. That's what we do for consensus methods, and it seems to work well. We can drop the earliest consensus methods, and recent tor versions just keep working. If we need an incompatible change, we can make it another protocol version, and recommend then require support for it. So here's an edited version of my notes on that ticket: There are a few instances of ">=" and "=" confusion in protocol versions. We should try to fix them all. It only matters when we remove protocol versions. We haven't really specified, tested, or exercised this functionality in practice. And so our reviewers lack experience. (And when we did discover a need for it with NSS and LinkAuth, it was more complicated than we expected.) I'd like to see a proposal that tells us how to check future protocol versions as they are added. Along with a migration plan for disabling protocol versions. So let's also open a ticket to check for "any future version". We should replace all "=" checks with ">=". Let's make sure we check all the places where we use protocol versions, even if they don't have a summary flag. Overall, I think it would be helpful if future protocol versions were orthogonal. Or if they depend on earlier features, that dependency should be clearly documented. (For example, Relay=3 IPv6 extends depends on Relay=2 EXTEND2 cells. So if we were checking EXTEND2 cell support, it would be Relay=2 or Relay=3.) T -- teor -- signature.asc Description: Message signed with OpenPGP ___ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
Re: [tor-dev] Chutney code refactoring
Hi Caitlin, > On 15 May 2020, at 09:59, Nick Mathewson wrote: > > On Thu, May 14, 2020 at 7:04 PM c wrote: >> >>> On Sat, 18 Apr 2020 11:02:03 + >>> c wrote: >>> >>> I came across Node.specialize() which does not seem to be called >>> elsewhere, and I cannot guess at its purpose. >> >> I ran vulture (a static analyzer for dead code), trimmed the output to >> omit things I know were being used (some functions/attributes are used >> solely in Python 2), and found these unused names: >> >> lib/chutney/Templating.py:280: unused function 'getUpdateTime' (60% >> confidence) >> lib/chutney/TorNet.py:439: unused function 'specialize' (60% confidence) >> lib/chutney/TorNet.py:658: unused function '_getFreeVars' (60% confidence) >> lib/chutney/TorNet.py:1232: unused function 'isBootstrapped' (60% confidence) >> lib/chutney/TorNet.py:1799: unused function 'isInExpectedDirInfoDocs' (60% >> confidence) >> lib/chutney/TorNet.py:2151: unused function 'configure' (60% confidence) >> lib/chutney/TorNet.py:2191: unused function 'restart' (60% confidence) >> lib/chutney/TorNet.py:2286: unused function 'wait_for_bootstrap' (60% >> confidence) >> lib/chutney/Traffic.py:335: unused attribute 'am_closing' (60% confidence) >> lib/chutney/Traffic.py:345: unused attribute 'am_closing' (60% confidence) >> lib/chutney/Traffic.py:400: unused attribute 'pending_close' (60% confidence) >> lib/chutney/Traffic.py:406: unused attribute 'dot_repetitions' (60% >> confidence) >> >> Aside from isBootstrapped() which we discussed previously and are >> likely going to use, is there any code that stands out as unnecessary >> or dead? > > Hm. Of these: > > isInExpectedDirInfoDocs looks like it might once have done something useful. > > configure and restart and wait_for_bootstrap are all in use; they are > invoked by name, from the command line, at the end of runConfigFile, > where it says `return getattr(network, verb)()`. > > I think the rest are likely to be unused. You could try deleting functions, and then running tor's "make test-network-all". If that test still passes, then the code is probably unused (in practice). Chutney's CI does a few more tests for different tor versions, but I'm pretty sure they all use the same code. T signature.asc Description: Message signed with OpenPGP ___ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
Re: [tor-dev] Chutney code refactoring
On Thu, May 14, 2020 at 7:04 PM c wrote: > > On Sat, 18 Apr 2020 11:02:03 + > c wrote: > > > I came across Node.specialize() which does not seem to be called > > elsewhere, and I cannot guess at its purpose. > > I ran vulture (a static analyzer for dead code), trimmed the output to > omit things I know were being used (some functions/attributes are used > solely in Python 2), and found these unused names: > > lib/chutney/Templating.py:280: unused function 'getUpdateTime' (60% > confidence) > lib/chutney/TorNet.py:439: unused function 'specialize' (60% confidence) > lib/chutney/TorNet.py:658: unused function '_getFreeVars' (60% confidence) > lib/chutney/TorNet.py:1232: unused function 'isBootstrapped' (60% confidence) > lib/chutney/TorNet.py:1799: unused function 'isInExpectedDirInfoDocs' (60% > confidence) > lib/chutney/TorNet.py:2151: unused function 'configure' (60% confidence) > lib/chutney/TorNet.py:2191: unused function 'restart' (60% confidence) > lib/chutney/TorNet.py:2286: unused function 'wait_for_bootstrap' (60% > confidence) > lib/chutney/Traffic.py:335: unused attribute 'am_closing' (60% confidence) > lib/chutney/Traffic.py:345: unused attribute 'am_closing' (60% confidence) > lib/chutney/Traffic.py:400: unused attribute 'pending_close' (60% confidence) > lib/chutney/Traffic.py:406: unused attribute 'dot_repetitions' (60% > confidence) > > Aside from isBootstrapped() which we discussed previously and are > likely going to use, is there any code that stands out as unnecessary > or dead? Hm. Of these: isInExpectedDirInfoDocs looks like it might once have done something useful. configure and restart and wait_for_bootstrap are all in use; they are invoked by name, from the command line, at the end of runConfigFile, where it says `return getattr(network, verb)()`. I think the rest are likely to be unused. ___ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
Re: [tor-dev] Chutney code refactoring
On Sat, 18 Apr 2020 11:02:03 + c wrote: > I came across Node.specialize() which does not seem to be called > elsewhere, and I cannot guess at its purpose. I ran vulture (a static analyzer for dead code), trimmed the output to omit things I know were being used (some functions/attributes are used solely in Python 2), and found these unused names: lib/chutney/Templating.py:280: unused function 'getUpdateTime' (60% confidence) lib/chutney/TorNet.py:439: unused function 'specialize' (60% confidence) lib/chutney/TorNet.py:658: unused function '_getFreeVars' (60% confidence) lib/chutney/TorNet.py:1232: unused function 'isBootstrapped' (60% confidence) lib/chutney/TorNet.py:1799: unused function 'isInExpectedDirInfoDocs' (60% confidence) lib/chutney/TorNet.py:2151: unused function 'configure' (60% confidence) lib/chutney/TorNet.py:2191: unused function 'restart' (60% confidence) lib/chutney/TorNet.py:2286: unused function 'wait_for_bootstrap' (60% confidence) lib/chutney/Traffic.py:335: unused attribute 'am_closing' (60% confidence) lib/chutney/Traffic.py:345: unused attribute 'am_closing' (60% confidence) lib/chutney/Traffic.py:400: unused attribute 'pending_close' (60% confidence) lib/chutney/Traffic.py:406: unused attribute 'dot_repetitions' (60% confidence) Aside from isBootstrapped() which we discussed previously and are likely going to use, is there any code that stands out as unnecessary or dead? Caitlin ___ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
Re: [tor-dev] Support for full DNS resolution and DNSSEC validation
On 26 Apr (19:37:56), Christian Hofer wrote: > Hi there, Greetings Christian! > > I have a proposal regarding DNS name resolution. > > Ticket: https://trac.torproject.org/projects/tor/ticket/34004 > Proposal: > https://trac.torproject.org/projects/tor/attachment/ticket/34004/317-secure-dns-name-resolution.txt > Implementation: https://github.com/torproject/tor/pull/1869 First, this is quite impressive piece of work. I was _NOT_ expecting a 27k line diff ;). So the proposal looks very good. I like the idea very much. I honestly thought that you were about to propose a way for Tor to talk to an *external* DNS resolver client application (third part) but I see that client DNSSEC is basically implemented in tor with your patch which is... interesting? Before we go further, can you walk me through the reasons (if you had thought of it of course) why you didn't use something like libunbound? There are side effects of adding DNSSEC client support (with our own implementation) that we, people maintaining tor, have to become DNSSEC expert in some ways just to be able to understand what is happening in that code, fix issues but also possibly implement new features if any. That is where a third part library like unbound becomes very nice because they are the experts at providing such features. Of course, everytime we have to link to an external library we do it carefully and with considerations because of the "yet another attack" vector problem. But adding that much code to support a well known feature like DNSSEC also has huge implications for tor maintainability and security. Finally, something I noticed and made me itch a bit. You hardcoded a lot of .onions where one appears to be Cloudflare (?) resolver. What are the other addresses? I worry here because default options are always the one used the most so I'm concerned here about shipping hardcoded addresses _within_ our C code. Thanks! David -- dApigzB8NtOQEAlKqhqbshxjxOMakjiX9LGU9wvhFqs= signature.asc Description: PGP signature ___ tor-dev mailing list tor-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev
Re: [tor-dev] Proposal 319: RELAY_FRAGMENT cells
On 11 May (16:47:24), Nick Mathewson wrote:
> ```
> Filename: 319-wide-everything.md
> Title: RELAY_FRAGMENT cells
> Author: Nick Mathewson
> Created: 11 May 2020
> Status: Open
> ```
>
> (This proposal is part of the Walking Onions spec project.)
>
> # Introduction
>
> Proposal 249 described a system for `CREATE` cells to become wider, in order
> to
> accommodate hybrid crypto. And in order to send those cell bodies across
> circuits, it described a way to split `CREATE` cells into multiple `EXTEND`
> cells.
>
> But there are other cell types that can need to be wider too. For
> example, `INTRODUCE` and `RENDEZVOUS` cells also contain key material
> used for a handshake: if handshakes need to grow larger, then so do
> these cells.
>
> This proposal describes an encoding for arbitrary "wide" relay cells,
> that can be used to send a wide variant of anything.
>
> To be clear, although this proposal describes a way that all relay
> cells can become "wide", I do not propose that wide cells should
> actually be _allowed_ for all relay cell types.
>
> # Proposal
>
> We add a new relay cell type: `RELAY_FRAGMENT`. This cell type contains part
> of another relay cell. A `RELAY_FRAGEMENT` cell can either introduce a new
Typo: RELAY_FRAGEMENT --> RELAY_FRAGMENT
> fragmented cell, or can continue one that is already in progress.
>
> The format of a RELAY_FRAGMENT body is one of the following:
>
> // First body in a series
> struct fragment_begin {
>// What relay_command is in use for the underlying cell?
>u8 relay_command;
>// What will the total length of the cell be once it is reassembled?
>u16 total_len;
>// Bytes for the cell body
>u8 body[];
> }
>
> // all other cells.
> struct fragment_continued {
>// More bytes for the cell body.
>u8 body[];
> }
>
> To send a fragmented cell, first a party sends a RELAY_FRAGMENT cell
> containing a "fragment_begin" payload. This payload describes the total
> length of the cell, the relay command
>
> Fragmented cells other than the last one in sequence MUST be sent full of
> as much data as possible. Parties SHOULD close a circuit if they receive a
> non-full fragmented cell that is not the last fragment in a sequence.
>
> Fragmented cells MUST NOT be interleaved with other relay cells on a circuit,
> other than cells used for flow control. (Currently, this is only SENDME
> cells.) If any party receives any cell on a circuit, other than a flow
> control cell or a RELAY_FRAGEMENT cell, before the fragmented cell is
Typo: RELAY_FRAGEMENT --> RELAY_FRAGMENT
> complete, than it SHOULD close the circuit.
>
> Parties MUST NOT send extra data in fragmented cells beyond the amount given
> in the first 'total_len' field.
Should the circuit be closed or the fragments dropped?
>
> Not every relay command may be sent in a fragmented cell. In this proposal,
> we allow the following cell types to be fragmented: EXTEND2, EXTENDED2,
> INTRODUCE1, INTRODUCE2, RENDEZVOUS. Any party receiving a command that they
> believe should not be fragmented should close the circuit.
Probably we want RENDEZVOUS1 and RENDEZVOUS2.
>
> Not all lengths up to 65535 are valid lengths for a fragmented cell. Any
> length under 499 bytes SHOULD cause the circuit to close, since that could
> fit into a non-fragmented RELAY cell. Parties SHOULD enforce maximum lengths
> for cell types that they understand.
>
> All `RELAY_FRAGMENT` cells for the fragmented cell must have the
> same Stream ID. (For those cells allowed above, the Stream ID is
> always zero.) Implementations SHOULD close a circuit if they
> receive fragments with mismatched Stream ID.
>
> # Onion service concerns.
>
> We allocate a new extension for use in the ESTABLISH_INTRO by onion services,
> to indicate that they can receive a wide INTRODUCE2 cell. This extension
> contains:
>
> struct wide_intro2_ok {
> u16 max_len;
> }
>
> We allocate a new extension for use in the `ESTABLISH_RENDEZVOUS`
> cell, to indicate acceptance of wide `RENDEZVOUS2` cells. This
> extension contains:
>
> struct wide_rend2_ok {
> u16 max_len;
> }
>
> (Note that `ESTABLISH_RENDEZVOUS` cells do not currently have a an
> extension mechanism. They should be extended to use the same
> extension format as `ESTABLISH_INTRO` cells, with extensions placed
> after the rendezvous cookie.)
Why would a client need to announce wide cells in the ESTABLISH phase as
opposed to using protover "Relay=N" ?
The maximum length of a fragmented cell is capped to 2^16 (u16) so we don't
really need the establish process to inform us of the maximum expected length
but rather use the max_len in the first fragment?
Furthermore, ESTABLISH_INTRO has extensions (only 1 as of today) so they could
also be fragments themselves and thus I'm not sure I see the point of having
two different ways of "expecting" fragments for th
