-
Notifications
You must be signed in to change notification settings - Fork 588
Specify SAN validation precedence over Hostname validation #4039
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
Conversation
/approve |
// 3. If SubjectAltNames are specified, Hostname MUST NOT be used for authentication, | ||
// even if this would cause a failure in the case that the SubjectAltNames do not match. | ||
// If you want to use Hostname for authentication, you must add Hostname to the SubjectAltNames list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The wording here is tricky to get right. Trying to make the distinction between "hostname" as a concept, and as a field.
// 3. If SubjectAltNames are specified, Hostname MUST NOT be used for authentication, | |
// even if this would cause a failure in the case that the SubjectAltNames do not match. | |
// If you want to use Hostname for authentication, you must add Hostname to the SubjectAltNames list. | |
// 3. If SubjectAltNames are specified, only the values in that list will be used for authentication. The | |
// value of the Hostname field MUST NOT be used for authentication. If you want to use the value | |
// of the Hostname field for authentication, you MUST add it to the SubjectAltNames list. |
@youngnick might have a better suggestion here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the trick is to put the field Hostname
in backticks, and use the capitalized form, and use "hostname" for the concept.
How about this:
// 3. If SubjectAltNames are specified, Hostname MUST NOT be used for authentication, | |
// even if this would cause a failure in the case that the SubjectAltNames do not match. | |
// If you want to use Hostname for authentication, you must add Hostname to the SubjectAltNames list. | |
// 3. If SubjectAltNames are specified, `Hostname` MUST NOT be used for authentication, and the list of hostnames | |
// included in SubjectAltNames MUST be used instead. | |
// Generally, users SHOULD include the value of `Hostname` in their SubjectAltNames list unless they have a good reason not to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's great, thanks @youngnick!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rephrased it and used wording from GEP-3155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@youngnick @robscott PTAL
I added a suggestion here, but it's a small optimization, I think that either option is fine, and this change LGTM. |
Thanks @kl52752! /lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: candita, kl52752, youngnick The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind documentation
What this PR does / why we need it:
Documentation for BackendTLSPolicy is not complete regarding SAN validation.
Which issue(s) this PR fixes:
Relates to #3979
Does this PR introduce a user-facing change?: