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.

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

Credentials Management #888

Merged
merged 38 commits into from
Jan 13, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
409af50
Initial implementation of Credential Provider
awschristou Jan 4, 2020
51c094f
Remove getCredentialsFilename and getConfigFilename from UserCredenti…
awschristou Jan 4, 2020
74ae1c3
BaseCredentialsProviderFactory tests
awschristou Jan 6, 2020
1989d51
Profile Validation now returns text instead of throwing Error
awschristou Jan 6, 2020
aadf828
getStringHash utility method
awschristou Jan 6, 2020
cdfb8a9
Renamed types, produce Credentials, internalize CredentialProviderChain
awschristou Jan 6, 2020
102655c
Pull Default Region from Shared Credentials Profile
awschristou Jan 6, 2020
b7b6445
Get Profile HashCode
awschristou Jan 7, 2020
5ed5f73
When logging in, if credentials are already cached but a change was d…
awschristou Jan 7, 2020
9fd898f
Remove comment
awschristou Jan 7, 2020
6a83b2e
Updated the 'invalid credentials' notice, and added a "View Logs" but…
awschristou Jan 7, 2020
b5abf17
CredentialsProviderId tests
awschristou Jan 7, 2020
2ee351e
CredentialsProviderManager tests
awschristou Jan 8, 2020
dcb2d4c
SharedCredentialsProvider tests
awschristou Jan 8, 2020
f14b125
SharedCredentialsProviderFactory tests
awschristou Jan 8, 2020
f47fad3
Fix envvar side-effects that were affecting other tests
awschristou Jan 8, 2020
29c760f
Lint fix
awschristou Jan 8, 2020
ef8e981
Merge branch 'master' into awschristou/credentials
awschristou Jan 8, 2020
2440bbf
Changelogs
awschristou Jan 8, 2020
004d6b9
Migration - prevent first startup using the new Credentials Providers…
awschristou Jan 8, 2020
7380cce
Test fix
awschristou Jan 8, 2020
c848df8
Merge branch 'master' into awschristou/credentials
awschristou Jan 8, 2020
23f6962
Feedback - changelogs
awschristou Jan 8, 2020
c34820f
Feedback - label
awschristou Jan 8, 2020
150f199
Feedback - Credentials Type for Shared Credentials
awschristou Jan 8, 2020
4094ee4
Feedback - CredentialsProviderManager instance
awschristou Jan 9, 2020
4a0e321
Feedback - label
awschristou Jan 9, 2020
676ae7b
Merge branch 'master' into awschristou/credentials
awschristou Jan 9, 2020
3a7afa6
Shared Credentials Profiles without recognized properties are conside…
awschristou Jan 9, 2020
f7491f3
Feedback - legible code
awschristou Jan 9, 2020
7712177
Feedback - consistent use of Millis
awschristou Jan 9, 2020
5025c19
Changed CredentialsProviderId separator from : to |
awschristou Jan 9, 2020
6f33ac4
CredentialsStore now stores and retrieves Credentials and associated …
awschristou Jan 9, 2020
bc32e0d
Feedback - blank line
awschristou Jan 9, 2020
b262bc7
Feedback - immutable during activation login migration
awschristou Jan 9, 2020
6c76476
CredentialsProviderId is now a type
awschristou Jan 10, 2020
5f38544
Switched string hash utility from hand-rolled to sha256
awschristou Jan 13, 2020
ab3807e
Feedback - concat over spread operator
awschristou Jan 13, 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
2 changes: 1 addition & 1 deletion package.nls.json
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@
"AWS.message.credentials.error": "There was an issue trying to use credentials profile {0}: {1}",
"AWS.message.credentials.invalid": "Invalid Credentials {0}, see logs for more information.",
"AWS.message.credentials.invalid.help": "Get Help...",
"AWS.message.credentials.invalid.logs": "View logs...",
"AWS.message.credentials.invalid.logs": "View Logs...",
"AWS.message.enterProfileName": "Enter the name of the credential profile to use",
"AWS.message.info.cloudFormation.delete": "Deleted CloudFormation Stack {0}",
"AWS.message.error.cloudFormation.delete": "An error occurred while deleting CloudFormation Stack {0}. Please check the stack events on the AWS Console",
Expand Down
22 changes: 14 additions & 8 deletions src/credentials/activation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { profileSettingKey } from '../shared/constants'
import { CredentialsProfileMru } from '../shared/credentials/credentialsProfileMru'
import { SettingsConfiguration } from '../shared/settingsConfiguration'
import { LoginManager } from './loginManager'
import { CREDENTIALS_PROVIDER_ID_SEPARATOR, makeCredentialsProviderId } from './providers/credentialsProviderId'
import { CredentialsProviderId, fromString } from './providers/credentialsProviderId'
import { SharedCredentialsProvider } from './providers/sharedCredentialsProvider'

