Credentials Management #888

awschristou · 2020-01-08T22:26:12Z

Description

This change introduces a more managed approach to implementing Credentials Providers for the toolkit. The goal is to make it easier to onboard support for new credentials types moving forward. This design is very similar to the approach taken in the AWS Toolkit for JetBrains.

The design document is in #889

Integrating this new system brought about some functional changes and bug fixes.

New Behavior:

added logging for Shared Credentials profiles detection and validation
credentials are now shown by "credential provider id" instead of profile name (example: profile:default)
whenever users run the "Connect to AWS" command, the Toolkit's list of available credentials providers is refreshed (eg: pulling the latest updates from the shared credentials files)
Credentials are cached and re-used for the duration of a toolkit session. When logging in, if a Shared Credentials Profile has been modified since it was cached, the updated version of the profile will be used instead of the cached version now.
updated the "Invalid Credentials" notification, and added a button guiding users to the logs

Out of scope:

a follow-up PR will normalize all credentials related code locations
a follow-up PR will normalize all user facing text and all code names related to credential vs profile. "profile" terminology will be removed except where explicitly referencing profiles. Previously, the codebase wrongly used these terms rather interchangeably.

Testing

additional tests were added to the codebase
with logging on verbose level:
- bring up the 'connect to aws' credentials list successive times. See that the logs show that the shared credentials files are only loaded once
- bring up the 'connect to aws' credentials, then modify the shared credentials files. Bring up the credentials list again. See that the logs show that the shared credentials files were loaded twice
- set up profiles that make a cyclical reference, and that refer to a nonexistent profile. Bring up the 'connect to aws' credentials list, and see that these profiles are not in the list (and that their validation issues are logged)
- log in with profile X. Edit the credentials file to alter profile X. log in again with profile X in the same toolkit session. See that the logs show the profile was re-loaded and the new version of X was used to make a connection.

License

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…alsUtils

…etected, they are evicted from the cache and new credentials are used.

…ton.

… from producing an invalid credentials notification

.changes/next-release/Feature-1c091dfc-c519-4fa1-8f1a-7ab8d7db214b.json

.changes/next-release/Feature-681f79db-6ef7-4172-9f5a-52b42c93ede6.json

package.nls.json

src/shared/utilities/textUtilities.ts

src/credentials/activation.ts

src/shared/credentials/userCredentialsUtils.ts

src/credentials/providers/credentialsProviderManager.ts

src/credentials/providers/sharedCredentialsProviderFactory.ts

codecov-io · 2020-01-09T00:03:20Z

Codecov Report

❗ No coverage uploaded for pull request base (master@4a0e321). Click here to learn what that means.
The diff coverage is 81.25%.

@@            Coverage Diff            @@
##             master     #888   +/-   ##
=========================================
  Coverage          ?   57.32%           
=========================================
  Files             ?      163           
  Lines             ?     5819           
  Branches          ?      784           
=========================================
  Hits              ?     3336           
  Misses            ?     2483           
  Partials          ?        0

Impacted Files	Coverage Δ
src/credentials/activation.ts	`0% <0%> (ø)`
src/shared/defaultAwsContextCommands.ts	`0% <0%> (ø)`
src/lambda/wizards/samInitWizard.ts	`39.72% <0%> (ø)`
...redentials/providers/credentialsProviderManager.ts	`100% <100%> (ø)`
src/credentials/providers/credentialsProviderId.ts	`100% <100%> (ø)`
...ials/providers/sharedCredentialsProviderFactory.ts	`92.85% <100%> (ø)`
...redentials/providers/credentialsProviderFactory.ts	`100% <100%> (ø)`
src/shared/utilities/textUtilities.ts	`83.33% <100%> (ø)`
src/credentials/credentialsStore.ts	`100% <100%> (ø)`
...dentials/defaultCredentialSelectionDataProvider.ts	`38.96% <100%> (ø)`
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4a0e321...ab3807e. Read the comment docs.

bryceitoc9

Only through half of the PR right now but did want to provide some feedback before EOD

bryceitoc9 · 2020-01-08T22:47:52Z

package.nls.json

+    "AWS.message.credentials.invalid.help": "Get Help...",
+    "AWS.message.credentials.invalid.logs": "View logs",


Nit: I feel like we can preemptively make these generic (AWS.getHelp, AWS.viewLogs or something). Also, capitalize "Logs"

I'm going to leave these strings as credentials concerns for now, but a separate pass over all of the text to look for general-purpose opportunities is a good idea.

Capitalizing Logs

bryceitoc9 · 2020-01-08T22:50:12Z

