Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Refresh and validate credentials after setting env var creds #71

Merged
merged 3 commits into from
Jun 3, 2020
Merged

feat: Refresh and validate credentials after setting env var creds #71

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8177395
feat: Refresh and validate credentials after setting env var creds
clareliguori May 29, 2020
6ac6467
Positive test case
clareliguori Jun 3, 2020
d6ec41d
Merge branch 'master' into validate-credentials
mergify[bot] Jun 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions dist/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,50 @@ async function exportAccountId(maskAccountId, region) {
return accountId;
}

function loadCredentials() {
// Force the SDK to re-resolve credentials with the default provider chain.
//
// This action typically sets credentials in the environment via environment variables.
// The SDK never refreshes those env-var-based credentials after initial load.
// In case there were already env-var creds set in the actions environment when this action
// loaded, this action needs to refresh the SDK creds after overwriting those environment variables.
//
// The credentials object needs to be entirely recreated (instead of simply refreshed),
// because the credential object type could change when this action writes env var creds.
// For example, the first load could return EC2 instance metadata credentials
// in a self-hosted runner, and the second load could return environment credentials
// from an assume-role call in this action.
aws.config.credentials = null;

return new Promise((resolve, reject) => {
aws.config.getCredentials((err) => {
if (err) {
reject(err);
}
resolve(aws.config.credentials);
})
});
}

async function validateCredentials(expectedAccessKeyId) {
let credentials;
try {
credentials = await loadCredentials();

if (!credentials.accessKeyId) {
throw new Error('Access key ID empty after loading credentials');
}
} catch (error) {
throw new Error(`Credentials could not be loaded, please check your action inputs: ${error.message}`);
}

const actualAccessKeyId = credentials.accessKeyId;

if (expectedAccessKeyId && expectedAccessKeyId != actualAccessKeyId) {
throw new Error('Unexpected failure: Credentials loaded by the SDK do not match the access key ID configured by the action');
}
}

function getStsClient(region) {
return new aws.STS({
region,
Expand Down Expand Up @@ -305,6 +349,13 @@ async function run() {
exportCredentials({accessKeyId, secretAccessKey, sessionToken});
}

// Regardless of whether any source credentials were provided as inputs,
// validate that the SDK can actually pick up credentials. This validates
// cases where this action is on a self-hosted runner that doesn't have credentials
// configured correctly, and cases where the user intended to provide input
// credentials but the secrets inputs resolved to empty strings.
await validateCredentials(accessKeyId);

const sourceAccountId = await exportAccountId(maskAccountId, region);

// Get role credentials if configured to do so
Expand All @@ -318,6 +369,7 @@ async function run() {
roleSessionName
});
exportCredentials(roleCredentials);
await validateCredentials(roleCredentials.accessKeyId);
await exportAccountId(maskAccountId, region);
}
}
Expand Down
52 changes: 52 additions & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,50 @@ async function exportAccountId(maskAccountId, region) {
return accountId;
}

function loadCredentials() {
// Force the SDK to re-resolve credentials with the default provider chain.
//
// This action typically sets credentials in the environment via environment variables.
// The SDK never refreshes those env-var-based credentials after initial load.
// In case there were already env-var creds set in the actions environment when this action
// loaded, this action needs to refresh the SDK creds after overwriting those environment variables.
//
// The credentials object needs to be entirely recreated (instead of simply refreshed),
// because the credential object type could change when this action writes env var creds.
// For example, the first load could return EC2 instance metadata credentials
// in a self-hosted runner, and the second load could return environment credentials
// from an assume-role call in this action.
aws.config.credentials = null;

return new Promise((resolve, reject) => {
aws.config.getCredentials((err) => {
if (err) {
reject(err);
}
resolve(aws.config.credentials);
})
});
}

async function validateCredentials(expectedAccessKeyId) {
let credentials;
try {
credentials = await loadCredentials();

if (!credentials.accessKeyId) {
throw new Error('Access key ID empty after loading credentials');
}
} catch (error) {
throw new Error(`Credentials could not be loaded, please check your action inputs: ${error.message}`);
}

const actualAccessKeyId = credentials.accessKeyId;

if (expectedAccessKeyId && expectedAccessKeyId != actualAccessKeyId) {
throw new Error('Unexpected failure: Credentials loaded by the SDK do not match the access key ID configured by the action');
}
}

function getStsClient(region) {
return new aws.STS({
region,
Expand Down Expand Up @@ -172,6 +216,13 @@ async function run() {
exportCredentials({accessKeyId, secretAccessKey, sessionToken});
}

// Regardless of whether any source credentials were provided as inputs,
// validate that the SDK can actually pick up credentials. This validates
// cases where this action is on a self-hosted runner that doesn't have credentials
// configured correctly, and cases where the user intended to provide input
// credentials but the secrets inputs resolved to empty strings.
await validateCredentials(accessKeyId);

const sourceAccountId = await exportAccountId(maskAccountId, region);

// Get role credentials if configured to do so
Expand All @@ -185,6 +236,7 @@ async function run() {
roleSessionName
});
exportCredentials(roleCredentials);
await validateCredentials(roleCredentials.accessKeyId);
await exportAccountId(maskAccountId, region);
}
}
Expand Down
91 changes: 89 additions & 2 deletions index.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const core = require('@actions/core');
const assert = require('assert');

