-
-
Notifications
You must be signed in to change notification settings - Fork 227
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
Implement Auth.SSO #757
Implement Auth.SSO #757
Conversation
Attempting to use this when an expired
Is that acceptable, or should we catch it and emit something clearer? |
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.
Thanks for PRing this; I think it will be a great help to a lot of people. I've picked a couple of nits, but I think there's a more fundamental restructuring that needs to happen first.
The decision to try SSO as part of looking up a config profile should happen in ConfigFile.hs
, where it should be treated as "just another auth method". This probably means extending the ConfigProfile
type (the parsers for the different profiles are in the code immediately above; you will probably want to make a new constructor for SSO):
amazonka/lib/amazonka/src/Amazonka/Auth/ConfigFile.hs
Lines 216 to 227 in 29e4888
data ConfigProfile | |
= -- | Recognizes @aws_access_key_id@, @aws_secret_access_key@, and | |
-- optionally @aws_session_token@. | |
ExplicitKeys AuthEnv | |
| -- | Recognizes @role_arn@ and @source_profile@. | |
AssumeRoleFromProfile Text Text | |
| -- | Recognizes @role_arn@ and @credential_source@. | |
AssumeRoleFromCredentialSource Text CredentialSource | |
| -- | Recognizes @role_arn@, @role_session_name@, and | |
-- @web_identity_token_file@. | |
AssumeRoleWithWebIdentity Text (Maybe Text) FilePath | |
deriving stock (Eq, Show, Generic) |
Then in this case
-expression, check for your new SSO constructor and in that branch, go looking for cached JSON files. For each one you find, call into the functions you provide in Amazonka.Auth.SSO
:
amazonka/lib/amazonka/src/Amazonka/Auth/ConfigFile.hs
Lines 116 to 140 in 29e4888
env' <- case cp of | |
ExplicitKeys authEnv -> | |
pure env {envAuth = Identity $ Auth authEnv} | |
AssumeRoleFromProfile roleArn sourceProfileName -> do | |
seenProfiles <- lift get | |
if sourceProfileName `elem` seenProfiles | |
then | |
let trace = reverse seenProfiles ++ [last seenProfiles] | |
textTrace = Text.intercalate " -> " trace | |
in liftIO | |
. Exception.throwIO | |
. InvalidFileError | |
$ "Infinite source_profile loop: " <> textTrace | |
else do | |
lift . modify $ (sourceProfileName :) | |
sourceEnv <- evalConfig sourceProfileName | |
fromAssumedRole roleArn "amazonka-assumed-role" sourceEnv | |
AssumeRoleFromCredentialSource roleArn source -> do | |
sourceEnv <- case source of | |
Environment -> fromKeysEnv env | |
Ec2InstanceMetadata -> fromDefaultInstanceProfile env | |
EcsContainer -> fromContainerEnv env | |
fromAssumedRole roleArn "amazonka-assumed-role" sourceEnv | |
AssumeRoleWithWebIdentity roleArn mRoleSessionName tokenFile -> | |
fromWebIdentity tokenFile roleArn mRoleSessionName env |
When implemented this way, it will get handled in the same way as all other config-file-based methods, which means this hack to set region will apply to it also:
amazonka/lib/amazonka/src/Amazonka/Auth/ConfigFile.hs
Lines 84 to 86 in 29e4888
lookupRegion <&> \case | |
Nothing -> env' | |
Just r -> env' {envRegion = r} |
Please also add an entry for this PR (including giving yourself credit!) to lib/amazonka/CHANGELOG.md
; once we have the names of the new SSO functions nailed down the big table of auth methods will need a new entry too.
Attempting to use this when an expired
{uuid}.json
is present results in:ServiceError (...snip...)
Is that acceptable, or should we catch it and emit something clearer?
I think it's acceptable for now; STS errors seem to be thrown in the same way. If we want to pull them into Auth errors in response to user complaints, that's fine but I don't think we have to hold up this PR to do it.
Thank you for the detailed review! I misunderstood some things, but I think I understand better now. I'll basically be adding support for seeing the The I'll be sure to format, bounds, and changelog once I get the new approach dialed in. I should be able to push another attempt tonight or tomorrow. On the topic of formatting, would you be interested in my Restyled App on this project? It has support for ormolu and I could add cabal-fmt (and anything else really) relatively quickly. |
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.
All right, this is looking much better, thanks for your efforts. There are a lot of comments in this review, but most of them should be easy to address.
OK, I think everything is resolved with the exception of the service definition.
I'd also like to cleanup the git History before merge, to isolate the feature from other pre/re-factors -- unless you happen to have a "Squash" policy (or some other preference). Do you? |
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.
Thanks very much. One last round of nitpicking, otherwise I'm pretty happy with how this looks. I'm also happy with your CHANGELOG entries, so thanks for updating the big table.
Here's how I see the next few days going:
- Leave sso: GetRoleCredentialsResponse_roleCredentials is required #759 open for a day or two in case someone objects.
- (Me) Merge sso: GetRoleCredentialsResponse_roleCredentials is required #759 and regenerate service bindings.
- (You) While waiting for sso: GetRoleCredentialsResponse_roleCredentials is required #759, rebase/rearrange commits until you're happy. I usually merge rather than squash in this repository, so if you can make the history look nice that'd be cool. But I also wouldn't lose sleep over it.
- (You) Rebase on top of
main
once the regeneratedamazonka-sso
is available, so you're not coding against an errantMaybe
; also remove the temporary stack files. - (Me) Merge this PR.
OK! I think this is done from my end. I will,
Then you can Approve/Merge. |
Reads sso-related configuration and uses a cached JWT (created via aws sso login) to receive credentials for the configured Account and Role. See https://docs.aws.amazon.com/cli/latest/userguide/cli-configure-sso.html
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.
Great, now we wait for the new version of service definitions. I don't expect anyone to object so I'll do them over the next couple of days.
The situation with service definitions and the generator is more complex than I first anticipated. I'm going to merge this now, as removing the |
Thanks again for your work and patience. |
Thank you for shepherding this through, this'll make using our amazonka-based projects so much easier. |
REVERT ME: Configure for Stack
I had trouble with the nix setup; this worked straight away. I can remove it
before merge.
Expose Auth.ConfigFile functions for re-use
The way ConfigFile works right now, I couldn't draw the perfect seams to
totally DRY what I'm doing; but this seems like a good 80/20.
Implement Auth.SSO
Huzzah!