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 Credentials cache should be configurable and flexible instead of hard coded to 1 hour #36769

Closed
aabchoo opened this issue Oct 22, 2024 · 13 comments
Labels
area/aws stale stalebot believes this issue/PR has not been touched recently

Comments

@aabchoo
Copy link

aabchoo commented Oct 22, 2024

Title: AWS Credentials cache should be configurable and flexible instead of hard coded to 1 hour

Description:

Describe the desired behavior, what scenario it enables and how it
would be used.

AWS access id, secret key, and session tokens read from AWS credential file are cached for 1 hour. This can result in stale credentials due to caching happening before session tokens are refreshed, or when an invalid token is cached.

The desired behavior is split into two parts:

  1. Allow users to configure the caching TTL time
  2. If the AWS credential file has been modified, clear the cache and read the keys/tokens from the updated file

Behaviour #1 allows us to shorten/extend the cache TTL to match the timeframe our tokens are valid for

Behaviour #2 allows us to update credential file adhoc and have those credentials be used by EnvoyProxy without needing to restart the application or wait for the cache TTL

[optional Relevant Links:]

Any extra documentation required to understand the issue.

Code where TTL is hardcoded

@aabchoo aabchoo added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Oct 22, 2024
@mattklein123 mattklein123 added area/aws and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Oct 22, 2024
@aabchoo
Copy link
Author

aabchoo commented Oct 23, 2024

The two ways that I've thought about implementing desired behavior number 2 is:

  • Process that watches for a signal/event when the credential file has been modified (issue arises when signal is missed)
  • Loop that will check every x seconds to check if the file has been updated (redundant checks)

In the event of a update, a flag fileUpdated will be set to true, and needsRefresh will use that value and evaluate to true (regardless of cache time)

Would appreciate opinions on this!

@mathetake
Copy link
Member

i think this request sounds reasonable

cc @suniltheta @nbaws

@nbaws
Copy link
Contributor

nbaws commented Oct 23, 2024

this seems reasonable. i will take a look at implementing something along these lines after i've finished curl deprecation patch.

@nbaws
Copy link
Contributor

nbaws commented Nov 2, 2024

@aabchoo would item 2 in your list be sufficient - ie we would reread credentials regardless of the current expiration time if the underlying credentials file has been modified?

I have a PR for item 2 ready. However item 1 requires an API change and will need some more thinking as to the best place to implement.

@nbaws
Copy link
Contributor

nbaws commented Nov 12, 2024

@aabchoo ping

@aabchoo
Copy link
Author

aabchoo commented Nov 14, 2024

Hi @nbaws, apologies for the delayed response 🙇 I completely missed this.

Resolving item 2 will solve the problem! The plan was to have an external service refresh the credentials just prior to the credential expiration. I can live without item 1.

Thank you for the help and apologies again for missing the previous tags!

@nbaws
Copy link
Contributor

nbaws commented Nov 14, 2024

no problem :) i will submit the PR for this #2 shortly and i may look at #1 as part of another PR

Copy link

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 14, 2024
@mathetake
Copy link
Member

nostale

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 16, 2024
mattklein123 pushed a commit that referenced this issue Jan 2, 2025
…oad (#37834)

Commit Message: aws: update credential provider proto and support
credential file reload
Additional Description: Allows for customisation of the credential
provider settings, using the credential provider proto added in #36217
Risk Level: Low
Testing: Unit
Docs Changes: Updated
Release Notes: Updated
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] Addresses #36769 and partially addresses #37432 
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Nigel Brittain <[email protected]>
@nbaws
Copy link
Contributor

nbaws commented Jan 2, 2025

@aabchoo Please see the last example here https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/aws_request_signing_filter#example-configuration to see how you can implement file based reload for your credentials.
This should allow you to dynamically refresh these credential files as often as necessary.

Note that the envoy file watching implementation will require you to move the file into the watched directory, not just modify it inline.

Let me know if this addresses your use case.

@aabchoo
Copy link
Author

aabchoo commented Jan 3, 2025

We have pivoted off using the credential files recently, but this does address what we were intending to do. Thanks you for taking the time to implement this!

Copy link

github-actions bot commented Feb 3, 2025

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 3, 2025
Copy link

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aws stale stalebot believes this issue/PR has not been touched recently
Projects
None yet
Development

No branches or pull requests

4 participants