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

AWS authentication using OIDC web identity token #886

Closed
Klohto opened this issue Feb 22, 2021 · 13 comments · Fixed by #1209
Closed

AWS authentication using OIDC web identity token #886

Klohto opened this issue Feb 22, 2021 · 13 comments · Fixed by #1209

Comments

@Klohto
Copy link

Klohto commented Feb 22, 2021

WAL-G doesn't support native AWS authentication using OIDC web identity token and role arn environmental variables, AWS_WEB_IDENTITY_TOKEN_FILE and AWS_ROLE_ARN.

This is mainly useful on k8s, if you're using IAM roles linked with Service Accounts

This feature is natively supported by the GO AWS-SDK aws/aws-sdk-go#2667 and referenced even in the documentation


The current workaround is to assume web identity using aws sts assume-role-with-web-identity --role-arn $AWS_ROLE_ARN --role-session-name wal-g --web-identity-token file://$AWS_WEB_IDENTITY_TOKEN_FILE and parse the JSON output to export variables necessary for WAL-G. Not really ideal if the SDK supports it out of the box.
AWSCLI (v2) is pretty expensive for container image as a 110MB dependency...

Happy to help with this issue, just need a little bit of direction on where should I look. I'm assuming it's https://github.com/wal-g/storages/blob/master/s3/session.go right? From a quick look, it looks like the problem is with the default Config handler. That might also explain the freezing problem I'm encountering (#246 (comment)).

Fixing this would cover #246 as well as many more modes that the SDK supports out of the box.

@x4m
Copy link
Collaborator

x4m commented Feb 22, 2021

Hi! Do I understand it right that you want to set some vars from config?
If so just add them here

"AWS_SHARED_CREDENTIALS_FILE": true,

@Klohto
Copy link
Author

Klohto commented Feb 22, 2021

Hello! 👋

Well, not exactly. This feature is for authenticating using AWS SDK's session flow.
Basically, you supply JWT token and AWS Role and the SDK takes care of requesting temporary credentials as well as refreshing them.

Right now, there is no other way how to let WAL-G authenticate other than by supplying ACCESS_KEY, SECRET_KEY, and AWS_SESSION_TOKEN, even though the support from SDK is there.

I'm not entirely sure if adding these two vars to config.go would help, shouldn't the SDK read them by itself from the environment?

@x4m
Copy link
Collaborator

x4m commented Feb 22, 2021

So we need to allow two settings and make S3 storage not complain about the absence of ACCESS_KEY, SECRET_KEY in presence of these settings?

@x4m
Copy link
Collaborator

x4m commented Feb 22, 2021

It seems to me that you are right and we need to pass these vars somewhere here https://github.com/wal-g/storages/blob/master/s3/session.go#L92

@Klohto
Copy link
Author

Klohto commented Feb 22, 2021

I think calling NewWebIdentityCredentials, passing the vars, and adding that to providers should do it.

By the way, wouldn't be better to call something like NewConfig and then merge it with the specific values from CLI? That way the SDK handles the default flow of setting the values (Environment Variables > Shared Credentials file > Shared Configuration file > EC2 Instance Metadata) and you don't have to set these things manually.
Because from what I understand, session.NewSession() should take care of everything, but with the current flow of modifying the Config, so many values are overwritten some ENVs won't be propagated.

@x4m
Copy link
Collaborator

x4m commented Feb 22, 2021

I'd suggest doing the feature and refactoring in separate PRs. They both seem to make some sense, but reviewing the feature would be much easier.

@Klohto
Copy link
Author

Klohto commented Feb 22, 2021

Agreed 👍 I will see what can I do :)

@mathieusr
Copy link

Hello,

Any update on this subject ?

Thanks

@x4m
Copy link
Collaborator

x4m commented Jun 3, 2021

dunno, I hadn't seen much related code. Anyone willing to contribute implementation would be welcome :)

@mathieusr
Copy link

mathieusr commented Jun 7, 2021

Hello,

Thanks for your answer.

We can work on this issue. Base on the previous comments few changes are needed.
But before starting working on it, does anyone start to work on this issue to avoir duplicate pull request.

Thanks

@alexsanfran
Copy link

Hi @mathieusr just curious if there were any developments in this area?

@x4m
Copy link
Collaborator

x4m commented Nov 12, 2021

We definitely need to move this forward. Is anyone still interested in sending PR?

@sebastianwebber
Copy link

We definitely need to move this forward. Is anyone still interested in sending PR?

I'm.

Checking how to test this locally before sending any code. :D

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 a pull request may close this issue.

5 participants