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*

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);
   }
 
   /**

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', () => {

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));
 }

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 });

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.');

packages/cdk-assets/test/private/docker-credentials.test.ts

@@ -97,15 +97,27 @@ 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');
 
       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);