From 2b4a6ad9074d600286395f3b35b7f69b0351cbcd Mon Sep 17 00:00:00 2001 From: Sheen Capadngan Date: Mon, 19 May 2025 20:08:43 +0800 Subject: [PATCH] misc: addressed review comments --- .../20250512133213_add-external-ca-pki.ts | 10 +++------- backend/src/db/schemas/pki-subscribers.ts | 2 +- .../certificate-authority-endpoints.ts | 2 +- .../routes/v2/certificate-authority-router.ts | 1 - .../acme/acme-certificate-authority-fns.ts | 3 +++ .../acme/acme-certificate-authority-schemas.ts | 4 ++-- .../certificate-authority-queue.ts | 4 ++-- .../certificate-authority-service.ts | 2 +- .../services/certificate/certificate-dal.ts | 18 +++++++++++------- .../pki-subscriber/pki-subscriber-service.ts | 4 ++++ .../endpoints/pki/subscribers/issue-cert.mdx | 2 +- .../endpoints/pki/subscribers/order-cert.mdx | 4 ++++ docs/mint.json | 1 + frontend/src/hooks/api/pkiSubscriber/types.ts | 2 +- .../components/ExternalCaSection.tsx | 5 ++--- .../components/PkiSubscriberDetailsSection.tsx | 9 +++++---- 16 files changed, 42 insertions(+), 31 deletions(-) create mode 100644 docs/api-reference/endpoints/pki/subscribers/order-cert.mdx diff --git a/backend/src/db/migrations/20250512133213_add-external-ca-pki.ts b/backend/src/db/migrations/20250512133213_add-external-ca-pki.ts index 907022035..9ac60e5a0 100644 --- a/backend/src/db/migrations/20250512133213_add-external-ca-pki.ts +++ b/backend/src/db/migrations/20250512133213_add-external-ca-pki.ts @@ -12,12 +12,8 @@ export async function up(knex: Knex): Promise { t.uuid("certificateAuthorityId").nullable(); }); - const caRows = await knex(TableName.CertificateAuthority).select("*"); - if (caRows.length > 0) { - // @ts-expect-error intentional: migration - await knex(TableName.InternalCertificateAuthority).insert(caRows); - } - + // @ts-expect-error intentional: migration + await knex(TableName.InternalCertificateAuthority).insert(knex(TableName.CertificateAuthority).select("*")); await knex(TableName.InternalCertificateAuthority).update("certificateAuthorityId", knex.ref("id")); await knex.schema.alterTable(TableName.InternalCertificateAuthority, (t) => { @@ -90,7 +86,7 @@ export async function up(knex: Knex): Promise { t.string("ttl").nullable().alter(); t.string("lastOperationStatus"); t.text("lastOperationMessage"); - t.string("lastOperationAt"); + t.dateTime("lastOperationAt"); }); } } diff --git a/backend/src/db/schemas/pki-subscribers.ts b/backend/src/db/schemas/pki-subscribers.ts index 0f63d7fa8..e27fa0fd3 100644 --- a/backend/src/db/schemas/pki-subscribers.ts +++ b/backend/src/db/schemas/pki-subscribers.ts @@ -22,7 +22,7 @@ export const PkiSubscribersSchema = z.object({ status: z.string(), lastOperationStatus: z.string().nullable().optional(), lastOperationMessage: z.string().nullable().optional(), - lastOperationAt: z.string().nullable().optional() + lastOperationAt: z.date().nullable().optional() }); export type TPkiSubscribers = z.infer; diff --git a/backend/src/server/routes/v1/certificate-authority-routers/certificate-authority-endpoints.ts b/backend/src/server/routes/v1/certificate-authority-routers/certificate-authority-endpoints.ts index d5907bf12..701fb6d2e 100644 --- a/backend/src/server/routes/v1/certificate-authority-routers/certificate-authority-endpoints.ts +++ b/backend/src/server/routes/v1/certificate-authority-routers/certificate-authority-endpoints.ts @@ -204,7 +204,7 @@ export const registerCertificateAuthorityEndpoints = < server.route({ method: "DELETE", - url: `/:certificateAuthorityId`, + url: "/:certificateAuthorityId", config: { rateLimit: writeLimit }, diff --git a/backend/src/server/routes/v2/certificate-authority-router.ts b/backend/src/server/routes/v2/certificate-authority-router.ts index 0002de608..d8b434fdf 100644 --- a/backend/src/server/routes/v2/certificate-authority-router.ts +++ b/backend/src/server/routes/v2/certificate-authority-router.ts @@ -1,4 +1,3 @@ -/* eslint-disable @typescript-eslint/no-floating-promises */ import { z } from "zod"; import { EventType } from "@app/ee/services/audit-log/audit-log-types"; diff --git a/backend/src/services/certificate-authority/acme/acme-certificate-authority-fns.ts b/backend/src/services/certificate-authority/acme/acme-certificate-authority-fns.ts index 0e2361481..9fc8937a0 100644 --- a/backend/src/services/certificate-authority/acme/acme-certificate-authority-fns.ts +++ b/backend/src/services/certificate-authority/acme/acme-certificate-authority-fns.ts @@ -6,6 +6,7 @@ import { KeyObject } from "crypto"; import { TableName } from "@app/db/schemas"; import { BadRequestError, NotFoundError } from "@app/lib/errors"; import { OrgServiceActor } from "@app/lib/types"; +import { blockLocalAndPrivateIpAddresses } from "@app/lib/validator"; import { TAppConnectionDALFactory } from "@app/services/app-connection/app-connection-dal"; import { AppConnection, AWSRegion } from "@app/services/app-connection/app-connection-enums"; import { decryptAppConnection } from "@app/services/app-connection/app-connection-fns"; @@ -406,6 +407,8 @@ export const AcmeCertificateAuthorityFns = ({ ); } + await blockLocalAndPrivateIpAddresses(acmeCa.configuration.directoryUrl); + const acmeClient = new acme.Client({ directoryUrl: acmeCa.configuration.directoryUrl, accountKey diff --git a/backend/src/services/certificate-authority/acme/acme-certificate-authority-schemas.ts b/backend/src/services/certificate-authority/acme/acme-certificate-authority-schemas.ts index 44990feb6..56b3118cf 100644 --- a/backend/src/services/certificate-authority/acme/acme-certificate-authority-schemas.ts +++ b/backend/src/services/certificate-authority/acme/acme-certificate-authority-schemas.ts @@ -11,13 +11,13 @@ import { import { AcmeDnsProvider } from "./acme-certificate-authority-enums"; export const AcmeCertificateAuthorityConfigurationSchema = z.object({ - dnsAppConnectionId: z.string().trim().describe(CertificateAuthorities.CONFIGURATIONS.ACME.dnsAppConnectionId), + dnsAppConnectionId: z.string().uuid().trim().describe(CertificateAuthorities.CONFIGURATIONS.ACME.dnsAppConnectionId), // soon, differentiate via the provider property dnsProviderConfig: z.object({ provider: z.nativeEnum(AcmeDnsProvider).describe(CertificateAuthorities.CONFIGURATIONS.ACME.provider), hostedZoneId: z.string().trim().min(1).describe(CertificateAuthorities.CONFIGURATIONS.ACME.hostedZoneId) }), - directoryUrl: z.string().trim().min(1).describe(CertificateAuthorities.CONFIGURATIONS.ACME.directoryUrl), + directoryUrl: z.string().url().trim().min(1).describe(CertificateAuthorities.CONFIGURATIONS.ACME.directoryUrl), accountEmail: z.string().trim().min(1).describe(CertificateAuthorities.CONFIGURATIONS.ACME.accountEmail) }); diff --git a/backend/src/services/certificate-authority/certificate-authority-queue.ts b/backend/src/services/certificate-authority/certificate-authority-queue.ts index 192c1ae87..74970bf0c 100644 --- a/backend/src/services/certificate-authority/certificate-authority-queue.ts +++ b/backend/src/services/certificate-authority/certificate-authority-queue.ts @@ -156,7 +156,7 @@ export const certificateAuthorityQueueFactory = ({ await pkiSubscriberDAL.updateById(subscriberId, { lastOperationStatus: SubscriberOperationStatus.SUCCESS, lastOperationMessage: "Certificate ordered successfully", - lastOperationAt: new Date().toISOString() + lastOperationAt: new Date() }); } } catch (e: unknown) { @@ -164,7 +164,7 @@ export const certificateAuthorityQueueFactory = ({ await pkiSubscriberDAL.updateById(subscriberId, { lastOperationStatus: SubscriberOperationStatus.FAILED, lastOperationMessage: e.message, - lastOperationAt: new Date().toISOString() + lastOperationAt: new Date() }); } logger.error(e, `CaOrderCertificate Failed [subscriberId=${subscriberId}] [job=${job.name}]`); diff --git a/backend/src/services/certificate-authority/certificate-authority-service.ts b/backend/src/services/certificate-authority/certificate-authority-service.ts index b4000c678..7979cf684 100644 --- a/backend/src/services/certificate-authority/certificate-authority-service.ts +++ b/backend/src/services/certificate-authority/certificate-authority-service.ts @@ -140,7 +140,7 @@ export const certificateAuthorityServiceFactory = ({ type, disableDirectIssuance: ca.disableDirectIssuance, name: ca.internalCa?.friendlyName, - projectId, + projectId: finalProjectId, status, configuration: ca.internalCa } as TCertificateAuthority; diff --git a/backend/src/services/certificate/certificate-dal.ts b/backend/src/services/certificate/certificate-dal.ts index 9bad70054..9db473236 100644 --- a/backend/src/services/certificate/certificate-dal.ts +++ b/backend/src/services/certificate/certificate-dal.ts @@ -11,14 +11,18 @@ export const certificateDALFactory = (db: TDbClient) => { const certificateOrm = ormify(db, TableName.Certificate); const findLatestActiveCertForSubscriber = async ({ subscriberId }: { subscriberId: string }) => { - const cert = await db - .replicaNode()(TableName.Certificate) - .where({ pkiSubscriberId: subscriberId, status: CertStatus.ACTIVE }) - .where("notAfter", ">", new Date()) - .orderBy("notBefore", "desc") - .first(); + try { + const cert = await db + .replicaNode()(TableName.Certificate) + .where({ pkiSubscriberId: subscriberId, status: CertStatus.ACTIVE }) + .where("notAfter", ">", new Date()) + .orderBy("notBefore", "desc") + .first(); - return cert; + return cert; + } catch (error) { + throw new DatabaseError({ error, name: "Find latest active certificate for subscriber" }); + } }; const countCertificatesInProject = async ({ diff --git a/backend/src/services/pki-subscriber/pki-subscriber-service.ts b/backend/src/services/pki-subscriber/pki-subscriber-service.ts index 7bcc545cb..04da5e9d3 100644 --- a/backend/src/services/pki-subscriber/pki-subscriber-service.ts +++ b/backend/src/services/pki-subscriber/pki-subscriber-service.ts @@ -710,6 +710,10 @@ export const pkiSubscriberServiceFactory = ({ projectId }); + if (!subscriber) { + throw new NotFoundError({ message: `PKI subscriber named '${subscriberName}' not found` }); + } + const { permission } = await permissionService.getProjectPermission({ actor, actorId, diff --git a/docs/api-reference/endpoints/pki/subscribers/issue-cert.mdx b/docs/api-reference/endpoints/pki/subscribers/issue-cert.mdx index be57ab01b..c9c71c80d 100644 --- a/docs/api-reference/endpoints/pki/subscribers/issue-cert.mdx +++ b/docs/api-reference/endpoints/pki/subscribers/issue-cert.mdx @@ -1,4 +1,4 @@ --- title: "Issue Certificate" -openapi: "POST /api/v1/pki/subscribers/{subscriberName}/issue-cert" +openapi: "POST /api/v1/pki/subscribers/{subscriberName}/issue-certificate" --- diff --git a/docs/api-reference/endpoints/pki/subscribers/order-cert.mdx b/docs/api-reference/endpoints/pki/subscribers/order-cert.mdx new file mode 100644 index 000000000..93abf1433 --- /dev/null +++ b/docs/api-reference/endpoints/pki/subscribers/order-cert.mdx @@ -0,0 +1,4 @@ +--- +title: "Order Certificate" +openapi: "POST /api/v1/pki/subscribers/{subscriberName}/order-certificate" +--- diff --git a/docs/mint.json b/docs/mint.json index a2a75ca02..d12e41210 100644 --- a/docs/mint.json +++ b/docs/mint.json @@ -1469,6 +1469,7 @@ "api-reference/endpoints/pki/subscribers/delete", "api-reference/endpoints/pki/subscribers/issue-cert", "api-reference/endpoints/pki/subscribers/sign-cert", + "api-reference/endpoints/pki/subscribers/order-cert", "api-reference/endpoints/pki/subscribers/get-active-cert-bundle" ] }, diff --git a/frontend/src/hooks/api/pkiSubscriber/types.ts b/frontend/src/hooks/api/pkiSubscriber/types.ts index 4f6a7c5eb..6d04929e8 100644 --- a/frontend/src/hooks/api/pkiSubscriber/types.ts +++ b/frontend/src/hooks/api/pkiSubscriber/types.ts @@ -17,7 +17,7 @@ export type TPkiSubscriber = { name: string; commonName: string; status: PkiSubscriberStatus; - ttl: string; + ttl?: string; subjectAlternativeNames: string[]; keyUsages: CertKeyUsage[]; extendedKeyUsages: CertExtendedKeyUsage[]; diff --git a/frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaSection.tsx b/frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaSection.tsx index bfdc22208..d2f5248a6 100644 --- a/frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaSection.tsx +++ b/frontend/src/pages/cert-manager/CertificateAuthoritiesPage/components/ExternalCaSection.tsx @@ -26,7 +26,7 @@ export const ExternalCaSection = () => { const onRemoveCaSubmit = async (caId: string, type: CaType) => { try { - if (!currentWorkspace?.slug) return; + if (!currentWorkspace?.id) return; await deleteCa({ caId, type, projectId: currentWorkspace.id }); @@ -67,7 +67,7 @@ export const ExternalCaSection = () => { } catch (err) { console.error(err); createNotification({ - text: `Failed to ${status === CaStatus.ACTIVE ? "enabled" : "disabled"} CA`, + text: `Failed to ${status === CaStatus.ACTIVE ? "enable" : "disable"} CA`, type: "error" }); } @@ -84,7 +84,6 @@ export const ExternalCaSection = () => { {(isAllowed) => (