Fix sign-certificate logic and minor improvements on the certificates table

backend/src/services/certificate-v3/certificate-v3-service.test.ts

@@ -9,6 +9,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
 import { TPermissionServiceFactory } from "@app/ee/services/permission/permission-service-types";
 import { BadRequestError, ForbiddenRequestError, NotFoundError } from "@app/lib/errors";
 import { TCertificateDALFactory } from "@app/services/certificate/certificate-dal";
+import { TCertificateSecretDALFactory } from "@app/services/certificate/certificate-secret-dal";
 import { ACMESANType, CertificateOrderStatus, CertStatus } from "@app/services/certificate/certificate-types";
 import { TCertificateAuthorityDALFactory } from "@app/services/certificate-authority/certificate-authority-dal";
 import { CaStatus } from "@app/services/certificate-authority/certificate-authority-enums";
@@ -24,8 +25,17 @@ import { EnrollmentType } from "@app/services/certificate-profile/certificate-pr
 import { TCertificateTemplateV2ServiceFactory } from "@app/services/certificate-template-v2/certificate-template-v2-service";
 
 import { ActorType, AuthMethod } from "../auth/auth-type";
+import {
+  extractAlgorithmsFromCSR,
+  extractCertificateRequestFromCSR
+} from "../certificate-common/certificate-csr-utils";
 import { certificateV3ServiceFactory, TCertificateV3ServiceFactory } from "./certificate-v3-service";
 
+vi.mock("../certificate-common/certificate-csr-utils", () => ({
+  extractCertificateRequestFromCSR: vi.fn(),
+  extractAlgorithmsFromCSR: vi.fn()
+}));
+
 describe("CertificateV3Service", () => {
   let service: TCertificateV3ServiceFactory;
 
@@ -39,6 +49,10 @@ describe("CertificateV3Service", () => {
     })
   };
 
+  const mockCertificateSecretDAL: Pick<TCertificateSecretDALFactory, "findOne"> = {
+    findOne: vi.fn()
+  };
+
   const mockCertificateAuthorityDAL: Pick<TCertificateAuthorityDALFactory, "findByIdWithAssociatedCa"> = {
     findByIdWithAssociatedCa: vi.fn()
   };
@@ -101,8 +115,20 @@ describe("CertificateV3Service", () => {
       }
     });
 
