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

Add hostname validation to certs uploaded to nexus #4100

Merged
merged 7 commits into from
Sep 20, 2023

Conversation

jgallagher
Copy link
Contributor

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:

  1. The rack has an external name of myrack.oxide.computer, and every silo has a $SILO.sys.myrack.oxide.computer cert
  2. Add a new external name of something-else.oxide.computer
  3. Upload new certs to all silos with SAN $SILO.sys.something-else.oxide.computer
  4. Remove the external name 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:

  1. When the recovery silo is created (which is maybe wrong? I could see an argument for omitting this check, although since wicket is also performing it it should certainly pass)
  2. When any other silo is created
  3. When a cert is uploaded to an existing silo

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.

Copy link
Contributor

@luqmana luqmana left a 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!

certificates/src/lib.rs Outdated Show resolved Hide resolved
certificates/src/lib.rs Show resolved Hide resolved
certificates/src/lib.rs Show resolved Hide resolved
certificates/src/lib.rs Outdated Show resolved Hide resolved
nexus/src/app/certificate.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@luqmana luqmana left a comment

Choose a reason for hiding this comment

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

Thanks John!

@jgallagher jgallagher merged commit 373ddbd into main Sep 20, 2023
20 checks passed
@jgallagher jgallagher deleted the nexus-ssl-hostname-validation branch September 20, 2023 15:23
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.
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Validate CN and/or SAN in certificate matches external dns zone during RSS.
2 participants