Allow AWS Assume Role

[ekristen]

What does this PR change? What problem does it solve?

• Adds support for Assuming a Role in AWS

Resolves #4472

It adds the ability to allow the discovered credentials for AWS to assume another role and use those credentials for the actual run of restic.

Example: Personal IAM user (ekristen) has access to ec2 console but not s3 backup location but has the right to assume the role, backup, backup has full access to the s3 bucket, so this allows restic to assume a role.

Was the change previously discussed in an issue or on the forum?

Not really, opened #4472

Testing this Pull Request

Using the files from this gist create the folllowing, a test iam user, test iam role, and test s3 bucket. https://gist.github.com/ekristen/e20a976653d0624957add5b4d2ef64ba

1. Create a set of IAM creds for the test user and export them using AWS_ACCESS_KEY_ID and AWS_SECRET_ACCESS_KEY
2. Export AWS_ASSUME_ROLE_ARN with the value of your test iam role arn
3. Run restic against your newly created s3 bucket.

Note:* for more advanced use cases you can pass AWS_ASSUME_ROLE_SESSION_NAME and AWS_ASSUME_ROLE_EXTERNAL_ID and use conditions to trust based on values. Additionally you can pass AWS_ASSUME_ROLE_POLICY which is a JSON document for an IAM policy, this can be used to set a STS policy to restrict the STS credentials to a specific path as an example.

Checklist

Note: tests -- there aren't extensive existing tests on authentication methods, there's not a framework to build off of that I could see to add tests.

• I have read the contribution guidelines.
• I have enabled maintainer edits.
• I have added tests for all code changes.
• I have added documentation for relevant changes (in the manual).
• There's a new file in changelog/unreleased/ that describes the changes for our users (see template).
• I have run gofmt on the code in all commits.
• All commit messages are formatted in the same style as the other commits in the repo.
• I'm done! This pull request is ready for review.

[konidev20]

Hey @ekristen,
Currently, AssumeRole APIs issue tokens for a maximum lifetime of 12 hours. Ref: AssumeRole

This solution will work only for long-running operations which can be completed within 12 hours. Do you have any idea as to how long-running operations on the restic repository are affected by this?

I am assuming, we would need some kind of background go routine which routinely checks for the expiry of the STS token and will renew the STS token and update the client.

[ekristen]

This is true. I wonder how often backups exceed 12 hours and use s3 and would use assume role?

Creating a go func that goes into a loop at expiration/2 and updates the current credentials would work. However back to my original question of how often the exceeding 12 hours would happen.

I see two options

1. Update docs for initial implementation to state that there's a limit of 12 hours with role assumption and cannot use if backups exceed.
2. Work on a for loop to renew credentials at half the expiration time to allow further calls to continue to work.

[MichaelEischer]

Using the aws-sdk in addition to minio is not an option.

From a quick look at the minio-go documentation, it also supports assuming a role: https://pkg.go.dev/github.com/minio/minio-go/[email protected]/pkg/credentials#STSAssumeRole . Please use that approach instead. That will likely also just fix the expiry problems.

[ekristen]

@MichaelEischer Why is it a no-go, it's a small part of the library? The minio doesn't support all the necessary options like passing an ExternalId. I can write a wrapper to support the expiration.

Other than the above, is the rest of it acceptable? The environment variable names etc?

[MichaelEischer]

Why is it a no-go, it's a small part of the library?

It increases the binary size by 1MB, seems to work for AWS only, feels completely out of place compared to the existing credentials methods and so on. Mixing two SDKs that are both able to access S3 just feels very wrong.

The minio doesn't support all the necessary options like passing an ExternalId. I can write a wrapper to support the expiration.

I'd prefer to ask whether the minio developers are willing to add support for ExternalId to minio-go . There are already two AWS specific options for STSAssumeRole, That would be a 10-20 lines change on the minio side along with proper integration in the library, compared to the brittle 100+ lines implementation here.

[ekristen]

Gotcha. I will make the necessary changes and pull requests.

[ekristen]

@MichaelEischer changes as requested, however it depends upon changes to minio-go/v7 which I've opened a PR for here minio/minio-go#1887 -- I'm happy to leave my fork up if you'd like to merge as is, but if you'd like to wait and see if the PR can get merged mainstream, we can leave this open until that PR is merged and upgrade the dependency accordingly.

I've built a docker image with these changes and it is available here.

ghcr.io/ekristen/restic:0.16.0-ek1-minio

[MichaelEischer]

Thanks for updating the PR. Yes, I'd prefer to wait until the minio PR is merged.

[ekristen]

@MichaelEischer the minio PR was just merged minio/minio-go#1887 (comment) -- do we need to wait until it hits a release? If not I can update the branch to use the specific commit.

[ekristen]

Working on fully testing the changes right now, but for anyone else that wants to here's the docker image.

ghcr.io/ekristen/restic:0.16.0-ek2

[ekristen]

Had to make a tweak to the region code to make things align properly with assuming roles properly.

ghcr.io/ekristen/restic:0.16.0-ek3

[MichaelEischer]

@MichaelEischer the minio PR was just merged minio/minio-go#1887 (comment) -- do we need to wait until it hits a release? If not I can update the branch to use the specific commit.

I'd prefer to wait until the next version of the minio-go library is released, the next restic release will still take quite some time, so there's no need to rush it.

I'll try to have a closer look at the code in the next days/weeks.

[ekristen]

@MichaelEischer understood.

[ekristen]

@MichaelEischer thinking about this, I think we need to at least prefix these with RESTIC_ to avoid conflicts in the future.

[ekristen]

@MichaelEischer minio-go release a new version, this includes that update to make this PR work now.

I've also updated the env vars to be prefixed with RESTIC_ for the AWS side to prevent any possible collisions with other tooling.

Here's a build docker image for anyone that wants to test.

ghcr.io/ekristen/restic:0.16.0-ek5

[MichaelEischer]

@MichaelEischer minio-go release a new version, this includes that update to make this PR work now.

Thanks, as a heads-up I'm pretty busy at the moment, I probably won't be able to review the PR before Christmas, sorry.

[MichaelEischer]

Sorry for the long delay! I've squashed the commits (rebasing all the changes would have been too messy) and rebased them to the current master branch. That implicitly updates minio to v7.0.66.

I've added two remarks, please take a look. Other than that I'm happy with the changes.

Could you give the PR one final try to check that everything still works?

[ekristen]

Hey @MichaelEischer all good, glad you found time to come back around to this! I made a couple comments on your comments.

I'll go build and test this out and reply once done.

[ekristen]

Everything seems to be working for me just fine.

[MichaelEischer]

Thanks for testing, then let's merge it!