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

update S3 pre-signed credentials logic #10265

Merged
merged 4 commits into from
Feb 19, 2024
Merged

update S3 pre-signed credentials logic #10265

merged 4 commits into from
Feb 19, 2024

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Feb 17, 2024

Motivation

Currently in LocalStack, in order to use pre-signed URLs while validating the signature (with S3_SKIP_SIGNATURE_VALIDATION set to false), the user needs to set hardcoded credentials: test/test.

This is due to the fact that we did not retrieve the Secret Access Key linked to the sent Access Key Id, which is needed to take the incoming request server side, sign it as well and compare with the received signature.

We also used the TEST_AWS* credentials to generate the signature server side, which was a way for our test to always work as those are hardcoded.

Changes

  • Implement a way to retrieve the Secret Access Key of the user by directly accessing the moto backend, as there are no public API to retrieve the secret access key for obvious security reasons
  • however, as we don't create IAM users by default in LocalStack, we still need to rely on the hardcoded value of the secret access key to be test, so that we can properly validate pre-signed URL even the user does not provide credentials that actually exists in LocalStack.
  • update the tests to use the hardcoded S3 constants instead of the test credentials

Note: as we can use random Access Keys in LocalStack without having them being created in IAM, most of the time we won't be able to retrieve the secret key, and will need to fallback to the test secret access key.

However, we now support passing proper existing credentials and having the handler properly validate them.

\cc @dfangl as I'm directly accessing IAM internals, not sure what is your opinion on this, and if it can be done any other way?

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Feb 17, 2024
@bentsku bentsku self-assigned this Feb 17, 2024
Copy link

github-actions bot commented Feb 17, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 11s ⏱️
387 tests 336 ✅  51 💤 0 ❌
774 runs  672 ✅ 102 💤 0 ❌

Results for commit 792008e.

♻️ This comment has been updated with latest results.

@coveralls
Copy link

coveralls commented Feb 17, 2024

Coverage Status

coverage: 83.872% (+0.01%) from 83.86%
when pulling 792008e on s3-presigned-upgrade
into db3c42f on master.

Copy link

github-actions bot commented Feb 17, 2024

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 22m 9s ⏱️ +52s
2 643 tests +1  2 396 ✅ +1  247 💤 ±0  0 ❌ ±0 
2 645 runs  +1  2 396 ✅ +1  249 💤 ±0  0 ❌ ±0 

Results for commit 792008e. ± Comparison against base commit db3c42f.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review February 19, 2024 11:15
"""
from moto.iam.models import AccessKey, iam_backends

account_id = get_account_id_from_access_key_id(access_key_id)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Is it possible to use STS:GetSessionToken at this point and obtain the access key ID + secret access key programatically?

Copy link
Contributor Author

@bentsku bentsku Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so, I think GetSessionToken returns a set of temporary credentials which will be then be different than what was used to signed the request on the client side.

I think you would also need to create the client for STS with the Access Key ID and the Secret Access Key of the client used to sign the request?

Copy link
Contributor Author

@bentsku bentsku Feb 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made me think of @dfangl hackathon, where he must had to implement a similar logic to validate the signature of every request:
f758364

master...hackathon-sig

And I guess it was the same issue, about having credentials that are set in LocalStack.

So I'm just not sure about the reverse of the access key id <-> account id to access the store, or if I need to iterate in the same way.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be no API for getting the secret key, so I think this is the best way for now :)

@viren-nadkarni viren-nadkarni added this to the 3.2 milestone Feb 19, 2024
Copy link
Member

@viren-nadkarni viren-nadkarni left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this @bentsku, another step towards improving our test strategy 🙌

Copy link
Member

@dfangl dfangl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only really looked at the IAM/STS stuff, but looks good! Sadly there is no better way as far as I know :)

@bentsku bentsku merged commit 6880b98 into master Feb 19, 2024
35 checks passed
@bentsku bentsku deleted the s3-presigned-upgrade branch February 19, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants