Fix setting pending request TTL pass the access expiry time

lib/services/access_request.go

@@ -1187,45 +1187,50 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 			req.SetSuggestedReviewers(apiutils.Deduplicate(m.SuggestedReviewers))
 		}
 
-		now := m.clock.Now().UTC()
-
-		// Calculate the expiration time of the Access Request (how long it
-		// will await approval).
-		ttl, err := m.requestTTL(ctx, identity, req)
-		if err != nil {
-			return trace.Wrap(err)
-		}
-		req.SetExpiry(now.Add(ttl))
-
-		maxDuration, err := m.calculateMaxAccessDuration(req)
+		// Calculate the expiration time of the elevated certificate that will
+		// be issued if the Access Request is approved.
+		sessionTTL, err := m.sessionTTL(ctx, identity, req)
 		if err != nil {
 			return trace.Wrap(err)
 		}
 
-		// Calculate the expiration time of the elevated certificate that will
-		// be issued if the Access Request is approved.
-		sessionTTL, err := m.sessionTTL(ctx, identity, req)
+		maxDuration, err := m.calculateMaxAccessDuration(req, sessionTTL)
 		if err != nil {
 			return trace.Wrap(err)
 		}
 
 		// If the maxDuration flag is set, consider it instead of only using the session TTL.
+		var maxAccessDuration time.Duration
+		now := m.clock.Now().UTC()
 		if maxDuration > 0 {
 			req.SetSessionTLL(now.Add(min(sessionTTL, maxDuration)))
-			ttl = maxDuration
+			maxAccessDuration = maxDuration
 		} else {
 			req.SetSessionTLL(now.Add(sessionTTL))
-			ttl = sessionTTL
+			maxAccessDuration = sessionTTL
 		}
 
-		accessTTL := now.Add(ttl)
-		req.SetAccessExpiry(accessTTL)
+		// 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(accessTTL)
+		req.SetMaxDuration(accessExpiry)
+
+		// Setting access expiry before calling `calculatePendingRequesetTTL`
+		// matters since the func relies on this adjusted expiry.
+		req.SetAccessExpiry(accessExpiry)
+
+		// Calculate the expiration time of the Access Request (how long it
+		// will await approval).
+		requestTTL, err := m.calculatePendingRequestTTL(req)
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		req.SetExpiry(now.Add(requestTTL))
 
 		if req.GetAssumeStartTime() != nil {
 			assumeStartTime := *req.GetAssumeStartTime()
-			if err := types.ValidateAssumeStartTime(assumeStartTime, accessTTL, req.GetCreationTime()); err != nil {
+			if err := types.ValidateAssumeStartTime(assumeStartTime, accessExpiry, req.GetCreationTime()); err != nil {
 				return trace.Wrap(err)
 			}
 		}
@@ -1237,7 +1242,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 // calculateMaxAccessDuration calculates the maximum time for the access request.
 // The max duration time is the minimum of the max_duration time set on the request
 // and the max_duration time set on the request role.
-func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest) (time.Duration, error) {
+func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest, sessionTTL time.Duration) (time.Duration, error) {
 	// Check if the maxDuration time is set.
 	maxDurationTime := req.GetMaxDuration()
 	if maxDurationTime.IsZero() {
@@ -1277,16 +1282,32 @@ func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest) (
 		}
 	}
 
+	// minAdjDuration can end up being 0, if any role does not have
+	// field `max_duration` defined.
+	// In this case, return the smaller value between the sessionTTL
+	// and the requested max duration.
+	if minAdjDuration == 0 && maxDuration < sessionTTL {
+		return maxDuration, nil
+	}
+
 	return minAdjDuration, nil
 }
 
-// requestTTL calculates the TTL of the Access Request (how long it will await
-// approval).
-func (m *RequestValidator) requestTTL(ctx context.Context, identity tlsca.Identity, r types.AccessRequest) (time.Duration, error) {
+// calculatePendingRequestTTL calculates the TTL of the Access Request (how long it will await
+// approval). request TTL is capped to the smaller value between the const requsetTTL and the
+// access request access expiry.
+func (m *RequestValidator) calculatePendingRequestTTL(r types.AccessRequest) (time.Duration, error) {
+	accessExpiryTTL := r.GetAccessExpiry().Sub(m.clock.Now().UTC())
+
 	// If no expiration provided, use default.
 	expiry := r.Expiry()
 	if expiry.IsZero() {
-		expiry = m.clock.Now().UTC().Add(requestTTL)
+		// Guard against the default expiry being greater than access expiry.
+		if requestTTL < accessExpiryTTL {
+			expiry = m.clock.Now().UTC().Add(requestTTL)
+		} else {
+			expiry = m.clock.Now().UTC().Add(accessExpiryTTL)
+		}
 	}
 
 	if expiry.Before(m.clock.Now().UTC()) {
@@ -1297,8 +1318,13 @@ func (m *RequestValidator) requestTTL(ctx context.Context, identity tlsca.Identi
 	// than the maximum value allowed. Used to return a sensible error to the
 	// user.
 	requestedTTL := expiry.Sub(m.clock.Now().UTC())
-	if !r.Expiry().IsZero() && requestedTTL > requestTTL {
-		return 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL.Round(time.Minute), requestTTL.Round(time.Minute))
+	if !r.Expiry().IsZero() {
+		if requestedTTL > requestTTL {
+			return 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL.Round(time.Minute), requestTTL.Round(time.Minute))
+		}
+		if requestedTTL > accessExpiryTTL {
+			return 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL.Round(time.Minute), accessExpiryTTL.Round(time.Minute))
+		}
 	}
 
 	return requestedTTL, nil