export interface CredentialsInitializeParameters {
Expand All @@ -31,18 +31,16 @@ export async function loginWithMostRecentCredentials(
toolkitSettings: SettingsConfiguration,
loginManager: LoginManager
): Promise<void> {
let previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '')
const previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '')
if (previousCredentialsId) {
// Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format,
Copy link
Contributor

Choose a reason for hiding this comment

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

I get that this will make it more seamless for a user but I don't care to have this lying around if it's solely for that purpose. I'd say just invalidate the existing credentials and have the user log back in...it's fast enough that it shouldn't be an issue. At most, give them a one-version grace period and mark this for deletion in the next release afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be removed after a few versions. It makes for a better experience than seeing an "invalid credentials" notification, or a phantom log-out. It's low cost, the comment explains what is going on, and git history will tell us (if necessary) when this was added.

// treat it like a Shared Crdentials Provider.
if (previousCredentialsId.indexOf(CREDENTIALS_PROVIDER_ID_SEPARATOR) === -1) {
previousCredentialsId = makeCredentialsProviderId({
credentialType: SharedCredentialsProvider.getCredentialsType(),
credentialTypeId: previousCredentialsId
})
const loginCredentialsId = tryMakeCredentialsProviderId(previousCredentialsId) ?? {
credentialType: SharedCredentialsProvider.getCredentialsType(),
credentialTypeId: previousCredentialsId
}

await loginManager.login(previousCredentialsId)
await loginManager.login(loginCredentialsId)
} else {
await loginManager.logout()
}
Expand Down Expand Up @@ -79,3 +77,11 @@ function updateConfigurationWhenAwsContextChanges(
})
)
}

