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

Issue628 dns names #637

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Issue628 dns names #637

wants to merge 2 commits into from

Conversation

ekr
Copy link
Collaborator

@ekr ekr commented Nov 25, 2024

No description provided.

@ekr ekr requested a review from chris-wood as a code owner November 25, 2024 01:32
@ekr
Copy link
Collaborator Author

ekr commented Nov 25, 2024

@paulwouters: PTAL

Copy link
Contributor

@martinthomson martinthomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of the MUST here. I have a proposal (unpublished) that points out that the public_name field can be any value as long as the client has a means of validating it.

That means that as long as the client needs to follow the validation rules in this document, then it will need to validate the public name on the basis of it being valid. But that's just one way to perform that validation (and a particularly expensive one as it happens). It is entirely possible to define other validation methods that do not require that the name be valid for DNS.

If I may, can I suggest instead that these requirements be moved to {{auth-public-name}} with a note there that says that clients MAY ignore ECH configs for which validation is obviously impossible. Then you would list there things like the IP address matching and name construction as examples of things for which it is impossible to obtain a valid DNS-backed certificate.

Comment on lines +299 to +301
requirement). Additionally, clients MUST ignore the `ECHConfig` if the
length of any label in the DNS name is longer than 63 octets, as this
is the maximum length of a DNS label.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many would properly ignore such an ECH config?

Comment on lines +296 to +299
IPv4 address {{!RFC791}} in textual or hexadecimal form (IPv6
addresses are invalid DNS names due to the presence of the ":"
character, and thus are excluded by the previous
requirement). Additionally, clients MUST ignore the `ECHConfig` if the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The concept of a "valid DNS name" is not well-defined. In general, DNS "labels" can contain any sequence of up to 63 octets. There is no uniform standard for representing DNS names as strings.

IPv6 addresses are not valid hostnames, nor are they permissible FQDNs under ICANN rules, but that is not the same thing.

I don't think we need the parenthetical anyway though.

Suggested change
IPv4 address {{!RFC791}} in textual or hexadecimal form (IPv6
addresses are invalid DNS names due to the presence of the ":"
character, and thus are excluded by the previous
requirement). Additionally, clients MUST ignore the `ECHConfig` if the
IPv4 address {{!RFC791}} in textual or hexadecimal form.
Additionally, clients MUST ignore the `ECHConfig` if the

martinthomson added a commit to martinthomson/draft-ietf-tls-esni that referenced this pull request Nov 26, 2024
This is partly motivated by my interests in doing something evil,
but mostly this is because coupling name validity to ECH config
validity is a layering violation.  It's fine to invoke some name
validation, but that should be dictated by the needs of the thing that
ends up relying on that name.

This corrects both problems.

In doing this, I realized that RFC 791 says nothing about the IP address
textual format.  That's problematic, but I couldn't come up with
anything better in short notice.

Closes tlswg#628.
Closes tlswg#637.
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

Successfully merging this pull request may close these issues.

4 participants