Update SVCB #1341
Update SVCB #1341
Conversation
|
Breaking changes in this record I fine, it's still draft IIRC
That sgtm, internal consistency is a good thing to have |
| // | ||
| // It is incumbent upon the user of this library to reject the RRSet if | ||
| // or avoid constructing such an RRSet that: | ||
| // - "mandatory" is included as one of the keys of mandatory |
There was a problem hiding this comment.
if you want this to make a list in godoc you need empty lines before and after. Maybe just make this more prose?
There was a problem hiding this comment.
I can't turn this list into a paragraph; it would be unreadable.
svcb.go
Outdated
| type SVCBECHConfig struct { | ||
| ECH []byte | ||
| type SVCBECH struct { | ||
| ECHConfigList []byte // includes the redundant length prefix |
There was a problem hiding this comment.
this is too long. Just Config? What does the draft use?
There was a problem hiding this comment.
The draft uses ECHConfigList which is taken from the RFC draft of ECH/ESNI. Makes sense to keep the original name so that an ESNI dev can easily see what this represents.
|
@miekg Many thanks for your review. I have addresses your comments except "if you want this to make a list in godoc you need empty lines before and after. Maybe just make this more prose?" because I don't know how to turn this into a paragraph without making it unreadable.
On the SVCB side, they decided to update their example, after which I updated the relevant code here. How this library treats IPv6-IPv4 subtypes is now SVCB-compliant and self-consistent. |
There was a problem hiding this comment.
Most of the change is fine, but I really don't think we can or should make backwards incompatible changes. Nothing here was marked as WIP or anything. Considering the name changes don't have any actual impact on the code, I think they should be left as-is regardless of whether that differs from the draft spec or not. I'm fine with the other changes though.
|
Thanks for the review. I addressed the issues. |
|
Hello again @miekg . I have been impatiently waiting for Cloudflare to implement ECH over SVCB so it would be great if we could get this merged unless further reviews are planned. Many thanks. |
|
👍thank you |
* Rename ECH, bump draft number * AliasForm new treatment * alpn is no longer mandatory by default * Document the non-empty value requirement * new test cases * more test cases * Continue forbidding v4-map-v6 but not v4-embed-v6 miekg#1067 (comment) and MikeBishop/dns-alt-svc#361 * Update documentation * revert rename ech * Reword AliasMode with key=value pairs
The only breaking change worth mentioning is renaming SVCB_ECHCONFIG to SVCB_ECH. We can avoid this and use its legacy name internally if you wish.
I had chosen ( #1067 (comment) ) to forbid IPv4 addresses in ipv6hint to be consistent with the rest of the library. I don't know why ( MikeBishop/dns-alt-svc#361 ) but the SVCB spec requires otherwise.
@miekg @tmthrgd Would you like to review?