Fix issue with dry-run access_request.spec.expires overwrite#59761
Merged
Fix issue with dry-run access_request.spec.expires overwrite#59761
Conversation
r0mant
reviewed
Sep 30, 2025
| req.SetRequestReason("Dry run, this request will not be created. If you see this, there is a bug.") | ||
| if err := tc.WithRootClusterClient(ctx, func(clt authclient.ClientI) error { | ||
| req, err = clt.CreateAccessRequestV2(ctx, req) | ||
| _, err := clt.CreateAccessRequestV2(ctx, req) |
Collaborator
There was a problem hiding this comment.
What is the effect of not using the request returned by dry run? Or to phrase it differently, do you know why we're using this request in the first place and why does it return request with a wrong TTL set?
Contributor
Author
There was a problem hiding this comment.
The reason why we are making the requests is best described in the original PR description #13586
After some digging I don't think there is any other consequence than computing the requestable roles for the resource ID. So I modified the code to copy the calculated roles back to the original request.
kopiczko
force-pushed
the
kopiczko/access_req_ttl_bug__client_side
branch
from
ba11b34 to
5c0a0d0
Compare
github-actions
bot
added
size/sm
tsh
r0mant
approved these changes
Oct 2, 2025
Collaborator
|
@nklaassen Do you mind taking a look at this as well since you worked on this part of the code a couple of years ago? |
nklaassen
approved these changes
Oct 2, 2025
This was referenced Oct 2, 2025
kopiczko
force-pushed
the
kopiczko/access_req_ttl_bug__client_side
branch
from
5c0a0d0 to
c5df613
Compare
kopiczko
force-pushed
the
kopiczko/access_req_ttl_bug__client_side
branch
from
c5df613 to
72fd243
Compare
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue #59606
After
access_request.spec.max_durationfield was introduced we started to set thespec.expiryto themax_durationif the conditions are met (i.e. max_duration is set and any of the roles allowing the requested resource has .spec.allow.request.max_duration set).This that if the conditions for setting
max_durationare met,access_request.spec.expirymay not (and usually is not) shorter than the current login session duration.This in turn means that if such a request goes through the dry-run call first, the
spec.expiryis set tospec.max_durationand this validation fails.This PR is a little bit hacky fix dropping the request from the dry-run as there is no easy way out of this situation on the server side. The proper fix should be covered with #46001 (part 2) where we should revisit the whole access request spec and make things less confusing than they are right now.
Backports:
changelog: Fix the issue with automatic access requests for
tsh sshwhenspec.allow.request.max_durationis set on the requester role.