-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add hostname validation to certs uploaded to nexus #4100
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
luqmana
reviewed
Sep 19, 2023
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.
Thanks for hooking up the rest of this!
luqmana
approved these changes
Sep 19, 2023
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.
Thanks John!
jgallagher
added a commit
that referenced
this pull request
Dec 11, 2023
The additional cert validation added in #4100 broke the ability for silo admins to upload new certs, because it introduced a call to fetch the rack DNS configuration (in order to assemble the FQDNs for the silo to check that the cert is valid for them). This PR fixes that by using an elevated internal privilege for that DNS config lookup. Fixes #4532.
1 task
jgallagher
added a commit
that referenced
this pull request
Dec 12, 2023
The additional cert validation added in #4100 broke the ability for silo admins to upload new certs, because it introduced a call to fetch the rack DNS configuration (in order to assemble the FQDNs for the silo to check that the cert is valid for them). This PR fixes that by using an elevated internal privilege for that DNS config lookup. Fixes #4532.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the
CertificateValidator
from taking "0 or 1" hostname to validate to taking a list of hostnames, and the cert passes validation if any hostname from the list matches. This is to support a future world where we support adding and removing external DNS zone names, where an operator might perform an operation like:myrack.oxide.computer
, and every silo has a$SILO.sys.myrack.oxide.computer
certsomething-else.oxide.computer
$SILO.sys.something-else.oxide.computer
myrack.oxide.computer
To be clear this is a hypothetical world, because today we don't support modifying the external DNS name at all. But step 3 from this sequence is the argument for why the validation logic is "any of the hostnames" instead of "all of the hostnames": we want to add certs for the new hostname (that don't need to also work for the old hostname we're about to remove).
The check is performed three places:
In all cases we build the list of hostnames for the validator by combining the list of external DNS zone names (which is currently always length 1) with the silo's
.sys
prefix. For the recovery silo specifically we build the list of length 1 by hand; for other silos and when adding to an existing silo, we query CRDB for all external DNS zone names.I haven't yet deployed this to a full system and want to do that before merging, but this passes tests and we're about to have a busy week, so I wanted to go ahead and open it as an easy way to keep it on the radar.
Fixes #4045.