Repository navigation
Silo certificates tab - #3410
david-crespo wants to merge 7 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@claude review (testing this feature) |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
app/pages/system/silos/SiloCertificatesTab.tsx— After deleting a silo certificate, fleet admins get a toast naming the certificate's UUID rather than the name they clicked. The toast readsvariables.path.certificate. The delete call at line 130 sets that value tocert.id, because the silo selector has to be left out when the cert is given by ID. Fix: take the name from the row that was deleted, not from the request path. For example, showcert.namein the toast from thedoDeleteclosure, or pass the name toonSuccesssome other way, and keep the ID-based delete call unchanged.Why this was flagged
An admin opens /system/silos/maze-war/certificates and picks Delete from the row menu for
maze-war-cert. ThedoDeletecall at SiloCertificatesTab.tsx:539 isdeleteCert({ path: { certificate: cert.id } }), sovariables.path.certificateholds the UUID7d36cbfc-9dba-4600-9816-323a22b4c719. TheonSuccesshandler at SiloCertificatesTab.tsx:527 shows that value, so the toast says "Certificate 7d36cbfc-... deleted" instead of the name the admin selected in the confirm modal. Every other delete toast in this flow, such as the create toast at silo-certificate-create.tsx:207, shows the resource name. The e2e test in test/e2e/silos.e2e.ts only closes the toast and never checks its text, so it doesn't catch this.Verification: nit. In app/pages/system/silos/SiloCertificatesTab.tsx, the
onSuccess(_data, variables)handler on lines 114-118 showsCertificate <HL>{variables.path.certificate}</HL> deleted. Line 130 callsdeleteCert({ path: { certificate: cert.id } }). So the toast shows the certificate's UUID, while the confirm modal shows the name (label: cert.name, line 131). Only the toast text is wrong.
| sha256Fingerprint: toHex(await cert.getThumbprint('SHA-256')).join(':'), | ||
| keyAlgorithm: describeKey(cert.publicKey.algorithm), | ||
| selfSigned: cert.subject === cert.issuer, | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Parse every cert in a PEM chain, leaf first by convention. Returns null if | ||
| * there are no PEM blocks or any block fails to parse. | ||
| */ | ||
| export async function parseCertChain(pem: string): Promise<ParsedCert[] | null> { | ||
| const blocks = pem.match(PEM_BLOCKS_RE) | ||
| if (!blocks) return null | ||
| try { | ||
| return await Promise.all(blocks.map(parseOne)) | ||
| } catch { | ||
| return null |
There was a problem hiding this comment.
🟡 (optional) If the console is opened over plain HTTP (not localhost), the certificate detail view shows "Couldn't parse certificate" and hides every parsed field, even for valid certs. The fingerprint at line 47 calls cert.getThumbprint('SHA-256'), which uses WebCrypto crypto.subtle. Browsers leave that undefined outside secure contexts, so the call throws. The catch at lines 62-63 then turns the whole chain into null. Fix: let the fingerprint fail on its own, e.g. catch around getThumbprint and leave the fingerprint unset, so domains, issuer, validity, serial and the chain still render. ChainList's key={c.sha256Fingerprint} then needs a fallback key.
Why this was flagged
The trigger is the console being served from an insecure origin, for example a rack or dev setup reached at http://. In that setup window.crypto.subtle is undefined. Opening /system/silos/:silo/certificates/:certificate runs parseCertChain(cert.cert) (silo-certificate-view.tsx). That function awaits Promise.all(blocks.map(parseOne)) (certificate.ts:61). Inside parseOne, line 47 awaits cert.getThumbprint('SHA-256'), which calls crypto.subtle.digest and throws. The bare catch { return null } at certificate.ts:62-63 throws away all the fields that parsed fine. The view then takes the !isPending && !leaf branch and shows the notice "Couldn't parse certificate" / "Expected one or more X.509 certificates in PEM format." for a valid PEM, with no Domains, Issuer, Expires or Chain section. This is new code, so the base has no equivalent. No safeguard isolates the one optional async field from the rest of the parse.
Verification: Mechanism: app/util/certificate.ts:47 computes sha256Fingerprint: toHex(await cert.getThumbprint('SHA-256')).join(':') inside parseOne. On an http non-localhost origin it is undefined, so the digest call throws and getThumbprint rejects. parseCertChain runs return await Promise.all(blocks.map(parseOne)) with catch { return null } (lines 60-64).
| siloCertificateList({ query, cookies }) { | ||
| requireFleetViewer(cookies) | ||
| const silo = lookup.silo({ silo: query.silo }) | ||
| const certs = db.certificates | ||
| .filter((c) => c.siloId === silo.id) |
There was a problem hiding this comment.
🟡 (optional) In the mock API, a silo created with TLS certificates shows an empty Certificates tab. The new tab reads only db.certificates. The existing siloCreate handler never writes body.tls_certificates to that table. Fix: in siloCreate (handlers.ts:1943), push each entry of tls_certificates into db.certificates with the new silo's siloId, dropping key. Also leave tls_certificates out of the stored silo object, so the private keys are not stored on it.
Why this was flagged
In dev or e2e against the mock, a user creates a silo through the silo-create form and adds a cert with the TLS certs field. siloCreate (mock-api/msw/handlers.ts:1943-1962) spreads ...body into the silo record, tls_certificates included. It never inserts anything into db.certificates. The new siloCertificateList handler filters db.certificates by siloId (handlers.ts:2505-2508). So /system/silos//certificates shows "No certificates", even though the silo was created with one. Real Nexus stores those certs as the silo's certificates. Before this change no UI read silo certificates, so the gap was invisible. The new tab makes it reachable, and it shows mock behaviour that contradicts the API.
Verification: siloCreate (mock-api/msw/handlers.ts:1943-1962) pushes to db.silos, siloQuotas, siloProvisioned and siloSettings, and never to db.certificates. The new siloCertificateList (handlers.ts:2505-2511) returns only db.certificates.filter((c) => c.siloId === silo.id). So every silo created in the mock shows "No certificates" on its tab, even though the user just added one.
|
Decent result, those are real issues. Not sure we care about the http thing, but we can do a fallback anyway. |
Prototype to make sure oxidecomputer/omicron#11470 is solid. Needs polish but it works. I think for deletes we'll want to do the same thing we're discussing for deleting your own role, making the user type something to confirm it.