+    vi.mocked(extractCertificateRequestFromCSR).mockReturnValue({
+      commonName: "test.example.com",
+      keyUsages: [CertKeyUsageType.DIGITAL_SIGNATURE],
+      extendedKeyUsages: [CertExtendedKeyUsageType.SERVER_AUTH]
+    });
+
+    vi.mocked(extractAlgorithmsFromCSR).mockReturnValue({
+      keyAlgorithm: "RSA_2048" as any,
+      signatureAlgorithm: "RSA-SHA256" as any
+    });
+
     service = certificateV3ServiceFactory({
       certificateDAL: mockCertificateDAL,
+      certificateSecretDAL: mockCertificateSecretDAL,
       certificateAuthorityDAL: mockCertificateAuthorityDAL,
       certificateProfileDAL: mockCertificateProfileDAL,
       certificateTemplateV2Service: mockCertificateTemplateV2Service,
@@ -647,6 +673,11 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
       vi.mocked(mockCertificateTemplateV2Service.getTemplateV2ById).mockResolvedValue(mockTemplate);
+      vi.mocked(mockCertificateTemplateV2Service.validateCertificateRequest).mockResolvedValue({
+        isValid: true,
+        errors: [],
+        warnings: []
+      });
       vi.mocked(mockInternalCaService.signCertFromCa).mockResolvedValue(mockSignResult as any);
       vi.mocked(mockCertificateDAL.findOne).mockResolvedValue(mockCertRecord);
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(mockCertRecord);
@@ -1586,6 +1617,7 @@ describe("CertificateV3Service", () => {
     it("should successfully renew eligible certificate", async () => {
       // Mock the initial findById call
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockOriginalCert);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
       vi.mocked(mockCertificateTemplateV2Service.getTemplateV2ById).mockResolvedValue(mockTemplate);
@@ -1650,6 +1682,7 @@ describe("CertificateV3Service", () => {
         errors: ["Subject alternative name not allowed"],
         warnings: []
       });
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(mockOriginalCert);
@@ -1705,11 +1738,37 @@ describe("CertificateV3Service", () => {
       ).rejects.toThrow("Only certificates issued from a profile can be renewed");
     });
 
+    it("should reject renewal if certificate was issued from CSR (external private key)", async () => {
+      vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockOriginalCert);
+      vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue(null as any);
+
+      vi.mocked(mockCertificateDAL.transaction).mockImplementation(async (callback: (tx: any) => Promise<unknown>) => {
+        const mockTx = {};
+        return callback(mockTx);
+      });
+
+      await expect(
+        service.renewCertificate({
+          certificateId: "cert-123",
+          ...mockActor
+        })
+      ).rejects.toThrow(ForbiddenRequestError);
+
+      await expect(
+        service.renewCertificate({
+          certificateId: "cert-123",
+          ...mockActor
+        })
+      ).rejects.toThrow("certificates issued from CSR (external private key) cannot be renewed");
+    });
+
     it("should reject renewal if certificate is already renewed", async () => {
       const alreadyRenewedCert = { ...mockOriginalCert, renewedByCertificateId: "cert-456" };
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(alreadyRenewedCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(alreadyRenewedCert);
@@ -1743,6 +1802,7 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(expiredCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(expiredCert);
@@ -1776,6 +1836,7 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(revokedCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(revokedCert);
@@ -1806,6 +1867,7 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockOriginalCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(inactiveCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(mockOriginalCert);
@@ -1842,6 +1904,7 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockOriginalCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(shortLivedCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       // Mock updateById to handle the renewal error logging
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(mockOriginalCert);
@@ -1873,6 +1936,7 @@ describe("CertificateV3Service", () => {
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockOriginalCert);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile);
       vi.mocked(mockCertificateAuthorityDAL.findByIdWithAssociatedCa).mockResolvedValue(mockCA);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
       vi.mocked(mockCertificateTemplateV2Service.getTemplateV2ById).mockResolvedValue(mockTemplate);
       vi.mocked(mockCertificateTemplateV2Service.validateCertificateRequest).mockResolvedValue({
         isValid: true,
@@ -1929,6 +1993,7 @@ describe("CertificateV3Service", () => {
 
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockCert as any);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile as any);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
       vi.mocked(mockCertificateDAL.updateById).mockResolvedValue(mockCert as any);
 
       const result = await service.updateRenewalConfig({
@@ -2004,6 +2069,7 @@ describe("CertificateV3Service", () => {
 
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockCert as any);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile as any);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       await expect(
         service.updateRenewalConfig({
@@ -2048,6 +2114,7 @@ describe("CertificateV3Service", () => {
 
       vi.mocked(mockCertificateDAL.findById).mockResolvedValue(mockCert as any);
       vi.mocked(mockCertificateProfileDAL.findByIdWithConfigs).mockResolvedValue(mockProfile as any);
+      vi.mocked(mockCertificateSecretDAL.findOne).mockResolvedValue({ id: "secret-123", certId: "cert-123" } as any);
 
       await expect(
         service.updateRenewalConfig({

backend/src/services/certificate-v3/certificate-v3-service.ts

@@ -12,6 +12,7 @@ import {
 import { BadRequestError, ForbiddenRequestError, NotFoundError } from "@app/lib/errors";
 import { ActorAuthMethod, ActorType } from "@app/services/auth/auth-type";
 import { TCertificateDALFactory } from "@app/services/certificate/certificate-dal";
+import { TCertificateSecretDALFactory } from "@app/services/certificate/certificate-secret-dal";
 import {
   CertExtendedKeyUsage,
   CertificateOrderStatus,
@@ -32,6 +33,10 @@ import { EnrollmentType } from "@app/services/certificate-profile/certificate-pr
 import { TCertificateTemplateV2ServiceFactory } from "@app/services/certificate-template-v2/certificate-template-v2-service";
 
 import { CertSubjectAlternativeNameType } from "../certificate-common/certificate-constants";
+import {
+  extractAlgorithmsFromCSR,
+  extractCertificateRequestFromCSR
+} from "../certificate-common/certificate-csr-utils";
 import {
   bufferToString,
   buildCertificateSubjectFromTemplate,
@@ -58,6 +63,7 @@ import {
 
 type TCertificateV3ServiceFactoryDep = {
   certificateDAL: Pick<TCertificateDALFactory, "findOne" | "findById" | "updateById" | "transaction">;
+  certificateSecretDAL: Pick<TCertificateSecretDALFactory, "findOne">;
   certificateAuthorityDAL: Pick<TCertificateAuthorityDALFactory, "findByIdWithAssociatedCa">;
   certificateProfileDAL: Pick<TCertificateProfileDALFactory, "findByIdWithConfigs">;
   certificateTemplateV2Service: Pick<
@@ -296,8 +302,28 @@ const parseTtlToDays = (ttl: string): number => {
   }
 };
 
+const calculateFinalRenewBeforeDays = (
+  profile: { apiConfig?: { autoRenew?: boolean; renewBeforeDays?: number } },
+  ttl: string,
+  certificateExpiryDate: Date
+): number | undefined => {
+  if (!profile.apiConfig?.autoRenew || !profile.apiConfig.renewBeforeDays) {
+    return undefined;
+  }
+
+  const certificateTtlInDays = parseTtlToDays(ttl);
+  const renewBeforeDays = calculateRenewalThreshold(profile.apiConfig.renewBeforeDays, certificateTtlInDays);
+
+  if (!renewBeforeDays) {
+    return undefined;
+  }
+
+  return isValidRenewalTiming(renewBeforeDays, certificateExpiryDate) ? renewBeforeDays : undefined;
+};
+
 export const certificateV3ServiceFactory = ({
   certificateDAL,
+  certificateSecretDAL,
   certificateAuthorityDAL,
   certificateProfileDAL,
   certificateTemplateV2Service,
@@ -412,11 +438,11 @@ export const certificateV3ServiceFactory = ({
       throw new NotFoundError({ message: "Certificate was issued but could not be found in database" });
     }
 
-    const certificateTtlInDays = parseTtlToDays(certificateRequest.validity.ttl);
-    const renewBeforeDays = calculateRenewalThreshold(profile.apiConfig?.renewBeforeDays, certificateTtlInDays);
-
-    const finalRenewBeforeDays =
-      renewBeforeDays && isValidRenewalTiming(renewBeforeDays, new Date(cert.notAfter)) ? renewBeforeDays : undefined;
+    const finalRenewBeforeDays = calculateFinalRenewBeforeDays(
+      profile,
+      certificateRequest.validity.ttl,
+      new Date(cert.notAfter)
+    );
 
     await certificateDAL.updateById(cert.id, {
       profileId,
@@ -442,8 +468,6 @@ export const certificateV3ServiceFactory = ({
     validity,
     notBefore,
     notAfter,
-    signatureAlgorithm,
-    keyAlgorithm,
     actor,
     actorId,
     actorAuthMethod,
@@ -480,22 +504,27 @@ export const certificateV3ServiceFactory = ({
       throw new NotFoundError({ message: "Certificate template not found for this profile" });
     }
 
+    const certificateRequest = extractCertificateRequestFromCSR(csr);
+    const mappedCertificateRequest = mapEnumsForValidation(certificateRequest);
+
+    const { keyAlgorithm: extractedKeyAlgorithm, signatureAlgorithm: extractedSignatureAlgorithm } =
+      extractAlgorithmsFromCSR(csr);
+
+    const validationResult = await certificateTemplateV2Service.validateCertificateRequest(
+      profile.certificateTemplateId,
+      mappedCertificateRequest
+    );
+
+    if (!validationResult.isValid) {
+      throw new BadRequestError({
+        message: `Certificate request validation failed: ${validationResult.errors.join(", ")}`
+      });
+    }
+
     validateAlgorithmCompatibility(ca, template);
 
-    const effectiveSignatureAlgorithm = signatureAlgorithm;
-    const effectiveKeyAlgorithm = keyAlgorithm;
-
-    if (template.algorithms?.keyAlgorithm && !effectiveKeyAlgorithm) {
-      throw new BadRequestError({
-        message: "Key algorithm is required by template policy but not provided in request"
-      });
-    }
-
-    if (template.algorithms?.signature && !effectiveSignatureAlgorithm) {
-      throw new BadRequestError({
-        message: "Signature algorithm is required by template policy but not provided in request"
-      });
-    }
+    const effectiveSignatureAlgorithm = extractedSignatureAlgorithm;
+    const effectiveKeyAlgorithm = extractedKeyAlgorithm;
 
     const { certificate, certificateChain, issuingCaCertificate, serialNumber } =
       await internalCaService.signCertFromCa({
@@ -516,11 +545,7 @@ export const certificateV3ServiceFactory = ({
       throw new NotFoundError({ message: "Certificate was signed but could not be found in database" });
     }
 
-    const certificateTtlInDays = parseTtlToDays(validity.ttl);
-    const renewBeforeDays = calculateRenewalThreshold(profile.apiConfig?.renewBeforeDays, certificateTtlInDays);
-
-    const finalRenewBeforeDays =
-      renewBeforeDays && isValidRenewalTiming(renewBeforeDays, new Date(cert.notAfter)) ? renewBeforeDays : undefined;
+    const finalRenewBeforeDays = calculateFinalRenewBeforeDays(profile, validity.ttl, new Date(cert.notAfter));
 
     await certificateDAL.updateById(cert.id, {
       profileId,
@@ -675,6 +700,14 @@ export const certificateV3ServiceFactory = ({
         });
       }
 
+      const certificateSecret = await certificateSecretDAL.findOne({ certId: originalCert.id }, tx);
+      if (!certificateSecret) {
+        throw new ForbiddenRequestError({
+          message:
+            "Certificate is not eligible for renewal: certificates issued from CSR (external private key) cannot be renewed"
+        });
+      }
+
       if (!internal) {
         const { permission } = await permissionService.getProjectPermission({
           actor,
@@ -791,8 +824,7 @@ export const certificateV3ServiceFactory = ({
       const notBefore = new Date();
       const notAfter = new Date(Date.now() + parseTtlToDays(ttl) * 24 * 60 * 60 * 1000);
 
-      const certificateTtlInDays = parseTtlToDays(ttl);
-      const finalRenewBeforeDays = calculateRenewalThreshold(profile.apiConfig?.renewBeforeDays, certificateTtlInDays);
+      const finalRenewBeforeDays = calculateFinalRenewBeforeDays(profile, ttl, notAfter);
 
       const { certificate, certificateChain, issuingCaCertificate, serialNumber } =
         await internalCaService.issueCertFromCa({
@@ -907,6 +939,14 @@ export const certificateV3ServiceFactory = ({
       });
     }
 
+    const certificateSecret = await certificateSecretDAL.findOne({ certId: certificate.id });
+    if (!certificateSecret) {
+      throw new ForbiddenRequestError({
+        message:
+          "Certificate is not eligible for auto-renewal: certificates issued from CSR (external private key) cannot be auto-renewed"
+      });
+    }
+
     if (certificate.status !== CertStatus.ACTIVE) {
       throw new BadRequestError({
         message: `Certificate is not eligible for auto-renewal: certificate status is ${certificate.status}, must be active`

backend/src/services/certificate-v3/certificate-v3-types.ts

@@ -35,8 +35,6 @@ export type TSignCertificateFromProfileDTO = {
   };
   notBefore?: Date;
   notAfter?: Date;
-  signatureAlgorithm?: string;
-  keyAlgorithm?: string;
 } & Omit<TProjectPermission, "projectId">;
 
 export type TOrderCertificateFromProfileDTO = {