From c2c87d9dd861a25dcbd9aa830e81ecb4d76ba509 Mon Sep 17 00:00:00 2001 From: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com> Date: Mon, 10 Jan 2022 04:21:32 -0500 Subject: [PATCH] fix(pipelines): `DockerCredential.dockerHub()` silently fails auth (#18313) ### Problem: `DockerCredential.dockerHub()` silently failed to authenticate users, resulting in unexpected and intermittent throttling due to docker's policy for unauthenticated users. ### Reason: `.dockerHub()` added `index.docker.io` to the domain credentials, but the actual docker command [authenticated](https://github.com/moby/moby/blob/1e71c6cffedb79e3def696652753ea43cdc47b99/registry/config.go#L35) with `https://index.docker.io/v1/` which it was unable to find as a domain credential, thus failing to trigger `docker-credential-cdk-assets` during the `docker --config build` call. Furthermore, the credential `DockerCredential.customRegistry('https://index.docker.io/v1/', secret)` alone does not work. This would successfully trigger `docker-credential-cdk-assets` but fail to authenticate because of how `cdk-assets` handles credential lookup. The command strips the endpoint into just a hostname so in this case we try `fetchDockerLoginCredentials(awsClient, config, 'index.docker.io')` which fails: https://github.com/aws/aws-cdk/blob/4fb0309e3b93be276ab3e2d510ffc2ce35823dcd/packages/cdk-assets/bin/docker-credential-cdk-assets.ts#L32-L38 So the workaround for this bug was to specify both domains as credentials, each to satisfy a separate step of the process: ```ts dockerCredentials: [ pipelines.DockerCredential.dockerHub(secret), pipelines.DockerCredential.customRegistry('https://index.docker.io/v1/', secret), ], ``` ### Solution: This PR introduces two separate changes to address both problems. First, we change the hardcoded domain in `DockerCredential.dockerHub()` to be `https://index.docker.io/v1/`. This allows us to successfully trigger `docker-credential-cdk-assets` when the `docker --config build` command is called. Next, to make sure the credential lookup succeeds, we check for both the complete endpoint and the domain name. In this case, we will check for both `https://index.docker.io/v1/` as well as `index.docker.io`. Since `https://index.docker.io/v1/` exists in the credentials helper, authentication will succeed. Why do we still check for the domain `index.docker.io`? I don't know how custom registries or ecr works in this context and believe it to be beyond the scope of the PR. It's possible that they require the domain only for lookup. ### Testing: The change to credential lookups is unit tested in `docker-credentials.test.ts`. I confirmed that the change to `DockerCredential.dockerHub()` is successful by configuring a mock `cdk-docker-creds.json` file and successfully `cdk deploy`ing a docker image that depends on a private repository. This isn't a common use case but ensures that failure to authenticate results in failure every time. Thanks @james-mathiesen for the suggestion. ### Contributors: Thanks to @nohack for the code in `cdk-assets`. Fixes #15737. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../@aws-cdk/pipelines/lib/docker-credentials.ts | 4 ++-- .../pipelines/test/docker-credentials.test.ts | 2 +- .../cdk-assets/bin/docker-credential-cdk-assets.ts | 10 ++-------- .../cdk-assets/lib/private/docker-credentials.ts | 11 ++++++++--- packages/cdk-assets/lib/private/docker.ts | 11 ++++++++++- .../test/private/docker-credentials.test.ts | 14 +++++++++++++- 6 files changed, 36 insertions(+), 16 deletions(-) diff --git a/packages/@aws-cdk/pipelines/lib/docker-credentials.ts b/packages/@aws-cdk/pipelines/lib/docker-credentials.ts index 77b7d2c1b4381..05144d4957771 100644 --- a/packages/@aws-cdk/pipelines/lib/docker-credentials.ts +++ b/packages/@aws-cdk/pipelines/lib/docker-credentials.ts @@ -10,10 +10,10 @@ import { Fn } from '@aws-cdk/core'; export abstract class DockerCredential { /** * Creates a DockerCredential for DockerHub. - * Convenience method for `fromCustomRegistry('index.docker.io', opts)`. + * Convenience method for `customRegistry('https://index.docker.io/v1/', opts)`. */ public static dockerHub(secret: secretsmanager.ISecret, opts: ExternalDockerCredentialOptions = {}): DockerCredential { - return new ExternalDockerCredential('index.docker.io', secret, opts); + return new ExternalDockerCredential('https://index.docker.io/v1/', secret, opts); } /** diff --git a/packages/@aws-cdk/pipelines/test/docker-credentials.test.ts b/packages/@aws-cdk/pipelines/test/docker-credentials.test.ts index a2b5fc2c577dd..902c13a4129b7 100644 --- a/packages/@aws-cdk/pipelines/test/docker-credentials.test.ts +++ b/packages/@aws-cdk/pipelines/test/docker-credentials.test.ts @@ -29,7 +29,7 @@ describe('ExternalDockerCredential', () => { test('dockerHub defaults registry domain', () => { const creds = cdkp.DockerCredential.dockerHub(secret); - expect(Object.keys(creds._renderCdkAssetsConfig())).toEqual(['index.docker.io']); + expect(Object.keys(creds._renderCdkAssetsConfig())).toEqual(['https://index.docker.io/v1/']); }); test('minimal example only renders secret', () => { diff --git a/packages/cdk-assets/bin/docker-credential-cdk-assets.ts b/packages/cdk-assets/bin/docker-credential-cdk-assets.ts index b04f2ba8510bc..6dccb5521cf55 100644 --- a/packages/cdk-assets/bin/docker-credential-cdk-assets.ts +++ b/packages/cdk-assets/bin/docker-credential-cdk-assets.ts @@ -29,14 +29,8 @@ async function main() { } // Read the domain to fetch from stdin - let rawDomain = fs.readFileSync(0, { encoding: 'utf-8' }).trim(); - // Paranoid handling to ensure new URL() doesn't throw if the schema is missing. - // Not convinced docker will ever pass in a url like 'index.docker.io/v1', but just in case... - rawDomain = rawDomain.includes('://') ? rawDomain : `https://${rawDomain}`; - const domain = new URL(rawDomain).hostname; - - const credentials = await fetchDockerLoginCredentials(new DefaultAwsClient(), config, domain); - + let endpoint = fs.readFileSync(0, { encoding: 'utf-8' }).trim(); + const credentials = await fetchDockerLoginCredentials(new DefaultAwsClient(), config, endpoint); // Write the credentials back to stdout fs.writeFileSync(1, JSON.stringify(credentials)); } diff --git a/packages/cdk-assets/lib/private/docker-credentials.ts b/packages/cdk-assets/lib/private/docker-credentials.ts index b5c3f42139581..923d18d70a3ee 100644 --- a/packages/cdk-assets/lib/private/docker-credentials.ts +++ b/packages/cdk-assets/lib/private/docker-credentials.ts @@ -39,12 +39,17 @@ export function cdkCredentialsConfig(): DockerCredentialsConfig | undefined { } /** Fetches login credentials from the configured source (e.g., SecretsManager, ECR) */ -export async function fetchDockerLoginCredentials(aws: IAws, config: DockerCredentialsConfig, domain: string) { - if (!Object.keys(config.domainCredentials).includes(domain)) { +export async function fetchDockerLoginCredentials(aws: IAws, config: DockerCredentialsConfig, endpoint: string) { + // Paranoid handling to ensure new URL() doesn't throw if the schema is missing + // For official docker registry, docker will pass https://index.docker.io/v1/ + endpoint = endpoint.includes('://') ? endpoint : `https://${endpoint}`; + const domain = new URL(endpoint).hostname; + + if (!Object.keys(config.domainCredentials).includes(domain) && !Object.keys(config.domainCredentials).includes(endpoint)) { throw new Error(`unknown domain ${domain}`); } - const domainConfig = config.domainCredentials[domain]; + let domainConfig = config.domainCredentials[domain] ?? config.domainCredentials[endpoint]; if (domainConfig.secretsManagerSecretId) { const sm = await aws.secretsManagerClient({ assumeRoleArn: domainConfig.assumeRoleArn }); diff --git a/packages/cdk-assets/lib/private/docker.ts b/packages/cdk-assets/lib/private/docker.ts index e1fc54429f18f..aed2631ab2852 100644 --- a/packages/cdk-assets/lib/private/docker.ts +++ b/packages/cdk-assets/lib/private/docker.ts @@ -124,8 +124,17 @@ export class Docker { private async execute(args: string[], options: ShellOptions = {}) { const configArgs = this.configDir ? ['--config', this.configDir] : []; + const pathToCdkAssets = path.resolve(__dirname, '..', '..', 'bin'); try { - await shell(['docker', ...configArgs, ...args], { logger: this.logger, ...options }); + await shell(['docker', ...configArgs, ...args], { + logger: this.logger, + ...options, + env: { + ...process.env, + ...options.env, + PATH: `${pathToCdkAssets}${path.delimiter}${options.env?.PATH ?? process.env.PATH}`, + }, + }); } catch (e) { if (e.code === 'ENOENT') { throw new Error('Unable to execute \'docker\' in order to build a container asset. Please install \'docker\' and try again.'); diff --git a/packages/cdk-assets/test/private/docker-credentials.test.ts b/packages/cdk-assets/test/private/docker-credentials.test.ts index 6b521c67457b6..19160ccd0c880 100644 --- a/packages/cdk-assets/test/private/docker-credentials.test.ts +++ b/packages/cdk-assets/test/private/docker-credentials.test.ts @@ -97,8 +97,12 @@ describe('fetchDockerLoginCredentials', () => { await expect(fetchDockerLoginCredentials(aws, config, 'misconfigured.example.com')).rejects.toThrow(/unknown credential type/); }); + test('does not throw on correctly configured raw domain', async () => { + expect(fetchDockerLoginCredentials(aws, config, 'https://secret.example.com/v1/')).resolves; + }); + describe('SecretsManager', () => { - test('returns the credentials sucessfully if configured correctly', async () => { + test('returns the credentials sucessfully if configured correctly - domain', async () => { mockSecretWithSecretString({ username: 'secretUser', secret: 'secretPass' }); const creds = await fetchDockerLoginCredentials(aws, config, 'secret.example.com'); @@ -106,6 +110,14 @@ describe('fetchDockerLoginCredentials', () => { expect(creds).toEqual({ Username: 'secretUser', Secret: 'secretPass' }); }); + test('returns the credentials successfully if configured correctly - raw domain', async () => { + mockSecretWithSecretString({ username: 'secretUser', secret: 'secretPass' }); + + const creds = await fetchDockerLoginCredentials(aws, config, 'https://secret.example.com'); + + expect(creds).toEqual({ Username: 'secretUser', Secret: 'secretPass' }); + }); + test('throws when SecretsManager returns an error', async () => { const errMessage = "Secrets Manager can't find the specified secret."; aws.mockSecretsManager.getSecretValue = mockedApiFailure('ResourceNotFoundException', errMessage);