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 Documentation #889

Merged
merged 13 commits into from
Jan 14, 2020
Merged

Conversation

awschristou
Copy link
Contributor

Description

Documentation related to the Credentials Management system in #888

License

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

Copy link
Contributor

@hunterwerlla hunterwerlla left a comment

Choose a reason for hiding this comment

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

Some minor comments on the doc, but very excited to have fixed credential management in the toolkit

@@ -0,0 +1,59 @@
<?xml version="1.0" encoding="UTF-8" standalone="no"?><svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" contentScriptType="application/ecmascript" contentStyleType="text/css" height="434px" preserveAspectRatio="none" style="width:838px;height:434px;" version="1.1" viewBox="0 0 838 434" width="838px" zoomAndPan="magnify"><defs><filter height="300%" id="f9y5e0500bl5" width="300%" x="-1" y="-1"><feGaussianBlur result="blurOut" stdDeviation="2.0"/><feColorMatrix in="blurOut" result="blurOut2" type="matrix" values="0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 .4 0"/><feOffset dx="4.0" dy="4.0" in="blurOut2" result="blurOut3"/><feBlend in="SourceGraphic" in2="blurOut3" mode="normal"/></filter></defs><g><!--MD5=[626eed683b636928253c4c4840380f7f]
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you increase the font size

designs/credentials/credentials-management.md Show resolved Hide resolved
designs/credentials/credentials-management.md Show resolved Hide resolved
designs/credentials/credentials-management.md Show resolved Hide resolved

All Credentials Providers are uniquely identified by a Credentials Provider Id. These take the format `(credentials type):(credentials type id)`. For example, Shared Credentials Providers have the type `profile`, and the id represents the profile name. So if shared credentials contained a profile named "foo", its corresponding Credentials Provider Id would be `profile:foo`.

Credentials Provider Id may be surfaced to users, however it is an internal identification construct.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "may be" -> is surfaced

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 currently surfaced, but that may not always be the case


When the user connects to AWS in the Toolkit, a Credentials Provider is requested, which is then used to obtain credentials. The toolkit requests a Credentials Provider by checking which credentials provider factories support the provider's credentials type. The factories of interest are queried to see which (if any) have the requested Credentials Provider.

At the time of writing this document, there is only support for Shared Credentials. If additional credentials support was implemented (and this document was not updated), it would be found in [/src/credentials/providers](/src/credentials/providers).
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe delete "If additional credentials support was implemented (and this document was not updated)," and replace it with "Additional credentials providers are placed in"


Profiles are retrieved from the user's shared credentials files. The profile is handled and validated differently based on which fields are present. Handling and validation logic can be found in [sharedCredentialsProvider.ts](/src/credentials/providers/sharedCredentialsProvider.ts).

Profiles that are not considered valid are not provided to the toolkit. When connecting in the toolkit, the user is not able to select these Credentials to work with. Validation issues detected are written to the logs to help users understand why a profile is not available in the toolkit.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are they just not shown at all? Maybe we need to surface them somehow? it seems weird to not provide them at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this is the same in JetBrains. It is too spammy to pop notifications each time one or more invalid profiles are detected.

Also: rewriting away the double negative in that paragraph.

- credential_process
- region

Credentials Providers for Shared Credentials are only ever refreshed when the user brings up the credential selection list. If a profile is considered to have changed since it was last used in the current toolkit session, Credentials are produced from the updated profile.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not intuitive. We should offer either a more proactive approach to refreshing credentials, or an explicit credential refresh action. Why should a user have to change their credentials to the credentials they already selected?

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 seems like a reasonable behavior to start with. The only times that shared credentials files should be updated are to add new credentials, or to remedy an error in an existing profile. In these cases, it is reasonable to understand that you would need to take a corresponding action in the toolkit.

I'd suggest to release this behavior, and use customer feedback to drive whether or not additional complexities are needed in this area.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd at the very least like to see an alternate design or even considerations of this in the design doc. I'm fine with shipping this in the form of code, but it doesn't make sense to leave it out of the design doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added that to the doc now


### Credentials Provider Id

All Credentials Providers are uniquely identified by a Credentials Provider Id. These take the format `(credentials type):(credentials type id)`. For example, Shared Credentials Providers have the type `profile`, and the id represents the profile name. So if shared credentials contained a profile named "foo", its corresponding Credentials Provider Id would be `profile:foo`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we find a different separator? Colons are allowed in profile names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I couldn't find a spec for profile names. Do you have a link?

Copy link
Contributor

Choose a reason for hiding this comment

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

Can't find one either, found this through trial and error. Was going to propose a colon and a space but I just tested it and spaces work :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline, we'll switch this to a pipe |

@codecov-io
Copy link

codecov-io commented Jan 9, 2020

Codecov Report

Merging #889 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

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 86e0604...5ff6dee. Read the comment docs.

@awschristou awschristou changed the base branch from awschristou/credentials to master January 13, 2020 21:02
@awschristou awschristou merged commit d21bf1b into master Jan 14, 2020
@awschristou awschristou deleted the awschristou/credentials-docs branch January 14, 2020 00:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants