-
Notifications
You must be signed in to change notification settings - Fork 83
feat(identity): add support for retrieving IAM user credentials #1869
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
feat(identity): add support for retrieving IAM user credentials #1869
Conversation
10d0215 to
07d8227
Compare
| [AuthorizationFlowKind.DeviceCode]: deviceCodeFlow, | ||
| [AuthorizationFlowKind.Pkce]: authorizationCodePkceFlow, | ||
| } | ||
| const qPermissions = [ |
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.
There might be better ways to check whether the user has permission to access Q than to use this list with SimulatePrincipalPolicy. Thoughts?
| ssoCache, | ||
| autoRefresher, | ||
| { showUrl, showMessageRequest, showProgress }, | ||
| this.features.identityManagement.sendGetMfaCode, |
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.
For client->server requests/notifications wrap them and pass that in to IdentityService in the 4th parameter (see line 58 above for examples).
| private readonly ssoCache: SsoCache, | ||
| private readonly autoRefresher: SsoTokenAutoRefresher, | ||
| private readonly handlers: SsoFlowParams['handlers'], | ||
| private readonly sendGetMfaCode: (params: GetMfaCodeParams) => Promise<GetMfaCodeResult>, |
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.
Per the other comment, refactor the previous parameter to generically support handlers (not just SsoFlowParams, consider renaming if appropriate). All current and future handlers should come in through that parameter.
| const options = { ...getIamCredentialOptionsDefaults, ...params.options } | ||
|
|
||
| token.onCancellationRequested(_ => { | ||
| if (options.callStsOnInvalidIamCredential) { |
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.
This may apply beyond just cases where AssumeRole is called given the various ways of retrieving credentials. You should verify with Nik and/or whoever defined metrics for this project to understand the expectations here.
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.
I discussed this with Nik and we're thinking we should emit this metric whenever getIamCredential is cancelled, not just when callStsOnInvalidIamCredential is true.
| } | ||
| } | ||
|
|
||
| async getIamCredential(params: GetIamCredentialParams, token: CancellationToken): Promise<GetIamCredentialResult> { |
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.
The impl for this is likely to get HUGE as more profile types are added. This impl should be moved under ../iam/ (similar to ../sso/) with the function here being mostly input/output validation, logging, and handing off primary impl to code under that namespace.
| credentials = { | ||
| accessKeyId: profile.settings!.aws_access_key_id!, | ||
| secretAccessKey: profile.settings!.aws_secret_access_key!, | ||
| sessionToken: profile.settings!.aws_session_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.
Should expiration property be here as well?
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.
No, expiration is not a profile field according to the CLI docs, so it would not be part of profile.settings either.
| if (!response?.EvaluationResults?.every(result => result.EvalDecision === 'allowed')) { | ||
| throw new AwsError( | ||
| `User or assumed role has insufficient permissions.`, | ||
| AwsErrorCodes.E_INVALID_PROFILE |
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.
We should use a different error code here, some like E_PERMISSION_DENIED (there may already be something like this).
|
|
||
| emitMetric('Succeeded') | ||
| return { | ||
| id: profile.name, |
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.
In the case of cached temporary credentials, you'll probably want to use the cached file name here. I think you may want to return the credential kind here as well as if the client ever needs to pass these details back to the server (say for refresh) the server would need that info. You probably want to create an enclosing type (similar to SsoToken, maybe IamCredentials) for these 3 properties as they need to be kept together and it'll be easier to reuse in Params/Result types in the language-servers protocol as well as the client to keep them all together in one reference/variable.
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.
I can change the id to match the SEP in the next PR, which focuses on temporary credentials.
| } | ||
|
|
||
| // Validate permissions | ||
| if (options.validatePermissions) { |
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.
I think we need to reconsider validationPermissions as a boolean as we'll likely have more permission sets than just Q in the future. While it would be ideal for language-servers to have some standard permission sets, clients need the flexibility to provide permission sets themselves (similar to SSO scopes). The language-server-runtimes protocol should advertise permission sets (string[] of permissions), but validatePermissions should be a string array to accept permissions.
This functionality probably makes sense as a separate protocol request as well, but that can be added in the future if needed. This is a two-way door doing it just here for now.
| } | ||
|
|
||
| // Simulate permissions on the identity associated with the credentials | ||
| private async simulatePermissions( |
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.
This should be moved under ../sts/ namespace as well.
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.
../iam/ might make more sense, since any IAM credentials can have their permissions simulated, not just those found from AssumeRole. This function also calls the SimulatePrincipalPolicy API from the IAM client.
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.
Yep, ../iam/ is correct.
| } | ||
| } | ||
| // If the profile does not match any profile type, mark it as an unknown profile | ||
| if (profile.kinds.length === 0) { |
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.
What happens to profile kinds of unknown downstream as this profile will not have any settings associated with it? If I recall correctly, it will delete the profile. Is that the intent here?
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.
The profile is only deleted if it is saved without settings. I suppose fetching and updating 0 fields on the profile would unintentionally delete the profile, so I could add a setting when loading an unknown profile.
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.
Actually, reading the comments on the save method below:
// If a setting is set to undefined, null, or an empty string, it will be removed from shared
// config files. If the settings property is set to undefined or null, the entire section will
// be removed from the shared config files. This is equivalent to deleting a section.
// Any settings or sections in the shared config files that are not passed into data will
// be preserved as-is.
I think you were ok with your original change as settings is set to an empty object in line 53, so the unknown profile type profile would not present any settings (which is logically correct since we don't know what it is) and if it were to be saved as an unknown profile type, the save method would just preserve what is stored in the shared config file already without adding anything. We shouldn't add the dummy setting at all. You can revert this to your original change (just remove lines 70-71 that you added for the dummy setting).
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.
Just a slight correction: if a profile were saved as an unknown profile type, I think the profile service would save the new settings while preserving the old ones. And if the new settings were empty, the profile service would delete the profile similar to the other profile types. Either way, your suggestion sounds good.
| const showMessageRequest: ShowMessageRequest = (params: ShowMessageRequestParams) => | ||
| this.features.lsp.window.showMessageRequest(params) | ||
| const showProgress: ShowProgress = this.features.lsp.sendProgress | ||
| const sendGetMfaCode: SendGetMfaCode = this.features.identityManagement.sendGetMfaCode |
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.
Be careful with this assignment vs wrapping in a lambda closure. If the method called (sendGetMfaCode) requires this in its impl, it may fail. I see sendProgress on the line above is done this way. I can only assume it isn't relying on this in its current impl is why it is working (or maybe it doesn't). It's just a modified impl over in the language-server-runtimes repo away from breaking.
9b69727 to
fdd5dfe
Compare
4f5fc2d to
d093e42
Compare
82eeaf9 to
e96e1c1
Compare
## Problem The IAM type changes are behind the changes in aws/language-servers#1869 and aws/language-servers#1846. As a result, the language-servers PRs are unable to compile. ## Solution This is part of #572. - Rename validatePermissions to permissionSets and make it accept a list of permissions instead of validating only 1 set of permissions - Wrap credentials from getIamCredentialResult into an intermediate object - Add credentials override and additional error codes to getIamCredentials - Add mfaSerial to GetMfaSerialResult and optionalize it in GetMfaSerialParams <!--- REMINDER: - Read CONTRIBUTING.md first. - Add test coverage for your changes. - Link to related issues/commits. - Testing: how did you test your changes? - Screenshots if applicable --> ## License By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
fdd5dfe to
6f37a8a
Compare
Problem
The identity LSP does not support retrieving IAM user credentials. This forces IDE extensions to implement IAM credentials retrieval, which leads to code duplication and added complexity.
Solution
This is part of #1981 and is built on top of #1845.
This PR adds the getIamCredential handler to retrieve IAM user credentials from the shared config files and validate the user's permissions. Alternate ways to retrieve IAM credentials, such as from role assumption and credentials process, will be implemented in the next few PRs.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.