From 580de0565b998d543596ff332c71d2ee23d75119 Mon Sep 17 00:00:00 2001 From: x Date: Wed, 30 Apr 2025 22:24:26 -0400 Subject: [PATCH] review fixes --- ...203304_certificates-ca-relation-removal.ts | 10 ++-- .../server/routes/v1/certificate-router.ts | 6 +- .../certificate/certificate-service.ts | 55 ++++++++++++------- .../components/CertificateImportModal.tsx | 4 +- 4 files changed, 46 insertions(+), 29 deletions(-) diff --git a/backend/src/db/migrations/20250429203304_certificates-ca-relation-removal.ts b/backend/src/db/migrations/20250429203304_certificates-ca-relation-removal.ts index 1413f5096..411ef171e 100644 --- a/backend/src/db/migrations/20250429203304_certificates-ca-relation-removal.ts +++ b/backend/src/db/migrations/20250429203304_certificates-ca-relation-removal.ts @@ -4,11 +4,6 @@ import { TableName } from "../schemas"; export async function up(knex: Knex): Promise { if (await knex.schema.hasTable(TableName.Certificate)) { - await knex.schema.alterTable(TableName.Certificate, (t) => { - t.uuid("caId").nullable().alter(); - t.uuid("caCertId").nullable().alter(); - }); - const hasProjectIdColumn = await knex.schema.hasColumn(TableName.Certificate, "projectId"); if (!hasProjectIdColumn) { await knex.transaction(async (trx) => { @@ -29,6 +24,11 @@ export async function up(knex: Knex): Promise { }); }); } + + await knex.schema.alterTable(TableName.Certificate, (t) => { + t.uuid("caId").nullable().alter(); + t.uuid("caCertId").nullable().alter(); + }); } } diff --git a/backend/src/server/routes/v1/certificate-router.ts b/backend/src/server/routes/v1/certificate-router.ts index 6984aadcb..271e0f776 100644 --- a/backend/src/server/routes/v1/certificate-router.ts +++ b/backend/src/server/routes/v1/certificate-router.ts @@ -192,8 +192,8 @@ export const registerCertRouter = async (server: FastifyZodProvider) => { projectSlug: z.string().trim().min(1).describe(CERTIFICATES.IMPORT.projectSlug), certificatePem: z.string().trim().min(1).describe(CERTIFICATES.IMPORT.certificatePem), - privateKeyPem: z.string().trim().describe(CERTIFICATES.IMPORT.privateKeyPem), - chainPem: z.string().trim().describe(CERTIFICATES.IMPORT.chainPem), + privateKeyPem: z.string().trim().min(1).describe(CERTIFICATES.IMPORT.privateKeyPem), + chainPem: z.string().trim().min(1).describe(CERTIFICATES.IMPORT.chainPem), friendlyName: z.string().trim().optional().describe(CERTIFICATES.IMPORT.friendlyName), pkiCollectionId: z.string().trim().optional().describe(CERTIFICATES.IMPORT.pkiCollectionId) @@ -491,7 +491,7 @@ export const registerCertRouter = async (server: FastifyZodProvider) => { ...req.auditLogInfo, projectId: cert.projectId, event: { - type: EventType.DELETE_CERT, + type: EventType.GET_CERT_BODY, metadata: { certId: cert.id, cn: cert.commonName, diff --git a/backend/src/services/certificate/certificate-service.ts b/backend/src/services/certificate/certificate-service.ts index 5b1f30c8a..aa2456349 100644 --- a/backend/src/services/certificate/certificate-service.ts +++ b/backend/src/services/certificate/certificate-service.ts @@ -279,29 +279,49 @@ export const certificateServiceFactory = ({ // Verify the certificate chain const chainCerts = splitPemChain(chainPem).map((pem) => new x509.X509Certificate(pem)); + + // Remove leaf cert from the chain if it's present + if (chainCerts[0].equal(leafCert)) { + chainCerts.splice(0, 1); + } + if (chainCerts.length === 0) { throw new BadRequestError({ message: "Certificate chain must contain at least one issuer certificate" }); } - const chainValidationPromises = chainCerts.map((issuerCert) => - leafCert.verify({ publicKey: issuerCert.publicKey }).catch(() => false) - ); + // Verify leaf certificate is signed by the first certificate in the chain + const isLeafVerified = await leafCert.verify({ publicKey: chainCerts[0].publicKey }).catch(() => false); + if (!isLeafVerified) { + throw new BadRequestError({ message: "Leaf certificate verification against chain failed" }); + } - const results = await Promise.all(chainValidationPromises); + // Verify the entire chain of trust + const verificationPromises = chainCerts.slice(0, -1).map(async (currentCert, index) => { + const issuerCert = chainCerts[index + 1]; + return currentCert.verify({ publicKey: issuerCert.publicKey }).catch(() => false); + }); - if (!results.some((result) => result === true)) { - throw new BadRequestError({ message: "Certificate chain verification failed" }); + const verificationResults = await Promise.all(verificationPromises); + + if (verificationResults.some((result) => !result)) { + throw new BadRequestError({ + message: "Certificate chain verification failed: broken trust chain" + }); } // Verify private key matches the certificate + let privateKey; try { - const message = Buffer.from("certificate-verification-test"); + privateKey = createPrivateKey(privateKeyPem); + } catch (err) { + throw new BadRequestError({ message: "Invalid private key format" }); + } - const privateKey = createPrivateKey(privateKeyPem); + try { + const message = Buffer.from(Buffer.alloc(32)); const publicKey = createPublicKey(certificatePem); - const signature = sign(null, message, privateKey); const isValid = verify(null, message, publicKey, signature); @@ -309,7 +329,10 @@ export const certificateServiceFactory = ({ throw new BadRequestError({ message: "Private key does not match certificate" }); } } catch (err) { - throw new BadRequestError({ message: "Invalid private key format" }); + if (err instanceof BadRequestError) { + throw err; + } + throw new BadRequestError({ message: "Error verifying private key against certificate" }); } // Get certificate attributes @@ -394,15 +417,9 @@ export const certificateServiceFactory = ({ return txCert; } catch (error: unknown) { - if ( - typeof error === "object" && - error !== null && - "error" in error && - error.error && - typeof error.error === "object" && - "code" in error.error && - error.error.code === "23505" - ) { + // @ts-expect-error We're expecting a database error + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + if (error?.error?.code === "23505") { throw new BadRequestError({ message: "Certificate serial already exists in your project" }); } throw error; diff --git a/frontend/src/pages/cert-manager/CertificatesPage/components/CertificateImportModal.tsx b/frontend/src/pages/cert-manager/CertificatesPage/components/CertificateImportModal.tsx index 1272b067c..2331d0810 100644 --- a/frontend/src/pages/cert-manager/CertificatesPage/components/CertificateImportModal.tsx +++ b/frontend/src/pages/cert-manager/CertificatesPage/components/CertificateImportModal.tsx @@ -22,8 +22,8 @@ import { CertificateContent } from "./CertificateContent"; const schema = z.object({ certificatePem: z.string().trim().min(1, "Certificate PEM is required"), - privateKeyPem: z.string().trim(), - chainPem: z.string().trim(), + privateKeyPem: z.string().trim().min(1, "Private Key PEM is required"), + chainPem: z.string().trim().min(1, "Certificate Chain PEM is required"), friendlyName: z.string(), collectionId: z.string().optional()