From e01bb9e8dbbd415a1027930c49fd8e59e893189f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ir=C3=A9n=C3=A9e?= Date: Fri, 3 Jul 2026 11:22:34 +0100 Subject: [PATCH] fix(core): Improve external secrets provider failure logs (#33104) --- .../__tests__/secrets-provider-errors.test.ts | 48 ++++ .../errors/secrets-provider-errors.ts | 112 +++++++++ .../__tests__/aws-secrets-manager.test.ts | 111 ++++++++- .../__tests__/azure-key-vault.test.ts | 185 +++++++++++++- .../__tests__/gcp-secrets-manager.test.ts | 188 +++++++++++++- .../providers/__tests__/infisical.test.ts | 85 +++++++ .../providers/__tests__/one-password.test.ts | 73 +++++- .../providers/__tests__/vault.test.ts | 116 +++++++++ .../providers/aws-secrets-manager.ts | 186 +++++++++++--- .../azure-key-vault/azure-key-vault.ts | 202 ++++++++++++--- .../gcp-secrets-manager.ts | 233 +++++++++++++----- .../providers/infisical.ts | 169 +++++++++---- .../providers/one-password.ts | 168 +++++++++---- .../external-secrets.ee/providers/vault.ts | 206 ++++++++++++---- 14 files changed, 1767 insertions(+), 315 deletions(-) create mode 100644 packages/cli/src/modules/external-secrets.ee/errors/__tests__/secrets-provider-errors.test.ts create mode 100644 packages/cli/src/modules/external-secrets.ee/errors/secrets-provider-errors.ts diff --git a/packages/cli/src/modules/external-secrets.ee/errors/__tests__/secrets-provider-errors.test.ts b/packages/cli/src/modules/external-secrets.ee/errors/__tests__/secrets-provider-errors.test.ts new file mode 100644 index 00000000000..416c489aaaf --- /dev/null +++ b/packages/cli/src/modules/external-secrets.ee/errors/__tests__/secrets-provider-errors.test.ts @@ -0,0 +1,48 @@ +import { + buildHttpProviderErrorContext, + buildFailureSummaryLogContext, +} from '../secrets-provider-errors'; + +describe('buildHttpProviderErrorContext', () => { + it('extracts statusCode without duplicating it in errorCode', () => { + const error = Object.assign(new Error('Request failed'), { + response: { status: 403 }, + }); + + expect(buildHttpProviderErrorContext(error)).toEqual({ + statusCode: 403, + }); + }); + + it('extracts transport and SDK codes', () => { + const error = Object.assign(new Error('Forbidden'), { + response: { status: 403 }, + code: 'FORBIDDEN', + }); + + expect(buildHttpProviderErrorContext(error)).toEqual({ + statusCode: 403, + errorCode: 'FORBIDDEN', + }); + }); +}); + +describe('buildFailureSummaryLogContext', () => { + it('returns null when there are no failures', () => { + expect(buildFailureSummaryLogContext([])).toBeNull(); + }); + + it('summarizes failures with capped sample names', () => { + expect( + buildFailureSummaryLogContext([ + { name: 'secret-a', errorCode: 5 }, + { name: 'secret-b', errorCode: 5 }, + { name: 'secret-c', errorCode: 7 }, + ]), + ).toEqual({ + failedCount: 3, + errorCodes: { '5': 2, '7': 1 }, + sampleSecretNames: ['secret-a', 'secret-b', 'secret-c'], + }); + }); +}); diff --git a/packages/cli/src/modules/external-secrets.ee/errors/secrets-provider-errors.ts b/packages/cli/src/modules/external-secrets.ee/errors/secrets-provider-errors.ts new file mode 100644 index 00000000000..576c5a33ae9 --- /dev/null +++ b/packages/cli/src/modules/external-secrets.ee/errors/secrets-provider-errors.ts @@ -0,0 +1,112 @@ +import type { Logger } from '@n8n/backend-common'; +import { httpStatusFromError, isConnectionRefusedError } from '@n8n/backend-network'; + +export type SafeContextValue = string | number | boolean | undefined; +type AggregateContextValue = Record | string[]; +type LogContextValue = SafeContextValue | AggregateContextValue; +export type LogContext = Record; + +export type SecretsProviderOperation = + | 'initialize' + | 'connect' + | 'disconnect' + | 'update' + | 'test' + | 'tokenRefresh'; + +export type HttpProviderErrorLogContext = LogContext & { + errorCode?: SafeContextValue; + statusCode?: number; +}; + +export const UPDATE_FAILURE_SAMPLE_SIZE = 5; + +export function buildHttpProviderErrorContext(error: unknown): HttpProviderErrorLogContext { + const context: HttpProviderErrorLogContext = {}; + + if (isConnectionRefusedError(error)) { + context.errorCode = 'ECONNREFUSED'; + } + + const statusCode = httpStatusFromError(error); + if (statusCode !== undefined) { + context.statusCode = statusCode; + } + + if ( + context.errorCode === undefined && + typeof error === 'object' && + error !== null && + 'code' in error + ) { + const { code } = error; + if ((typeof code === 'string' || typeof code === 'number') && code !== statusCode) { + context.errorCode = code; + } + } + + if ( + context.errorCode === undefined && + context.statusCode === undefined && + error instanceof Error + ) { + context.errorCode = error.name; + } + + return context; +} + +export function buildFailureSummaryLogContext( + failures: Array<{ name: string; errorCode: SafeContextValue }>, +): { + failedCount: number; + errorCodes: Record; + sampleSecretNames: string[]; +} | null { + if (failures.length === 0) { + return null; + } + + const errorCodes: Record = {}; + for (const failure of failures) { + const key = String(failure.errorCode ?? 'unknown'); + errorCodes[key] = (errorCodes[key] ?? 0) + 1; + } + + return { + failedCount: failures.length, + errorCodes, + sampleSecretNames: failures.slice(0, UPDATE_FAILURE_SAMPLE_SIZE).map((failure) => failure.name), + }; +} + +type LogSecretsProviderOperationFailureParams = { + logger: Logger; + message: string; + providerName: string; + providerDisplayName: string; +} & SecretsProviderOperationFailureParams; + +export type SecretsProviderOperationFailureParams = { + operation: SecretsProviderOperation; + error: unknown; + context?: LogContext; +}; + +export function logSecretsProviderOperationFailure({ + logger, + message, + providerName, + providerDisplayName, + operation, + error, + context = {}, +}: LogSecretsProviderOperationFailureParams): void { + logger.warn(message, { + providerName, + providerDisplayName, + operation, + errorName: error instanceof Error ? error.name : undefined, + ...context, + }); +} diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/aws-secrets-manager.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/aws-secrets-manager.test.ts index a1dc79010ed..d399a0aff01 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/aws-secrets-manager.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/aws-secrets-manager.test.ts @@ -1,5 +1,6 @@ import type { Mock } from 'vitest'; import { SecretsManager } from '@aws-sdk/client-secrets-manager'; +import type { Logger } from '@n8n/backend-common'; import type { OutboundHttp, HttpTransport } from '@n8n/backend-network'; import { mock } from 'vitest-mock-extended'; import type { Agent as HttpAgent } from 'node:http'; @@ -9,6 +10,19 @@ import { AwsSecretsManager, type AwsSecretsManagerContext } from '../aws-secrets vi.mock('@aws-sdk/client-secrets-manager'); +function createAwsSdkError( + name: string, + options: { httpStatusCode?: number; Code?: string; code?: string | number } = {}, +): Error { + return Object.assign(new Error(name), { + name, + ...(options.Code !== undefined ? { Code: options.Code } : {}), + ...(options.code !== undefined ? { code: options.code } : {}), + $metadata: + options.httpStatusCode !== undefined ? { httpStatusCode: options.httpStatusCode } : {}, + }); +} + describe('AwsSecretsManager', () => { const region = 'eu-central-1'; const accessKeyId = 'FAKE-ACCESS-KEY-ID'; @@ -17,13 +31,60 @@ describe('AwsSecretsManager', () => { const context = mock(); const listSecretsSpy = vi.spyOn(SecretsManager.prototype, 'listSecrets'); const batchGetSpy = vi.spyOn(SecretsManager.prototype, 'batchGetSecretValue'); + const logger = mock(); let awsSecretsManager: AwsSecretsManager; beforeEach(() => { vi.resetAllMocks(); + logger.scoped.mockReturnValue(logger); - awsSecretsManager = new AwsSecretsManager(); + awsSecretsManager = new AwsSecretsManager(logger); + }); + + describe('error context', () => { + it('extracts legacy Code property', () => { + const error = Object.assign(new Error('Access denied'), { Code: 'AccessDenied' }); + + expect(awsSecretsManager['getAwsErrorCode'](error)).toBe('AccessDenied'); + expect(awsSecretsManager['awsErrorContext'](error)).toEqual({ errorCode: 'AccessDenied' }); + }); + + it('extracts string and numeric error codes from code property', () => { + expect( + awsSecretsManager['getAwsErrorCode']( + Object.assign(new Error('Connection failed'), { code: 'ECONNREFUSED' }), + ), + ).toBe('ECONNREFUSED'); + expect( + awsSecretsManager['getAwsErrorCode'](Object.assign(new Error('Timeout'), { code: 408 })), + ).toBe(408); + }); + + it('extracts AWS SDK exception name and HTTP status code', () => { + expect( + awsSecretsManager['awsErrorContext']( + createAwsSdkError('AccessDeniedException', { httpStatusCode: 403 }), + ), + ).toEqual({ + errorCode: 'AccessDeniedException', + statusCode: 403, + }); + }); + + it('falls back to Error.name for generic errors', () => { + expect(awsSecretsManager['getAwsErrorCode'](new Error('Something went wrong'))).toBe('Error'); + expect(awsSecretsManager['awsErrorContext'](new Error('Something went wrong'))).toEqual({ + errorCode: 'Error', + }); + }); + + it('returns empty context for non-error values', () => { + expect(awsSecretsManager['getAwsErrorCode']('not an error')).toBeUndefined(); + expect(awsSecretsManager['getAwsErrorCode'](null)).toBeUndefined(); + expect(awsSecretsManager['awsErrorContext']('not an error')).toEqual({}); + expect(awsSecretsManager['awsErrorContext'](null)).toEqual({}); + }); }); describe('transport wiring', () => { @@ -66,12 +127,29 @@ describe('AwsSecretsManager', () => { await awsSecretsManager.init(context); listSecretsSpy.mockImplementation(() => { - throw new Error('Invalid credentials'); + throw createAwsSdkError('AccessDeniedException', { + httpStatusCode: 403, + Code: 'AccessDenied', + }); }); await awsSecretsManager.connect(); expect(awsSecretsManager.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledTimes(1); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect AWS Secrets Manager provider', + expect.objectContaining({ + providerName: 'awsSecretsManager', + providerDisplayName: 'AWS Secrets Manager', + operation: 'connect', + region, + authMethod: 'iamUser', + errorName: 'AccessDeniedException', + errorCode: 'AccessDenied', + statusCode: 403, + }), + ); }); }); @@ -197,4 +275,33 @@ describe('AwsSecretsManager', () => { expect(awsSecretsManager.getSecret('secret2')).toBe('secret2-value'); expect(awsSecretsManager.getSecret('secret3')).toBe('secret3-value'); }); + + it('should log and rethrow update failures', async () => { + context.settings = { + region, + authMethod: 'iamUser', + accessKeyId, + secretAccessKey, + }; + await awsSecretsManager.init(context); + + listSecretsSpy.mockImplementation(() => { + throw new Error('Failed to list secrets'); + }); + + await expect(awsSecretsManager.update()).rejects.toThrow('Failed to list secrets'); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to update AWS Secrets Manager provider secrets', + expect.objectContaining({ + providerName: 'awsSecretsManager', + providerDisplayName: 'AWS Secrets Manager', + operation: 'update', + region, + authMethod: 'iamUser', + errorName: expect.any(String), + errorCode: expect.any(String), + }), + ); + }); }); diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/azure-key-vault.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/azure-key-vault.test.ts index d0923af5799..35beecc2b17 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/azure-key-vault.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/azure-key-vault.test.ts @@ -1,6 +1,9 @@ +import { AuthenticationError } from '@azure/identity'; import { SecretClient } from '@azure/keyvault-secrets'; import type { KeyVaultSecret } from '@azure/keyvault-secrets'; +import type { Logger } from '@n8n/backend-common'; import { UnexpectedError } from 'n8n-workflow'; +import type { Mock } from 'vitest'; import { mock } from 'vitest-mock-extended'; import { AzureKeyVault } from '../azure-key-vault/azure-key-vault'; @@ -9,11 +12,153 @@ import type { AzureKeyVaultContext } from '../azure-key-vault/types'; vi.mock('@azure/identity'); vi.mock('@azure/keyvault-secrets'); -describe('AzureKeyVault', () => { - const azureKeyVault = new AzureKeyVault(); +function createRestErrorLike( + message: string, + { statusCode, code }: { statusCode?: number; code?: string }, +): Error { + return Object.assign(new Error(message), { + name: 'RestError', + statusCode, + code, + }); +} - afterEach(() => { +describe('AzureKeyVault', () => { + const logger = mock(); + let azureKeyVault: AzureKeyVault; + + beforeEach(() => { vi.clearAllMocks(); + logger.scoped.mockReturnValue(logger); + azureKeyVault = new AzureKeyVault(logger); + }); + + describe('error context', () => { + it('extracts statusCode and errorCode from RestError-like errors', () => { + const error = createRestErrorLike('Permission denied', { + statusCode: 403, + code: 'Forbidden', + }); + + expect(azureKeyVault['azureErrorContext'](error)).toEqual({ + statusCode: 403, + errorCode: 'Forbidden', + }); + }); + + it('extracts errorCode from RestError-like errors without statusCode', () => { + const error = createRestErrorLike('Connection failed', { + code: 'REQUEST_SEND_ERROR', + }); + + expect(azureKeyVault['azureErrorContext'](error)).toEqual({ + errorCode: 'REQUEST_SEND_ERROR', + }); + }); + + it('extracts statusCode and errorCode from AuthenticationError', () => { + const error = Object.assign( + new AuthenticationError(401, { + error: 'invalid_client', + error_description: 'Invalid client secret', + }), + { + statusCode: 401, + errorResponse: { error: 'invalid_client' }, + }, + ); + + expect(azureKeyVault['azureErrorContext'](error)).toEqual({ + statusCode: 401, + errorCode: 'invalid_client', + }); + }); + + it('falls back to Error.name for generic errors', () => { + expect(azureKeyVault['azureErrorContext'](new Error('Something went wrong'))).toEqual({ + errorCode: 'Error', + }); + }); + + it('returns empty context for non-error values', () => { + expect(azureKeyVault['azureErrorContext']('not an error')).toEqual({}); + expect(azureKeyVault['azureErrorContext'](null)).toEqual({}); + }); + }); + + it('should log failed client setup while preserving error state', async () => { + await azureKeyVault.init( + mock({ + settings: { + vaultName: 'my-vault', + tenantId: 'my-tenant-id', + clientId: 'my-client-id', + clientSecret: 'my-client-secret', + }, + }), + ); + + const setupError = new Error('Invalid configuration'); + const SecretClientMock = SecretClient as unknown as Mock; + SecretClientMock.mockImplementationOnce(() => { + throw setupError; + }); + + await azureKeyVault.connect(); + + expect(azureKeyVault.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect Azure Key Vault provider', + expect.objectContaining({ + providerName: 'azureKeyVault', + providerDisplayName: 'Azure Key Vault', + operation: 'connect', + vaultName: 'my-vault', + errorName: expect.any(String), + errorCode: expect.any(String), + }), + ); + }); + + it('should log test failures with Azure error context', async () => { + await azureKeyVault.init( + mock({ + settings: { + vaultName: 'my-vault', + tenantId: 'my-tenant-id', + clientId: 'my-client-id', + clientSecret: 'my-client-secret', + }, + }), + ); + + await azureKeyVault.connect(); + + const restError = Object.assign(new Error('Permission denied'), { + name: 'RestError', + statusCode: 403, + code: 'Forbidden', + }); + + vi.spyOn(SecretClient.prototype, 'listPropertiesOfSecrets').mockReturnValue({ + next: vi.fn().mockRejectedValue(restError), + } as never); + + const [success, message] = await azureKeyVault.test(); + + expect(success).toBe(false); + expect(message).toBe('Permission denied'); + expect(logger.warn).toHaveBeenCalledWith( + 'Azure Key Vault provider test failed', + expect.objectContaining({ + providerName: 'azureKeyVault', + operation: 'test', + errorName: 'RestError', + statusCode: 403, + errorCode: 'Forbidden', + vaultName: 'my-vault', + }), + ); }); it('should update cached secrets', async () => { @@ -137,6 +282,25 @@ describe('AzureKeyVault', () => { expect(azureKeyVault.getSecret('good')).toBe('fine'); expect(azureKeyVault.hasSecret('bad')).toBe(false); + expect(logger.debug).toHaveBeenCalledWith( + 'Could not read Azure Key Vault secret "bad"', + expect.objectContaining({ + providerName: 'azureKeyVault', + operation: 'update', + secretName: 'bad', + vaultName: 'my-vault', + }), + ); + expect(logger.warn).toHaveBeenCalledWith( + 'Skipped unreadable Azure Key Vault secrets during update', + expect.objectContaining({ + providerName: 'azureKeyVault', + operation: 'update', + vaultName: 'my-vault', + failedCount: 1, + sampleSecretNames: ['bad'], + }), + ); }); it('should throw when every getSecret fails and leave the previous cache unchanged', async () => { @@ -185,6 +349,21 @@ describe('AzureKeyVault', () => { expect(thrown.message).toBe('Could not read any secrets from Azure Key Vault'); expect(thrown.cause).toEqual(expect.objectContaining({ message: 'Key Vault unavailable' })); } + expect(logger.warn).toHaveBeenCalledWith( + 'Could not read any secrets from Azure Key Vault', + expect.objectContaining({ + providerName: 'azureKeyVault', + operation: 'update', + vaultName: 'my-vault', + failedCount: 1, + sampleSecretNames: ['only-secret'], + errorName: 'Error', + }), + ); + expect(logger.warn).not.toHaveBeenCalledWith( + 'Skipped unreadable Azure Key Vault secrets during update', + expect.anything(), + ); expect(azureKeyVault.getSecret('only-secret')).toBe('cached-value'); }); }); diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/gcp-secrets-manager.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/gcp-secrets-manager.test.ts index b398b18a139..2c618ce6150 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/gcp-secrets-manager.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/gcp-secrets-manager.test.ts @@ -1,6 +1,8 @@ import { SecretManagerServiceClient } from '@google-cloud/secret-manager'; import type { google } from '@google-cloud/secret-manager/build/protos/protos'; +import type { Logger } from '@n8n/backend-common'; import { UserError } from 'n8n-workflow'; +import type { Mock } from 'vitest'; import { mock } from 'vitest-mock-extended'; import { GcpSecretsManager } from '../gcp-secrets-manager/gcp-secrets-manager'; @@ -18,11 +20,42 @@ const VALID_SERVICE_ACCOUNT_KEY = (projectId: string) => '-----BEGIN PRIVATE KEY-----\nMIIEvQIBADANBgkqhkiG9w0BAQEFAASCBKcwggSjAgEAAoIBAQC\n-----END PRIVATE KEY-----', }); -describe('GCP Secrets Manager', () => { - const gcpSecretsManager = new GcpSecretsManager(); +function createGrpcError(message: string, code: number): Error { + return Object.assign(new Error(message), { code }); +} - afterEach(() => { +describe('GCP Secrets Manager', () => { + const logger = mock(); + let gcpSecretsManager: GcpSecretsManager; + + beforeEach(() => { vi.clearAllMocks(); + logger.scoped.mockReturnValue(logger); + gcpSecretsManager = new GcpSecretsManager(logger); + }); + + describe('error codes', () => { + it('extracts numeric gRPC status codes', () => { + expect(gcpSecretsManager['getGcpErrorCode'](createGrpcError('NOT_FOUND', 5))).toBe(5); + expect(gcpSecretsManager['getGcpErrorCode'](createGrpcError('PERMISSION_DENIED', 7))).toBe(7); + }); + + it('extracts string error codes', () => { + const error = Object.assign(new Error('Connection failed'), { code: 'ECONNREFUSED' }); + + expect(gcpSecretsManager['getGcpErrorCode'](error)).toBe('ECONNREFUSED'); + }); + + it('returns undefined for generic errors without a code', () => { + expect( + gcpSecretsManager['getGcpErrorCode'](new Error('Something went wrong')), + ).toBeUndefined(); + }); + + it('returns undefined for non-error values', () => { + expect(gcpSecretsManager['getGcpErrorCode']('not an error')).toBeUndefined(); + expect(gcpSecretsManager['getGcpErrorCode'](null)).toBeUndefined(); + }); }); describe('init validation', () => { @@ -45,6 +78,15 @@ describe('GCP Secrets Manager', () => { await expect( gcpSecretsManager.init(mock({ settings })), ).rejects.toThrow(UserError); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to initialize GCP Secrets Manager provider', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + providerDisplayName: 'GCP Secrets Manager', + operation: 'initialize', + errorName: expect.any(String), + }), + ); }); it('should throw UserError when JSON lacks client_email', async () => { @@ -75,6 +117,36 @@ describe('GCP Secrets Manager', () => { }); }); + it('should log failed client setup while preserving error state', async () => { + const PROJECT_ID = 'my-project-id'; + + await gcpSecretsManager.init( + mock({ + settings: { serviceAccountKey: VALID_SERVICE_ACCOUNT_KEY(PROJECT_ID) }, + }), + ); + + const setupError = new Error('Invalid configuration'); + const SecretManagerServiceClientMock = SecretManagerServiceClient as unknown as Mock; + SecretManagerServiceClientMock.mockImplementationOnce(() => { + throw setupError; + }); + + await gcpSecretsManager.connect(); + + expect(gcpSecretsManager.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect GCP Secrets Manager provider', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + providerDisplayName: 'GCP Secrets Manager', + operation: 'connect', + projectId: PROJECT_ID, + errorName: expect.any(String), + }), + ); + }); + it('should update cached secrets', async () => { /** * Arrange @@ -139,6 +211,33 @@ describe('GCP Secrets Manager', () => { expect(gcpSecretsManager.getSecret('secret3')).toBeUndefined(); // no value }); + it('should log failed connection tests while preserving the result', async () => { + const PROJECT_ID = 'my-project-id'; + + await gcpSecretsManager.init( + mock({ + settings: { serviceAccountKey: VALID_SERVICE_ACCOUNT_KEY(PROJECT_ID) }, + }), + ); + await gcpSecretsManager.connect(); + + vi.spyOn(SecretManagerServiceClient.prototype, 'initialize').mockRejectedValue( + new Error('Invalid credentials'), + ); + + await expect(gcpSecretsManager.test()).resolves.toEqual([false, 'Invalid credentials']); + expect(logger.warn).toHaveBeenCalledWith( + 'GCP Secrets Manager provider test failed', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + providerDisplayName: 'GCP Secrets Manager', + operation: 'test', + errorName: 'Error', + projectId: PROJECT_ID, + }), + ); + }); + it('should throw a generic error when accessing secret versions', async () => { /** * Arrange @@ -178,16 +277,19 @@ describe('GCP Secrets Manager', () => { ] as GcpSecretVersionResponse[]; }); - /** - * Act - */ - try { - await gcpSecretsManager.connect(); - await gcpSecretsManager.update(); - } catch (error) { - expect(error).toBeInstanceOf(Error); - expect(error.message).toBe('test error'); - } + await gcpSecretsManager.connect(); + + await expect(gcpSecretsManager.update()).rejects.toThrow('test error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to update GCP Secrets Manager provider secrets', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + providerDisplayName: 'GCP Secrets Manager', + operation: 'update', + errorName: 'Error', + projectId: PROJECT_ID, + }), + ); }); it('should handle errors when accessing secret versions (NOT_FOUND)', async () => { @@ -256,6 +358,26 @@ describe('GCP Secrets Manager', () => { expect(gcpSecretsManager.getSecret('secret1')).toBeUndefined(); // error case expect(gcpSecretsManager.getSecret('secret2')).toBe('value2'); expect(gcpSecretsManager.getSecret('secret3')).toBeUndefined(); // no value + expect(logger.debug).toHaveBeenCalledWith( + 'Skipping inaccessible GCP secret version', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + secretName: 'secret1', + errorCode: 5, + projectId: PROJECT_ID, + }), + ); + expect(logger.warn).toHaveBeenCalledWith( + 'Skipped inaccessible GCP secret versions during update', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + projectId: PROJECT_ID, + failedCount: 1, + sampleSecretNames: ['secret1'], + }), + ); }); it('should handle errors when accessing secret versions (PERMISSION_DENIED)', async () => { @@ -324,6 +446,26 @@ describe('GCP Secrets Manager', () => { expect(gcpSecretsManager.getSecret('secret1')).toBeUndefined(); // error case expect(gcpSecretsManager.getSecret('secret2')).toBe('value2'); expect(gcpSecretsManager.getSecret('secret3')).toBeUndefined(); // no value + expect(logger.debug).toHaveBeenCalledWith( + 'Skipping inaccessible GCP secret version', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + secretName: 'secret1', + errorCode: 7, + projectId: PROJECT_ID, + }), + ); + expect(logger.warn).toHaveBeenCalledWith( + 'Skipped inaccessible GCP secret versions during update', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + projectId: PROJECT_ID, + failedCount: 1, + sampleSecretNames: ['secret1'], + }), + ); }); it('should handle errors when accessing secret versions (UNAVAILABLE)', async () => { @@ -392,5 +534,25 @@ describe('GCP Secrets Manager', () => { expect(gcpSecretsManager.getSecret('secret1')).toBeUndefined(); // error case expect(gcpSecretsManager.getSecret('secret2')).toBe('value2'); expect(gcpSecretsManager.getSecret('secret3')).toBeUndefined(); // no value + expect(logger.debug).toHaveBeenCalledWith( + 'Skipping inaccessible GCP secret version', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + secretName: 'secret1', + errorCode: 14, + projectId: PROJECT_ID, + }), + ); + expect(logger.warn).toHaveBeenCalledWith( + 'Skipped inaccessible GCP secret versions during update', + expect.objectContaining({ + providerName: 'gcpSecretsManager', + operation: 'update', + projectId: PROJECT_ID, + failedCount: 1, + sampleSecretNames: ['secret1'], + }), + ); }); }); diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/infisical.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/infisical.test.ts index 05e74d2c430..0e1645c6e4f 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/infisical.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/infisical.test.ts @@ -60,10 +60,33 @@ const WORKSPACE_PATH = `/api/v1/workspace/${PROJECT_ID}`; const LOGIN_PATH = '/api/v1/auth/universal-auth/login'; const SECRETS_PATH = '/api/v4/secrets'; +const infisicalConnectSettingsLogContext = { + siteURL: SITE_URL, + projectId: PROJECT_ID, + authMethod: 'universalAuth', +}; + +const infisicalTestSettingsLogContext = { + siteURL: SITE_URL, + projectId: PROJECT_ID, +}; + +const infisicalUpdateSettingsLogContext = { + siteURL: SITE_URL, + projectId: PROJECT_ID, + environment: ENVIRONMENT, + secretPath: SECRET_PATH, +}; + describe('InfisicalProvider', () => { const logger = mockInstance(Logger); logger.scoped.mockReturnValue(logger); + beforeEach(() => { + vi.clearAllMocks(); + logger.scoped.mockReturnValue(logger); + }); + function createProvider(routes: Route[]) { const { outboundHttp, httpRequest, requests } = createFakeOutboundHttp( routes, @@ -191,6 +214,17 @@ describe('InfisicalProvider', () => { const [success, message] = await provider.test(); expect(success).toBe(false); expect(message).toBe('Connection refused. Check the Site URL.'); + expect(logger.warn).toHaveBeenCalledWith( + 'Infisical provider test failed', + expect.objectContaining({ + providerName: 'infisical', + providerDisplayName: 'Infisical', + ...infisicalTestSettingsLogContext, + operation: 'test', + endpoint: 'workspace', + errorCode: 'ECONNREFUSED', + }), + ); }); }); @@ -203,6 +237,16 @@ describe('InfisicalProvider', () => { await provider.connect(); expect(provider.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect Infisical provider', + expect.objectContaining({ + providerName: 'infisical', + providerDisplayName: 'Infisical', + ...infisicalConnectSettingsLogContext, + operation: 'connect', + statusCode: 401, + }), + ); }); }); @@ -274,6 +318,47 @@ describe('InfisicalProvider', () => { expect(secretsCalls[1].headers).toMatchObject({ Authorization: 'Bearer refreshed-token' }); }); + it('logs and rethrows update failures', async () => { + const { provider } = await connectedProvider([ + { method: 'GET', pathname: SECRETS_PATH, status: 500, body: { message: 'Failed' } }, + ]); + + await expect(provider.update()).rejects.toThrow('Request failed with status 500'); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to update Infisical provider secrets', + expect.objectContaining({ + providerName: 'infisical', + providerDisplayName: 'Infisical', + ...infisicalUpdateSettingsLogContext, + operation: 'update', + endpoint: 'secrets', + statusCode: 500, + }), + ); + }); + + it('logs token refresh failures before attempting to reconnect', async () => { + const { provider } = await initProvider([ + { method: 'POST', pathname: LOGIN_PATH, networkError: 'ECONNREFUSED' }, + ]); + const connect = vi.spyOn(provider, 'connect').mockResolvedValue(); + + await (provider as unknown as { tokenRefresh: () => Promise }).tokenRefresh(); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to refresh Infisical token. Attempting reconnect.', + expect.objectContaining({ + providerName: 'infisical', + providerDisplayName: 'Infisical', + ...infisicalConnectSettingsLogContext, + operation: 'tokenRefresh', + errorCode: 'ECONNREFUSED', + }), + ); + expect(connect).toHaveBeenCalled(); + }); + it('caches secrets from imports alongside top-level secrets', async () => { const { provider } = await connectedProvider([ { diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/one-password.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/one-password.test.ts index 83e48f87f23..c61f7daf9ff 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/one-password.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/one-password.test.ts @@ -1,8 +1,9 @@ +import { Logger } from '@n8n/backend-common'; +import { mockInstance } from '@n8n/backend-test-utils'; import { UserError } from 'n8n-workflow'; import { mock } from 'vitest-mock-extended'; -import { OnePasswordProvider } from '../one-password'; -import type { OnePasswordContext } from '../one-password'; +import { OnePasswordProvider, type OnePasswordContext } from '../one-password'; const mockListVaults = vi.fn(); const mockListItems = vi.fn(); @@ -17,10 +18,14 @@ vi.mock('@1password/connect', () => ({ })); describe('OnePasswordProvider', () => { - const provider = new OnePasswordProvider(); + const logger = mockInstance(Logger); + logger.scoped.mockReturnValue(logger); + let provider: OnePasswordProvider; - afterEach(() => { + beforeEach(() => { vi.clearAllMocks(); + logger.scoped.mockReturnValue(logger); + provider = new OnePasswordProvider(logger); }); describe('init validation', () => { @@ -29,6 +34,15 @@ describe('OnePasswordProvider', () => { await expect(provider.init(mock({ settings }))).rejects.toThrow( UserError, ); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to initialize 1Password provider', + expect.objectContaining({ + providerName: 'onePassword', + providerDisplayName: '1Password', + operation: 'initialize', + errorName: expect.any(String), + }), + ); }); it('should throw UserError when serverUrl is whitespace', async () => { @@ -80,11 +94,26 @@ describe('OnePasswordProvider', () => { }), ); - mockListVaults.mockRejectedValue(new Error('Unauthorized')); + mockListVaults.mockRejectedValue( + Object.assign(new Error('Unauthorized'), { + response: { status: 401 }, + }), + ); await provider.connect(); expect(provider.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect 1Password provider', + expect.objectContaining({ + providerName: 'onePassword', + providerDisplayName: '1Password', + operation: 'connect', + errorName: 'Error', + statusCode: 401, + serverUrl: 'http://localhost:8080', + }), + ); }); }); @@ -115,10 +144,24 @@ describe('OnePasswordProvider', () => { mockListVaults.mockResolvedValue([]); await provider.connect(); - mockListVaults.mockRejectedValue(new Error('Connection refused')); + mockListVaults.mockRejectedValue( + Object.assign(new Error('Connection refused'), { code: 'ECONNREFUSED' }), + ); const result = await provider.test(); expect(result).toEqual([false, 'Connection refused']); + expect(logger.warn).toHaveBeenCalledWith( + '1Password provider test failed', + expect.objectContaining({ + providerName: 'onePassword', + providerDisplayName: '1Password', + operation: 'test', + errorName: 'Error', + errorCode: 'ECONNREFUSED', + endpoint: 'vaults', + serverUrl: 'http://localhost:8080', + }), + ); }); }); @@ -271,6 +314,24 @@ describe('OnePasswordProvider', () => { expect(provider.hasSecret('Empty Fields')).toBe(false); }); + + it('should log and rethrow update failures', async () => { + mockListVaults.mockRejectedValue(new Error('Service unavailable')); + + await expect(provider.update()).rejects.toThrow('Service unavailable'); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to update 1Password provider secrets', + expect.objectContaining({ + providerName: 'onePassword', + providerDisplayName: '1Password', + operation: 'update', + errorName: 'Error', + endpoint: 'secrets', + serverUrl: 'http://localhost:8080', + }), + ); + }); }); describe('getSecret / hasSecret / getSecretNames', () => { diff --git a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/vault.test.ts b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/vault.test.ts index 6f36ee96374..6df31ddc7a1 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/__tests__/vault.test.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/__tests__/vault.test.ts @@ -74,6 +74,12 @@ describe('VaultProvider', () => { // Use preferGet so list requests are plain GETs with `?list=true`. mockInstance(ExternalSecretsConfig, { preferGet: true }); + beforeEach(() => { + vi.clearAllMocks(); + logger.scoped.mockReturnValue(logger); + mockInstance(ExternalSecretsConfig, { preferGet: true }); + }); + function createProvider(routes: Route[], settings = vaultSettings) { const { outboundHttp, httpRequest, requests } = createFakeOutboundHttp( routes, @@ -196,6 +202,42 @@ describe('VaultProvider', () => { expect(provider.state).toBe('connected'); }); + it('logs username/password authentication failures while preserving error state', async () => { + const settings = { + ...vaultSettings, + settings: { + ...vaultSettings.settings, + authMethod: 'usernameAndPassword', + username: 'alice', + password: 's3cret', + }, + }; + const { provider } = await initProvider( + [ + { + method: 'POST', + pathname: '/v1/auth/userpass/login/alice', + status: 401, + body: { errors: [] }, + }, + ], + settings, + ); + + await provider.connect(); + + expect(provider.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Vault provider username/password authentication failed', + expect.objectContaining({ + operation: 'connect', + authMethod: 'usernameAndPassword', + providerName: 'vault', + statusCode: 401, + }), + ); + }); + it('uses the LIST verb when preferGet is disabled', async () => { mockInstance(ExternalSecretsConfig, { preferGet: false }); @@ -284,6 +326,33 @@ describe('VaultProvider', () => { expect(provider.hasSecret('forbidden')).toBe(false); expect(provider.getSecretNames()).toHaveLength(0); + expect(logger.debug).toHaveBeenCalledWith( + 'Vault provider failed to list KV secrets', + expect.objectContaining({ + operation: 'update', + mountPath: 'forbidden/', + kvVersion: '2', + vaultApiPath: 'forbidden/metadata/?list=true', + statusCode: 403, + }), + ); + }); + + it('should log and rethrow full update failures', async () => { + const { provider } = await initProvider([ + { method: 'GET', pathname: '/v1/sys/mounts', status: 500, body: { errors: [] } }, + ]); + + await expect(provider.update()).rejects.toThrow('Request failed with status 500'); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to update Vault provider secrets', + expect.objectContaining({ + operation: 'update', + providerName: 'vault', + statusCode: 500, + }), + ); }); }); @@ -432,6 +501,53 @@ describe('VaultProvider', () => { expect(message).toBe( 'Connection refused. Please check the host and port of the server are correct.', ); + expect(logger.warn).toHaveBeenCalledWith( + 'Vault provider test failed', + expect.objectContaining({ + operation: 'test', + vaultApiPath: 'auth/token/lookup-self', + errorCode: 'ECONNREFUSED', + }), + ); + }); + + it('logs connect failures with the connection test failure message', async () => { + const { provider } = await initProvider([ + { method: 'GET', pathname: '/v1/auth/token/lookup-self', status: 404, body: {} }, + ]); + + await provider.connect(); + + expect(provider.state).toBe('error'); + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to connect Vault provider', + expect.objectContaining({ + operation: 'connect', + authMethod: 'token', + providerName: 'vault', + }), + ); + }); + }); + + describe('connection logging', () => { + it('logs token refresh failures before attempting to reconnect', async () => { + const { provider } = await initProvider([ + { method: 'POST', pathname: '/v1/auth/token/renew-self', networkError: 'ECONNREFUSED' }, + ]); + const connect = vi.spyOn(provider, 'connect').mockResolvedValue(); + + await (provider as unknown as { tokenRefresh: () => Promise }).tokenRefresh(); + + expect(logger.warn).toHaveBeenCalledWith( + 'Failed to renew Vault token. Attempting to reconnect.', + expect.objectContaining({ + operation: 'tokenRefresh', + authMethod: 'token', + errorCode: 'ECONNREFUSED', + }), + ); + expect(connect).toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/modules/external-secrets.ee/providers/aws-secrets-manager.ts b/packages/cli/src/modules/external-secrets.ee/providers/aws-secrets-manager.ts index 55180db3726..48b2d5b35d4 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/aws-secrets-manager.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/aws-secrets-manager.ts @@ -2,18 +2,27 @@ import type { SecretsManager, SecretsManagerClientConfig } from '@aws-sdk/client import { Logger } from '@n8n/backend-common'; import { OutboundHttp } from '@n8n/backend-network'; import { Container } from '@n8n/di'; -import type { INodeProperties } from 'n8n-workflow'; +import { type INodeProperties } from 'n8n-workflow'; import { DOCS_HELP_NOTICE } from '../constants'; +import { + logSecretsProviderOperationFailure, + type LogContext, + type SecretsProviderOperationFailureParams, +} from '../errors/secrets-provider-errors'; import { UnknownAuthTypeError } from '../errors/unknown-auth-type.error'; -import { SecretsProvider } from '../types'; -import type { SecretsProviderSettings } from '../types'; +import { SecretsProvider, type SecretsProviderSettings } from '../types'; -type Secret = { +export type Secret = { secretName: string; secretValue: string; }; +export type AwsSecretsManagerSettings = { + region: string; + authMethod: 'iamUser' | 'autoDetect'; +}; + export type AwsSecretsManagerContext = SecretsProviderSettings< { region: string; @@ -100,6 +109,8 @@ export class AwsSecretsManager extends SecretsProvider { private client: SecretsManager; + private settings: AwsSecretsManagerSettings; + constructor( private readonly logger = Container.get(Logger), private readonly outboundHttp = Container.get(OutboundHttp), @@ -109,49 +120,69 @@ export class AwsSecretsManager extends SecretsProvider { } async init(context: AwsSecretsManagerContext) { - this.assertAuthType(context); - const { region, authMethod } = context.settings; - const clientConfig: SecretsManagerClientConfig = { region }; + this.settings = { region, authMethod }; - if (authMethod === 'iamUser') { - const { accessKeyId, secretAccessKey } = context.settings; - clientConfig.credentials = { accessKeyId, secretAccessKey }; + try { + this.assertAuthType(context); + + const clientConfig: SecretsManagerClientConfig = { region }; + + if (authMethod === 'iamUser') { + const { accessKeyId, secretAccessKey } = context.settings; + clientConfig.credentials = { accessKeyId, secretAccessKey }; + } + + // Drive the AWS SDK's HTTP transport through n8n's outbound client, + // so its calls reuse our agents (proxy + TLS) like every other outbound request. + // SigV4 signing and the credential chain stay with the SDK. + clientConfig.requestHandler = this.outboundHttp + .transport({ + ssrf: 'disabled', // fixed AWS-resolved Secrets Manager host, not user-controlled + }) + .getNodeAgent(); + + const { SecretsManager } = await import('@aws-sdk/client-secrets-manager'); + this.client = new SecretsManager(clientConfig); + + this.logger.debug('AWS Secrets Manager provider initialized'); + } catch (error) { + this.logOperationFailure('Failed to initialize AWS Secrets Manager provider', { + operation: 'initialize', + error, + }); + throw error; } - - // Drive the AWS SDK's HTTP transport through n8n's outbound client, - // so its calls reuse our agents (proxy + TLS) like every other outbound request. - // SigV4 signing and the credential chain stay with the SDK. - clientConfig.requestHandler = this.outboundHttp - .transport({ - ssrf: 'disabled', // fixed AWS-resolved Secrets Manager host, not user-controlled - }) - .getNodeAgent(); - - const { SecretsManager } = await import('@aws-sdk/client-secrets-manager'); - this.client = new SecretsManager(clientConfig); - - this.logger.debug('AWS Secrets Manager provider initialized'); } async test(): Promise<[boolean] | [boolean, string]> { try { - await this.client.listSecrets({ MaxResults: 1 }); + await this.verifyConnection(); return [true]; } catch (e) { const error = e instanceof Error ? e : new Error(`${e}`); + this.logOperationFailure('AWS Secrets Manager provider test failed', { + operation: 'test', + error, + context: this.awsErrorContext(error), + }); return [false, error.message]; } } protected async doConnect(): Promise { - const [wasSuccessful, errorMsg] = await this.test(); + try { + await this.verifyConnection(); - if (!wasSuccessful) { - throw new Error(errorMsg || 'Connection failed'); + this.logger.debug('AWS Secrets Manager provider connected'); + } catch (error) { + this.logOperationFailure('Failed to connect AWS Secrets Manager provider', { + operation: 'connect', + error, + context: this.awsErrorContext(error), + }); + throw error; } - - this.logger.debug('AWS Secrets Manager provider connected'); } async disconnect() { @@ -159,15 +190,24 @@ export class AwsSecretsManager extends SecretsProvider { } async update() { - const secrets = await this.fetchAllSecrets(); + try { + const secrets = await this.fetchAllSecrets(); - const supportedSecrets = secrets; + const supportedSecrets = secrets; - this.cachedSecrets = Object.fromEntries( - supportedSecrets.map((s) => [s.secretName, s.secretValue]), - ); + this.cachedSecrets = Object.fromEntries( + supportedSecrets.map((s) => [s.secretName, s.secretValue]), + ); - this.logger.debug('AWS Secrets Manager provider secrets updated'); + this.logger.debug('AWS Secrets Manager provider secrets updated'); + } catch (error) { + this.logOperationFailure('Failed to update AWS Secrets Manager provider secrets', { + operation: 'update', + error, + context: this.awsErrorContext(error), + }); + throw error; + } } getSecret(name: string) { @@ -239,4 +279,78 @@ export class AwsSecretsManager extends SecretsProvider { arr.slice(index * size, (index + 1) * size), ); } + + private async verifyConnection(): Promise { + await this.client.listSecrets({ MaxResults: 1 }); + } + + private isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null; + } + + private getAwsErrorCode(error: unknown): string | number | undefined { + if (this.isRecord(error)) { + if ('Code' in error && typeof error.Code === 'string') { + return error.Code; + } + + if ('code' in error) { + const { code } = error; + if (typeof code === 'string' || typeof code === 'number') { + return code; + } + } + } + + if (error instanceof Error) { + return error.name; + } + + if (this.isRecord(error) && typeof error.name === 'string') { + return error.name; + } + + return undefined; + } + + private awsErrorContext(error: unknown): LogContext { + const context: LogContext = {}; + + const errorCode = this.getAwsErrorCode(error); + if (errorCode !== undefined) { + context.errorCode = errorCode; + } + + if (this.isRecord(error) && '$metadata' in error) { + const metadata = error.$metadata; + if (this.isRecord(metadata) && typeof metadata.httpStatusCode === 'number') { + context.statusCode = metadata.httpStatusCode; + } + } + + return context; + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + const context: LogContext = { ...params.context }; + if (this.settings?.region) { + context.region = this.settings.region; + } + if (this.settings?.authMethod) { + context.authMethod = this.settings.authMethod; + } + + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context, + }); + } } diff --git a/packages/cli/src/modules/external-secrets.ee/providers/azure-key-vault/azure-key-vault.ts b/packages/cli/src/modules/external-secrets.ee/providers/azure-key-vault/azure-key-vault.ts index 182d23fff9e..47bc40234c7 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/azure-key-vault/azure-key-vault.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/azure-key-vault/azure-key-vault.ts @@ -1,3 +1,4 @@ +import { AuthenticationError } from '@azure/identity'; import type { SecretClient } from '@azure/keyvault-secrets'; import { Logger } from '@n8n/backend-common'; import { Container } from '@n8n/di'; @@ -6,8 +7,21 @@ import { type INodeProperties, UnexpectedError } from 'n8n-workflow'; import type { AzureKeyVaultContext } from './types'; import { DOCS_HELP_NOTICE } from '../../constants'; +import { + buildFailureSummaryLogContext, + type HttpProviderErrorLogContext, + type LogContext, + logSecretsProviderOperationFailure, + type SafeContextValue, + type SecretsProviderOperationFailureParams, +} from '../../errors/secrets-provider-errors'; import { SecretsProvider } from '../../types'; +type AzureHttpLikeError = Error & { + statusCode?: number; + code?: string; +}; + export class AzureKeyVault extends SecretsProvider { name = 'azureKeyVault'; @@ -76,16 +90,25 @@ export class AzureKeyVault extends SecretsProvider { } protected async doConnect(): Promise { - const { vaultName, tenantId, clientId, clientSecret } = this.settings; + try { + const { vaultName, tenantId, clientId, clientSecret } = this.settings; - const { ClientSecretCredential } = await import('@azure/identity'); - const { SecretClient } = await import('@azure/keyvault-secrets'); + const { ClientSecretCredential } = await import('@azure/identity'); + const { SecretClient } = await import('@azure/keyvault-secrets'); - // TODO: Not routed through OutboundHttp for now. It would require `@azure/core-rest-pipeline`, which is not worth it just to share agents. - const credential = new ClientSecretCredential(tenantId, clientId, clientSecret); - this.client = new SecretClient(`https://${vaultName}.vault.azure.net/`, credential); + // TODO: Not routed through OutboundHttp for now. It would require `@azure/core-rest-pipeline`, which is not worth it just to share agents. + const credential = new ClientSecretCredential(tenantId, clientId, clientSecret); + this.client = new SecretClient(`https://${vaultName}.vault.azure.net/`, credential); - this.logger.debug('Azure Key Vault provider connected'); + this.logger.debug('Azure Key Vault provider connected'); + } catch (error) { + this.logOperationFailure('Failed to connect Azure Key Vault provider', { + operation: 'connect', + error, + context: this.azureErrorContext(error), + }); + throw error; + } } async test(): Promise<[boolean] | [boolean, string]> { @@ -95,6 +118,11 @@ export class AzureKeyVault extends SecretsProvider { await this.client.listPropertiesOfSecrets().next(); return [true]; } catch (error: unknown) { + this.logOperationFailure('Azure Key Vault provider test failed', { + operation: 'test', + error, + context: this.azureErrorContext(error), + }); return [false, error instanceof Error ? error.message : 'Unknown error']; } } @@ -104,42 +132,91 @@ export class AzureKeyVault extends SecretsProvider { } async update() { - const secretNames: string[] = []; - for await (const secret of this.client.listPropertiesOfSecrets()) { - if (secret.enabled === false) continue; - secretNames.push(secret.name); - } + try { + const secretNames: string[] = []; + for await (const secret of this.client.listPropertiesOfSecrets()) { + if (secret.enabled === false) continue; + secretNames.push(secret.name); + } - const promises = await Promise.allSettled( - secretNames.map(async (name) => { - const { value } = await this.client.getSecret(name); - return { name, value }; - }), - ); + const promises = await Promise.allSettled( + secretNames.map(async (name) => { + const { value } = await this.client.getSecret(name); + return { name, value }; + }), + ); - const updated: Record = {}; - const readErrors: Error[] = []; - for (const [index, promiseResult] of promises.entries()) { - if (promiseResult.status === 'fulfilled') { - const { name, value } = promiseResult.value; - if (value !== undefined) updated[name] = value; - } else { - const error = ensureError(promiseResult.reason); - readErrors.push(error); - this.logger.warn(`Could not read Azure Key Vault secret "${secretNames[index]}"`, { - error, + const updated: Record = {}; + const readErrors: Error[] = []; + const failedSecrets: Array<{ name: string; errorCode: SafeContextValue }> = []; + for (const [index, promiseResult] of promises.entries()) { + if (promiseResult.status === 'fulfilled') { + const { name, value } = promiseResult.value; + if (value !== undefined) updated[name] = value; + } else { + const error = ensureError(promiseResult.reason); + readErrors.push(error); + const secretName = secretNames[index]; + const errorContext = this.azureErrorContext(error); + this.logger.debug(`Could not read Azure Key Vault secret "${secretName}"`, { + providerName: this.name, + operation: 'update', + vaultName: this.settings.vaultName, + secretName, + ...errorContext, + }); + failedSecrets.push({ + name: secretName, + errorCode: errorContext.errorCode ?? 'unknown', + }); + } + } + + const failureSummary = buildFailureSummaryLogContext(failedSecrets); + const isTotalFailure = + secretNames.length > 0 && Object.keys(updated).length === 0 && readErrors.length > 0; + + if (failureSummary && !isTotalFailure) { + this.logOperationFailure('Skipped unreadable Azure Key Vault secrets during update', { + operation: 'update', + error: + readErrors[0] ?? new Error('One or more Azure Key Vault secrets could not be read'), + context: { + ...this.azureErrorContext(readErrors[0]), + ...failureSummary, + }, }); } - } - if (secretNames.length > 0 && Object.keys(updated).length === 0 && readErrors.length > 0) { - throw new UnexpectedError('Could not read any secrets from Azure Key Vault', { - cause: readErrors[0], + if (isTotalFailure) { + const error = readErrors[0]; + this.logOperationFailure('Could not read any secrets from Azure Key Vault', { + operation: 'update', + error, + context: { + ...this.azureErrorContext(error), + ...failureSummary, + }, + }); + throw new UnexpectedError('Could not read any secrets from Azure Key Vault', { + cause: error, + }); + } + + this.cachedSecrets = updated; + this.logger.debug('Azure Key Vault provider secrets updated'); + } catch (error) { + if (error instanceof UnexpectedError) { + throw error; + } + + this.logOperationFailure('Failed to update Azure Key Vault provider secrets', { + operation: 'update', + error, + context: this.azureErrorContext(error), }); + throw error; } - - this.cachedSecrets = updated; - this.logger.debug('Azure Key Vault provider secrets updated'); } getSecret(name: string) { @@ -153,4 +230,57 @@ export class AzureKeyVault extends SecretsProvider { getSecretNames() { return Object.keys(this.cachedSecrets); } + + private isAzureHttpLikeError(error: unknown): error is AzureHttpLikeError { + if (!(error instanceof Error)) return false; + + const candidate = error as AzureHttpLikeError; + return ( + error.name === 'RestError' || + typeof candidate.statusCode === 'number' || + typeof candidate.code === 'string' + ); + } + + private azureErrorContext(error: unknown): HttpProviderErrorLogContext { + if (error instanceof AuthenticationError) { + return { + statusCode: error.statusCode, + errorCode: error.errorResponse?.error, + }; + } + + if (this.isAzureHttpLikeError(error)) { + return { + statusCode: error.statusCode, + errorCode: error.code, + }; + } + + if (error instanceof Error) { + return { errorCode: error.name }; + } + + return {}; + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + const context: LogContext = { ...params.context }; + if (this.settings?.vaultName) { + context.vaultName = this.settings.vaultName; + } + + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context, + }); + } } diff --git a/packages/cli/src/modules/external-secrets.ee/providers/gcp-secrets-manager/gcp-secrets-manager.ts b/packages/cli/src/modules/external-secrets.ee/providers/gcp-secrets-manager/gcp-secrets-manager.ts index 2ac2283d46d..7a60b8d273e 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/gcp-secrets-manager/gcp-secrets-manager.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/gcp-secrets-manager/gcp-secrets-manager.ts @@ -1,7 +1,6 @@ import type { protos, SecretManagerServiceClient as GcpClient } from '@google-cloud/secret-manager'; import { Logger } from '@n8n/backend-common'; import { Container } from '@n8n/di'; -import { ensureError } from '@n8n/utils/errors/ensure-error'; import { jsonParse, UserError, type INodeProperties } from 'n8n-workflow'; import type { @@ -10,6 +9,13 @@ import type { RawGcpSecretAccountKey, } from './types'; import { DOCS_HELP_NOTICE } from '../../constants'; +import { + buildFailureSummaryLogContext, + type LogContext, + logSecretsProviderOperationFailure, + type SafeContextValue, + type SecretsProviderOperationFailureParams, +} from '../../errors/secrets-provider-errors'; import { SecretsProvider } from '../../types'; export class GcpSecretsManager extends SecretsProvider { @@ -44,22 +50,40 @@ export class GcpSecretsManager extends SecretsProvider { } async init(context: GcpSecretsManagerContext) { - this.settings = this.parseSecretAccountKey(context.settings.serviceAccountKey); + try { + this.settings = this.parseSecretAccountKey(context.settings.serviceAccountKey); + } catch (error) { + this.logOperationFailure('Failed to initialize GCP Secrets Manager provider', { + operation: 'initialize', + error, + }); + throw error; + } } protected async doConnect(): Promise { - const { projectId, privateKey, clientEmail } = this.settings; + try { + const { projectId, privateKey, clientEmail } = this.settings; - const { SecretManagerServiceClient: GcpClient } = await import('@google-cloud/secret-manager'); + const { SecretManagerServiceClient: GcpClient } = await import( + '@google-cloud/secret-manager' + ); - // TODO: gRPC bypasses @n8n/backend-network, so the configured proxy and SSRF/DNS rules are not enforced here. - // Route through it once it supports a gRPC transport. - this.client = new GcpClient({ - credentials: { client_email: clientEmail, private_key: privateKey }, - projectId, - }); + // TODO: gRPC bypasses @n8n/backend-network, so the configured proxy and SSRF/DNS rules are not enforced here. + // Route through it once it supports a gRPC transport. + this.client = new GcpClient({ + credentials: { client_email: clientEmail, private_key: privateKey }, + projectId, + }); - this.logger.debug('GCP Secrets Manager provider connected'); + this.logger.debug('GCP Secrets Manager provider connected'); + } catch (error) { + this.logOperationFailure('Failed to connect GCP Secrets Manager provider', { + operation: 'connect', + error, + }); + throw error; + } } async test(): Promise<[boolean] | [boolean, string]> { @@ -69,6 +93,10 @@ export class GcpSecretsManager extends SecretsProvider { await this.client.initialize(); return [true]; } catch (error: unknown) { + this.logOperationFailure('GCP Secrets Manager provider test failed', { + operation: 'test', + error, + }); return [false, error instanceof Error ? error.message : 'Unknown error']; } } @@ -78,73 +106,104 @@ export class GcpSecretsManager extends SecretsProvider { } async update() { - const { projectId } = this.settings; + try { + const { projectId } = this.settings; - const [rawSecretNames] = await this.client.listSecrets({ - parent: `projects/${projectId}`, - }); + const [rawSecretNames] = await this.client.listSecrets({ + parent: `projects/${projectId}`, + }); - const secretNames = rawSecretNames.reduce((acc, cur) => { - if (!cur.name) return acc; + const secretNames = rawSecretNames.reduce((acc, cur) => { + if (!cur.name) return acc; - const secretName = cur.name.split('/').pop(); + const secretName = cur.name.split('/').pop(); - if (secretName) acc.push(secretName); + if (secretName) acc.push(secretName); - return acc; - }, []); + return acc; + }, []); - const promises = secretNames.map(async (name) => { - let versions: - | [ - protos.google.cloud.secretmanager.v1.IAccessSecretVersionResponse, - protos.google.cloud.secretmanager.v1.IAccessSecretVersionRequest | undefined, - {} | undefined, - ] - | undefined; + const skippedSecrets: Array<{ name: string; errorCode: SafeContextValue }> = []; + let firstSkippedError: unknown; - try { - versions = await this.client.accessSecretVersion({ - name: `projects/${projectId}/secrets/${name}/versions/latest`, - }); - } catch (error) { - // Only handle expected error codes that indicate the secret is not accessible - // PERMISSION_DENIED (7), NOT_FOUND (5), UNAVAILABLE (14) - const errorCode = error?.code; - if (errorCode === 7 || errorCode === 5 || errorCode === 14) { - this.logger.info( - `Skipping GCP secret: ${name}, version: latest as the version is not accessible`, - { - error: ensureError(error), - }, - ); - } else { - // Rethrow unexpected errors to avoid masking broader failures - throw error; + const promises = secretNames.map(async (name) => { + let versions: + | [ + protos.google.cloud.secretmanager.v1.IAccessSecretVersionResponse, + protos.google.cloud.secretmanager.v1.IAccessSecretVersionRequest | undefined, + {} | undefined, + ] + | undefined; + + try { + versions = await this.client.accessSecretVersion({ + name: `projects/${projectId}/secrets/${name}/versions/latest`, + }); + } catch (error) { + // Only handle expected error codes that indicate the secret is not accessible + // PERMISSION_DENIED (7), NOT_FOUND (5), UNAVAILABLE (14) + const errorCode = this.getGcpErrorCode(error); + if (errorCode === 7 || errorCode === 5 || errorCode === 14) { + if (firstSkippedError === undefined) { + firstSkippedError = error; + } + this.logger.debug('Skipping inaccessible GCP secret version', { + providerName: this.name, + operation: 'update', + projectId, + secretName: name, + errorCode, + }); + skippedSecrets.push({ + name, + errorCode: errorCode ?? 'unknown', + }); + } else { + // Rethrow unexpected errors to avoid masking broader failures + throw error; + } } + + if (!Array.isArray(versions) || !versions.length) return null; + + const [latestVersion] = versions; + + if (!latestVersion.payload?.data) return null; + + const value = latestVersion.payload.data.toString(); + + if (!value) return null; + + return { name, value }; + }); + + const results = await Promise.all(promises); + + this.cachedSecrets = results.reduce>((acc, cur) => { + if (cur) acc[cur.name] = cur.value; + return acc; + }, {}); + + const failureSummary = buildFailureSummaryLogContext(skippedSecrets); + if (failureSummary) { + this.logOperationFailure('Skipped inaccessible GCP secret versions during update', { + operation: 'update', + error: + firstSkippedError instanceof Error + ? firstSkippedError + : new Error('One or more GCP secret versions were inaccessible'), + context: failureSummary, + }); } - if (!Array.isArray(versions) || !versions.length) return null; - - const [latestVersion] = versions; - - if (!latestVersion.payload?.data) return null; - - const value = latestVersion.payload.data.toString(); - - if (!value) return null; - - return { name, value }; - }); - - const results = await Promise.all(promises); - - this.cachedSecrets = results.reduce>((acc, cur) => { - if (cur) acc[cur.name] = cur.value; - return acc; - }, {}); - - this.logger.debug('GCP Secrets Manager provider secrets updated'); + this.logger.debug('GCP Secrets Manager provider secrets updated'); + } catch (error) { + this.logOperationFailure('Failed to update GCP Secrets Manager provider secrets', { + operation: 'update', + error, + }); + throw error; + } } getSecret(name: string) { @@ -168,6 +227,9 @@ export class GcpSecretsManager extends SecretsProvider { const projectId = secretAccountKey.project_id?.trim(); if (!clientEmail || !privateKey) { + this.logger.warn( + 'Service account key must contain "client_email" and "private_key" fields. Use the downloaded service account JSON key file from Google Cloud Console.', + ); throw new UserError( 'Service account key must contain "client_email" and "private_key" fields. Use the downloaded service account JSON key file from Google Cloud Console.', ); @@ -179,4 +241,39 @@ export class GcpSecretsManager extends SecretsProvider { privateKey, }; } + + private getGcpErrorCode(error: unknown): number | string | undefined { + if (typeof error === 'object' && error !== null && 'code' in error) { + const { code } = error; + if (typeof code === 'number' || typeof code === 'string') { + return code; + } + } + + return undefined; + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + const context: LogContext = { ...params.context }; + const errorCode = this.getGcpErrorCode(params.error); + if (errorCode !== undefined) { + context.errorCode = errorCode; + } + if (this.settings?.projectId) { + context.projectId = this.settings.projectId; + } + + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context, + }); + } } diff --git a/packages/cli/src/modules/external-secrets.ee/providers/infisical.ts b/packages/cli/src/modules/external-secrets.ee/providers/infisical.ts index 90af083ad9f..84d75714451 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/infisical.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/infisical.ts @@ -9,12 +9,18 @@ import { Container } from '@n8n/di'; import { type INodeProperties, UnexpectedError } from 'n8n-workflow'; import { DOCS_HELP_NOTICE } from '../constants'; +import { + buildHttpProviderErrorContext, + logSecretsProviderOperationFailure, + type LogContext, + type SecretsProviderOperationFailureParams, +} from '../errors/secrets-provider-errors'; import type { SecretsProviderSettings } from '../types'; import { SecretsProvider } from '../types'; -type InfisicalAuthMethod = 'universalAuth'; +export type InfisicalAuthMethod = 'universalAuth'; -interface InfisicalSettings { +export interface InfisicalSettings { siteURL: string; projectId: string; environment: string; @@ -26,25 +32,25 @@ interface InfisicalSettings { clientSecret: string; } -interface InfisicalUniversalAuthLoginResponse { +export interface InfisicalUniversalAuthLoginResponse { accessToken: string; expiresIn: number; accessTokenMaxTTL: number; tokenType: string; } -interface InfisicalSecret { +export interface InfisicalSecret { secretKey: string; secretValue: string; } -interface InfisicalImport { +export interface InfisicalImport { secrets: InfisicalSecret[]; secretPath: string; environment: string; } -interface InfisicalListSecretsResponse { +export interface InfisicalListSecretsResponse { secrets: InfisicalSecret[]; imports: InfisicalImport[]; } @@ -178,19 +184,27 @@ export class InfisicalProvider extends SecretsProvider { protected async doConnect(): Promise { this.refreshAbort = new AbortController(); - if (this.settings.authMethod === 'universalAuth') { - if (!this.settings.clientId || !this.settings.clientSecret) { - throw new UnexpectedError('Client ID and Client Secret are required for Universal Auth'); + try { + if (this.settings.authMethod === 'universalAuth') { + if (!this.settings.clientId || !this.settings.clientSecret) { + throw new UnexpectedError('Client ID and Client Secret are required for Universal Auth'); + } + await this.loginUniversalAuth(); } - await this.loginUniversalAuth(); - } - const [testSuccess, testMessage] = await this.test(); - if (!testSuccess) { - throw new Error(testMessage ?? 'Connection test failed'); - } + await this.verifyWorkspaceAccess(); - this.setupTokenRefresh(); + this.setupTokenRefresh(); + } catch (error) { + const context = + error instanceof UnexpectedError ? undefined : buildHttpProviderErrorContext(error); + this.logOperationFailure('Failed to connect Infisical provider', { + operation: 'connect', + error, + context, + }); + throw error; + } } async disconnect(): Promise { @@ -206,32 +220,18 @@ export class InfisicalProvider extends SecretsProvider { async test(): Promise<[boolean] | [boolean, string]> { try { - const resp = await this.http.request({ - url: `/api/v1/workspace/${encodeURIComponent(this.settings.projectId)}`, - method: 'GET', - returnFullResponse: true, - ignoreHttpStatusErrors: true, // Resolve non-2xx responses instead of throwing so the status checks below can return tailored messages. + await this.verifyWorkspaceAccess(); + return [true]; + } catch (error) { + this.logOperationFailure('Infisical provider test failed', { + operation: 'test', + error, + context: { + ...buildHttpProviderErrorContext(error), + endpoint: 'workspace', + }, }); - if (resp.statusCode >= 200 && resp.statusCode < 300) { - return [true]; - } - - if (resp.statusCode === 401) { - return [false, 'Invalid credentials']; - } - if (resp.statusCode === 403) { - return [ - false, - 'Permission denied. Verify the machine identity has access to this project.', - ]; - } - if (resp.statusCode === 404) { - return [false, 'Project not found. Check the Project ID and Site URL.']; - } - - return [false, `Unexpected response from Infisical (status ${resp.statusCode}).`]; - } catch (error) { if (isConnectionRefusedError(error)) { return [false, 'Connection refused. Check the Site URL.']; } @@ -244,17 +244,31 @@ export class InfisicalProvider extends SecretsProvider { throw new UnexpectedError('Update attempted on Infisical before authentication'); } - await this.ensureTokenFresh(); - try { - this.cacheSecrets(await this.fetchSecrets()); - } catch (error) { - if (httpStatusFromError(error) === 401) { - this.logger.debug('Infisical token rejected during update; re-authenticating and retrying'); - await this.loginUniversalAuth(); + await this.ensureTokenFresh(); + + try { this.cacheSecrets(await this.fetchSecrets()); - return; + } catch (error) { + if (httpStatusFromError(error) === 401) { + this.logger.debug( + 'Infisical token rejected during update; re-authenticating and retrying', + ); + await this.loginUniversalAuth(); + this.cacheSecrets(await this.fetchSecrets()); + return; + } + throw error; } + } catch (error) { + this.logOperationFailure('Failed to update Infisical provider secrets', { + operation: 'update', + error, + context: { + ...buildHttpProviderErrorContext(error), + endpoint: 'secrets', + }, + }); throw error; } } @@ -337,8 +351,12 @@ export class InfisicalProvider extends SecretsProvider { await this.loginUniversalAuth(); if (this.refreshAbort.signal.aborted) return; this.setupTokenRefresh(); - } catch { - this.logger.error('Failed to refresh Infisical token. Attempting reconnect.'); + } catch (error) { + this.logOperationFailure('Failed to refresh Infisical token. Attempting reconnect.', { + operation: 'tokenRefresh', + error, + context: buildHttpProviderErrorContext(error), + }); void this.connect(); } }; @@ -364,4 +382,55 @@ export class InfisicalProvider extends SecretsProvider { } return {}; } + + private async verifyWorkspaceAccess(): Promise { + const resp = await this.http.request({ + url: `/api/v1/workspace/${encodeURIComponent(this.settings.projectId)}`, + method: 'GET', + returnFullResponse: true, + ignoreHttpStatusErrors: true, // Resolve non-2xx responses instead of throwing so the status checks below can return tailored messages. + }); + + if (resp.statusCode >= 200 && resp.statusCode < 300) { + return; + } + + if (resp.statusCode === 401) { + throw new Error('Invalid credentials'); + } + if (resp.statusCode === 403) { + throw new Error('Permission denied. Verify the machine identity has access to this project.'); + } + if (resp.statusCode === 404) { + throw new Error('Project not found. Check the Project ID and Site URL.'); + } + + throw new Error(`Unexpected response from Infisical (status ${resp.statusCode}).`); + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + const context: LogContext = { ...params.context }; + if (this.settings) { + const { siteURL, projectId, authMethod, environment, secretPath } = this.settings; + Object.assign(context, { siteURL, projectId }); + if (params.operation === 'connect' || params.operation === 'tokenRefresh') { + context.authMethod = authMethod; + } else if (params.operation === 'update') { + Object.assign(context, { environment, secretPath }); + } + } + + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context, + }); + } } diff --git a/packages/cli/src/modules/external-secrets.ee/providers/one-password.ts b/packages/cli/src/modules/external-secrets.ee/providers/one-password.ts index 4efaeecae06..869f70f44c8 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/one-password.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/one-password.ts @@ -4,6 +4,12 @@ import { Container } from '@n8n/di'; import { UserError, type IDataObject, type INodeProperties } from 'n8n-workflow'; import { DOCS_HELP_NOTICE } from '../constants'; +import { + buildHttpProviderErrorContext, + logSecretsProviderOperationFailure, + type LogContext, + type SecretsProviderOperationFailureParams, +} from '../errors/secrets-provider-errors'; import { SecretsProvider, type SecretsProviderSettings } from '../types'; export type OnePasswordContext = SecretsProviderSettings<{ @@ -11,6 +17,11 @@ export type OnePasswordContext = SecretsProviderSettings<{ accessToken: string; }>; +type OnePasswordSettings = { + serverUrl: string; + accessToken: string; +}; + export class OnePasswordProvider extends SecretsProvider { name = 'onePassword'; @@ -45,7 +56,7 @@ export class OnePasswordProvider extends SecretsProvider { private client: OPConnect; - private settings: { serverUrl: string; accessToken: string }; + private settings: OnePasswordSettings; constructor(private readonly logger = Container.get(Logger)) { super(); @@ -53,51 +64,72 @@ export class OnePasswordProvider extends SecretsProvider { } async init(context: OnePasswordContext) { - const trimmedServerUrl = context.settings.serverUrl?.trim(); - const trimmedAccessToken = context.settings.accessToken?.trim(); + try { + const trimmedServerUrl = context.settings.serverUrl?.trim(); + const trimmedAccessToken = context.settings.accessToken?.trim(); - if (!trimmedServerUrl) { - throw new UserError('Connect Server URL is required.'); + if (!trimmedServerUrl) { + throw new UserError('Connect Server URL is required.'); + } + + if (!trimmedAccessToken) { + throw new UserError('Access Token is required.'); + } + + this.settings = { + serverUrl: trimmedServerUrl, + accessToken: trimmedAccessToken, + }; + } catch (error) { + this.logOperationFailure('Failed to initialize 1Password provider', { + operation: 'initialize', + error, + }); + throw error; } - - if (!trimmedAccessToken) { - throw new UserError('Access Token is required.'); - } - - this.settings = { - serverUrl: trimmedServerUrl, - accessToken: trimmedAccessToken, - }; } protected async doConnect(): Promise { - const { OnePasswordConnect } = await import('@1password/connect'); + try { + const { OnePasswordConnect } = await import('@1password/connect'); - // TODO: the @1password/connect SDK exposes no transport/agent injection hook, - // so requests bypass @n8n/backend-network and the configured proxy and SSRF/DNS rules are not enforced here. - // Route through it once the SDK supports a custom client. - this.client = OnePasswordConnect({ - serverURL: this.settings.serverUrl, - token: this.settings.accessToken, - keepAlive: true, - }); + // TODO: the @1password/connect SDK exposes no transport/agent injection hook, + // so requests bypass @n8n/backend-network and the configured proxy and SSRF/DNS rules are not enforced here. + // Route through it once the SDK supports a custom client. + this.client = OnePasswordConnect({ + serverURL: this.settings.serverUrl, + token: this.settings.accessToken, + keepAlive: true, + }); - const [wasSuccessful, errorMessage] = await this.test(); + await this.verifyConnection(); - if (!wasSuccessful) { - throw new Error(errorMessage || 'Connection failed'); + this.logger.debug('1Password provider connected'); + } catch (error) { + this.logOperationFailure('Failed to connect 1Password provider', { + operation: 'connect', + error, + context: buildHttpProviderErrorContext(error), + }); + throw error; } - - this.logger.debug('1Password provider connected'); } async test(): Promise<[boolean] | [boolean, string]> { if (!this.client) return [false, 'Client not initialized']; try { - await this.client.listVaults(); + await this.verifyConnection(); return [true]; } catch (error: unknown) { + this.logOperationFailure('1Password provider test failed', { + operation: 'test', + error, + context: { + ...buildHttpProviderErrorContext(error), + endpoint: 'vaults', + }, + }); return [false, error instanceof Error ? error.message : 'Unknown error']; } } @@ -107,38 +139,50 @@ export class OnePasswordProvider extends SecretsProvider { } async update() { - const vaults = await this.client.listVaults(); + try { + const vaults = await this.client.listVaults(); - const secrets: Record = {}; + const secrets: Record = {}; - for (const vault of vaults) { - if (!vault.id) continue; + for (const vault of vaults) { + if (!vault.id) continue; - const items = await this.client.listItems(vault.id); + const items = await this.client.listItems(vault.id); - for (const item of items) { - if (!item.id || !item.title) continue; + for (const item of items) { + if (!item.id || !item.title) continue; - const fullItem = await this.client.getItemById(vault.id, item.id); + const fullItem = await this.client.getItemById(vault.id, item.id); - if (!fullItem.fields?.length) continue; + if (!fullItem.fields?.length) continue; - const fieldValues: IDataObject = {}; - for (const field of fullItem.fields) { - if (field.label && field.value) { - fieldValues[field.label] = field.value; + const fieldValues: IDataObject = {}; + for (const field of fullItem.fields) { + if (field.label && field.value) { + fieldValues[field.label] = field.value; + } } + + if (Object.keys(fieldValues).length === 0) continue; + + secrets[item.title] = fieldValues; } - - if (Object.keys(fieldValues).length === 0) continue; - - secrets[item.title] = fieldValues; } + + this.cachedSecrets = secrets; + + this.logger.debug('1Password provider secrets updated'); + } catch (error) { + this.logOperationFailure('Failed to update 1Password provider secrets', { + operation: 'update', + error, + context: { + ...buildHttpProviderErrorContext(error), + endpoint: 'secrets', + }, + }); + throw error; } - - this.cachedSecrets = secrets; - - this.logger.debug('1Password provider secrets updated'); } getSecret(name: string): IDataObject { @@ -152,4 +196,28 @@ export class OnePasswordProvider extends SecretsProvider { getSecretNames() { return Object.keys(this.cachedSecrets); } + + private async verifyConnection(): Promise { + await this.client.listVaults(); + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + const context: LogContext = { ...params.context }; + if (this.settings?.serverUrl) { + context.serverUrl = this.settings.serverUrl; + } + + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context, + }); + } } diff --git a/packages/cli/src/modules/external-secrets.ee/providers/vault.ts b/packages/cli/src/modules/external-secrets.ee/providers/vault.ts index 5e12f855e4f..cf1ac62110a 100644 --- a/packages/cli/src/modules/external-secrets.ee/providers/vault.ts +++ b/packages/cli/src/modules/external-secrets.ee/providers/vault.ts @@ -5,15 +5,20 @@ import { OutboundHttp, } from '@n8n/backend-network'; import { Container } from '@n8n/di'; -import type { - IDataObject, - IHttpRequestMethods, - IHttpRequestOptions, - IN8nHttpFullResponse, - INodeProperties, +import { + type IDataObject, + type IHttpRequestMethods, + type IHttpRequestOptions, + type IN8nHttpFullResponse, + type INodeProperties, } from 'n8n-workflow'; import { DOCS_HELP_NOTICE } from '../constants'; +import { + buildHttpProviderErrorContext, + logSecretsProviderOperationFailure, + type SecretsProviderOperationFailureParams, +} from '../errors/secrets-provider-errors'; import { ExternalSecretsConfig } from '../external-secrets.config'; import type { SecretsProviderSettings } from '../types'; import { SecretsProvider } from '../types'; @@ -291,33 +296,47 @@ export class VaultProvider extends SecretsProvider { } protected async doConnect(): Promise { - // Authenticate based on method - if (this.settings.authMethod === 'token') { - this.#currentToken = this.settings.token; - } else if (this.settings.authMethod === 'usernameAndPassword') { - this.#currentToken = await this.authUsernameAndPassword( - this.settings.username, - this.settings.password, - ); - if (!this.#currentToken) { - throw new Error('Failed to authenticate with Username and Password'); + try { + // Authenticate based on method + if (this.settings.authMethod === 'token') { + this.#currentToken = this.settings.token; + } else if (this.settings.authMethod === 'usernameAndPassword') { + this.#currentToken = await this.authUsernameAndPassword( + this.settings.username, + this.settings.password, + ); + if (!this.#currentToken) { + throw new Error('Failed to authenticate with Username and Password'); + } + } else if (this.settings.authMethod === 'appRole') { + this.#currentToken = await this.authAppRole(this.settings.roleId, this.settings.secretId); + if (!this.#currentToken) { + throw new Error('Failed to authenticate with AppRole'); + } } - } else if (this.settings.authMethod === 'appRole') { - this.#currentToken = await this.authAppRole(this.settings.roleId, this.settings.secretId); - if (!this.#currentToken) { - throw new Error('Failed to authenticate with AppRole'); + + // Test connection + const [testSuccess, failureMessage] = await this.test(); + if (!testSuccess) { + throw new Error(failureMessage ?? 'Connection test failed'); } - } - // Test connection - const [testSuccess] = await this.test(); - if (!testSuccess) { - throw new Error('Connection test failed'); + // Setup token refresh + [this.#tokenInfo] = await this.getTokenInfo(); + this.setupTokenRefresh(); + } catch (error) { + if (!this.isVaultAuthFailure(error)) { + this.logOperationFailure('Failed to connect Vault provider', { + operation: 'connect', + error, + context: { + ...buildHttpProviderErrorContext(error), + authMethod: this.settings.authMethod, + }, + }); + } + throw error; } - - // Setup token refresh - [this.#tokenInfo] = await this.getTokenInfo(); - this.setupTokenRefresh(); } async disconnect(): Promise { @@ -367,8 +386,15 @@ export class VaultProvider extends SecretsProvider { } this.setupTokenRefresh(); - } catch { - this.logger.error('Failed to renew Vault token. Attempting to reconnect.'); + } catch (error) { + this.logOperationFailure('Failed to renew Vault token. Attempting to reconnect.', { + operation: 'tokenRefresh', + error, + context: { + ...buildHttpProviderErrorContext(error), + authMethod: this.settings.authMethod, + }, + }); void this.connect(); } }; @@ -386,7 +412,15 @@ export class VaultProvider extends SecretsProvider { }); return body.auth.client_token; - } catch { + } catch (error) { + this.logOperationFailure('Vault provider username/password authentication failed', { + operation: 'connect', + error, + context: { + ...buildHttpProviderErrorContext(error), + authMethod: 'usernameAndPassword', + }, + }); return null; } } @@ -401,7 +435,15 @@ export class VaultProvider extends SecretsProvider { }); return body.auth.client_token; - } catch { + } catch (error) { + this.logOperationFailure('Vault provider AppRole authentication failed', { + operation: 'connect', + error, + context: { + ...buildHttpProviderErrorContext(error), + authMethod: 'appRole', + }, + }); return null; } } @@ -438,7 +480,18 @@ export class VaultProvider extends SecretsProvider { // non-standard `LIST` verb works; `preferGet` swaps it for `GET ?list=true`. const method = (shouldPreferGet ? 'GET' : 'LIST') as IHttpRequestMethods; listBody = await this.#http.request>({ url, method }); - } catch { + } catch (error) { + const shouldPreferGet = Container.get(ExternalSecretsConfig).preferGet; + const vaultApiPath = `${listPath}${shouldPreferGet ? '?list=true' : ''}`; + const errorContext = buildHttpProviderErrorContext(error); + this.logger.debug('Vault provider failed to list KV secrets', { + providerName: this.name, + operation: 'update', + mountPath, + kvVersion, + vaultApiPath, + ...errorContext, + }); return null; } const data = Object.fromEntries( @@ -463,7 +516,16 @@ export class VaultProvider extends SecretsProvider { key, kvVersion === '2' ? (secretBody.data.data as IDataObject) : secretBody.data, ]; - } catch { + } catch (error) { + const errorContext = buildHttpProviderErrorContext(error); + this.logger.debug('Vault provider failed to read KV secret', { + providerName: this.name, + operation: 'update', + mountPath, + kvVersion, + secretPath, + ...errorContext, + }); return null; } }), @@ -542,23 +604,33 @@ export class VaultProvider extends SecretsProvider { } async update(): Promise { - const kvMounts = await this.discoverKvMounts(); + try { + const kvMounts = await this.discoverKvMounts(); - const secrets = Object.fromEntries( - ( - await Promise.all( - kvMounts.map(async ({ path, version }): Promise<[string, IDataObject] | null> => { - const value = await this.getKVSecrets(path, version, ''); - if (value === null) { - return null; - } - return [path.substring(0, path.length - 1), value[1]]; - }), - ) - ).filter((entry): entry is [string, IDataObject] => entry !== null), - ); - this.cachedSecrets = secrets; - this.logger.debug('Vault provider secrets updated'); + const secrets = Object.fromEntries( + ( + await Promise.all( + kvMounts.map(async ({ path, version }): Promise<[string, IDataObject] | null> => { + const value = await this.getKVSecrets(path, version, ''); + if (value === null) { + return null; + } + return [path.substring(0, path.length - 1), value[1]]; + }), + ) + ).filter((entry): entry is [string, IDataObject] => entry !== null), + ); + this.cachedSecrets = secrets; + + this.logger.debug('Vault provider secrets updated'); + } catch (error) { + this.logOperationFailure('Failed to update Vault provider secrets', { + operation: 'update', + error, + context: buildHttpProviderErrorContext(error), + }); + throw error; + } } async test(): Promise<[boolean] | [boolean, string]> { @@ -574,6 +646,15 @@ export class VaultProvider extends SecretsProvider { return await this.testSecretAccess(); } catch (error) { + this.logOperationFailure('Vault provider test failed', { + operation: 'test', + error, + context: { + ...buildHttpProviderErrorContext(error), + vaultApiPath: 'auth/token/lookup-self', + }, + }); + if (isConnectionRefusedError(error)) { return [ false, @@ -630,4 +711,27 @@ export class VaultProvider extends SecretsProvider { ignoreHttpStatusErrors: true, // Resolve non-2xx responses instead of throwing so the status checks below can return tailored messages. }); } + + private isVaultAuthFailure(error: unknown): boolean { + return ( + error instanceof Error && + (error.message === 'Failed to authenticate with Username and Password' || + error.message === 'Failed to authenticate with AppRole') + ); + } + + private logOperationFailure( + message: string, + params: SecretsProviderOperationFailureParams, + ): void { + logSecretsProviderOperationFailure({ + logger: this.logger, + message, + providerName: this.name, + providerDisplayName: this.displayName, + operation: params.operation, + error: params.error, + context: params.context ?? {}, + }); + } }