Fix review feedbacks

backend/src/ee/services/pki-acme/pki-acme-challenge-service.ts

@@ -1,5 +1,6 @@
 import { getConfig } from "@app/lib/config/env";
 import { BadRequestError, NotFoundError } from "@app/lib/errors";
+import { isPrivateIp } from "@app/lib/ip/ipRange";
 import { logger } from "@app/lib/logger";
 import { TPkiAcmeChallengeDALFactory } from "./pki-acme-challenge-dal";
 import {
@@ -53,6 +54,10 @@ export const pkiAcmeChallengeServiceFactory = ({
         throw new BadRequestError({ message: "Only HTTP-01 challenges are supported for now" });
       }
       let host = challenge.auth.identifierValue;
+      // check if host is a private ip address
+      if (isPrivateIp(host)) {
+        throw new BadRequestError({ message: "Private IP addresses are not allowed" });
+      }
       if (appCfg.isAcmeDevelopmentMode && appCfg.ACME_DEVELOPMENT_HTTP01_CHALLENGE_HOST_OVERRIDES[host]) {
         host = appCfg.ACME_DEVELOPMENT_HTTP01_CHALLENGE_HOST_OVERRIDES[host];
         logger.warn(

backend/src/ee/services/pki-acme/pki-acme-service.ts

@@ -7,6 +7,7 @@ import { TCertificateProfileDALFactory } from "@app/services/certificate-profile
 import * as x509 from "@peculiar/x509";
 
 import { KeyStorePrefixes, TKeyStoreFactory } from "@app/keystore/keystore";
+import { isPrivateIp } from "@app/lib/ip/ipRange";
 import { ActorType } from "@app/services/auth/auth-type";
 import {
   EnrollmentType,
@@ -497,7 +498,9 @@ export const pkiAcmeServiceFactory = ({
           if (identifier.type !== AcmeIdentifierType.DNS) {
             throw new AcmeUnsupportedIdentifierError({ detail: "Only DNS identifiers are supported" });
           }
-          // TODO: reuse existing authorizations for this identifier if they exist
+          if (isPrivateIp(identifier.value)) {
+            throw new AcmeUnsupportedIdentifierError({ detail: "Private IP addresses are not allowed" });
+          }
           const auth = await acmeAuthDAL.create(
             {
               accountId: account.id,
@@ -600,8 +603,6 @@ export const pkiAcmeServiceFactory = ({
           throw new AcmeOrderNotReadyError({ message: "ACME order has expired" });
         }
         const { csr } = payload;
-        // TODO: validate the CSR and return badCSR error if it's invalid
-        // TODO: this should be the same transaction?
         let errorToReturn: Error | undefined;
         try {
           const { certificateId } = await certificateV3Service.signCertificateFromProfile({

backend/src/server/routes/index.ts

@@ -2169,6 +2169,7 @@ export const registerRoutes = async (
     certificateAuthorityDAL,
     certificateProfileDAL,
     certificateTemplateV2Service,
+    acmeAccountDAL,
     internalCaService: internalCertificateAuthorityService,
     permissionService,
     certificateSyncDAL,

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

@@ -30,6 +30,7 @@ import {
   extractCertificateRequestFromCSR
 } from "../certificate-common/certificate-csr-utils";
 import { certificateV3ServiceFactory, TCertificateV3ServiceFactory } from "./certificate-v3-service";
+import { TPkiAcmeAccountDALFactory } from "@app/ee/services/pki-acme/pki-acme-account-dal";
 
 vi.mock("../certificate-common/certificate-csr-utils", () => ({
   extractCertificateRequestFromCSR: vi.fn(),
@@ -69,6 +70,10 @@ describe("CertificateV3Service", () => {
     getTemplateV2ById: vi.fn()
   };
 
+  const mockAcmeAccountDAL: Pick<TPkiAcmeAccountDALFactory, "findById"> = {
+    findById: vi.fn()
+  };
+
   const mockInternalCaService: Pick<TInternalCertificateAuthorityServiceFactory, "signCertFromCa" | "issueCertFromCa"> =
     {
       signCertFromCa: vi.fn(),
@@ -132,6 +137,7 @@ describe("CertificateV3Service", () => {
       certificateAuthorityDAL: mockCertificateAuthorityDAL,
       certificateProfileDAL: mockCertificateProfileDAL,
       certificateTemplateV2Service: mockCertificateTemplateV2Service,
+      acmeAccountDAL: mockAcmeAccountDAL,
       internalCaService: mockInternalCaService,
       permissionService: mockPermissionService,
       certificateSyncDAL: {

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

@@ -64,12 +64,14 @@ import {
   TSignCertificateFromProfileDTO,
   TUpdateRenewalConfigDTO
 } from "./certificate-v3-types";
+import { TPkiAcmeAccountDALFactory } from "@app/ee/services/pki-acme/pki-acme-account-dal";
 
 type TCertificateV3ServiceFactoryDep = {
   certificateDAL: Pick<TCertificateDALFactory, "findOne" | "findById" | "updateById" | "transaction">;
   certificateSecretDAL: Pick<TCertificateSecretDALFactory, "findOne">;
   certificateAuthorityDAL: Pick<TCertificateAuthorityDALFactory, "findByIdWithAssociatedCa">;
   certificateProfileDAL: Pick<TCertificateProfileDALFactory, "findByIdWithConfigs">;
+  acmeAccountDAL: Pick<TPkiAcmeAccountDALFactory, "findById">;
   certificateTemplateV2Service: Pick<
     TCertificateTemplateV2ServiceFactory,
     "validateCertificateRequest" | "getTemplateV2ById"
@@ -93,6 +95,7 @@ const validateProfileAndPermissions = async (
   actorAuthMethod: ActorAuthMethod,
   actorOrgId: string,
   certificateProfileDAL: Pick<TCertificateProfileDALFactory, "findByIdWithConfigs">,
+  acmeAccountDAL: Pick<TPkiAcmeAccountDALFactory, "findById">,
   permissionService: Pick<TPermissionServiceFactory, "getProjectPermission">,
   requiredEnrollmentType: EnrollmentType
 ) => {
@@ -107,10 +110,16 @@ const validateProfileAndPermissions = async (
     });
   }
 
-  // XXX: NOT SURE IF THIS IS SECURE TO BY PASS THE PERMISSION CHECK FOR ACME ACCOUNTS
-  //      may need to consider this carefully
-  // TODO: check actor/profile ownership as well
   if (actor === ActorType.ACME_ACCOUNT && requiredEnrollmentType === EnrollmentType.ACME) {
+    const account = await acmeAccountDAL.findById(actorId);
+    if (!account) {
+      throw new NotFoundError({ message: "ACME account not found" });
+    }
+    if (account.profileId !== profile.id) {
+      throw new ForbiddenRequestError({
+        message: "ACME account is not associated with this profile"
+      });
+    }
     return profile;
   }
 
@@ -343,6 +352,7 @@ export const certificateV3ServiceFactory = ({
   certificateSecretDAL,
   certificateAuthorityDAL,
   certificateProfileDAL,
+  acmeAccountDAL,
   certificateTemplateV2Service,
   internalCaService,
   permissionService,
@@ -365,6 +375,7 @@ export const certificateV3ServiceFactory = ({
       actorAuthMethod,
       actorOrgId,
       certificateProfileDAL,
+      acmeAccountDAL,
       permissionService,
       EnrollmentType.API
     );