-
Notifications
You must be signed in to change notification settings - Fork 434
feat: add support for metrics to track sts calls made to disabled reg… #906
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
base: master
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dheeraj-coding The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Welcome @dheeraj-coding! |
Hi @dheeraj-coding. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
instanceRegion = instanceRegionOutput.Region | ||
} | ||
|
||
acctClient := account.NewFromConfig(cfg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need additional permissions which could be breaking change. Is there a need for this, can we get any information based on failure code from sts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Today, such cross region calls do not fail, but they succeed as the bearer token that we validate is simply a presignedURL. Just tried reproducing this behavior locally,
- Use account A with region HKG
$ aws-iam-authenticator token -i 126db5ea-e932-4251-b738-818fee4ed20c --role arn:aws:iam::509399613853:role/Admin --token-only --region ap-east-1
- Use the generated token and validate the with credentials exported for account B
$ aws-iam-authenticator verify -t <token> -i 126db5ea-e932-4251-b738-818fee4ed20c
Response:
&{ARN:arn:aws:sts::509399613853:assumed-role/Admin/aws-go-sdk-1756362745475065571 CanonicalARN:arn:aws:iam::509399613853:role/Admin AccountID:509399613853 UserID:AROAXNGUVIGO4AYZOL27A SessionName:aws-go-sdk-1756362745475065571 AccessKeyID:ASIAXNGUVIGOZP6UQQZD}
There is no error from STS to enforce this behavior. I could make it fail silently just like when imds clients are unavailable ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
iam roles are global, is there a reason we think token from one region should not be allowed in other region? We have separation at the partition level currently.
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What this PR does / why we need it:
authentication token is composed of a presigned URL to an STS endpoint. This sts endpoint is simply
curl
ed to verify token. There is a possibility that a token from account A with supported regions as HKG, can request authentication against a cluster in account B opted into TLV region.This behavior breaks data regionalization principle tenets, we must add validations to fail such requests automatically, but this change would be a breaking change to ensure we enforce this behavior in a soft manner we want to track the different number of requests that perform this invalid cross region calls. This PR contains changes necessary to add a new prometheus metric for such cross region calls.
Testing
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #