Define a "mandatory" SvcParamKey #205
Conversation
draft-ietf-dnsop-svcb-https.md
Outdated
| function correctly for clients that ignore this SvcParamKey. Each SVCB | ||
| protocol mapping SHOULD specify a default set of essential keys. The | ||
| SvcParamKey "mandatory" is used to indicate the set of essential keys for this | ||
| RR (overriding the default set). |
There was a problem hiding this comment.
Overriding or adding to the default set? The later seems more usable?
There was a problem hiding this comment.
Overriding seems more powerful, since it allows making default-essential keys optional. Given that this feature should be very rare, I figured usability was not too important.
Then again, maybe clients have to have an understanding of all the default-essential keys for a scheme anyway, so I guess we can just make it additive.
There was a problem hiding this comment.
Changed: I made it additional instead.
draft-ietf-dnsop-svcb-https.md
Outdated
| In a ServiceForm RR, a SvcParamKey is considered "essential" if the RR will not | ||
| function correctly for clients that ignore this SvcParamKey. Each SVCB | ||
| protocol mapping SHOULD specify a default set of essential keys. The | ||
| SvcParamKey "mandatory" is used to indicate the set of essential keys for this |
There was a problem hiding this comment.
Is there a reason to switch between the words "essential" and "mandatory"? Using both is somewhat distracting.
There was a problem hiding this comment.
I’d prefer to stick with “mandatory” and “default mandatory set”. Agreed that the mix is a bit confusing.
There was a problem hiding this comment.
I chose this wording because "mandatory", in my view, is extremely confusing: "mandatory" sounds like domain owners are required to publish or use the "mandatory" keys, and clients are expected to enforce that they are present (neither of which is true). These keys aren't "mandatory"; they're "mandatory-to-understand-if-present". "Non-ignorable".
If you can think of a clearer way to express this I would appreciate a suggestion!
There was a problem hiding this comment.
I changed this to "mandatory"/"automatically mandatory".
draft-ietf-dnsop-svcb-https.md
Outdated
| | 6 | ipv6hint | IPv6 address hints | (This document) | | ||
| | 7 | mandatory | Essential keys in this RR | (This document) | |
There was a problem hiding this comment.
Did we want to give it ID 0 ? (I liked the idea of having it first.) I guess we can't make it 1 because we don't want to move alpn?
There was a problem hiding this comment.
I’d rather us not move alpn if that’s okay :)
Any reason not to give this 0? It is a meta-key, so making it 0 seems elegant.
There was a problem hiding this comment.
We'd previously discussed leaving 0 unassigned so that APIs could use it to mean something like "null". If we don't think that's actually useful then I'm OK with making this 0.
EDIT: I think the use case would be for a function that parses a key name and returns the uint16 code. In some programming languages, it might be convenient to be able to return 0 if the name couldn't be parsed. Naturally, there are also many other possible solutions.
There was a problem hiding this comment.
I changed "mandatory" to ID 0 and made key65535 (a.k.a. -1) explicitly reserved as "invalid key".
draft-ietf-dnsop-svcb-https.md
Outdated
| network byte order, concatenated in ascending order. | ||
|
|
||
| This SvcParamKey is always automatically mandatory, and MUST NOT appear in its | ||
| own value list. |
There was a problem hiding this comment.
Perhaps reframe as:
"This SvcParamKey is always automatically mandatory. Automatically mandatory entries MUST NOT appear in the value list."
(Unless we want to allow them to appear as doing so is idempotent?)
There was a problem hiding this comment.
I don't want to make this "MUST NOT", because zone file parsers can't detect it (they are not scheme-aware), and I would like to avoid introducing syntax errors that are caught by clients but not by the zone file parser.
* Implement SVCB * Fix serialization and deserialization of double quotes * More effort (?) 4 months old commit * DEBUG * _ * Presentation format serialization/deserialization * _ Remove generated * Progress on presentation format parse & write * _ * Finish parsing presentation format * Regenerate * Pack unpack * Move to svcb.go Scan_rr.go and types.go should be untouched now * 🐛 Thanks ghedo * Definitions * TypeHTTPSSVC * Generated and isDuplicate * Goodbye lenient functions Now private key=value pairs have to be defined as structs too. They are no longer automatically named as KeyNNNNN * Encode/decode * Experimental svc * Read method * Implement some of the methods, use trick... to report where the error is while reading it. This should be applied to EDNS too. Todo: Find if case can only contain e := new(SVC_ALPN) and rest moved out Also fix two compile errors * Add SVC_LOCAL methods, reorder, remove alpn value, bugs * Errors * Alpn, make it build * Correct testsuite * Fully implement parser Change from keeping a state variable to reading in one iteration until the key=value pair is fully consumed * Simplify and document EDNS should be simplified too * Attempt to fix fuzzer And Alpn bug * A bug and change type values to match @ghedo's implementation * IP bug Also there are two ip duplicating patterns, one with copy, one with append. Maybe change it to be consistent. * Check for strictly increasing keys as required * Don't panic on invalid alpn * Redundant check, don't modify original array * Size calculation * Fix the fuzzer, match the style * 65535 is reserved too, don't delay errors * Check keyNNN, check for aliasform having values * IPvNHint is an array * Fix ipvNHint * Rename everything * Unrecognized keys according to the updated specification * Skip zero-length structs in generators. Fix CI * Doc cleanup * Off by one * Add parse tests * Check if private key doesn't collide with known key, invalid tests * Disallow IPv4 as IPv6. More tests. Related #1107 * Style fixes * More consistency, more tests * 🐛 Deep copy as in the documentation a := make([]net.IP, 1) a[0] = net.ParseIP("1.1.1.1").To4() b := append(make([]net.IP, 0, 1), a...) b[0] = net.ParseIP("3.1.1.1").To4() fmt.Println(a[0][0]) * Make tests readable * Move valid parse tests to different file * 🐛 One of previous commits not fully committed * Test binary single value encoding/decoding and full encode/decode * Add worst-case grows to builders, 🐛 Wrong visible character range, redundant tests * Testing improvements And don't convert to IPv4 twice * Doc update only * Document worst case allocations and ipv6 can be at most of length 39, not 40 * Redundant IP copy, consistent IPv6 behavior, fix deep copy * isDuplicate for SVCB * Optimizations * echoconfig * Svc => SVCB * Fix CI * Regenerate after REBASE (2) Rebased twice on 15th and 20th May * Rename svc, use escapeByte. * Fix parsing whitespaces between quotes, rename ECHOHOConfig * resolve Remove svcbFieldLen Use reverseInt Uppercase SVCB Rename key_value "invalid" => bad Alpn comments > 65535 check Unneeded slices * a little more read => parse IP array meaning Force pushed because forgot to change read in svcb_test.go * HTTPSSVC -> HTTPS * Use new values * mandatory code MikeBishop/dns-alt-svc#205 * Resolve comments Rename svcb-pairs Remove SVCB_PRIVATE ranges Comment on SVCB_KEY65535 ParseError return l.token rename svcbKeyToString and svcbStringToKey privatize SVCBKeyToString, SVCBStringToKey * Refactor 1 Rename sorted, originalPairs Use append instead of copy Use svcb_RESERVED instead of 65535, with it now being private "type SVCBKey uint16" * Refactor 2 svcbKeyToString as method svcbStringToKey updated after key 0 :bug: mandatory has missing key Rename str idx < 0 * Refactor 3 Use l.token as z var key, value string Comment wrap 0: Sentences with '.' keyValue => kv * Refactor 4 * Refactor 5 len() int * Refactor 6 * Refactor 7 * Test remove parsing * Error messages * Rewrite two estimate comments * parse shouldn't modify original array :bug: * Remove two unneeded comments * Address review comments Push 2 because can't build fuzzer python Push 3 to try again * Simplify argument duplication as per tmthrgd's suggestion And add the relevant test Force push edit: Make sorting code fit into one line * Rewrite ECHConfig and address the review * Remove the optional tab * Add To4() Check * More cleanup and fix mandatory not sorting bug
* Implement SVCB * Fix serialization and deserialization of double quotes * More effort (?) 4 months old commit * DEBUG * _ * Presentation format serialization/deserialization * _ Remove generated * Progress on presentation format parse & write * _ * Finish parsing presentation format * Regenerate * Pack unpack * Move to svcb.go Scan_rr.go and types.go should be untouched now * 🐛 Thanks ghedo * Definitions * TypeHTTPSSVC * Generated and isDuplicate * Goodbye lenient functions Now private key=value pairs have to be defined as structs too. They are no longer automatically named as KeyNNNNN * Encode/decode * Experimental svc * Read method * Implement some of the methods, use trick... to report where the error is while reading it. This should be applied to EDNS too. Todo: Find if case can only contain e := new(SVC_ALPN) and rest moved out Also fix two compile errors * Add SVC_LOCAL methods, reorder, remove alpn value, bugs * Errors * Alpn, make it build * Correct testsuite * Fully implement parser Change from keeping a state variable to reading in one iteration until the key=value pair is fully consumed * Simplify and document EDNS should be simplified too * Attempt to fix fuzzer And Alpn bug * A bug and change type values to match @ghedo's implementation * IP bug Also there are two ip duplicating patterns, one with copy, one with append. Maybe change it to be consistent. * Check for strictly increasing keys as required * Don't panic on invalid alpn * Redundant check, don't modify original array * Size calculation * Fix the fuzzer, match the style * 65535 is reserved too, don't delay errors * Check keyNNN, check for aliasform having values * IPvNHint is an array * Fix ipvNHint * Rename everything * Unrecognized keys according to the updated specification * Skip zero-length structs in generators. Fix CI * Doc cleanup * Off by one * Add parse tests * Check if private key doesn't collide with known key, invalid tests * Disallow IPv4 as IPv6. More tests. Related miekg#1107 * Style fixes * More consistency, more tests * 🐛 Deep copy as in the documentation a := make([]net.IP, 1) a[0] = net.ParseIP("1.1.1.1").To4() b := append(make([]net.IP, 0, 1), a...) b[0] = net.ParseIP("3.1.1.1").To4() fmt.Println(a[0][0]) * Make tests readable * Move valid parse tests to different file * 🐛 One of previous commits not fully committed * Test binary single value encoding/decoding and full encode/decode * Add worst-case grows to builders, 🐛 Wrong visible character range, redundant tests * Testing improvements And don't convert to IPv4 twice * Doc update only * Document worst case allocations and ipv6 can be at most of length 39, not 40 * Redundant IP copy, consistent IPv6 behavior, fix deep copy * isDuplicate for SVCB * Optimizations * echoconfig * Svc => SVCB * Fix CI * Regenerate after REBASE (2) Rebased twice on 15th and 20th May * Rename svc, use escapeByte. * Fix parsing whitespaces between quotes, rename ECHOHOConfig * resolve Remove svcbFieldLen Use reverseInt Uppercase SVCB Rename key_value "invalid" => bad Alpn comments > 65535 check Unneeded slices * a little more read => parse IP array meaning Force pushed because forgot to change read in svcb_test.go * HTTPSSVC -> HTTPS * Use new values * mandatory code MikeBishop/dns-alt-svc#205 * Resolve comments Rename svcb-pairs Remove SVCB_PRIVATE ranges Comment on SVCB_KEY65535 ParseError return l.token rename svcbKeyToString and svcbStringToKey privatize SVCBKeyToString, SVCBStringToKey * Refactor 1 Rename sorted, originalPairs Use append instead of copy Use svcb_RESERVED instead of 65535, with it now being private "type SVCBKey uint16" * Refactor 2 svcbKeyToString as method svcbStringToKey updated after key 0 :bug: mandatory has missing key Rename str idx < 0 * Refactor 3 Use l.token as z var key, value string Comment wrap 0: Sentences with '.' keyValue => kv * Refactor 4 * Refactor 5 len() int * Refactor 6 * Refactor 7 * Test remove parsing * Error messages * Rewrite two estimate comments * parse shouldn't modify original array :bug: * Remove two unneeded comments * Address review comments Push 2 because can't build fuzzer python Push 3 to try again * Simplify argument duplication as per tmthrgd's suggestion And add the relevant test Force push edit: Make sorting code fit into one line * Rewrite ECHConfig and address the review * Remove the optional tab * Add To4() Check * More cleanup and fix mandatory not sorting bug
Fixes #166