AccessRequest: Stop using Expires as MaxDuration

[kopiczko]

No description provided.

[r0mant]

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

[kopiczko]

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.

[r0mant]

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]

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)

[kopiczko]

We decided to not rush access_request changes and proceed with the ugly but safe client-side fix #59761
This resource fields mess should be addressed in part 2 of #46001