const aws = require('aws-sdk');
const run = require('.');

jest.mock('@actions/core');
Expand Down Expand Up @@ -49,6 +49,9 @@ const mockStsAssumeRole = jest.fn();

jest.mock('aws-sdk', () => {
return {
config: {
getCredentials: jest.fn()
},
STS: jest.fn(() => ({
getCallerIdentity: mockStsCallerIdentity,
assumeRole: mockStsAssumeRole,
Expand Down Expand Up @@ -82,6 +85,27 @@ describe('Configure AWS Credentials', () => {
}
});

aws.config.getCredentials.mockReset();
aws.config.getCredentials
.mockImplementationOnce(callback => {
if (!aws.config.credentials) {
aws.config.credentials = {
accessKeyId: FAKE_ACCESS_KEY_ID,
secretAccessKey: FAKE_SECRET_ACCESS_KEY
}
}
callback(null);
})
.mockImplementationOnce(callback => {
if (!aws.config.credentials) {
aws.config.credentials = {
accessKeyId: FAKE_STS_ACCESS_KEY_ID,
secretAccessKey: FAKE_STS_SECRET_ACCESS_KEY
}
}
callback(null);
});

mockStsAssumeRole.mockImplementation(() => {
return {
promise() {
Expand Down Expand Up @@ -134,6 +158,59 @@ describe('Configure AWS Credentials', () => {
expect(core.setSecret).toHaveBeenCalledWith(FAKE_ACCOUNT_ID);
});

test('action with no accessible credentials fails', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we've got a couple tests for error cases, but is there any way to meaningfully test the positive case (correctly configuring diff creds in sequence)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I made this integ test that I confirmed fails prior to the change and passes after, but let me try to add a unit test too
#70

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

process.env.SHOW_STACK_TRACE = 'false';
const mockInputs = {'aws-region': FAKE_REGION};
core.getInput = jest
.fn()
.mockImplementation(mockGetInput(mockInputs));
aws.config.getCredentials.mockReset();
aws.config.getCredentials.mockImplementation(callback => {
callback(new Error('No credentials to load'));
});

await run();

expect(core.setFailed).toHaveBeenCalledWith("Credentials could not be loaded, please check your action inputs: No credentials to load");
});

test('action with empty credentials fails', async () => {
process.env.SHOW_STACK_TRACE = 'false';
const mockInputs = {'aws-region': FAKE_REGION};
core.getInput = jest
.fn()
.mockImplementation(mockGetInput(mockInputs));
aws.config.getCredentials.mockReset();
aws.config.getCredentials.mockImplementation(callback => {
aws.config.credentials = {
accessKeyId: ''
}
callback(null);
});

await run();

expect(core.setFailed).toHaveBeenCalledWith("Credentials could not be loaded, please check your action inputs: Access key ID empty after loading credentials");
});

test('action fails when credentials are not set in the SDK correctly', async () => {
process.env.SHOW_STACK_TRACE = 'false';
core.getInput = jest
.fn()
.mockImplementation(mockGetInput(ASSUME_ROLE_INPUTS));
aws.config.getCredentials.mockReset();
aws.config.getCredentials.mockImplementation(callback => {
aws.config.credentials = {
accessKeyId: FAKE_ACCESS_KEY_ID
}
callback(null);
});

await run();

expect(core.setFailed).toHaveBeenCalledWith("Unexpected failure: Credentials loaded by the SDK do not match the access key ID configured by the action");
});

test('session token is optional', async () => {
const mockInputs = {...CREDS_INPUTS, 'aws-region': 'eu-west-1'};
core.getInput = jest
Expand All @@ -154,12 +231,19 @@ describe('Configure AWS Credentials', () => {
expect(core.setSecret).toHaveBeenCalledWith(FAKE_ACCOUNT_ID);
});

test('session token is cleared if necessary', async () => {
test('existing env var creds are cleared', async () => {
const mockInputs = {...CREDS_INPUTS, 'aws-region': 'eu-west-1'};
core.getInput = jest
.fn()
.mockImplementation(mockGetInput(mockInputs));
process.env.AWS_ACCESS_KEY_ID = 'foo';
process.env.AWS_SECRET_ACCESS_KEY = 'bar';
process.env.AWS_SESSION_TOKEN = 'helloworld';
aws.config.credentials = {
accessKeyId: 'foo',
secretAccessKey: 'bar',
sessionToken: 'helloworld'
};

await run();
expect(mockStsAssumeRole).toHaveBeenCalledTimes(0);
Expand All @@ -174,6 +258,9 @@ describe('Configure AWS Credentials', () => {
expect(core.exportVariable).toHaveBeenCalledWith('AWS_REGION', 'eu-west-1');
expect(core.setOutput).toHaveBeenCalledWith('aws-account-id', FAKE_ACCOUNT_ID);
expect(core.setSecret).toHaveBeenCalledWith(FAKE_ACCOUNT_ID);
expect(aws.config.credentials.accessKeyId).toBe(FAKE_ACCESS_KEY_ID);
expect(aws.config.credentials.secretAccessKey).toBe(FAKE_SECRET_ACCESS_KEY);
expect(aws.config.credentials.sessionToken).toBeUndefined();
});

test('validates region name', async () => {
Expand Down