function tryMakeCredentialsProviderId(credentialsProviderId: string): CredentialsProviderId | undefined {
try {
return fromString(credentialsProviderId)
} catch (err) {
return undefined
}
}
51 changes: 24 additions & 27 deletions src/credentials/credentialsStore.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
/*!
* Copyright 2019 Amazon.com, Inc. or its affiliates. All Rights Reserved.
* Copyright 2019-2020 Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/

import * as AWS from 'aws-sdk'
import { CredentialsProvider } from './providers/credentialsProvider'
import { asString, CredentialsProviderId } from './providers/credentialsProviderId'

interface CredentialsData {
export interface CachedCredentials {
credentials: AWS.Credentials
credentialsHashCode: number
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a hash of the contents that were used to produce the credentials.
Its purpose is to detect when things like Shared Credentials Profiles change during a toolkit session.

}
Expand All @@ -14,7 +16,7 @@ interface CredentialsData {
* Simple cache for credentials
*/
export class CredentialsStore {
private readonly credentialsCache: { [key: string]: CredentialsData }
private readonly credentialsCache: { [key: string]: CachedCredentials }

public constructor() {
this.credentialsCache = {}
Expand All @@ -23,41 +25,36 @@ export class CredentialsStore {
/**
* Returns undefined if credentials are not stored for given ID
*/
public async getCredentials(credentialsId: string): Promise<AWS.Credentials | undefined> {
return this.credentialsCache[credentialsId]?.credentials
public async getCredentials(credentialsProviderId: CredentialsProviderId): Promise<CachedCredentials | undefined> {
return this.credentialsCache[asString(credentialsProviderId)]
}

/**
* If credentials are not stored, the provided create function is called. Created credentials are then stored.
* If credentials are not stored, the credentialsProvider is used to produce credentials (which are then stored).
*/
public async getCredentialsOrCreate(
credentialsId: string,
createCredentialsFn: (credentialsId: string) => Promise<CredentialsData>
): Promise<AWS.Credentials> {
const credentials = await this.getCredentials(credentialsId)

if (credentials) {
return credentials
public async getOrCreateCredentials(
credentialsProviderId: CredentialsProviderId,
credentialsProvider: CredentialsProvider
): Promise<CachedCredentials> {
let credentials = await this.getCredentials(credentialsProviderId)

if (!credentials) {
credentials = {
credentials: await credentialsProvider.getCredentials(),
credentialsHashCode: credentialsProvider.getHashCode()
}

this.credentialsCache[asString(credentialsProviderId)] = credentials
}

const newCredentials = await createCredentialsFn(credentialsId)
this.credentialsCache[credentialsId] = newCredentials

return newCredentials.credentials
}

/**
* Returns undefined if credentials are not stored for given ID
*/
public getCredentialsHashCode(credentialsId: string): number | undefined {
return this.credentialsCache[credentialsId]?.credentialsHashCode
return credentials
}

/**
* Evicts credentials from storage
*/
public invalidateCredentials(credentialsId: string) {
public invalidateCredentials(credentialsProviderId: CredentialsProviderId) {
// tslint:disable-next-line:no-dynamic-delete
delete this.credentialsCache[credentialsId]
delete this.credentialsCache[asString(credentialsProviderId)]
}
}
61 changes: 33 additions & 28 deletions src/credentials/loginManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import { getAccountId } from '../shared/credentials/accountId'
import { getLogger } from '../shared/logger'
import { CredentialsStore } from './credentialsStore'
import { CredentialsProvider } from './providers/credentialsProvider'
import { getCredentialsProviderManagerInstance } from './providers/credentialsProviderManager'
import { asString, CredentialsProviderId } from './providers/credentialsProviderId'
import { CredentialsProviderManager } from './providers/credentialsProviderManager'

export class LoginManager {
private readonly credentialsStore: CredentialsStore = new CredentialsStore()
Expand All @@ -24,42 +25,46 @@ export class LoginManager {
* Establishes a Credentials for the Toolkit to use. Essentially the Toolkit becomes "logged in".
* If an error occurs while trying to set up and verify these credentials, the Toolkit is "logged out".
*/
public async login(credentialsId: string): Promise<void> {
public async login(credentialsProviderId: CredentialsProviderId): Promise<void> {
try {
const provider = await getCredentialsProviderManagerInstance().getCredentialsProvider(credentialsId)
const provider = await CredentialsProviderManager.getInstance().getCredentialsProvider(
credentialsProviderId
)
if (!provider) {
throw new Error(`Could not find Credentials Provider for ${credentialsId}`)
throw new Error(`Could not find Credentials Provider for ${asString(credentialsProviderId)}`)
}

await this.updateCredentialsStore(credentialsId, provider)
await this.updateCredentialsStore(credentialsProviderId, provider)

const credentials = await this.credentialsStore.getCredentials(credentialsId)
if (!credentials) {
throw new Error(`No credentials found for id ${credentialsId}`)
const storedCredentials = await this.credentialsStore.getCredentials(credentialsProviderId)
if (!storedCredentials) {
throw new Error(`No credentials found for id ${asString(credentialsProviderId)}`)
}

// TODO : Get a region relevant to the partition for these credentials -- https://github.com/aws/aws-toolkit-vscode/issues/188
const accountId = await getAccountId(credentials, 'us-east-1')
const accountId = await getAccountId(storedCredentials.credentials, 'us-east-1')
if (!accountId) {
throw new Error('Could not determine Account Id for credentials')
}

await this.awsContext.setCredentials({
credentials: credentials,
credentialsId: credentialsId,
credentials: storedCredentials.credentials,
credentialsId: asString(credentialsProviderId),
accountId: accountId,
defaultRegion: provider.getDefaultRegion()
})
} catch (err) {
getLogger().error(
`Error trying to connect to AWS with Credentials Provider ${credentialsId}. Toolkit will now disconnect from AWS.`,
`Error trying to connect to AWS with Credentials Provider ${asString(
credentialsProviderId
)}. Toolkit will now disconnect from AWS.`,
err as Error
)
this.credentialsStore.invalidateCredentials(credentialsId)
this.credentialsStore.invalidateCredentials(credentialsProviderId)

await this.logout()

this.notifyUserInvalidCredentials(credentialsId)
this.notifyUserInvalidCredentials(credentialsProviderId)
}
}

Expand All @@ -73,31 +78,31 @@ export class LoginManager {
/**
* Updates the CredentialsStore if the credentials are considered different
*/
private async updateCredentialsStore(credentialsId: string, provider: CredentialsProvider): Promise<void> {
const currentHash = this.credentialsStore.getCredentialsHashCode(credentialsId)
if (provider.getHashCode() !== currentHash) {
getLogger().verbose(`Credentials for ${credentialsId} have changed, using updated credentials.`)
this.credentialsStore.invalidateCredentials(credentialsId)
private async updateCredentialsStore(
credentialsProviderId: CredentialsProviderId,
provider: CredentialsProvider
): Promise<void> {
const storedCredentials = await this.credentialsStore.getCredentials(credentialsProviderId)
if (provider.getHashCode() !== storedCredentials?.credentialsHashCode) {
getLogger().verbose(
`Credentials for ${asString(credentialsProviderId)} have changed, using updated credentials.`
)
this.credentialsStore.invalidateCredentials(credentialsProviderId)
}

await this.credentialsStore.getCredentialsOrCreate(credentialsId, async () => {
return {
credentials: await provider.getCredentials(),
credentialsHashCode: provider.getHashCode()
}
})
await this.credentialsStore.getOrCreateCredentials(credentialsProviderId, provider)
}

private notifyUserInvalidCredentials(credentialProviderId: string) {
private notifyUserInvalidCredentials(credentialProviderId: CredentialsProviderId) {
const getHelp = localize('AWS.message.credentials.invalid.help', 'Get Help...')
const viewLogs = localize('AWS.message.credentials.invalid.logs', 'View logs...')
const viewLogs = localize('AWS.message.credentials.invalid.logs', 'View Logs...')

vscode.window
.showErrorMessage(
localize(
'AWS.message.credentials.invalid',
'Invalid Credentials {0}, see logs for more information.',
credentialProviderId
asString(credentialProviderId)
),
getHelp,
viewLogs
Expand Down
4 changes: 3 additions & 1 deletion src/credentials/providers/credentialsProvider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { CredentialsProviderId } from './credentialsProviderId'

export interface CredentialsProvider {
getCredentialsProviderId(): string
getCredentialsProviderId(): CredentialsProviderId
getDefaultRegion(): string | undefined
getHashCode(): number
getCredentials(): Promise<AWS.Credentials>
Expand Down
7 changes: 4 additions & 3 deletions src/credentials/providers/credentialsProviderFactory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
*/

import { CredentialsProvider } from './credentialsProvider'
import { CredentialsProviderId, isEqual } from './credentialsProviderId'

/**
* Responsible for producing CredentialsProvider objects for a Credential Type
*/
export interface CredentialsProviderFactory {
getCredentialType(): string
listProviders(): CredentialsProvider[]
getProvider(credentialsProviderId: string): CredentialsProvider | undefined
getProvider(credentialsProviderId: CredentialsProviderId): CredentialsProvider | undefined
refresh(): Promise<void>
}

Expand All @@ -24,9 +25,9 @@ export abstract class BaseCredentialsProviderFactory<T extends CredentialsProvid
return [...this.providers]
}

public getProvider(credentialsProviderId: string): CredentialsProvider | undefined {
public getProvider(credentialsProviderId: CredentialsProviderId): CredentialsProvider | undefined {
for (const provider of this.providers) {
if (provider.getCredentialsProviderId() === credentialsProviderId) {
if (isEqual(provider.getCredentialsProviderId(), credentialsProviderId)) {
return provider
}
}
Expand Down
30 changes: 17 additions & 13 deletions src/credentials/providers/credentialsProviderId.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,28 +3,32 @@
* SPDX-License-Identifier: Apache-2.0
*/

export const CREDENTIALS_PROVIDER_ID_SEPARATOR = ':'
const CREDENTIALS_PROVIDER_ID_SEPARATOR = ':'

export interface CredentialsProviderIdComponents {
credentialType: string
credentialTypeId: string
export interface CredentialsProviderId {
readonly credentialType: string
readonly credentialTypeId: string
}

export function makeCredentialsProviderIdComponents(credentialsProviderId: string): CredentialsProviderIdComponents {
const chunks = credentialsProviderId.split(CREDENTIALS_PROVIDER_ID_SEPARATOR)
export function asString(credentialsProviderId: CredentialsProviderId): string {
return [credentialsProviderId.credentialType, credentialsProviderId.credentialTypeId].join(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use a template literal?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personal preference on this one, I find this more legible than ${credentialsProviderId.credentialType}${CREDENTIALS_PROVIDER_ID_SEPARATOR}${credentialsProviderId.credentialTypeId}

CREDENTIALS_PROVIDER_ID_SEPARATOR
)
}

if (chunks.length !== 2) {
export function fromString(credentialsProviderId: string): CredentialsProviderId {
const separatorPos = credentialsProviderId.indexOf(CREDENTIALS_PROVIDER_ID_SEPARATOR)

if (separatorPos === -1) {
throw new Error(`Unexpected credentialsProviderId format: ${credentialsProviderId}`)
}

return {
credentialType: chunks[0],
credentialTypeId: chunks[1]
credentialType: credentialsProviderId.substring(0, separatorPos),
credentialTypeId: credentialsProviderId.substring(separatorPos + 1)
}
}

export function makeCredentialsProviderId(credentialsProviderIdComponents: CredentialsProviderIdComponents): string {
return [credentialsProviderIdComponents.credentialType, credentialsProviderIdComponents.credentialTypeId].join(
CREDENTIALS_PROVIDER_ID_SEPARATOR
)
export function isEqual(idA: CredentialsProviderId, idB: CredentialsProviderId): boolean {
return idA.credentialType === idB.credentialType && idA.credentialTypeId === idB.credentialTypeId
}
31 changes: 15 additions & 16 deletions src/credentials/providers/credentialsProviderManager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,22 +5,13 @@

import { CredentialsProvider } from './credentialsProvider'
import { CredentialsProviderFactory } from './credentialsProviderFactory'
import { makeCredentialsProviderIdComponents } from './credentialsProviderId'

let credentialsProviderManagerInstance: CredentialsProviderManager | undefined

export function getCredentialsProviderManagerInstance(): CredentialsProviderManager {
if (!credentialsProviderManagerInstance) {
credentialsProviderManagerInstance = new CredentialsProviderManager()
}

return credentialsProviderManagerInstance
}
import { CredentialsProviderId } from './credentialsProviderId'

/**
* Responsible for providing the Toolkit with all available CredentialsProviders.
*/
export class CredentialsProviderManager {
private static INSTANCE: CredentialsProviderManager | undefined
private readonly providerFactories: CredentialsProviderFactory[] = []

public async getAllCredentialsProviders(): Promise<CredentialsProvider[]> {
Expand All @@ -35,10 +26,10 @@ export class CredentialsProviderManager {
return providers
}

public async getCredentialsProvider(credentialsProviderId: string): Promise<CredentialsProvider | undefined> {
const credentialsType = makeCredentialsProviderIdComponents(credentialsProviderId).credentialType

const factories = this.getFactories(credentialsType)
public async getCredentialsProvider(
credentialsProviderId: CredentialsProviderId
): Promise<CredentialsProvider | undefined> {
const factories = this.getFactories(credentialsProviderId.credentialType)
for (const factory of factories) {
await factory.refresh()

Expand All @@ -55,7 +46,15 @@ export class CredentialsProviderManager {
this.providerFactories.push(factory)
}

private getFactories(credentialsType: string) {
private getFactories(credentialsType: string): CredentialsProviderFactory[] {
return this.providerFactories.filter(f => f.getCredentialType() === credentialsType)
}

public static getInstance(): CredentialsProviderManager {
if (!CredentialsProviderManager.INSTANCE) {
CredentialsProviderManager.INSTANCE = new CredentialsProviderManager()
}

return CredentialsProviderManager.INSTANCE
}
}
Loading