Initial cert validation test#2582
Open
benjaminleonard wants to merge 20 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
benjaminleonard
commented
Nov 27, 2024
…mputer/console into tls-cert-soft-validation
Contributor
Author
|
Added tests, improved wildcard handling and also a message in case the certificate cannot be parsed. 
Message copy needs work. |
Contributor
Author
|
Do we have a good way of getting the domain? I vaguely remember some discussion around with the IdP setup. |
Contributor
|
This looks awesome, thanks for picking it up. I think initial rack install will certainly benefit from it. |
Contributor
Author
Do we need some copy to caveat incase that is not correct. I remember some talk about a proxy perhaps throwing it off? |
david-crespo
reviewed
Jan 7, 2025
| const { data: certValidation } = useQuery({ | ||
| queryKey: ['validateImage', ...(file ? [file.name, file.size, file.lastModified] : [])], | ||
| queryFn: file ? () => file.text().then(parseCertificate) : skipToken, | ||
| }) |
Collaborator
There was a problem hiding this comment.
@charliepark pointed out we might want to set the stale time directly here. I'm going to experiment with that and also maybe tweaking the query key.
Collaborator
|
Tweaked the copy and fixed a bug where it would only do validation if the silo name was non-empty in the parent form. I changed it so it only opts out of the part of the validation that requires the silo name. 
|
The hardcoded r2.oxide-preview.com produces a false "domain mismatch" warning on every rack that isn't r2 (dogfood, colo, customer installs). Reuse the same helper the IdP create form uses to derive the suffix from window.location.
- Rename stale 'validateImage' query key to 'validateCert'
- Drop redundant *.sys.<domain> match arm: it's subsumed by
expectedDomain matching (a *.sys.<domain> SAN already matches
<silo>.sys.<domain>) and only the early return guards the
empty-siloName case anyway
- Rename commonName to commonNames (it's a string[])
- Spread certValidation ?? {} so the undefined-spread isn't subtle
- Tighten "Could not be parsed" copy
getField('CN') and .map() both already return string[]. Replace the
hand-wavy bare-'*' comment with the RFC 6125 reference.
- Normalize case and strip trailing dots on both pattern and domain up
front (RFC 6125 §6.4 case-insensitive, RFC 4592 trailing-dot FQDN).
Wildcard branch was previously case-sensitive.
- Reject pathological '*.' which would have matched any 2-label domain
via endsWith('') being true.
Users frequently paste a chain (leaf + intermediates). The previous single-PEM parse would throw and report "Couldn't parse" for a valid file. Extract the first PEM block (the leaf, by convention) and parse that. Side benefit: tolerates leading whitespace / BOM.
parseCertificate now returns notAfter; CertDomainNotice renders a 'notice'-variant message when the cert is past its expiry. Expiry and domain-mismatch warnings can stack — they're independent. Factored the duplicated docs-link block into a SiloCertsDocsLink helper.
- Assert "Couldn't parse certificate" appears after the existing garbage chooseFile upload - Swap the second cert upload to a real (expired, non-matching) PEM and assert both the expiry and mismatch notices render, plus that the parse-failure notice is gone
Before the bad-PEM step, upload a self-signed cert whose SANs cover other-silo.sys.placeholder and isn't expired. Confirms the soft-validation notices are absent in the happy path, not just present in the unhappy one.
Contributor
Author
|
Updated from main and a little review: Major
Smaller polish
Tests
Can we get one last sanity check and revisit merging? |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #2580 and related to #2578
Idea from @augustuswm to soft validate the cert. Unfortunately I think we need a library to parse the cert but I lazy imported it to avoid adding to the main bundle since most users will not see this.
Principally the risk here is what I dont know about certs:
parseCertificate()doesn't catchBut feels, like the image soft validation, that it could be a good quality of life improvement for something that is potentially very painful when not done correctly. We might want to label the
CNandSANon the messages list of found names.