fix(ecr): use ec2 instance credentials when no credentials are provided

[Flydiverny]

The tests are a bit silly and verbose but I wanted to add some kind of test 😄

Fixes #64

#64 (comment)
Copy pastad in below:

---

I did some testing to simulate what happens in the login-action

Given this step below I will get the expected output

      - uses: actions/github-script@v5
        with:
          script: |
            await exec.getExecOutput(await io.which('aws', true), ['sts', 'get-caller-identity'])
          result-encoding: string

{
    "UserId": "REDACTED:i-REDACTED",
    "Account": "REDACTED",
    "Arn": "arn:aws:sts::REDACTED:assumed-role/REDACTED/i-REDACTED"
}

Where as if I added the process.env vars as the script does the step fails

      - uses: actions/github-script@v5
        with:
          script: |
            let username = ''
            let password = ''
            process.env.AWS_ACCESS_KEY_ID = username || process.env.AWS_ACCESS_KEY_ID;
            process.env.AWS_SECRET_ACCESS_KEY = password || process.env.AWS_SECRET_ACCESS_KEY;
            await exec.getExecOutput(await io.which('aws', true), ['sts', 'get-caller-identity'])
          result-encoding: string

An error occurred (InvalidClientTokenId) when calling the GetCallerIdentity operation: The security token included in the request is invalid.
Error: Unhandled error: Error: The process '/usr/local/bin/aws' failed with exit code 254

a simple change to how we set the env vars should resolve it

      - uses: actions/github-script@v5
        with:
          script: |
            let username = ''
            let password = ''
            if (username) {
              process.env.AWS_ACCESS_KEY_ID = username;
            }
            if (password) {
              process.env.AWS_SECRET_ACCESS_KEY = password;
            }
            await exec.getExecOutput(await io.which('aws', true), ['sts', 'get-caller-identity'])
          result-encoding: string

Which will get me my EC2 credentials again :)

Created a PR #114

Originally posted by @Flydiverny in #64 (comment)

[Flydiverny]

@crazy-max Would appreciate a review here, what's the best way to end up in your todo? 😇🙏

[crazy-max]

@Flydiverny Looks like dist has not been generated. See https://github.com/docker/login-action/blob/master/.github/CONTRIBUTING.md#submitting-a-pull-request

[crazy-max]

This logic is the same as the current one: https://onecompiler.com/nodejs/3xmb4n6tq

[Flydiverny]

The outcome is sadly different

const util = require('util');
const exec = util.promisify(require('child_process').exec);
async function printenv() {
    process.env.TEST_ENV_BANANA = undefined
    process.env.TEST_ENV_APPLE = undefined
    delete process.env.TEST_ENV_APPLE
  try {
      const { stdout, stderr } = await exec('printenv | grep TEST_ENV_');
      console.log('stdout:', stdout);
      console.log('stderr:', stderr);
  } catch (err) {
     console.error(err);
  };
};
printenv();

As you can see in the case of TEST_ENV_BANANA the environment variable is set in the subprocess.
Unlike TEST_ENV_APPLE which was deleted instead of undefined
This causes issue when using EC2 metadata api for getting AWS credentials, as now the AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY will both be set to undefined eg they have a value and therefor the AWS cli will try to use them instead of falling back thru the credential chain and use the ec2 metadata api.

[Flydiverny]

So the problem is when

 process.env.AWS_SECRET_ACCESS_KEY = password ||  process.env.AWS_SECRET_ACCESS_KEY

if process.env.AWS_SECRET_ACCESS_KEY and password are both undefined we end up assigning process.env.AWS_SECRET_ACCESS_KEY = undefined which causes this env var to be set for subprocesses.

[Flydiverny]

Here's an updated onecompiler link where you can see that the result will differ
https://onecompiler.com/nodejs/3xmbfxhf6
https://onecompiler.com/nodejs/3xmbg2nzw

[crazy-max]

Oh ok so this is related to sub steps in the workflow right?

[Flydiverny]

Yes!
If you want to use this action on a self-hosted runner on AWS, the runner can have access to login to AWS ECR without having to set any credentials

      - name: Login to ECR
        uses: docker/login-action@v1
        with:
          registry: <aws-account-number>.dkr.ecr.<region>.amazonaws.com

Eventually the action will run the aws cli to get the login token, line 77 or 81 here. Which would be affected by any process.env changes done in node here since it a child process

login-action/src/aws.ts

Lines 74 to 85 in 1cce165

	export const getDockerLoginCmds = async (cliVersion: string, registry: string, region: string, accountIDs: string[]): Promise<string[]> => {
	let ecrCmd = (await isPubECR(registry)) ? 'ecr-public' : 'ecr';
	if (semver.satisfies(cliVersion, '>=2.0.0') || (await isPubECR(registry))) {
	return execCLI([ecrCmd, 'get-login-password', '--region', region]).then(pwd => {
	return [`docker login --username AWS --password ${pwd} ${registry}`];
	});
	} else {
	return execCLI([ecrCmd, 'get-login', '--region', region, '--registry-ids', accountIDs.join(' '), '--no-include-email']).then(dockerLoginCmds => {
	return dockerLoginCmds.trim().split(`\n`);
	});
	}
	};

We use the docker/login-action for or other workflows so it would be nice to be able to use it on our self-hosted runners as well without having to specify the credentials in those workflows :)

Why node.js propagates env vars as undefined I have no clue 😄

[crazy-max]

Ok I think instead we could set the env var to the current process in getExecOutput. I will open a PR with this solution and come back to you. I keep yours open in case mine doesn't fit your needs.

[Flydiverny]

@Flydiverny Looks like dist has not been generated. See https://github.com/docker/login-action/blob/master/.github/CONTRIBUTING.md#submitting-a-pull-request

My bad! Updated :)

[crazy-max]

LGTM. Will see what we can do about #114 (comment) in a follow-up.