From 4a2b7ecfe4f7c9318d7034989d9cb7719dbcfb05 Mon Sep 17 00:00:00 2001 From: Carlos Monastyrski Date: Mon, 6 Oct 2025 16:07:49 -0300 Subject: [PATCH] Address PR comments --- .../aws-certificate-manager-pki-sync-fns.ts | 157 ++++++------------ .../aws-certificate-manager.mdx | 2 - docs/integrations/app-connections/aws.mdx | 4 +- 3 files changed, 56 insertions(+), 107 deletions(-) diff --git a/backend/src/services/pki-sync/aws-certificate-manager/aws-certificate-manager-pki-sync-fns.ts b/backend/src/services/pki-sync/aws-certificate-manager/aws-certificate-manager-pki-sync-fns.ts index 7a73f64ee..c439ca50d 100644 --- a/backend/src/services/pki-sync/aws-certificate-manager/aws-certificate-manager-pki-sync-fns.ts +++ b/backend/src/services/pki-sync/aws-certificate-manager/aws-certificate-manager-pki-sync-fns.ts @@ -4,7 +4,6 @@ import RE2 from "re2"; import { z } from "zod"; import { BadRequestError, NotFoundError } from "@app/lib/errors"; -import { logger } from "@app/lib/logger"; import { TAppConnectionDALFactory } from "@app/services/app-connection/app-connection-dal"; import { AppConnection, AWSRegion } from "@app/services/app-connection/app-connection-enums"; import { decryptAppConnectionCredentials } from "@app/services/app-connection/app-connection-fns"; @@ -237,6 +236,40 @@ export const awsCertificateManagerPkiSyncFactory = ({ kmsService, appConnectionDAL }: TAwsCertificateManagerPkiSyncFactoryDeps) => { + const deleteCertificateFromAcm = async ( + acm: AWS.ACM, + certificateArn: string, + operation: string, + syncId: string, + throwOnError = false + ): Promise<{ arn: string; success: boolean; error?: Error }> => { + try { + await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), { + operation, + syncId + }); + return { arn: certificateArn, success: true }; + } catch (error) { + const errorObj = error instanceof Error ? error : new Error("Unknown error"); + + if (throwOnError) { + throw new PkiSyncError({ + message: `Failed to remove certificate from AWS Certificate Manager: ${errorObj.message}`, + cause: errorObj, + context: { + certificateArn, + operation + } + }); + } + + return { + arn: certificateArn, + success: false, + error: errorObj + }; + } + }; const $getAwsAcmCertificates = async ( acm: AWS.ACM, syncId = "unknown" @@ -252,7 +285,7 @@ export const awsCertificateManagerPkiSyncFactory = ({ do { const listParams: AWS.ACM.ListCertificatesRequest = { - CertificateStatuses: ["ISSUED"], // Only get active certificates + CertificateStatuses: ["ISSUED"], NextToken: nextToken, MaxItems: 100 }; @@ -290,7 +323,7 @@ export const awsCertificateManagerPkiSyncFactory = ({ try { certificateContent = await acm.getCertificate({ CertificateArn: certSummary.CertificateArn }).promise(); } catch (error) { - logger.error({ certificateArn: certSummary.CertificateArn, error }, "Cannot export certificate content"); + // Certificate content cannot be imported } } @@ -316,14 +349,14 @@ export const awsCertificateManagerPkiSyncFactory = ({ const failedFetches = certificateResults.filter((result) => result.status === "rejected"); if (failedFetches.length > 0) { - logger.warn( - { - syncId, + throw new PkiSyncError({ + message: `Failed to fetch ${failedFetches.length} certificate details from AWS Certificate Manager`, + shouldRetry: true, + context: { failedCount: failedFetches.length, totalCount: certificateSummaries.length - }, - "Some certificate details could not be fetched from AWS Certificate Manager" - ); + } + }); } const res: Record< @@ -369,7 +402,8 @@ export const awsCertificateManagerPkiSyncFactory = ({ const activeCertificateNames = Object.keys(certificateMap); - Object.entries(certificateMap).forEach(([certName, { cert, privateKey, certificateChain }]) => { + Object.entries(certificateMap).forEach(([certName, certData]) => { + const { cert, privateKey, certificateChain } = certData; const certificateName = generateCertificateName(certName, pkiSync); const existingCert = Object.values(acmCertificates).find((acmCert) => @@ -381,16 +415,14 @@ export const awsCertificateManagerPkiSyncFactory = ({ try { validateCertificateContent(cert, privateKey); } catch (validationError) { - logger.error( - { - syncId: pkiSync.id, - certName, + throw new PkiSyncError({ + message: `Certificate validation failed for ${certName}: ${validationError instanceof Error ? validationError.message : String(validationError)}`, + shouldRetry: false, + context: { certificateName, - error: validationError - }, - "Certificate validation failed, skipping" - ); - return; + certName + } + }); } if (shouldUpdateCert) { @@ -453,17 +485,6 @@ export const awsCertificateManagerPkiSyncFactory = ({ return { key, name, success: true, response }; } catch (error) { const errorMessage = error instanceof Error ? error.message : "Unknown error"; - logger.error( - { - syncId: pkiSync.id, - certificateKey: key, - certificateName: name, - error, - errorMessage - }, - "Failed to import certificate to AWS Certificate Manager" - ); - throw new PkiSyncError({ message: `Failed to import certificate ${key} to AWS Certificate Manager: ${errorMessage}`, cause: error instanceof Error ? error : new Error(errorMessage), @@ -489,26 +510,7 @@ export const awsCertificateManagerPkiSyncFactory = ({ if (certificatesToRemove.length > 0) { removeResults = await executeWithConcurrencyLimit( certificatesToRemove, - async (certificateArn) => { - try { - await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), { - operation: "delete-certificate", - syncId: pkiSync.id - }); - return { arn: certificateArn, success: true }; - } catch (error) { - logger.error( - { error, syncId: pkiSync.id, certificateArn }, - "Failed to remove expired/removed certificate from AWS Certificate Manager" - ); - - return { - arn: certificateArn, - success: false, - error: error instanceof Error ? error : new Error("Unknown error") - }; - } - }, + async (certificateArn) => deleteCertificateFromAcm(acm, certificateArn, "delete-certificate", pkiSync.id), { operation: "remove-certificates", syncId: pkiSync.id } ); @@ -517,17 +519,6 @@ export const awsCertificateManagerPkiSyncFactory = ({ ); removedCertificates = successfulRemovals.length; failedRemovals = removeResults.length - removedCertificates; - - if (failedRemovals > 0) { - logger.warn( - { - syncId: pkiSync.id, - failedRemovals, - successfulRemovals: removedCertificates - }, - "Some expired/removed certificates could not be removed from AWS Certificate Manager" - ); - } } const details: { @@ -549,15 +540,6 @@ export const awsCertificateManagerPkiSyncFactory = ({ error: errorMessage }; }); - - logger.error( - { - syncId: pkiSync.id, - failedUploads: details.failedUploads, - failedCount: failedUploads.length - }, - "Some certificates failed to import to AWS Certificate Manager" - ); } if (failedRemovals > 0 && removeResults.length > 0) { @@ -576,15 +558,6 @@ export const awsCertificateManagerPkiSyncFactory = ({ .filter((item): item is { name: string; error: string } => item !== null); details.failedRemovals = actualFailedRemovals; - - logger.warn( - { - syncId: pkiSync.id, - failedRemovals: details.failedRemovals, - successfulRemovals: removedCertificates - }, - "Some expired/removed certificates could not be removed from AWS Certificate Manager" - ); } return { @@ -626,30 +599,8 @@ export const awsCertificateManagerPkiSyncFactory = ({ const results = await executeWithConcurrencyLimit( certificateArnsToRemove, - async (certificateArn) => { - try { - await withRateLimitRetry(() => acm.deleteCertificate({ CertificateArn: certificateArn }).promise(), { - operation: "delete-specific-certificate", - syncId: pkiSync.id - }); - - return { arn: certificateArn, success: true }; - } catch (error) { - logger.error( - { error, syncId: pkiSync.id, certificateArn }, - "Failed to remove specific certificate from AWS Certificate Manager" - ); - - throw new PkiSyncError({ - message: `Failed to remove certificate from AWS Certificate Manager: ${(error as Error)?.message || "Unknown error"}`, - cause: error as Error, - context: { - certificateArn, - region: (pkiSync.destinationConfig as TAwsCertificateManagerPkiSyncConfig).region - } - }); - } - }, + async (certificateArn) => + deleteCertificateFromAcm(acm, certificateArn, "delete-specific-certificate", pkiSync.id, true), { operation: "remove-specific-certificates", syncId: pkiSync.id } ); diff --git a/docs/documentation/platform/pki/certificate-syncs/aws-certificate-manager.mdx b/docs/documentation/platform/pki/certificate-syncs/aws-certificate-manager.mdx index 7561b9d26..de064bf4e 100644 --- a/docs/documentation/platform/pki/certificate-syncs/aws-certificate-manager.mdx +++ b/docs/documentation/platform/pki/certificate-syncs/aws-certificate-manager.mdx @@ -7,7 +7,6 @@ description: "Learn how to configure an AWS Certificate Manager Certificate Sync - Set up and configure a [Certificate Authority](/documentation/platform/pki/overview) - Create an [AWS Connection](/integrations/app-connections/aws) -- Ensure your network security policies allow incoming requests from Infisical to this certificate sync provider, if network restrictions apply. The AWS Certificate Manager Certificate Sync requires the following ACM permissions to be set on the IAM user/role @@ -125,7 +124,6 @@ Your AWS Certificate Manager Certificate Sync will: - **Automatic Deployment**: Deploy new certificates issued by your PKI subscriber to AWS Certificate Manager - **Certificate Updates**: Update certificates in AWS Certificate Manager when renewals occur - **Expiration Handling**: Optionally remove expired certificates from AWS Certificate Manager (if enabled) -- **Format Preservation**: Maintain certificate format and metadata during sync operations - **Tagging**: Automatically tag certificates with an InfisicalCertificate tag for easy identification and management diff --git a/docs/integrations/app-connections/aws.mdx b/docs/integrations/app-connections/aws.mdx index 63c1205e4..4c4f4a8c2 100644 --- a/docs/integrations/app-connections/aws.mdx +++ b/docs/integrations/app-connections/aws.mdx @@ -205,7 +205,7 @@ Infisical supports two methods for connecting to AWS. - **ListCertificates**: Lists all certificates in the account - **ImportCertificate**: Imports certificates from Infisical into AWS Certificate Manager - - **ExportCertificate**: Exports certificates for synchronization (only works with imported certificates) + - **ExportCertificate**: Exports certificates for synchronization - **DeleteCertificate**: Removes certificates that are no longer managed by Infisical - **DescribeCertificate** and **GetCertificate**: Retrieves certificate details for comparison during sync - Tag-related permissions: Manages certificate tags for identification and organization @@ -421,7 +421,7 @@ Infisical supports two methods for connecting to AWS. - **ListCertificates**: Lists all certificates in the account - **ImportCertificate**: Imports certificates from Infisical into AWS Certificate Manager - - **ExportCertificate**: Exports certificates for synchronization (only works with imported certificates) + - **ExportCertificate**: Exports certificates for synchronization - **DeleteCertificate**: Removes certificates that are no longer managed by Infisical - **DescribeCertificate** and **GetCertificate**: Retrieves certificate details for comparison during sync - Tag-related permissions: Manages certificate tags for identification and organization