-
Notifications
You must be signed in to change notification settings - Fork 84
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
[BUG/FIX] Azure RBAC fix refresh token logic #398
Conversation
b5f3891
to
1ee71dc
Compare
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
e4120ce
to
41b5caa
Compare
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
41b5caa
to
c35f02d
Compare
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
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.
LGTM, I'll let you decide if you want to apply my comment about the returned payload from MSI adapter
Also rename the PR with [BUG/FIX] in the title |
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
LGTM overall, PTAL at couple of comments on UTs |
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.
LGTM on Arc Side but need review on AKS Side since we are touching the token interface.
Signed-off-by: Sai Sankar Gochhayat <saisankargochhayat@gmail.com>
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.
LGTM
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.
LGTM
This PR aims to fix the refresh token logic for Azure RBAC.
The existing refresh token logic relies on the
expires_in
value from the token response -{"access_token":"","expires_in":"86700","refresh_token":"","expires_on":"1732881796","not_before":"1732795096","resource":"https://management.azure.com","token_type":"Bearer"}
However in the case when the token is not fresh and returned from cache, the
expires_in
value misleads Guard to think the token doesn't need refreshing since it adds the current_time + expires_in to set the new expiry time.In this PR we directly use the
expires_on
to set the expiry time accurately.The PR also increases the tokenExpiryDelta to 5 minutes to ensure the token is refreshed 5 mins earlier than actual expiry. This is done on both AuthZ (rbac.go) and AuthN (graph.go) flow.
Removed
expires_in
to be returned from msi_adapter in favor ofexpires_on
Using the current implementation guard now sets the expires property correctly, i.e. knows it expires at 2024-11-29 21:05:56 +0000 UTC in the below example rather than doing - current-time + 86700
Guard log -
Tested KAP flow -
RBAC non proxy flow -