-
Notifications
You must be signed in to change notification settings - Fork 81
feat(identity): add STS credential management #1846
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 STS credential management #1846
Conversation
74ebd75
to
29724b6
Compare
57cd7e7
to
1d30f5e
Compare
import { IamFlowParams, simulatePermissions } from './utils' | ||
import { createHash } from 'crypto' | ||
|
||
const sourceProfileRecursionMax = 5 |
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.
Is recursion of source profiles even allowed? How do the CLI and SDK handle this?
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.
Yes, source profile recursion/chaining is allowed according to this assume-role SEP
const key = JSON.stringify({ | ||
RoleArn: params.profile.settings?.role_arn, | ||
RoleSessionName: params.profile.settings?.role_session_name, | ||
SerialNumber: params.profile.settings?.mfa_serial, |
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.
Minor nit: Keep the name as MfaSerial as it is more descriptive and immediately indicative of what this field contains. Unless the SEP prescribed this 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.
The disk-cache SEP prescribed the name SerialNumber
try { | ||
const parentCredentials = await this.getParentCredential(params) | ||
const stsClient = new STSClient({ | ||
region: params.profile.settings?.region || 'us-east-1', |
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.
Please make 'us-east-1' a const defined at the class level instead of a magic string. Also, since this logic is used in multiple places, consider wrapping getting an STS client in a function and using that throughout the code.
AwsErrorCodes, | ||
IamCredential, | ||
IamCredentialId, | ||
IamCredentials, |
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.
Why is there an IamCredential
and IamCredentials
types? This will be confusing.
There is a similar naming issue with SSOToken
and SsoToken
I think, which was due to SSOToken being a type imported from a 3rd-party package. Is that the case 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.
The IamCredential
is supposed to be a wrapper object for an id, profile kind, and IamCredentials as suggested in this comment, but I was struggling to come up with a name for this. Any naming suggestions would be appreciated.
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 see:
Check failure on line 3 in server/aws-lsp-identity/src/iam/iamProvider.ts
GitHub Actions
/ Test public NPM packages
'"@aws/language-server-runtimes/server-interface"' has no exported member named 'IamCredential'. Did you mean 'IamCredentials'?
Maybe this isn't an issue and IamCredential
doesn't exist and shouldn't have been imported here? If it does exist, where is it? I couldn't find it in language-server-runtimes.
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.
IamCredential
is defined in this PR. It was added after the previous runtimes PR was approved, so it's not on the main runtimes branch yet.
const stsClient = new STSClient({ region: region || 'us-east-1', credentials: credentials }) | ||
const identity = await stsClient.send(new GetCallerIdentityCommand({})) | ||
if (!identity.Arn) { | ||
throw new AwsError('Caller identity ARN not found.', 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.
This isn't an invalid profile, is there an existing or can you create a new error code specific to this error? Also, the more places an error code is reused in the code base, the less useful it becomes for debugging later. The intent of the error codes is a concrete value that can be kept overtime, even as the error messages may evolve. In the absence of error codes, people would attempt to string match on the error message which is extremely fragile.
}) | ||
} | ||
|
||
private ignoreDoesNotExistOrThrow(error: unknown): Promise<undefined> { |
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 this is in the SSO variant of this file as well. If so, consider putting in a common utils library in aws-lsp-core.
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 uses a different error message and error code.
// Based on: | ||
// https://github.com/smithy-lang/smithy-typescript/blob/main/packages/shared-ini-file-loader/src/getSSOTokenFilepath.ts | ||
export function getStsCredentialFilepath(id: string): string { | ||
return join(getHomeDir(), '.aws', 'flare', 'cache', `${id}.json`) |
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.
Let's be sure "flare" is the correct name here. I'll ping you with further details.
} | ||
|
||
// Based on: | ||
// https://github.com/smithy-lang/smithy-typescript/blob/main/packages/shared-ini-file-loader/src/getSSOTokenFromFile.ts |
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.
Speaking of this, did you check the AWS SDK for JS to see if there were already functions for reading/writing STS cache files? Or see what AWS CLI code is using to do this? There might be reusable code already.
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 did not see any STS cache functions inside of @aws-sdk
and @smithy/shared-ini-file-loader
. I'm not sure if this is the primary AWS CLI credentials code, but it only references .aws
for the SSO cache.
|
||
// Check if credential is still valid (not in refresh window) | ||
if (nowMillis < expirationMillis) { | ||
this.observability.logging.log('STS credential before refresh window. Returning current STS credential.') |
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.
Are these log messages and comments here consistent with what is happening? Is this how they're written in the SSO counterpart to this class? Seems a bit off.
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 is how it's written in the SSO counterpart. I could change the message to 'STS credential expiration is before...' and do something similar for SSO.
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.
It looks like it's the refresh window handling for SSO. Is that how the SEP defines handling it (e.g. try 5 minutes before at a rate of no more than every 20 or 30 seconds) for STS 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.
I could not find a SEP for STS/temporary credential refresh, so I'm not sure.
|
||
let delayMs: number | ||
|
||
if (nowMillis < expirationMillis - refreshWindowMillis) { |
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.
Was there a SEP specifically for STS creds refresh handling or did you just duplicate the SSO handling? If so, there is an opportunity for a base class here as there is some complicated code here that we shouldn't copy/paste reuse or it will likely lead to inconsistency/bugs in the future.
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 was duplicated from SSO handling. Sure, I can refactor this.
stsCredentialDetail.lastRefreshMillis = Date.now() | ||
|
||
// Passing refresh function into here is easier than refreshing from STS cache | ||
const newCredentials = await refreshCallback() |
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.
It's a bit easier to pass refreshCallback into watch instead of the STS cache with the current setup, since generateStsCredential is tightly coupled with iamProvider. Moving refreshCallback to STS cache would make it closer to the SSO implementation, but requires generateStsCredential to be a util function and uncoupled from iamProvider.
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 changes being made to the existing SSO flow, be sure this functionality is all evaluated in amazon-q-eclipse as well.
AwsErrorCodes, | ||
IamCredential, | ||
IamCredentialId, | ||
IamCredentials, |
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 see:
Check failure on line 3 in server/aws-lsp-identity/src/iam/iamProvider.ts
GitHub Actions
/ Test public NPM packages
'"@aws/language-server-runtimes/server-interface"' has no exported member named 'IamCredential'. Did you mean 'IamCredentials'?
Maybe this isn't an issue and IamCredential
doesn't exist and shouldn't have been imported here? If it does exist, where is it? I couldn't find it in language-server-runtimes.
|
||
// Check if credential is still valid (not in refresh window) | ||
if (nowMillis < expirationMillis) { | ||
this.observability.logging.log('STS credential before refresh window. Returning current STS credential.') |
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.
It looks like it's the refresh window handling for SSO. Is that how the SEP defines handling it (e.g. try 5 minutes before at a rate of no more than every 20 or 30 seconds) for STS as well?
} catch (e) { | ||
this.sourceProfileRecursionCount = 0 | ||
throw e | ||
// Get the credentials directly from the 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.
Just to be sure, the intent here is that if a profile has both creds defined in it and a sourceProfile listed, you would prioritize using the creds provided and ignore the sourceProfile, correct?
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.
Yes, it should prioritize the profile creds over the sourceProfile.
return response.EvaluationResults.every(result => result.EvalDecision === 'allowed') | ||
} | ||
|
||
export async function checkMfaRequired( |
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.
Shouldn't permissions
be hard-coded as sts:AssumeRole
instead of passed in? Shouldn't this call validatePermissions
instead of duplicating the code? If the latter, is the function needed or should you just call validatePermissions
from iamProvider.ts directly above (passing in sts:AssumeRole
)?
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 is slightly different because it checks whether the MFA condition is present on any permission instead of whether access is granted. Hardcoding sts:AssumeRole
would achieve the same outcome for now, but would not let you check MFA on other permissions if that was needed in the future.
credentials: IamCredentials, | ||
permissions: string[], | ||
region?: string | ||
): Promise<SimulatePrincipalPolicyCommandOutput> { |
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.
Is there an intended change to the return type here?
): Promise<SimulatePrincipalPolicyCommandOutput> { | ||
// Convert the credentials into an identity | ||
const stsClient = new STSClient({ region: region || 'us-east-1', credentials: credentials }) | ||
const stsClient = new STSClient({ region: region, credentials: credentials }) |
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.
Why not keep the region || defaultRegion
in place? Did tsc complain since region: string
is no longer optional?
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 thought it was cleaner to make it a default argument. I can revert this if needed.
} | ||
|
||
// Simulate permissions on the identity | ||
const iamClient = new IAMClient({ region: region || 'us-east-1', credentials: credentials }) | ||
const iamClient = new IAMClient({ region: region, credentials: credentials }) |
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.
Same as above about defaulting to defaultRegion handling.
35d8bd1
to
7a8e61a
Compare
4f5fc2d
to
d093e42
Compare
82eeaf9
to
e96e1c1
Compare
7a8e61a
to
48a1bc5
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.
74e9e70
to
99f379d
Compare
f06d8a6
to
9345d1d
Compare
Problem
The identity LSP does not support retrieving IAM role 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 #1869.
This PR adds the option to assume a role and generate STS credentials if the language client requests an IAM credential using a RoleSourceProfile. After the STS credential is generated, the identity LSP caches it into
.aws/cli/cache
and manages its lifecycle, including expiration, invalidation, and refresh.Note: This PR currently fails the CI pipeline because it depends on changes from aws/language-server-runtimes#599.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.