-
-
Notifications
You must be signed in to change notification settings - Fork 226
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
Credential chain #746
Credential chain #746
Conversation
We want the Env' withAuth in the final position because `authenticate` will eventually take a function of type like: `Env' withAuth -> m Env`
CC:
|
Question for the floor: would it be worth adding pattern synonyms where there's a compatible new name? |
I like it. |
Another question: Should we detect infinite loops in the AWS config files? The other SDKs do, and we probably should. |
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 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.
Changes look good, I like the new design!
We should (eventually) - but I don't think it should get in the way of getting this released. |
I think it should be quick (execStateT while traversing the profile stuff), so I'll try to do it on this PR. Most other stuff should be moved into separate issues, which I'll do before merging this. |
We pin to hashable-1.3.4.1 as it's the last 1.3.x.x version and stack snapshots for GHC 8.10.7 still don't have hashable >=1.3.4.0 (which we need for amazonka-dynamodb and amazonka-dynamodb-streams)n Also remove references to .hs-boot files which no longer exist.
faee1a7
to
d33c559
Compare
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 some quick things I noticed while skimming. Nice work man.
d33c559
to
f8a1ebe
Compare
f8a1ebe
to
57933fb
Compare
Going to leave this bake for a few days longer now that I've updated work code to run on it, but I think there's nothing stopping a merge early next week. |
This has been working well for as at work, so I'm going to merge it. |
Rewrite the authentication code to support something like an explicit "credential chain", in the vein of the official AWS SDKs. The
Credentials
data type is removed in favour of authentication functions of typeEnv' withEnv -> m Env
orEnv -> m Env
; that's a function that takes an inputEnv'
(either indifferent to authentication or in the case ofsts:AssumeRole
, requiring it), and produces a new outputEnv
. This allows library clients much greater control over how they get the first set of access keys - if AWS releases some new way to acquire credentials, it's possible to plug that in much more easily than before.This PR also breaks up the authentication into a bunch of separate modules. This has the nice side-effect of removing the
.hs-boot
files which came in when we started supportingsts:AssumeRoleWithWebIdentity
.On the library consumer side, the new
Amazonka.Auth.STS.fromAssumedRole
makes it easy to spin off a newEnv
under an assumed role, and the parser for credentials/config files has been rewritten to support many authentication methods supported by the official SDKs.This also provides a workaround for the VPC issues in #271 - it's possible to write something like
newEnv fromDefaultInstanceProfile
and have amazonka skip theisEC2
check. (The IMDS client connects by IP; only theisEC2
check useshttp://instance-data
.)Migration for most users will be to replace
newEnv Discover
withnewEnv discover
.Recommend reviewing commit-by-commit.
Remaining work:
profile
prefix in config as "works, but not supported"~/.aws/credentials
exists but~/.aws/config
doesn't.Closes #514
Closes #476
Closes #629