src/credentials/activation.ts

    if (previousCredentialsId) {
+        // Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format,


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.

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.

bryceitoc9 · 2020-01-08T22:54:13Z

src/credentials/credentialsStore.ts

@@ -19,26 +24,33 @@ 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]
+        return this.credentialsCache[credentialsId]?.credentials
    }

    /**
     * If credentials are not stored, the provided create function is called. Created credentials are then stored.
     */
    public async getCredentialsOrCreate(


Nit: change the name to getOrCreateCredentials; put the actions first.

Unfortunately, this was added in a separate PR, so the rename belongs in a separate PR.

The cache has been updated to treat Credentials + Hash as one object, so I added the rename as well.

src/credentials/loginManager.ts

bryceitoc9 · 2020-01-08T23:44:18Z

src/credentials/loginManager.ts

            })
        } catch (err) {
-            getLogger().error('Error logging in', err as Error)
+            getLogger().error(
+                `Error trying to connect to AWS with Credentials Provider ${credentialsId}. Toolkit will now disconnect from AWS.`,


Nit: the toolkit wasn't connected to AWS in the first place.

bryceitoc9 · 2020-01-09T00:48:00Z

src/credentials/providers/sharedCredentialsProvider.ts

+            expectedProperties.push(
+                SHARED_CREDENTIAL_PROPERTIES.AWS_ACCESS_KEY_ID,
+                SHARED_CREDENTIAL_PROPERTIES.AWS_SECRET_ACCESS_KEY
+            )


I feel like you can just roll the filter statement from the getMissingProperties call into here and remove the statement.

I don't follow.

Remove the getMissingProperties function and just call the array filter here.

That felt more repetitive (see line 78). I figured there's more likelihood of adding similar checks in the future for other (future) property pairings.

src/credentials/providers/sharedCredentialsProvider.ts

bryceitoc9 · 2020-01-09T17:32:29Z

src/credentials/providers/sharedCredentialsProvider.ts

+            expectedProperties.push(
+                SHARED_CREDENTIAL_PROPERTIES.AWS_ACCESS_KEY_ID,
+                SHARED_CREDENTIAL_PROPERTIES.AWS_SECRET_ACCESS_KEY
+            )


Remove the getMissingProperties function and just call the array filter here.

src/credentials/providers/sharedCredentialsProvider.ts

src/credentials/providers/sharedCredentialsProviderFactory.ts

bryceitoc9 · 2020-01-09T17:46:48Z

src/credentials/sharedCredentials.ts

+
+export async function updateAwsSdkLoadConfigEnvironmentVariable(): Promise<void> {
+    const configFileExists = await SystemUtilities.fileExists(getConfigFilename())
+    process.env.AWS_SDK_LOAD_CONFIG = configFileExists ? 'true' : ''


= configFileExists.toString() ?

bryceitoc9 · 2020-01-09T17:48:36Z

src/credentials/sharedCredentials.ts

+    return profile
+}
+
+export async function updateAwsSdkLoadConfigEnvironmentVariable(): Promise<void> {


Do we need to explicitly set this value?

Unfortunately, we want this envvar to reflect reality as close to when we're using shared credentials as possible. Otherwise you can get into states where errors are raised due to missing files, or where profile data is ignored in newly created files.

bryceitoc9 · 2020-01-09T17:53:46Z

src/shared/utilities/textUtilities.ts

+/**
+ * Hashes are not guaranteed to be stable across toolkit versions.
+ */
+export function getStringHash(text: string): number {


Can we just use a library?

Prefer not to use a library -- we're pushing credentials profile data through this

bryceitoc9 · 2020-01-09T18:01:21Z

src/test/credentials/provider/credentialsProviderId.test.ts

+} from '../../../credentials/providers/credentialsProviderId'
+import { assertThrowsError } from '../../shared/utilities/assertUtils'
+
+describe('makeCredentialsProviderIdComponents', async () => {


Perhaps test a profile with a colon in the name.

bryceitoc9 · 2020-01-09T18:08:51Z

src/test/credentials/provider/sharedCredentialsProvider.test.ts

+function assertSubstringsInText(text: string | undefined, ...substrings: string[]) {
+    assert.ok(text)
+    substrings.forEach(substring => assert.notStrictEqual(text!.indexOf(substring), -1))
+}


Pull this out into the generic test utils.

It makes sense to do that when there is a test that has a need for this to be shared

I'm shocked that we don't have a single utils dir and (I don't think) any tests that check substrings. This stays.

…red invalid

src/credentials/loginManager.ts

src/credentials/providers/credentialsProviderFactory.ts

src/credentials/providers/credentialsProviderManager.ts

src/credentials/providers/credentialsProviderId.ts

src/credentials/providers/sharedCredentialsProvider.ts

…properties as a single object

kiiadi · 2020-01-08T23:14:27Z

src/awsexplorer/defaultRegion.ts

-    const credentialReaderWriter = new DefaultCredentialsFileReaderWriter()
-
-    const profileRegion = await credentialReaderWriter.getDefaultRegion(profileName)
+    const profileRegion = awsContext.getCredentialDefaultRegion()


is this something that should be attached to the credential itself rather than a top-level on awsContext

awsContext contains an AwsContextCredentials object, which then backs these calls. Would it make more sense to have a single getter for that object instead of getters for each property?

kiiadi · 2020-01-08T23:31:31Z

src/credentials/activation.ts

@@ -29,8 +31,17 @@ export async function loginWithMostRecentCredentials(
    toolkitSettings: SettingsConfiguration,
    loginManager: LoginManager
 ): Promise<void> {
-    const previousCredentialsId = toolkitSettings.readSetting(profileSettingKey, '')
+    let previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '')


nit: I really don't like the "create a mutable variable, apply a conditional and maybe re-assign it's value cos it's convenient" pattern - I know it's tightly scoped but I really like to avoid mutability where possible. It's a smell.

I think you're better off changing this to something like :

const previousCredentialsId = toolkitSettings.readSetting<string>(profileSettingKey, '') if (previousCredentialsId) { // Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format, // treat it like a Shared Crdentials Provider. const loginCredential = previousCredentialsId.indexOf(CREDENTIALS_PROVIDER_ID_SEPARATOR) === -1 ? previousCredentialsId = makeCredentialsProviderId({ credentialType: SharedCredentialsProviderFactory.CREDENTIAL_TYPE, credentialTypeId: previousCredentialsId }) : previousCredentialsId await loginManager.login(loginCredential) } else { await loginManager.logout() }

I completely agree. Prefer immutable where possible. Will change

kiiadi · 2020-01-08T23:36:34Z

src/credentials/credentialsStore.ts

@@ -5,11 +5,16 @@

 import * as AWS from 'aws-sdk'

+interface CredentialsData {
+    credentials: AWS.Credentials
+    credentialsHashCode: number


what is this for?

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.

kiiadi · 2020-01-08T23:39:05Z

src/credentials/loginManager.ts

            })
        } catch (err) {
-            getLogger().error('Error logging in', err as Error)
+            getLogger().error(
+                `Error trying to connect to AWS with Credentials Provider ${credentialsId}. Toolkit will now disconnect from AWS.`,


Are these bubbled to the user? Should they be localized?

They're only surfaced to the user in the form of logs. Might be a good idea to surface the error to the user through the interface, but we definitely don't want to localize the logs.

They are only logged (not placed in a notification). The current approach has been to only localize contents that are sent to the UI, and not logged materials.

Take a look at the revamped notification in the PR description. The intent was to steer people to the logs if they were interested, without having to route specific details about the error into the UI.

kiiadi · 2020-01-09T18:46:32Z

src/credentials/providers/credentialsProviderFactory.ts

+
+export abstract class BaseCredentialsProviderFactory<T extends CredentialsProvider>
+    implements CredentialsProviderFactory {
+    protected providers: T[] = []


Is an array / list really the right data-structure here? We seem to be doing lookup-style operations mostly that point to a dictionary / map being a better choice.

The top access is a mix of "list all providers" and "get provider x" (the doc is currently open in #889 ). Collection still seems reasonable to me -- the lookups don't seem like they would have a heavy performance.

kiiadi · 2020-01-09T18:48:48Z

src/credentials/providers/credentialsProviderId.ts

+}
+
+export function makeCredentialsProviderIdComponents(credentialsProviderId: string): CredentialsProviderIdComponents {
+    const chunks = credentialsProviderId.split(CREDENTIALS_PROVIDER_ID_SEPARATOR)


This "splitting by :" logic seems to have leaked into a bunch of places. A "CredentialProvider" has a type and identifier which together form a 'compound-style' key that we can use to store them - but couldn't that concern be localized into a credential ID object itself?

I think it is only explicit in the tests, for readability. Anywhere in the toolkit code goes through the make/decode methods.

I initially stopped short of creating a credential provider id object. Since the intent was to support additional data in the future (using the separator), its probably saving future pain to make and pass around an object now instead of a string.

I'll give this a go.

src/credentials/providers/credentialsProviderManager.ts

bryceitoc9 · 2020-01-13T17:42:50Z

src/credentials/providers/credentialsProviderId.ts

-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(


Why not just use a template literal?

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

awschristou added 21 commits January 3, 2020 17:13

Initial implementation of Credential Provider

409af50

Remove getCredentialsFilename and getConfigFilename from UserCredenti…

51c094f

…alsUtils

BaseCredentialsProviderFactory tests

74ae1c3

Profile Validation now returns text instead of throwing Error

1989d51

getStringHash utility method

aadf828

Renamed types, produce Credentials, internalize CredentialProviderChain

cdfb8a9

Pull Default Region from Shared Credentials Profile

102655c

Get Profile HashCode

b7b6445

When logging in, if credentials are already cached but a change was d…

5ed5f73

…etected, they are evicted from the cache and new credentials are used.

Remove comment

9fd898f

Updated the 'invalid credentials' notice, and added a "View Logs" but…

6a83b2e

…ton.

CredentialsProviderId tests

b5abf17

CredentialsProviderManager tests

2ee351e

SharedCredentialsProvider tests

dcb2d4c

SharedCredentialsProviderFactory tests

f14b125

Fix envvar side-effects that were affecting other tests

f47fad3

Lint fix

29c760f

Merge branch 'master' into awschristou/credentials

ef8e981

Changelogs

2440bbf

Migration - prevent first startup using the new Credentials Providers…

004d6b9

… from producing an invalid credentials notification

Test fix

7380cce

awschristou requested a review from a team as a code owner January 8, 2020 22:26

This was referenced Jan 8, 2020

Credentials Management Documentation #889

Merged

Credentials caching is too aggressive #705

Closed

Update Toolkit with changes to credentials file #168

Closed

hunterwerlla reviewed Jan 8, 2020

View reviewed changes

awschristou added 4 commits January 8, 2020 15:21

Merge branch 'master' into awschristou/credentials

c848df8

Feedback - changelogs

23f6962

Feedback - label

c34820f

Feedback - Credentials Type for Shared Credentials

150f199

Feedback - CredentialsProviderManager instance

4094ee4

bryceitoc9 reviewed Jan 9, 2020

View reviewed changes

Feedback - label

4a0e321

bryceitoc9 reviewed Jan 9, 2020

View reviewed changes

awschristou added 4 commits January 9, 2020 10:20

Merge branch 'master' into awschristou/credentials

676ae7b

Shared Credentials Profiles without recognized properties are conside…

3a7afa6

…red invalid

Feedback - legible code

f7491f3

Feedback - consistent use of Millis

7712177

hunterwerlla reviewed Jan 9, 2020

View reviewed changes

src/credentials/loginManager.ts Outdated Show resolved Hide resolved

src/credentials/loginManager.ts Outdated Show resolved Hide resolved

Changed CredentialsProviderId separator from : to |

5025c19

hunterwerlla reviewed Jan 9, 2020

View reviewed changes

awschristou added 2 commits January 9, 2020 13:48

CredentialsStore now stores and retrieves Credentials and associated …

6f33ac4

…properties as a single object

Feedback - blank line

bc32e0d

kiiadi reviewed Jan 9, 2020

View reviewed changes

awschristou added 2 commits January 9, 2020 14:20

Feedback - immutable during activation login migration

b262bc7

CredentialsProviderId is now a type

6c76476

hunterwerlla approved these changes Jan 13, 2020

View reviewed changes

src/credentials/providers/credentialsProviderManager.ts Outdated Show resolved Hide resolved

awschristou added 2 commits January 13, 2020 09:47

Switched string hash utility from hand-rolled to sha256

5f38544

Feedback - concat over spread operator

ab3807e

bryceitoc9 approved these changes Jan 13, 2020

View reviewed changes

awschristou merged commit 86e0604 into master Jan 13, 2020

justinmk3 deleted the awschristou/credentials branch January 18, 2020 00:59

justinmk3 mentioned this pull request Jan 18, 2020

"Connect to AWS" error: unable to get local issuer certificate #899

Open

bryceitoc9 mentioned this pull request Sep 3, 2020

Hangs on startup after trying to use credential_process #855

Closed

		"AWS.message.credentials.invalid.help": "Get Help...",
		"AWS.message.credentials.invalid.logs": "View logs",

		if (previousCredentialsId) {
		// Migrate from older Toolkits - If the last providerId isn't in the new CredentialProviderId format,

Credentials Management #888

Credentials Management #888

Conversation

awschristou commented Jan 8, 2020 • edited Loading

Description

Testing

License

codecov-io commented Jan 9, 2020 • edited Loading

Codecov Report

bryceitoc9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awschristou commented Jan 8, 2020 •

edited

Loading

codecov-io commented Jan 9, 2020 •

edited

Loading