<html><head><meta http-equiv="content-type" content="text/html; charset=utf-8"></head><body dir="auto"><div></div><div>Hi Nick,</div><div><br></div><div>Juga asked me to comment on your review, so she could read it before our bandwidth meeting this week. If I don't comment on a suggestion, you should assume I agree with it.</div><div><br></div><div>Backwards Compatibility</div><div><br></div><div>Nick asked about backwards compatibility. This format uses semantic versioning. Tor 0.2.9 - 0.3.3 reads format version 1.0.0. It also reads format 1.1.0, but ignores the new features with warnings.</div><div><br></div><div>If we want to introduce an incompatible format, we should call it 2.0.0, because semantic versioning requires a major increment for breaking changes.</div><div><br></div><div>Here's how we could add the new format:</div><div>* The new format should have a new torrc option.</div><div>* Tor should be modified to support the new format, and we should put time on the roadmap for people to work on implementing, testing, or reviewing it.</div><div>* Either we should backport the new format to the latest stable release, or sbws should produce both formats.</div><div><br></div><div>The current implementation has at least one security bug, some weird order restrictions, and some line length restrictions. So I would support re-implementing it using the standard directory document parsing code. Even if that takes more time.</div><div><br></div><div>Testing the format</div><div><br></div><div>Most of us don't have a spare directory authority for testing.</div><div><br></div><div>If you run chutney with my bwfile branch, all the authorities in the network read /tmp/bwfile for every consensus. Look for the warnings at the end of the chutney output.</div><div><br></div><div>The basic-min network is fast:</div><div>chutney/tools/test-network.sh --flavour basic-min</div><div><br></div><div>Here's the branch:</div><div><a href="https://github.com/teor2345/chutney/commit/ebdb4760fbcae40979ab248e4208c27a71cccb11">https://github.com/teor2345/chutney/commit/ebdb4760fbcae40979ab248e4208c27a71cccb11</a></div><div><br></div><div>I've already found one minor security bug using this branch: #26007.</div><div><br></div><div>Next Steps</div><div><br></div><div>I'm going to be away next week for a week and a half. I encourage other people to make decisions while I'm away, so we can keep making progress.</div><div><br>On 1 May 2018, at 22:36, Nick Mathewson <<a href="mailto:nickm@alum.mit.edu">nickm@alum.mit.edu</a>> wrote:<br><br></div><blockquote type="cite"><div><div dir="ltr"><div>Hi, Juga! <br><br>This is a review of the document from <a href="https://raw.githubusercontent.com/juga0/torspec/c7f06023dd1d5d47adad128de541f8eba2a13bfb/bandwidth-file-spec.txt">https://raw.githubusercontent.com/juga0/torspec/c7f06023dd1d5d47adad128de541f8eba2a13bfb/bandwidth-file-spec.txt</a> , which I *think* is the same as the document you have below.<br></div><div><br></div><div>I'm reviewing this as though it were a fully new format, since I'm not sure how much we already have locked-in based on existing code, and how much is new. We might decide that backward compatibility is more important than consistency, and if so, we won't want to take all of my recommendations here.<br></div><div><div><br>> Tor Bandwidth Measurements Document Format<br>> juga<br>> teor<br>><br>> 1. Scope and preliminaries<br>><br>> This document describes the format of Tor's bandwidth measurements<br></div></div></div></div></blockquote><div><br></div><div>Replace measurements document with list?</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> document, version 1.0.0 and later.<br><br>Suggestion: Maybe explicitly say "1.0.0, 1.1.0, and later"?<br></div></div></div></div></blockquote><div><blockquote type="cite"><br></blockquote></div><blockquote type="cite"><div><div dir="ltr"><div><div>> Since Tor version 0.2.4.12-alpha the directory<br>> authorities use the bandwidth measurements document called<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">Replace measurements document with list?</span></div><div><br></div><blockquote type="cite"><div><div dir="ltr"><div><div>> "V3BandwidthsFile" and produced by Torflow [1]<br>> (format described in README.spec.txt [2]).<br><br>Recommendation: "Format described in Torflow's README.spec.txt".<br><br>Explanation needed: Is this a new format, or a new specification of the<br>existing format? Let's say so here.<br></div></div></div></div></blockquote><div><br></div><div>A new specification for the existing format 1.0.0.</div><div>A new format 1.1.0, which is backwards compatible with 1.0.0 parsers.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>Question: If this is a different format, and we're calling it version<br>1.0.0, what should we call the old one? But later it seems that we're<br>introducing 1.1.0, and we're calling the old one 1.0.0.<br></div></div></div></div></blockquote><div><br></div><div>"The Legacy Torflow format" or just "legacy"?</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>Suggestion: let's be explicit that we're only describing the format<br>here, and *not* describing how bwauths generate their data.<br></div></div></div></div></blockquote><div><br></div><div>I agree. We want to leave room for peerflow and future schemes.</div><div><span style="background-color: rgba(255, 255, 255, 0);">So we might want to:</span></div><div><span style="background-color: rgba(255, 255, 255, 0);">* replace every "measurements document" with "list"</span></div><div><span style="background-color: rgba(255, 255, 255, 0);">* replace every "measurements scanner" with "generator"</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL<br>> NOT", "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and<br>> "OPTIONAL" in this document are to be interpreted as described in<br>> RFC 2119.<br>><br>> 1.2. Acknowledgements<br>><br>> The original bandwidth measurement scanner (Torflow)</div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">Replace measurement scanner with generator?</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div> and format was<br>> created by mike. Teor suggested to write this specification while<br>> contributing on pastly's new bandwidth scanner implementation.<br>><br>> This specification was revised after feedback from:<br>><br>> XXX<br></div></div></div></div></blockquote><div><br></div><div>Please update.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> 1.3 Outline<br>><br>> The bandwidth measurements mentioned in sections 3.4.1 and 3.4.2<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">Hmm, the dir-spec calls them measurements.</span></div><div><span style="background-color: rgba(255, 255, 255, 0);">Maybe we should fix it as well.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> of "Tor directory protocol" (dir-spec.txt) [3] are obtained<br>> by bandwidth authorities,</div></div></div></div></blockquote><div><br></div><div>Is a bandwidth authority a directory authority that votes for bandwidths?</div><div>Or is it a bandwidth generator that produces the bandwidth file?</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>which generate a file storing information<br>> on relays' measured bandwidth capacities.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">Remove "measured".</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>><br>> 1.4. Format Versions<br>><br>> 1.0.0 - The legacy fallback bandwidth measurements document format<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">Instead of "bandwidth measurements document format", say "bandwidth list"?</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>><br>> 1.1.0 - Adds key_value lines to the header, format version,<br>> optional ones and section separator.<br><br>Information: Let's repeat in this section which versions of Tor can<br>consume these versions.<br></div></div></div></div></blockquote><div><br></div><div>All Tor versions can consume format version 1.0.0.</div><div>All Tor versions can consume format version 1.1.0, but they warn on header lines.</div><div>See <a href="https://trac.torproject.org/projects/tor/ticket/25960">https://trac.torproject.org/projects/tor/ticket/25960</a></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> 2. Format details<br>><br>> Bandwidth measurements MUST contain the following sections:<br></div></div></div></div></blockquote><div><br></div><div>And if they don't, the file SHOULD be ignored.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> - Header (exactly once)<br>> - Relays measurements (zero or more times)<br><br>Grammar suggestion: "Relay measurements".<br></div></div></div></div></blockquote><div><br></div><div>Replace "measurements" with "bandwidths"?</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> 2.1. Definitions<br>><br>> The following nonterminals are defined in dir-spec.txt, sections<br>> 1.2., 2.1.1., <a href="http://2.1.3.">2.1.3.</a>:<br>><br>> Int<br>> SP (space)<br>> NL (newline)<br>> Keyword<br>> ArgumentChar<br>> fingerprint (hexdigest)<br><br></div><div>Does this have to start with a "$" ? I think it does. Maybe we should be explicit about that.</div></div></div></div></blockquote><div><br></div><div>It does. And we should.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> nickname<br>><br>> Nonterminals defined in "Tor Directory List Format" (dir-list-spec.txt),<br>> section <a href="http://2.2.1.">2.2.1.</a>:<br>><br>> version_number<br>><br>> We define the following nonterminals:<br>><br>> value ::= ArgumentChar+<br></div></div></div></div></blockquote><div><br></div><div>Excluding SP</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> key_value ::= Keyword "=" value<br>> line ::= ArgumentChar* NL<br>> timestamp ::= Int<br>> bandwidth ::= Int<br>> relay_line ::= key_value (SP key_value)* NL<br>><br>> 2.2. Header format<br>><br>> Some header lines MUST appear in specific positions, as documented below.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if they don't, the file SHOULD be ignored.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> All other lines can appear in any order.<br>><br>> There MUST NOT be multiple key_value header lines with the same key.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if there are, the parser SHOULD choose an arbitrary line.</span></div><div><span style="background-color: rgba(255, 255, 255, 0);"><br></span></div><div><span style="background-color: rgba(255, 255, 255, 0);">All lines in the file MUST be 510 characters or less, to allow for the trailing newline and NUL characters. (The previous limit was 254 characters in Tor 0.2.6.2-alpha and earlier.)</span></div><div><span style="background-color: rgba(255, 255, 255, 0);"><br></span></div><div><span style="background-color: rgba(255, 255, 255, 0);">The parser MAY ignore longer lines.</span></div><div><span style="background-color: rgba(255, 255, 255, 0);"><br></span></div><div><span style="background-color: rgba(255, 255, 255, 0);">Should we lift this restriction in 1.1.0?</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>Maybe this line belongs below in the key_value section?<br><br>> It consists of:<br>><br>> timestamp NL<br>><br>> [At start, exactly once.]<br>><br>> The Unix Epoch time in seconds when the file was created.<br><br>Question: Why no keyword and equal sign here? Is this a legacy thing?<br></div></div></div></div></blockquote><div><br></div><div>Yes, tor expects a Unix timestamp on a single line by itself.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>Also, wouldn't it be more standard to have it be in YYYY-MM-DDTHH:MM:SS<br>format?<br></div></div></div></div></blockquote><div><br></div><div>Tor refuses to read bandwidth files unless they start with an integer on a line by itself. So this would be a breaking change.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "version=" version_number NL<br>><br>> [In second position, zero or one time.]<br>><br>> The specification document format version.<br>> It uses semantic versioning [5].<br>><br>> This line has been added in version 1.1.0 of this specification.<br>><br>> Version 1.0.0 documents do not contain this line, and the<br>> version_number is considered to be "1.0.0".<br><br>General concern: I question the use of = signs here in the headers. If<br>we use "SP" instead, then we can reuse a lot of the same machinery tor<br>currently uses to parse other documents.<br></div></div></div></div></blockquote><div><br></div><div>I think using SP is fine.</div><div><br></div><div>But if we want to re-use the parsing machinery, we probably need to add a keyword to the initial timestamp. That would be a breaking change.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "software=" value NL<br>><br>> [Zero or one time.]<br>><br>> The name of the software that created the document.<br>><br>> This line has been added in version 1.1.0 of this specification.<br>><br>> Version 1.0.0 documents do not contain this line, and the software is<br>> considered to be "torflow".<br>><br>> "software_version=" value NL<br>><br>> [Zero or one time.]<br>><br>> The version of the software that created the document.<br>> The version may be a version_number, a git commit, or some other<br>> version scheme.<br>><br>> This line has been added in version 1.1.0 of this specification.<br></div></div></div></div></blockquote><div><br></div><div>If we use SP as a separator, we can make these two lines:</div><div><br></div><div>"software" SP name_value SP version_value NL</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "scanner_started=" timestamp NL<br>><br>> [Zero or one time.]<br>><br>> The Unix Epoch time in seconds when the scanner that generates the<br>> measurements document started.<br>><br>> This line has been added in version 1.1.0 of this specification.<br><br>See note above about time format. YYYY-MM-DDTHH:MM:SS is how we specify<br>times elsewhere in Tor.<br></div></div></div></div></blockquote><div><br></div><div>This is a new field, so we can choose the format.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "earliest_measurement=" timestamp NL<br>><br>> [Zero or one time.]<br>><br>> The Unix Epoch time in seconds when the first relay measurement<br>> was obtained.<br>><br>> This line has been added in version 1.1.0 of this specification.<br><br>See note above about time format.<br><br>> key_value NL<br>><br>> [Zero or more times.]<br>><br>> Future format versions may include additional key_value header lines.<br>> Additional header lines will be accompanied by a minor version increment.<br>><br>> Implementations MAY add additional header lines as needed. This<br>> specification SHOULD be updated to avoid conflicting meanings for the<br>> same header keys.<br>><br>> Parsers MUST NOT rely on the order of these additional lines.<br>><br>> Additional header lines MUST NOT use any keywords specified in the<br>> relay measurements format.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if there are, the parser MAY ignore conflicting keywords.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> If a header line does not conform to this format, the line SHOULD be<br>> ignored by parsers.<br><br>Suggestion: say what recipients of this document should do with<br>unrecognized data. In general, it's good for forward compatibility to<br>say something like, "Recipients MUST ignore key_value lines if they do<br>not recognize the keyword. Recipients MUST ignore any extra material in<br>a line that they do not recognize."<br></div></div></div></div></blockquote><div><br></div><div>We should specify what parsers should do with every MUST in the document.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>Also see suggestion above about using SP as our separator rather than<br>"=" for consistency with other documents Tor parses.<br><br>> NL<br>><br>> [Zero or one time.]<br>><br>> The header ends.<br>><br>> This line has been added in version 1.1.0 of this specification.<br>><br>> For version 1.0.0 documents, the header ends when the first relay<br>> measurement line is found conforming to the next section.<br><br>Suggestion: Replace this empty line with an explicit keyword, for<br>consistency with other documents.<br><br>> 2.3. Relay measurements format<br>><br>> It consists of zero or more relay_line with the measurement results<br>> of relays in arbitrary order.<br>><br>> There can be at most one relay_line per relay identity (fingerprint).<br>><br>> There MUST NOT be multiple key_value pairs with the same key in the same<br>> relay_line.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if there are, the parser SHOULD choose an arbitrary value.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> Each relay_line MUST include the following key_value in arbitrary order:<br><br>Do existing implementations accept arbitrary order here?<br></div></div></div></div></blockquote><div><br></div><div>Existing Tor implementations do not accept node_id at the end of a line.</div><div><a href="https://trac.torproject.org/projects/tor/ticket/26004#ticket">https://trac.torproject.org/projects/tor/ticket/26004</a></div><div><br></div><div>We should:</div><div>* add this as a MUST NOT in 1.0.0, and</div><div>* allow it in 1.1.0, with a list of tor versions that support it</div><div><br></div><div>If we use the standard directory parser, each relay line will have to start with a keyword. Perhaps we should use "b" or "r" or "n". This would be a breaking change.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "node_id=" fingerprint<br>><br>> [Exactly once.]<br>><br>> The fingerprint of the relay being measured.<br><br>Suggestion: Add a field to hold the Ed25519 Identity of the relay being<br>measured. Say that implementations SHOULD include both RSA fingerprint<br>and Ed25519 identity, and that implementations SHOULD accept lines that<br>contain at least one of them.<br></div></div></div></div></blockquote><div><br></div><div>Suggestion: the ed25519 IDs should be base64 encoded, without a trailing =, because a trailing = makes the format ambiguous.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> "bw=" bandwidth<br>><br>> [Exactly once.]<br>><br>> The measured bandwidth of this relay.<br>><br>> Tor accepts zero bandwidths, but they trigger bugs in older Tor<br>> implementations. Therefore, implementations SHOULD NOT produce zero<br>> bandwidths. Instead, they SHOULD use one as their minimum bandwidth.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if there are zero bandwidths, the parser MAY ignore them.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> Multiple measurements can be aggregated using an averaging scheme, such<br>> as a mean, median, or decaying average.<br>><br>> Torflow scales bandwidths to kilobytes per second. Other implementations<br>> SHOULD use kilobytes per second for their initial bandwidth scaling.<br>><br>> If different implementations or configurations are used in votes for the<br>> same network, their measurements MAY need further scaling. See Appendix B<br>> for information about scaling, and one possible scaling method.<br>><br>> key_value<br>><br>> [Zero or more times.]<br><br>Technically, this isn't a key_value, because a "value" is made of<br>ArgumentChar, and ArgumentChar can contain spaces. So if we were<br>parsing<br> "foo=abc bar=def"<br>we might be parsing either one key_value ("foo", "abc bar=def") or two<br>("foo", "abc"), ("bar, "def").<br></div></div></div></div></blockquote><div><br></div><div>Let's exclude SP from value to resolve this issue.</div><br><blockquote type="cite"><div><div dir="ltr"><div><div>> Future format versions may include additional key_value pairs on a relay_line.<br>> Additional key_value pairs will be accompanied by a minor version increment.<br>><br>> Implementations MAY add additional relay key_value pairs as needed. This<br>> specification SHOULD be updated to avoid conflicting meanings for the<br>> same relay keys.<br>><br>> Parsers MUST NOT rely on the order of these additional key_value pairs.<br>><br>> Additional key_value pairs MUST NOT use any keywords specified in the<br>> header format.<br></div></div></div></div></blockquote><div><br></div><div><span style="background-color: rgba(255, 255, 255, 0);">And if there are, the parser MAY ignore conflicting keywords.</span></div><br><blockquote type="cite"><div><div dir="ltr"><div><div>As above, let's say that a parser should ignore key_value entries with<br>keywords that it doesn't recognize.<br><br>><br>> If a relay line does not conform to this format, the line SHOULD be<br>> ignored by parsers.<br>>…<br></div></div></div></div></blockquote><br><div>T</div></body></html>