Initial cert validation test#2582
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…mputer/console into tls-cert-soft-validation
|
Added tests, improved wildcard handling and also a message in case the certificate cannot be parsed. 
Message copy needs work. |
|
Do we have a good way of getting the domain? I vaguely remember some discussion around with the IdP setup. |
|
This looks awesome, thanks for picking it up. I think initial rack install will certainly benefit from it. |
Do we need some copy to caveat incase that is not correct. I remember some talk about a proxy perhaps throwing it off? |
| const { data: certValidation } = useQuery({ | ||
| queryKey: ['validateImage', ...(file ? [file.name, file.size, file.lastModified] : [])], | ||
| queryFn: file ? () => file.text().then(parseCertificate) : skipToken, | ||
| }) |
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.
|
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.
|
Updated from main and a little review: Major
Smaller polish
Tests
Can we get one last sanity check and revisit merging? |
|
I took out the dynamic import in 765210b because even though technically certs aren't required by the API in the silo create body, in practice they are necessarily pretty much all the time. So that import will always happen, might as well absorb it into the chunk for the form. Had the robot compare before/after: 
|
4 participants
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.