Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why allow multiple values for the same parameter at all? #95

Closed
davidben opened this issue Dec 17, 2019 · 10 comments
Closed

Why allow multiple values for the same parameter at all? #95

davidben opened this issue Dec 17, 2019 · 10 comments

Comments

@davidben
Copy link
Contributor

It seems the only parameters which allow this is ipv4hint and ipv6hint, which is not necessary because the value serialization supports multiple addresses (#94).

Why do we need this? I think we should only allow one value per parameter.

This means duplicate values can be rejected generically in the parser. Right now the spec says individual parameters silently ignore all but the first value. Silently ignoring things isn't great. Other protocols like Roughtime further require keys be sorted, which makes it easy for the parser to reject duplicates generically.

This also avoids questions when designing a new parameter as to whether to use multiple parameter values or to encode multiple structures in the value. We should have one way to do things, so let's use the latter.

@enygren
Copy link
Collaborator

enygren commented Dec 17, 2019

The other argument would be to always do the former (multiple parameter values) in the wireformat and to strongly discourage things from having values that encode multiple structures in the value. Presentation format multi-value parameters could be syntactic sugar and convert to to multiple parameters on the wire

@enygren
Copy link
Collaborator

enygren commented Dec 17, 2019

Agreed that we should be consistent here.

Presumably ESNIConfig is another use-case?

@davidben
Copy link
Contributor Author

davidben commented Dec 17, 2019

I believe HTTPSSVC's ESNI story, as currently specified, does not allow for multiple ESNIConfigs. It doesn't say anything and thus gets the default. This is indeed a mistake, but we're fixing it by doing the multiple values at the TLS level, which lets us handle it consistently at all sources of ESNI config. (See #88.)

It's also generally more compact to handle multiplicity within the value. Each HTTPSSVC value costs four bytes of framing per value. If the value is self-delimiting (true for addresses and ESNIConfig), you have a one-time four byte cost from HTTPSSVC, then each additional value costs no extra framing. If the value is not self-delimiting, you need length prefixes internally, costing two bytes per value. That's two bytes extra at one value, breaking even at two values, and a win beyond that.

(The difference comes from not repeating the parameter name each time.)

@davidben
Copy link
Contributor Author

@dmcardle FYI

@enygren
Copy link
Collaborator

enygren commented Dec 19, 2019

Your argument seems reasonable. We should make this clear and deterministic especially at the wireformat layer (eg, must only use the first instance of a parameter, provisioning systems should warn on duplicates, etc).

Allowing syntactic sugar at the presentation layer (eg, concatenation of parameters that are duplicated?) might be helpful usability-wise or could create many more problems and instead we should recommend that provisioning systems prevent this (or at least warn).

@bemasc
Copy link
Collaborator

bemasc commented Jan 15, 2020

I agree with this proposal, but it's going to collide heavily with #97, so I'd prefer to get that settled before updating the text. I have updated #97 to make transport names easier to delimit.

@dmcardle
Copy link

Could we revisit this? Given the current state of #97, it looks like it is expecting that multiple values are allowed, e.g. transport=foo transport=bar no-default-transport.

Equivalently, the wire format could simply encode a length-prefixed sequence of protocol IDs, eliminating the need for repeated SvcParamKeys. A slight annoyance is that the protocol IDs don't have fixed lengths. We could (1) invent uint16 values in our registry, (2) add length prefixes to each protocol ID, or (3) use null terminators.

@davidben
Copy link
Contributor Author

I think I've lost track of what "this" is and what is being revisited where. :-) I think the encoding should be:

  • You only get one parameter per key, with one value.
  • As is already the case, that value has whatever internal format it needs. That internal format may be a single structure, a list of structures, or something more complicated.
  • Duplicate parameters are a syntax error. The receiver treats this as a parse error.
  • Keys must be listed in ascending order, so the receiver can check for duplicates without maintaining much state. Non-sorted keys are a parse error.

Re protocol IDs not having fixed lengths, just add length prefixes as needed. That's all a multi-valued parameter is doing anyway. (I don't think we should use NUL-termination. That sort of thing is prone to injection problems. See also all the problems when things get stuck into C-style strings.)

(I still need to review #97, but I doubt a vector<transport> would work anyway given that you need a port number. If it ends up being a vector<tuple<transport, port>> or a different design, that further suggests a key-specific serialization. A vector<tuple<transport, port>> needs a length prefix on the transport, not on the overall tuple.)

@bemasc
Copy link
Collaborator

bemasc commented Feb 3, 2020

I think we pretty much have consensus on this encoding.

Since #97 is a major semantic change, I'd like to get it finalized and merged before we make this syntax change.

Regarding ports, the current plan is to have a single fixed port number for each SvcFieldValue, even if it has multiple transports. This port number applies to all transports. Endpoints with different port numbers will only be representable as separate RRs within the RRSet. I'm not aware of a use case that would motivate putting multiple port numbers in one RR.

bemasc pushed a commit that referenced this issue Feb 18, 2020
This includes some adjustments to make repeated keys
unnecessary.

Incorporates #94 by @davidben
Addresses #95
bemasc pushed a commit that referenced this issue Feb 18, 2020
This includes some adjustments to make repeated keys
unnecessary.

Incorporates #94 by @davidben
Addresses #95
enygren added a commit that referenced this issue Mar 5, 2020
* Only allow each SvcParamKey to appear once

This includes some adjustments to make repeated keys
unnecessary.

Incorporates #94 by @davidben
Addresses #95

* Reject the entire RRSet

* Reject the whole RRSet if any RR is malformed

Also clarify recursive resolver requirements.

* Incorporate review comments
@enygren
Copy link
Collaborator

enygren commented Apr 13, 2020

This has been incorporated.

@enygren enygren closed this as completed Apr 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants