-
Notifications
You must be signed in to change notification settings - Fork 55
feat(runtimes): add handlers for IAM and STS credentials management #599
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
Changes from all commits
22f77b0
d9c4753
db626ab
306c2b0
fbfb2d6
56fc78a
939241b
52b509b
d38dbb7
8adfdee
bb6d6a2
7dc451c
38961c9
7948b97
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import { | ||
| IamCredentials, | ||
| LSPErrorCodes, | ||
| ProgressType, | ||
| ProtocolNotificationType, | ||
|
|
@@ -15,21 +16,28 @@ export const AwsErrorCodes = { | |
| E_CANNOT_OVERWRITE_SSO_SESSION: 'E_CANNOT_OVERWRITE_SSO_SESSION', | ||
| E_CANNOT_READ_SHARED_CONFIG: 'E_CANNOT_READ_SHARED_CONFIG', | ||
| E_CANNOT_READ_SSO_CACHE: 'E_CANNOT_READ_SSO_CACHE', | ||
| E_CANNOT_READ_STS_CACHE: 'E_CANNOT_READ_STS_CACHE', | ||
| E_CANNOT_REFRESH_SSO_TOKEN: 'E_CANNOT_REFRESH_SSO_TOKEN', | ||
| E_CANNOT_REFRESH_STS_CREDENTIAL: 'E_CANNOT_REFRESH_STS_CREDENTIAL', | ||
| E_CANNOT_REGISTER_CLIENT: 'E_CANNOT_REGISTER_CLIENT', | ||
| E_CANNOT_CREATE_SSO_TOKEN: 'E_CANNOT_CREATE_SSO_TOKEN', | ||
| E_CANNOT_CREATE_STS_CREDENTIAL: 'E_CANNOT_CREATE_STS_CREDENTIAL', | ||
| E_CANNOT_WRITE_SHARED_CONFIG: 'E_CANNOT_WRITE_SHARED_CONFIG', | ||
| E_CANNOT_WRITE_SSO_CACHE: 'E_CANNOT_WRITE_SSO_CACHE', | ||
| E_CANNOT_WRITE_STS_CACHE: 'E_CANNOT_WRITE_STS_CACHE', | ||
| E_ENCRYPTION_REQUIRED: 'E_ENCRYPTION_REQUIRED', | ||
| E_INVALID_PROFILE: 'E_INVALID_PROFILE', | ||
| E_INVALID_SSO_CLIENT: 'E_INVALID_SSO_CLIENT', | ||
| E_INVALID_SSO_SESSION: 'E_INVALID_SSO_SESSION', | ||
| E_INVALID_SSO_TOKEN: 'E_INVALID_SSO_TOKEN', | ||
| E_INVALID_STS_CREDENTIAL: 'E_INVALID_STS_CREDENTIAL', | ||
| E_PROFILE_NOT_FOUND: 'E_PROFILE_NOT_FOUND', | ||
| E_RUNTIME_NOT_SUPPORTED: 'E_RUNTIME_NOT_SUPPORTED', | ||
| E_SSO_SESSION_NOT_FOUND: 'E_SSO_SESSION_NOT_FOUND', | ||
| E_SSO_TOKEN_EXPIRED: 'E_SSO_TOKEN_EXPIRED', | ||
| E_STS_CREDENTIAL_EXPIRED: 'E_STS_CREDENTIAL_EXPIRED', | ||
| E_SSO_TOKEN_SOURCE_NOT_SUPPORTED: 'E_SSO_TOKEN_SOURCE_NOT_SUPPORTED', | ||
| E_MFA_REQUIRED: 'E_MFA_REQUIRED', | ||
| E_TIMEOUT: 'E_TIMEOUT', | ||
| E_UNKNOWN: 'E_UNKNOWN', | ||
| E_CANCELLED: 'E_CANCELLED', | ||
|
|
@@ -47,10 +55,20 @@ export class AwsResponseError extends ResponseError<AwsResponseErrorData> { | |
| } | ||
|
|
||
| // listProfiles | ||
| export type ProfileKind = 'Unknown' | 'SsoTokenProfile' | ||
| export type ProfileKind = | ||
| | 'Unknown' | ||
| | 'SsoTokenProfile' | ||
| | 'IamCredentialsProfile' | ||
| | 'IamSourceProfileProfile' | ||
| | 'IamCredentialSourceProfile' | ||
| | 'IamCredentialProcessProfile' | ||
|
|
||
| export const ProfileKind = { | ||
| SsoTokenProfile: 'SsoTokenProfile', | ||
| IamCredentialsProfile: 'IamCredentialsProfile', | ||
| IamSourceProfileProfile: 'IamSourceProfileProfile', | ||
| IamCredentialSourceProfile: 'IamCredentialSourceProfile', | ||
| IamCredentialProcessProfile: 'IamCredentialProcessProfile', | ||
| Unknown: 'Unknown', | ||
| } as const | ||
|
|
||
|
|
@@ -64,6 +82,18 @@ export interface Profile { | |
| settings?: { | ||
| region?: string | ||
| sso_session?: string | ||
| aws_access_key_id?: string | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it make sense to separate Profile in to a "superset" type which consists of separate types SsoProfile and CredentialProfile?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not if profiles can be any combination of profile types. Making these fields optional would allow the combined profile type to be decided at runtime and checked using the profile service's current duck typer.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It might still make sense to separate the settings types while keeping them optional for better readability. Right now it's hard to determine which settings are for which type of profile
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd recommend reaching out to connorstw@ here as he is working on a related project the generate client code from an API definition for Flare. As that would be the primary consumer of this information, there may be better ways to capture it than information-only interfaces. |
||
| aws_secret_access_key?: string | ||
| aws_session_token?: string | ||
| role_arn?: string | ||
| role_session_name?: string | ||
| credential_process?: string | ||
| credential_source?: string | ||
| source_profile?: string | ||
| mfa_serial?: string | ||
| external_id?: string | ||
| credential_cache?: string | ||
| credential_cache_location?: string | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -218,6 +248,56 @@ export const getSsoTokenRequestType = new ProtocolRequestType< | |
| void | ||
| >('aws/identity/getSsoToken') | ||
|
|
||
| // getIamCredential | ||
| export type IamCredentialId = string // Opaque identifier | ||
|
|
||
| export interface GetIamCredentialOptions { | ||
| callStsOnInvalidIamCredential?: boolean | ||
| validatePermissions?: boolean | ||
| } | ||
|
|
||
| export const getIamCredentialOptionsDefaults = { | ||
| callStsOnInvalidIamCredential: true, | ||
| validatePermissions: true, | ||
| } satisfies GetIamCredentialOptions | ||
|
|
||
| export interface GetIamCredentialParams { | ||
| profileName: string | ||
| options?: GetIamCredentialOptions | ||
| } | ||
|
|
||
| export interface GetIamCredentialResult { | ||
| id: IamCredentialId | ||
| credentials: IamCredentials | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was encrypting these in-flight considered or discussed with AppSec similar to SSO tokens?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The credentials are encrypted in-flight using this code, but I have not discussed this with AppSec.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep, trying to recall the reasoning for the additional encryption for SSO tokens, but this is fine for now. |
||
| updateCredentialsParams: UpdateCredentialsParams | ||
| } | ||
|
|
||
| export const getIamCredentialRequestType = new ProtocolRequestType< | ||
| GetIamCredentialParams, | ||
| GetIamCredentialResult, | ||
| never, | ||
| AwsResponseError, | ||
| void | ||
| >('aws/identity/getIamCredential') | ||
|
|
||
| // getMfaCode | ||
| export interface GetMfaCodeParams { | ||
| mfaSerial: string | ||
| profileName: string | ||
| } | ||
|
|
||
| export interface GetMfaCodeResult { | ||
| code: string | ||
| } | ||
|
|
||
| export const getMfaCodeRequestType = new ProtocolRequestType< | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How does this work?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR, the identityService will request an MFA code from the language client if getIamCredential needs to call AssumeRole and the parent credentials' permission to assume roles is locked behind MFA. This assumes the language clients will implement a handler for getMfaCode.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I'll review in more detail in that PR when it is posted. |
||
| GetMfaCodeParams, | ||
| GetMfaCodeResult, | ||
| never, | ||
| AwsResponseError, | ||
| void | ||
| >('aws/identity/getMfaCode') | ||
|
|
||
| // invalidateSsoToken | ||
| export interface InvalidateSsoTokenParams { | ||
| ssoTokenId: SsoTokenId | ||
|
|
@@ -236,6 +316,23 @@ export const invalidateSsoTokenRequestType = new ProtocolRequestType< | |
| void | ||
| >('aws/identity/invalidateSsoToken') | ||
|
|
||
| // invalidateStsCredential | ||
| export interface InvalidateStsCredentialParams { | ||
| profileName: string | ||
| } | ||
|
|
||
| export interface InvalidateStsCredentialResult { | ||
| // Intentionally left blank | ||
| } | ||
|
|
||
| export const invalidateStsCredentialRequestType = new ProtocolRequestType< | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the plan for implementing this (i.e. what API(s) will you call)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR, invalidateStsCredential will delete the credential from the cache folder using fs unlink.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I'll review in more detail in that PR when it is posted. |
||
| InvalidateStsCredentialParams, | ||
| InvalidateStsCredentialResult, | ||
| never, | ||
| AwsResponseError, | ||
| void | ||
| >('aws/identity/invalidateStsCredential') | ||
|
|
||
| // ssoTokenChanged | ||
| export type Expired = 'Expired' | ||
| export type Refreshed = 'Refreshed' | ||
|
|
@@ -255,3 +352,20 @@ export interface SsoTokenChangedParams { | |
| export const ssoTokenChangedRequestType = new ProtocolNotificationType<SsoTokenChangedParams, void>( | ||
| 'aws/identity/ssoTokenChanged' | ||
| ) | ||
|
|
||
| // stsCredentialChanged | ||
| export type StsCredentialChangedKind = Refreshed | Expired | ||
|
|
||
| export const StsCredentialChangedKind = { | ||
| Expired: 'Expired', | ||
| Refreshed: 'Refreshed', | ||
| } as const | ||
|
|
||
| export interface StsCredentialChangedParams { | ||
| kind: StsCredentialChangedKind | ||
| stsCredentialId: IamCredentialId | ||
| } | ||
|
|
||
| export const stsCredentialChangedRequestType = new ProtocolNotificationType<StsCredentialChangedParams, void>( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the implementation plan for this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this PR, stsCredentialChanged will be used similarly to ssoTokenChanged. It will fire a notification whenever an IAM credential obtained from AssumedRole is refreshed.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I'll review in more detail in that PR when it is posted. |
||
| 'aws/identity/stsCredentialChanged' | ||
| ) | ||
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.
You should be reading and writing credentials into the shared config file ~/.aws/credentials. You should use E_CANNOT_READ_SHARED_CONFIG (and the WRITE variation) for these errors. STS is just a service to retrieve and only one way of getting IAM credentials. The naming convention here should be "IAM" instead of "STS".
Uh oh!
There was an error while loading. Please reload this page.
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.
According to the AWS CLI docs, credentials obtained from AssumeRole (which Flare will call if the profile contains a role_arn and some credential source) are cached in ~/.aws/cli/cache, while all other IAM credentials live in ~/.aws/credentials. Should we still only consider ~/.aws/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.
Good catch, I'll send you an SEP offline that goes into more detail and should be considered canon.
Uh oh!
There was an error while loading. Please reload this page.
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 that case, I think the naming convention can stay as "STS". Technically, the SEP uses the term "temporary credentials", but "STS" is more concise and distinct (and can't be confused with SSO tokens which are also temporary). The cache only stores credentials obtained from STS AssumeRole and not any other IAM credential, so an "STS" prefix fits.