Properly parse alpn values in SVCB #1363
Conversation
|
I thought it as already an RFC, but still an I-D https://datatracker.ietf.org/doc/html/draft-ietf-dnsop-svcb-https-08 then you should be able to get some changes through, esp. with I tried to code this and it was horrible. |
svcb.go
Outdated
| default: | ||
| str.WriteByte(e) | ||
| } | ||
| } else { |
There was a problem hiding this comment.
reverse this if, so the else part is just done + a continue and the current if-body can be pulled to the left
There was a problem hiding this comment.
Makes sense, done.
svcb.go
Outdated
| } | ||
| esc[i] = str.String() | ||
| } | ||
| return strings.Join(esc, ",") |
There was a problem hiding this comment.
can we just build the string in str that's already a Builder, so we can just add the new string there. + of course some fiddling for the last element
There was a problem hiding this comment.
Yes, this is straightforward. I should have done that to begin with, but I started with the code for SVCBLocal and didn't really think it through. 馃槀 Done!
svcb_test.go
Outdated
| t.Error("failed to parse RR: ", err) | ||
| continue | ||
| } | ||
| if !reflect.DeepEqual(v, rr.(*SVCB).Value[0].(*SVCBAlpn).Alpn) { |
There was a problem hiding this comment.
just range over the slices, no real need to pull in reflect for this
There was a problem hiding this comment.
Okay, I've refactored this. I figured that since this was test code reflect would be fine.
|
I think that all of the feedback has been addressed. |
|
As I mentioned on #1366, I think this PR is as reasonable and should be merged. I checked on the IETF dnsop list and apparently this extra escaping is just how it is, because parsing pain is apparently no reason to change a standard. 馃し |
Co-authored-by: Miek Gieben <miek@miek.nl>
* Modify the SVCBAlpn to properly parse/print * Remove debug * Change SVCB test from reflect to loop * Refactor SVCB code to reduce indentation * When stringifying SVCBAlpn, use strings.Builder for whole process * Update comment in svcb.go Co-authored-by: Miek Gieben <miek@miek.nl> * Describe why we use a specific size for the alpn buffer Co-authored-by: Miek Gieben <miek@miek.nl>
PR for #1361
This code adds more comprehensive parsing and string conversion to the alpn support in the HTTPS/SVCB types.
Before we were just splitting on
,, but this version actually parses as per the draft, which allows alpn with a,in the value. This adds a ton of error cases and a fair bit of code, so I'm hoping the RFC simplifies this to just forbidding,in the alpn completely (am I too late to suggest that?). On the plus side, the examples in the tests now actually return correct values.I used the existing
nextByte()function in thetypes.gofile, since I needed the same functionality. This means that we allow incorrect values like\456, but I figured fixing that was out of scope. Is it worth a separate PR? 馃 Note that the parser forSVCBLocalusesstrconv.ParseUint()and should actually bounds check, so we have different strictness/interpretations about escaped characters already.The
String()is also updated to handle outputting non-printable values, and also handles the double-escaping specified in the draft. I decided to output\\044instead of\,so that naive parsers (like the old one) would at least have a chance to split the output into something like what it should be (since there is no,because we use the numeric version). This shouldn't have any impact on the actual meaning, but I did have to update a couple of tests based on this format.I added a few more tests to sanity check that everything is working as it is supposed to.
I noticed that the "dohpath" support was merged in while I was working on this. This introduces another place where
String()will possibly produce incorrect output. 馃槥 I'll have a look tomorrow....