Skip to content

AccessRequest: Stop using Expires as MaxDuration #59708

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.

Sign up for GitHub

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

Closed
kopiczko wants to merge 1 commit into from
Closed

AccessRequest: Stop using Expires as MaxDuration #59708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions api/types/access_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -965,20 +965,20 @@ func NewAccessRequestAllowedPromotions(promotions []*AccessRequestAllowedPromoti
}

// ValidateAssumeStartTime returns error if start time is in an invalid range.
func ValidateAssumeStartTime(assumeStartTime time.Time, accessExpiry time.Time, creationTime time.Time) error {
func ValidateAssumeStartTime(assumeStartTime time.Time, maxDuration time.Time, creationTime time.Time) error {
// Guard against requesting a start time before the request creation time.
if assumeStartTime.Before(creationTime) {
return trace.BadParameter("assume start time has to be after %v", creationTime.Format(time.RFC3339))
}
// Guard against requesting a start time after access expiry.
if assumeStartTime.After(accessExpiry) || assumeStartTime.Equal(accessExpiry) {
return trace.BadParameter("assume start time must be prior to access expiry time at %v",
accessExpiry.Format(time.RFC3339))
if assumeStartTime.After(maxDuration) || assumeStartTime.Equal(maxDuration) {
return trace.BadParameter("assume start time must be prior to max duration time at %v",
maxDuration.Format(time.RFC3339))
}
// Access expiry can be greater than constants.MaxAssumeStartDuration, but start time
// should be on or before constants.MaxAssumeStartDuration.
maxAssumableStartTime := creationTime.Add(constants.MaxAssumeStartDuration)
if maxAssumableStartTime.Before(accessExpiry) && assumeStartTime.After(maxAssumableStartTime) {
if maxAssumableStartTime.Before(maxDuration) && assumeStartTime.After(maxAssumableStartTime) {
return trace.BadParameter("assume start time is too far in the future, latest time allowed is %v",
maxAssumableStartTime.Format(time.RFC3339))
}
Expand Down
20 changes: 7 additions & 13 deletions lib/services/access_request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ func ApplyAccessReview(req types.AccessRequest, rev types.AccessReview, author U
req.SetReviews(append(req.GetReviews(), rev))

if rev.AssumeStartTime != nil {
if err := types.ValidateAssumeStartTime(*rev.AssumeStartTime, req.GetAccessExpiry(), req.GetCreationTime()); err != nil {
if err := types.ValidateAssumeStartTime(*rev.AssumeStartTime, req.GetMaxDuration(), req.GetCreationTime()); err != nil {
return trace.Wrap(err)
}
req.SetAssumeStartTime(*rev.AssumeStartTime)
Expand All @@ -514,7 +514,7 @@ func ApplyAccessReview(req types.AccessRequest, rev types.AccessReview, author U
req.SetPromotedAccessListName(rev.GetAccessListName())
req.SetPromotedAccessListTitle(rev.GetAccessListTitle())
}
req.SetExpiry(req.GetAccessExpiry())
req.SetExpiry(req.GetMaxDuration())
return nil
}

Expand Down Expand Up @@ -1364,15 +1364,9 @@ func (m *RequestValidator) validate(ctx context.Context, req types.AccessRequest
maxAccessDuration = sessionTTL
}

// This is the final adjusted access expiry where both max duration
// and session TTL were taken into consideration.
accessExpiry := now.Add(maxAccessDuration)
// Adjusted max access duration is equal to the access expiry time.
req.SetMaxDuration(accessExpiry)

// Setting access expiry before calling `calculatePendingRequestTTL`
// matters since the func relies on this adjusted expiry.
req.SetAccessExpiry(accessExpiry)

@r0mant r0mant Sep 29, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

What is the effect of not setting access expiry here anymore?

@kopiczko kopiczko Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, but I don't have a simple answer. The very short answer is: the effect of not setting this is after running CreateAccessRequestV2 with request with DryRun: true the returned request is valid.

The long answer:

The issue is that executing CreateAccessRequestV2 with DryRun: true may lead producing invalid request because we were using AccessExpiry (i.e. spec.expires) for two purposes.

So on the call to CreateAccessRequestV2 we validate the request. At this point spec.expires is being used as the requested session TTL. This is done by sessionTTL call which truncates the session TTL to r.GetAccessExpiry() // value of spec.expires.

Then the same sessionTTL func checks if r.GetAccessExpiry() // value of spec.expires is not bigger than the calculated session TTL. This is because requesting session TTL longer than the remaining time of the session of the current logged in identity is an error.

Now we have sessionTTL calculated, and we have spec.expires <= sessionTTL validated.

After that (we are still in validation) there is this call req.SetAccessExpiry(accessExpiry) which basically sets spec.expires to the same value as spec.max_duration (it's a line after req.SetMaxDuration(accessExpiry) call) and from now on, we start using spec.expires as spec.max_duration (situation this PR changes).

If it's a dry-run request we receive back a request with spec.expires overwritten to the same value as spec.max_duration. And obviously max duration can be longer than the session TTL. So if you try to call CreateAccessRequestV2 with the request returned by the dry-run call you're in trouble because sessionTTL fails, because at this time spec.expires // from r.GetAccessExpiry() is now longer than the session TTL, unless maxDuration == sessionTTL.

BTW maxDuration == sessionTTL is pretty common. This happens when no requested role has max_duration set.

tsh ssh auto request runs dry-run first and that causes the problem. Possibly it isn't necessary to run dry-run first there, but I think we shouldn't have fields which semantics change during the execution because it makes it very hard to reason about the system. And surely, request successfully returned by a dry-run should be valid.

// This is the final adjusted max duration where both max duration and session TTL
// were taken into consideration.
req.SetMaxDuration(now.Add(maxAccessDuration))

// Calculate the expiration time of the Access Request (how long it
// will await approval).
Expand All @@ -1384,7 +1378,7 @@ func (m *RequestValidator) validate(ctx context.Context, req types.AccessRequest

if req.GetAssumeStartTime() != nil {
assumeStartTime := *req.GetAssumeStartTime()
if err := types.ValidateAssumeStartTime(assumeStartTime, accessExpiry, req.GetCreationTime()); err != nil {
if err := types.ValidateAssumeStartTime(assumeStartTime, req.GetMaxDuration(), req.GetCreationTime()); err != nil {
return trace.Wrap(err)
}
}
Expand Down Expand Up @@ -1487,7 +1481,7 @@ func (m *RequestValidator) maxDurationForRole(roleName string) time.Duration {
// approval). request TTL is capped to the smaller value between the const requestTTL and the
// access request access expiry.
func (m *RequestValidator) calculatePendingRequestTTL(r types.AccessRequest, now time.Time) (time.Duration, error) {
accessExpiryTTL := r.GetAccessExpiry().Sub(now)
accessExpiryTTL := r.GetMaxDuration().Sub(now)

@r0mant r0mant Sep 29, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I fully understand the fix. E.g. why are we replacing access expiry with max duration everywhere?

It seems you're trying to address the issue on validation side? Shouldn't we be setting correct expiration on the created access request instead?

We should be really careful making changes here without understanding the full impact so we don't break existing access request guarantees/properties.

@kopiczko kopiczko Sep 30, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because access expiry is not max duration. I should replace this line with maxDurationTTL := r.GetMaxDuration().Sub(now), because it's confusing now.

Access expiry is in fact the requested session TTL:

teleport/lib/services/access_request.go

Lines 1525 to 1526 in b27d44c

func (m *RequestValidator) sessionTTL(ctx context.Context, identity tlsca.Identity, r types.AccessRequest, now time.Time) (time.Duration, error) {
ttl, err := m.truncateTTL(ctx, identity, r.GetAccessExpiry(), r.GetRoles(), now)

I wish we could get rid of it and use Set/GetSessionTTL instead, but that would be a breaking change.

It seems you're trying to address the issue on validation side? Shouldn't we be setting correct expiration on the created access request instead?

This is because the validation is broken. To fix it on the client side I'd have to do code like:

		oldAccessExpiry := req.GetAccessExpiry()
		req, err = clt.CreateAccessRequestV2(ctx, req)
		if err == nil {
			req.SetAccessExpiry(oldAccessExpiry)
		}
		return trace.Wrap(err)

I can do that if the changes here are too scary, but TBH that basically looks like working around a bug.

We should be really careful making changes here without understanding the full impact so we don't break existing access request guarantees/properties.

I get that. The only bit that I don't fully get is how this

teleport/lib/services/access_request.go

Line 517 in b27d44c

req.SetExpiry(req.GetAccessExpiry())

stayed compatible after we started doing req.SetAccessExpiry(accessExpiry) which was introduced in https://github.com/gravitational/teleport/pull/39548/files#:~:text=req.SetAccessExpiry(accessExpiry)

But having said this it should be compatible because req.GetAccessExpiry() is currently set to the value of req.GetMaxDuration().

teleport/lib/services/access_request.go

Lines 1367 to 1375 in b27d44c

// This is the final adjusted access expiry where both max duration
// and session TTL were taken into consideration.
accessExpiry := now.Add(maxAccessDuration)
// Adjusted max access duration is equal to the access expiry time.
req.SetMaxDuration(accessExpiry)
// Setting access expiry before calling `calculatePendingRequestTTL`
// matters since the func relies on this adjusted expiry.
req.SetAccessExpiry(accessExpiry)


// If no expiration provided, use default.
expiry := r.Expiry()
Expand Down
27 changes: 13 additions & 14 deletions lib/services/access_request_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,7 +708,7 @@ func TestReviewThresholds(t *testing.T) {
propose: approve,
assumeStartTime: clock.Now().UTC().Add(10000 * time.Hour),
errCheck: func(tt require.TestingT, err error, i ...any) {
require.ErrorContains(tt, err, "assume start time must be prior to access expiry time", i...)
require.ErrorContains(tt, err, "assume start time must be prior to max duration time", i...)
},
},
},
Expand Down Expand Up @@ -2128,61 +2128,61 @@ func TestCalculatePendingRequestTTL(t *testing.T) {

tests := []struct {
desc string
// accessExpiryTTL == max access duration.
accessExpiryTTL time.Duration
// maxDuration == max access duration.
maxDuration time.Duration
// when the access request expires in the PENDING state.
requestPendingExpiryTTL time.Time
assertion require.ErrorAssertionFunc
expectedDuration time.Duration
}{
{
desc: "valid: requested ttl < access expiry",
accessExpiryTTL: requestTTL - (3 * day),
maxDuration: requestTTL - (3 * day),
requestPendingExpiryTTL: now.Add(requestTTL - (4 * day)),
expectedDuration: requestTTL - (4 * day),
assertion: require.NoError,
},
{
desc: "valid: requested ttl == access expiry",
accessExpiryTTL: requestTTL - (3 * day),
maxDuration: requestTTL - (3 * day),
requestPendingExpiryTTL: now.Add(requestTTL - (3 * day)),
expectedDuration: requestTTL - (3 * day),
assertion: require.NoError,
},
{
desc: "valid: requested ttl == default request ttl",
accessExpiryTTL: requestTTL,
maxDuration: requestTTL,
requestPendingExpiryTTL: now.Add(requestTTL),
expectedDuration: requestTTL,
assertion: require.NoError,
},
{
desc: "valid: no TTL request defaults to the const requestTTL if access expiry is larger",
accessExpiryTTL: requestTTL + (3 * day),
maxDuration: requestTTL + (3 * day),
expectedDuration: requestTTL,
assertion: require.NoError,
},
{
desc: "valid: no TTL request defaults to accessExpiry if const requestTTL is larger",
accessExpiryTTL: requestTTL - (3 * day),
maxDuration: requestTTL - (3 * day),
expectedDuration: requestTTL - (3 * day),
assertion: require.NoError,
},
{
desc: "invalid: requested ttl > access expiry",
accessExpiryTTL: requestTTL - (3 * day),
maxDuration: requestTTL - (3 * day),
requestPendingExpiryTTL: now.Add(requestTTL - (2 * day)),
assertion: require.Error,
},
{
desc: "invalid: requested ttl > default request TTL",
accessExpiryTTL: requestTTL + (1 * day),
maxDuration: requestTTL + (1 * day),
requestPendingExpiryTTL: now.Add(requestTTL + (1 * day)),
assertion: require.Error,
},
{
desc: "invalid: requested ttl < now",
accessExpiryTTL: requestTTL - (3 * day),
maxDuration: requestTTL - (3 * day),
requestPendingExpiryTTL: now.Add(-(3 * day)),
assertion: require.Error,
},
Expand Down Expand Up @@ -2213,7 +2213,7 @@ func TestCalculatePendingRequestTTL(t *testing.T) {
request, err := types.NewAccessRequest("some-id", "foo", "bar")
require.NoError(t, err)
request.SetExpiry(tt.requestPendingExpiryTTL)
request.SetAccessExpiry(now.Add(tt.accessExpiryTTL))
request.SetMaxDuration(now.Add(tt.maxDuration))

ttl, err := validator.calculatePendingRequestTTL(request, now)
tt.assertion(t, err)
Expand Down Expand Up @@ -3129,7 +3129,6 @@ func TestValidate_RequestedMaxDuration(t *testing.T) {

err = validator.validate(context.Background(), req, identity)
require.NoError(t, err)
require.Equal(t, now.Add(tt.expectedAccessDuration), req.GetAccessExpiry())
require.Equal(t, now.Add(tt.expectedAccessDuration), req.GetMaxDuration())
require.Equal(t, now.Add(tt.expectedSessionTTL), req.GetSessionTLL())
require.Equal(t, now.Add(tt.expectedPendingTTL), req.Expiry())
Expand Down Expand Up @@ -3180,7 +3179,7 @@ func TestValidate_RequestedPendingTTLAndMaxDuration(t *testing.T) {

err = validator.validate(context.Background(), req, identity)
require.NoError(t, err)
require.Equal(t, now.Add(requestedMaxDuration), req.GetAccessExpiry())
require.Equal(t, time.Time{}, req.GetAccessExpiry())
require.Equal(t, now.Add(requestedMaxDuration), req.GetMaxDuration())
require.Equal(t, now.Add(defaultSessionTTL), req.GetSessionTLL())
require.Equal(t, now.Add(requestedPendingTTL), req.Expiry())
Expand Down
2 changes: 1 addition & 1 deletion lib/services/local/dynamic_access.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ func (s *DynamicAccessService) SetAccessRequestState(ctx context.Context, params
}

if params.AssumeStartTime != nil {
if err := types.ValidateAssumeStartTime(*params.AssumeStartTime, req.GetAccessExpiry(), req.GetCreationTime()); err != nil {
if err := types.ValidateAssumeStartTime(*params.AssumeStartTime, req.GetMaxDuration(), req.GetCreationTime()); err != nil {
return nil, trace.Wrap(err)
}
req.SetAssumeStartTime(*params.AssumeStartTime)
Expand Down
Loading