Skip to content
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

Add the ability to make unsigned requests #708

Merged
merged 7 commits into from
Nov 16, 2021

Conversation

endgame
Copy link
Collaborator

@endgame endgame commented Nov 6, 2021

Add the ability to make unsigned requests. Tested by performing s3:GetObject against a bucket whose policy allowed access from "Principal": "*".

Ping @K0Te , since this may be a better way to handle the sts:AssumeRoleWithWebIdentity stuff. It also lets us work towards an amazonka/amazonka-auth split down the line.

Closes #702
Closes #602

lib/amazonka/src/Amazonka.hs Outdated Show resolved Hide resolved
@endgame endgame mentioned this pull request Nov 6, 2021
lib/amazonka/src/Amazonka.hs Outdated Show resolved Hide resolved
@endgame
Copy link
Collaborator Author

endgame commented Nov 14, 2021

I don't consider this critical for the 2.0 RC. If there's nothing else we need on that front, we should get it out the door first. Maybe if things are quiet during stabilisation we do this?

The urgent question, I guess: are you happy with the amazonka-core/amazonka-auth split we talked about previously?

  • Have two packages so amazonka-auth can depend on amazonka-sts without creating a dependency cycle;
  • amazonka re-exports from both for convenience and to avoid too much API breakage.

If so, then I think get the pieces lined up to make that possible (which shouldn't be much).

As for avoiding partiality in the API: one way would be to add some type-level tags that track whether or not an Env has access to credentials. If I remove NoCredentials from the Credentials type and instead provide a separate helper function, we can present a safe interface to callers. What do you think of that?

@brendanhay
Copy link
Owner

The current dependency graph is:

                         ┌─────► amazonka-<service> ──────┐
                         │                                │
    amazonka-core ───────┤                                ├──────► application
                         │                                │
                         └─────► amazonka ────────────────┘

And I understand your proposal as:

                       ┌─────► amazonka-<service> ─────────────────────────────────────┐
                       │                                                               │
  amazonka-core ───────┤                                                               ├──────► application
                       │                                                               │
                       └─────► amazonka-sts ────► amazonka-auth ──────► amazonka ──────┘

I'm probably misunderstanding, but is the additional library really necessary?

Regarding Env, your separate helper function proposal is preferable.

@endgame endgame force-pushed the unsigned-requests branch 2 times, most recently from 39feb9e to 959fe7c Compare November 15, 2021 11:31
@endgame
Copy link
Collaborator Author

endgame commented Nov 15, 2021

I'm probably misunderstanding, but is the additional library really necessary?

It is not. For some reason I thought generated services depended on amazonka, not amazonka-core.

After rewriting the Env type to have a withAuth type parameter, a lot of simplifications fell out. and made sts:AssumeRoleWithWebIdentity possible. I did have to break a couple of import loops with .hs-boot files but I think that if you're using AWS calls to get AWS creds, that's going to be the price of admission.

@K0Te - Can you please test it when you get a chance?

@endgame endgame force-pushed the unsigned-requests branch 2 times, most recently from 8d2e1c1 to b7a7d5c Compare November 15, 2021 11:34
@endgame
Copy link
Collaborator Author

endgame commented Nov 16, 2021

I do wonder if we should be parameterising the FromWebIdentity constructor some, so that the token doesn't have to be provided via envvars.

@endgame
Copy link
Collaborator Author

endgame commented Nov 16, 2021

I do wonder if we should be parameterising the FromWebIdentity constructor some, so that the token doesn't have to be provided via envvars.

But OTOH people can create their own EnvNoAuth and make the call themselves if they don't want to use our approximation to the default credential provider chain.

@brendanhay
Copy link
Owner

For some reason I thought generated services depended on amazonka, not amazonka-core.

You were correct up until I fat-fingered 524fde6 on main recently.

But OTOH people can create their own EnvNoAuth and make the call themselves if they don't want to use our approximation to the default credential provider chain.

That seems acceptable - It looks good to me, go ahead and merge whenever you're ready!

@endgame
Copy link
Collaborator Author

endgame commented Nov 16, 2021

Fixing a couple of documentation nits and giving @Fuuzetsu some well-deserved credit also, then will merge as soon as CI passes.

@endgame
Copy link
Collaborator Author

endgame commented Nov 16, 2021

CI takes ~3h and I built this locally. Hitting the merge button now.

@endgame endgame merged commit aeecded into brendanhay:main Nov 16, 2021
@endgame endgame deleted the unsigned-requests branch November 16, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic handling of AWS_ROLE_ARN and AWS_WEB_IDENTITY_TOKEN_FILE
2 participants