-
Notifications
You must be signed in to change notification settings - Fork 615
Feature/credential provider #5
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.
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
Closed
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4c9613a
Add credential provider helper methods
jeskew 8660839
Add environment variable credential provider
jeskew a9dcfb5
Add basic shared file provider
jeskew 04ecbeb
Add support for MFA and role assumption to shared ini provider
jeskew ae1cb8e
Add support for remote credential providers
jeskew 59276f2
Ensure the same homedir as would be resolved by the CLI is used
jeskew cb856cd
Use Credentials and CredentialProvider from types package
jeskew 90b3b53
Embed chain continuation flag in provider promise rejections
jeskew 1f2bc3c
Add support for generic ECS provider
jeskew fa42e77
Add support for environmental auth token in container credentials pro…
jeskew File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
/node_modules/ | ||
*.js | ||
*.js.map | ||
*.d.ts |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
interface FsModule { | ||
__addMatcher(toMatch: string, toReturn: string): void; | ||
__clearMatchers(): void; | ||
readFile: (path: string, encoding: string, cb: Function) => void | ||
} | ||
|
||
const fs: FsModule = <FsModule>jest.genMockFromModule('fs'); | ||
const matchers = new Map<string, string>(); | ||
|
||
function __addMatcher(toMatch: string, toReturn: string): void { | ||
matchers.set(toMatch, toReturn); | ||
} | ||
|
||
function __clearMatchers(): void { | ||
matchers.clear(); | ||
} | ||
|
||
function readFile( | ||
path: string, | ||
encoding: string, | ||
callback: (err: Error|null, data?: string) => void | ||
): void { | ||
for (let [matcher, data] of matchers.entries()) { | ||
if (matcher === path) { | ||
callback(null, data); | ||
return; | ||
} | ||
} | ||
|
||
callback(new Error('ENOENT: no such file or directory')); | ||
} | ||
|
||
fs.__addMatcher = __addMatcher; | ||
fs.__clearMatchers = __clearMatchers; | ||
fs.readFile = readFile; | ||
|
||
module.exports = fs; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
interface OsModule { | ||
homedir: () => string; | ||
} | ||
|
||
const os: OsModule = <OsModule>jest.genMockFromModule('os'); | ||
const path = require('path'); | ||
|
||
os.homedir = () => path.sep + path.join('home', 'user'); | ||
|
||
module.exports = os; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import {chain} from "../lib/chain"; | ||
import {fromCredentials} from "../lib/fromCredentials"; | ||
import {isCredentials} from "../lib/isCredentials"; | ||
import {CredentialError} from "../lib/CredentialError"; | ||
|
||
describe('chain', () => { | ||
it('should distill many credential providers into one', async () => { | ||
const provider = chain( | ||
fromCredentials({accessKeyId: 'foo', secretAccessKey: 'bar'}), | ||
fromCredentials({accessKeyId: 'baz', secretAccessKey: 'quux'}), | ||
); | ||
|
||
expect(isCredentials(await provider())).toBe(true); | ||
}); | ||
|
||
it('should return the resolved value of the first successful promise', async () => { | ||
const creds = {accessKeyId: 'foo', secretAccessKey: 'bar'}; | ||
const provider = chain( | ||
() => Promise.reject(new CredentialError('Move along')), | ||
() => Promise.reject(new CredentialError('Nothing to see here')), | ||
fromCredentials(creds) | ||
); | ||
|
||
expect(await provider()).toEqual(creds); | ||
}); | ||
|
||
it('should not invoke subsequent providers one resolves', async () => { | ||
const creds = {accessKeyId: 'foo', secretAccessKey: 'bar'}; | ||
const providers = [ | ||
jest.fn(() => Promise.reject(new CredentialError('Move along'))), | ||
jest.fn(() => Promise.resolve(creds)), | ||
jest.fn(() => fail('This provider should not be invoked')) | ||
]; | ||
|
||
expect(await chain(...providers)()).toEqual(creds); | ||
expect(providers[0].mock.calls.length).toBe(1); | ||
expect(providers[1].mock.calls.length).toBe(1); | ||
expect(providers[2].mock.calls.length).toBe(0); | ||
}); | ||
|
||
it( | ||
'should not invoke subsequent providers one is rejected with a terminal error', | ||
async () => { | ||
const providers = [ | ||
jest.fn(() => Promise.reject(new CredentialError('Move along'))), | ||
jest.fn(() => Promise.reject( | ||
new CredentialError('Stop here', false) | ||
)), | ||
jest.fn(() => fail('This provider should not be invoked')) | ||
]; | ||
|
||
await chain(...providers)().then( | ||
() => { throw new Error('The promise should have been rejected'); }, | ||
err => { | ||
expect(err.message).toBe('Stop here'); | ||
expect(providers[0].mock.calls.length).toBe(1); | ||
expect(providers[1].mock.calls.length).toBe(1); | ||
expect(providers[2].mock.calls.length).toBe(0); | ||
} | ||
); | ||
} | ||
); | ||
|
||
it('should reject chains with no links', async () => { | ||
await chain()().then( | ||
() => { throw new Error('The promise should have been rejected'); }, | ||
() => { /* Promise rejected as expected */ } | ||
); | ||
}); | ||
}); |
218 changes: 218 additions & 0 deletions
218
packages/credential-provider/__tests__/fromContainerMetadata.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,218 @@ | ||
import { | ||
ENV_CMDS_AUTH_TOKEN, | ||
ENV_CMDS_FULL_URI, | ||
ENV_CMDS_RELATIVE_URI, | ||
fromContainerMetadata | ||
} from "../lib/fromContainerMetadata"; | ||
import {httpGet} from "../lib/remoteProvider/httpGet"; | ||
import { | ||
fromImdsCredentials, | ||
ImdsCredentials | ||
} from "../lib/remoteProvider/ImdsCredentials"; | ||
import MockInstance = jest.MockInstance; | ||
import {RequestOptions} from "http"; | ||
|
||
interface HttpGet { | ||
(options: RequestOptions): Promise<Buffer>; | ||
} | ||
|
||
const mockHttpGet = <MockInstance<HttpGet>><any>httpGet; | ||
jest.mock('../lib/remoteProvider/httpGet', () => ({httpGet: jest.fn()})); | ||
|
||
const relativeUri = process.env[ENV_CMDS_RELATIVE_URI]; | ||
const fullUri = process.env[ENV_CMDS_FULL_URI]; | ||
const authToken = process.env[ENV_CMDS_AUTH_TOKEN]; | ||
|
||
beforeEach(() => { | ||
mockHttpGet.mockReset(); | ||
delete process.env[ENV_CMDS_RELATIVE_URI]; | ||
delete process.env[ENV_CMDS_FULL_URI]; | ||
delete process.env[ENV_CMDS_AUTH_TOKEN]; | ||
}); | ||
|
||
afterAll(() => { | ||
process.env[ENV_CMDS_RELATIVE_URI] = relativeUri; | ||
process.env[ENV_CMDS_FULL_URI] = fullUri; | ||
process.env[ENV_CMDS_AUTH_TOKEN] = authToken; | ||
}); | ||
|
||
describe('fromContainerMetadata', () => { | ||
const creds: ImdsCredentials = Object.freeze({ | ||
AccessKeyId: 'foo', | ||
SecretAccessKey: 'bar', | ||
Token: 'baz', | ||
Expiration: new Date().toISOString(), | ||
}); | ||
|
||
it( | ||
'should reject the promise with a terminal error if the container credentials environment variable is not set', | ||
async () => { | ||
await fromContainerMetadata()().then( | ||
() => { throw new Error('The promise should have been rejected'); }, | ||
err => { | ||
expect((err as any).tryNextLink).toBeFalsy(); | ||
} | ||
); | ||
} | ||
); | ||
|
||
it( | ||
`should inject an authorization header containing the contents of the ${ENV_CMDS_AUTH_TOKEN} environment variable if defined`, | ||
async () => { | ||
const token = 'Basic abcd'; | ||
process.env[ENV_CMDS_FULL_URI] = 'http://localhost:8080/path'; | ||
process.env[ENV_CMDS_AUTH_TOKEN] = token; | ||
mockHttpGet.mockReturnValue(Promise.resolve(JSON.stringify(creds))); | ||
|
||
await fromContainerMetadata()(); | ||
|
||
expect(mockHttpGet.mock.calls.length).toBe(1); | ||
const [options = {}] = mockHttpGet.mock.calls[0]; | ||
expect(options.headers).toMatchObject({ | ||
Authorization: token, | ||
}); | ||
} | ||
); | ||
|
||
describe(ENV_CMDS_RELATIVE_URI, () => { | ||
beforeEach(() => { | ||
process.env[ENV_CMDS_RELATIVE_URI] = '/relative/uri'; | ||
}); | ||
|
||
it( | ||
'should resolve credentials by fetching them from the container metadata service', | ||
async () => { | ||
mockHttpGet.mockReturnValue( | ||
Promise.resolve(JSON.stringify(creds)) | ||
); | ||
|
||
expect(await fromContainerMetadata()()) | ||
.toEqual(fromImdsCredentials(creds)); | ||
} | ||
); | ||
|
||
it( | ||
'should retry the fetching operation up to maxRetries times', | ||
async () => { | ||
const maxRetries = 5; | ||
for (let i = 0; i < maxRetries - 1; i++) { | ||
mockHttpGet.mockReturnValueOnce(Promise.reject('No!')); | ||
} | ||
mockHttpGet.mockReturnValueOnce( | ||
Promise.resolve(JSON.stringify(creds)) | ||
); | ||
|
||
expect(await fromContainerMetadata({maxRetries})()) | ||
.toEqual(fromImdsCredentials(creds)); | ||
expect(mockHttpGet.mock.calls.length).toEqual(maxRetries); | ||
} | ||
); | ||
|
||
it( | ||
'should retry responses that receive invalid response values', | ||
async () => { | ||
for (let key of Object.keys(creds)) { | ||
const invalidCreds: any = {...creds}; | ||
delete invalidCreds[key]; | ||
mockHttpGet.mockReturnValueOnce( | ||
Promise.resolve(JSON.stringify(invalidCreds)) | ||
); | ||
} | ||
mockHttpGet.mockReturnValueOnce( | ||
Promise.resolve(JSON.stringify(creds)) | ||
); | ||
|
||
await fromContainerMetadata({maxRetries: 100})(); | ||
expect(mockHttpGet.mock.calls.length) | ||
.toEqual(Object.keys(creds).length + 1); | ||
} | ||
); | ||
|
||
it('should pass relevant configuration to httpGet', async () => { | ||
const timeout = Math.ceil(Math.random() * 1000); | ||
mockHttpGet.mockReturnValue(Promise.resolve(JSON.stringify(creds))); | ||
await fromContainerMetadata({timeout})(); | ||
expect(mockHttpGet.mock.calls.length).toEqual(1); | ||
expect(mockHttpGet.mock.calls[0][0]).toEqual({ | ||
hostname: '169.254.170.2', | ||
path: process.env[ENV_CMDS_RELATIVE_URI], | ||
timeout, | ||
}); | ||
}); | ||
}); | ||
|
||
describe(ENV_CMDS_FULL_URI, () => { | ||
it('should pass relevant configuration to httpGet', async () => { | ||
process.env[ENV_CMDS_FULL_URI] = 'http://localhost:8080/path'; | ||
|
||
const timeout = Math.ceil(Math.random() * 1000); | ||
mockHttpGet.mockReturnValue(Promise.resolve(JSON.stringify(creds))); | ||
await fromContainerMetadata({timeout})(); | ||
expect(mockHttpGet.mock.calls.length).toEqual(1); | ||
const { | ||
protocol, | ||
hostname, | ||
path, | ||
port, | ||
timeout: actualTimeout, | ||
} = mockHttpGet.mock.calls[0][0]; | ||
expect(protocol).toBe('http:'); | ||
expect(hostname).toBe('localhost'); | ||
expect(path).toBe('/path'); | ||
expect(port).toBe(8080); | ||
expect(actualTimeout).toBe(timeout); | ||
}); | ||
|
||
it( | ||
`should prefer ${ENV_CMDS_RELATIVE_URI} to ${ENV_CMDS_FULL_URI}`, | ||
async () => { | ||
process.env[ENV_CMDS_RELATIVE_URI] = 'foo'; | ||
process.env[ENV_CMDS_FULL_URI] = 'http://localhost:8080/path'; | ||
|
||
const timeout = Math.ceil(Math.random() * 1000); | ||
mockHttpGet.mockReturnValue( | ||
Promise.resolve(JSON.stringify(creds)) | ||
); | ||
await fromContainerMetadata({timeout})(); | ||
expect(mockHttpGet.mock.calls.length).toEqual(1); | ||
expect(mockHttpGet.mock.calls[0][0]).toEqual({ | ||
hostname: '169.254.170.2', | ||
path: 'foo', | ||
timeout, | ||
}); | ||
} | ||
); | ||
|
||
it( | ||
'should reject the promise with a terminal error if a unexpected protocol is specified', | ||
async () => { | ||
process.env[ENV_CMDS_FULL_URI] = 'wss://localhost:8080/path'; | ||
|
||
await fromContainerMetadata()().then( | ||
() => { | ||
throw new Error('The promise should have been rejected'); | ||
}, | ||
err => { | ||
expect((err as any).tryNextLink).toBeFalsy(); | ||
} | ||
); | ||
} | ||
); | ||
|
||
it( | ||
'should reject the promise with a terminal error if a unexpected hostname is specified', | ||
async () => { | ||
process.env[ENV_CMDS_FULL_URI] = 'https://bucket.s3.amazonaws.com/key'; | ||
|
||
await fromContainerMetadata()().then( | ||
() => { | ||
throw new Error('The promise should have been rejected'); | ||
}, | ||
err => { | ||
expect((err as any).tryNextLink).toBeFalsy(); | ||
} | ||
); | ||
} | ||
); | ||
}); | ||
}); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
import {CredentialProvider, Credentials} from "@aws/types"; | ||
import {fromCredentials} from "../lib/fromCredentials"; | ||
|
||
describe('fromCredentials', () => { | ||
it('should convert credentials into a credential provider', async () => { | ||
const credentials: Credentials = { | ||
accessKeyId: 'foo', | ||
secretAccessKey: 'bar' | ||
}; | ||
const provider: CredentialProvider = fromCredentials(credentials); | ||
|
||
expect(typeof provider).toBe('function'); | ||
expect(provider()).toBeInstanceOf(Promise); | ||
expect(await provider()).toEqual(credentials); | ||
}); | ||
}); |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here's an interesting question. Should we move to the next Provider in the chain no matter what the error is (like we do today) or should we stop the chain if a Provider rejects due to bad config (like Python, e.g. Incorrect profile specified)?
I'm leaning towards the latter, since we've had some customers complain about the error messages our chain currently gives. Maybe this is a good first use for the cancellation token?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given the way the chain is linked together with
.catch
handlers, error messages for anything other than the EC2 provider will just get swallowed and replaced with a less useful message.Rather than a cancellation token, what if the providers always threw a
CredentialsError
with atryNextLink
boolean property? Bad config errors would set the property tofalse
, unforeseen errors would returnundefined
, and the next handler in the chain would only be invoked if the property weretrue
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that sounds reasonable.