Pin now time during the access request creation (#40325)

* Remove clockwork dependency and handle time internally

The `clockwork` package was removed and handling of time elements has been refactored in codebase. The current time was set once and then passed as an argument to the functions that need it, reducing calls to retrieve the current time. This change simplifies the code and makes it more robust against time drifts.

* Build fix

* Remove usage of fake clock in tests

Removed the use of fake clock in tsh_test.go and kube_test.go to imitate real-world behavior more accurately. This change is aimed at improving the credibility and reliability of those test cases.

* Refine grammar and punctuation in comments.

This commit updates the comments in `access_request.go` and `access_request_test.go` files to improve grammar and punctuation for better readability and understanding of the code. This includes correction of typographical errors, addition of punctuation where necessary, and clarification of some comments for more precise descriptions.

* Update TTL validation error messages

The code changes refine messages for validation errors related to TTL. The error messages returned when requested TTLs exceed the maximum allowed amounts are improved. Time is no longer rounded to minutes, providing more precise error messages.

lib/services/access_request.go

@@ -270,9 +270,9 @@ func CalculateAccessCapabilities(ctx context.Context, clock clockwork.Clock, clt
 }
 
 // applicableSearchAsRoles prunes the search_as_roles and only returns those
-// application for the given list of resourceIDs.
+// applications for the given list of resourceIDs.
 func (m *RequestValidator) applicableSearchAsRoles(ctx context.Context, resourceIDs []types.ResourceID, loginHint string) ([]string, error) {
-	// First collect all possible search_as_roles.
+	// First, collect all possible search_as_roles.
 	var rolesToRequest []string
 	for _, roleName := range m.Roles.AllowSearch {
 		if !m.CanSearchAsRole(roleName) {
@@ -349,9 +349,9 @@ type thresholdFilterContext struct {
 }
 
 // reviewPermissionContext is the top-level context used to evaluate
-// a user's review permissions.  It is functionally identical to the
+// a user's review permissions. It is functionally identical to the
 // thresholdFilterContext except that it does not expose review parameters.
-// this is because review permissions are used to determine which requests
+// This is because review permissions are used to determine which requests
 // a user is allowed to see, and therefore needs to be calculable prior
 // to construction of review parameters.
 type reviewPermissionContext struct {
@@ -363,7 +363,7 @@ type reviewPermissionContext struct {
 // syntax errors.  Used to help prevent users from accidentally writing incorrect
 // predicates.  This function should only be called by the auth server prior to
 // storing new/updated roles.  Normal role validation deliberately omits these
-// checks in order to allow us to extend the available namespaces without breaking
+// checks to allow us to extend the available namespaces without breaking
 // backwards compatibility with older nodes/proxies (which never need to evaluate
 // these predicates).
 func ValidateAccessPredicates(role types.Role) error {
@@ -462,7 +462,7 @@ func ApplyAccessReview(req types.AccessRequest, rev types.AccessReview, author U
 		req.SetAssumeStartTime(*rev.AssumeStartTime)
 	}
 
-	// request is still pending, so check to see if this
+	// the request is still pending, so check to see if this
 	// review introduces a state-transition.
 	res, err := calculateReviewBasedResolution(req)
 	if err != nil || res == nil {
@@ -487,7 +487,7 @@ func ApplyAccessReview(req types.AccessRequest, rev types.AccessReview, author U
 // checkReviewCompat performs basic checks to ensure that the specified review can be
 // applied to the specified request (part of review application logic).
 func checkReviewCompat(req types.AccessRequest, rev types.AccessReview) error {
-	// Proposal cannot be already resolved.
+	// The Proposal cannot be yet resolved.
 	if !rev.ProposedState.IsResolved() {
 		// Skip the promoted state in the error message. It's not a state that most people
 		// should be concerned with.
@@ -621,7 +621,7 @@ func calculateReviewBasedResolution(req types.AccessRequest) (*requestResolution
 	// of their approval thresholds.
 	approved := make(map[string]struct{})
 
-	// denied keeps track of whether or not we've seen *any* role get denied
+	// denied keeps track of whether we've seen *any* role get denied
 	// (which role does not currently matter since we short-circuit on the
 	// first denial to be triggered).
 	denied := false
@@ -668,11 +668,11 @@ ProcessReviews:
 	CheckRoleApprovals:
 		for role, thresholdSets := range req.GetRoleThresholdMapping() {
 			if _, ok := approved[role]; ok {
-				// role was marked approved during a previous iteration
+				// the role was marked approved during a previous iteration
 				continue CheckRoleApprovals
 			}
 
-			// iterate through all threshold sets.  All sets must have at least
+			// iterate through all threshold sets. All sets must have at least
 			// one threshold which has hit its approval count in order for the
 			// role to be considered approved.
 		CheckThresholdSets:
@@ -693,7 +693,7 @@ ProcessReviews:
 				}
 
 				// no thresholds met for this set. there may be additional roles/thresholds
-				// which did meet their requirements this iteration, but there is no point
+				// that did meet their requirements this iteration, but there is no point in
 				// processing them unless this set has also hit its requirements.  we therefore
 				// move immediately to processing the next review.
 				continue ProcessReviews
@@ -765,7 +765,7 @@ func GetTraitMappings(cms []types.ClaimMapping) types.TraitMappingSet {
 }
 
 // RequestValidatorGetter is the interface required by the request validation
-// functions used to get necessary resources.
+// functions used to get the necessary resources.
 type RequestValidatorGetter interface {
 	UserLoginStatesGetter
 	UserGetter
@@ -844,7 +844,7 @@ func (c *ReviewPermissionChecker) HasAllowDirectives() bool {
 }
 
 // CanReviewRequest checks if the user is allowed to review the specified request.
-// note that the ability to review a request does not necessarily imply that any specific
+// Note that the ability to review a request does not necessarily imply that any specific
 // approval/denial thresholds will actually match the user's review.  Matching one or more
 // thresholds is not a pre-requisite for review submission.
 func (c *ReviewPermissionChecker) CanReviewRequest(req types.AccessRequest) (bool, error) {
@@ -856,7 +856,7 @@ func (c *ReviewPermissionChecker) CanReviewRequest(req types.AccessRequest) (boo
 		return false, nil
 	}
 
-	// method allocates new array if an override has already been
+	// method allocates a new array if an override has already been
 	// called, so get the role list once in advance.
 	requestedRoles := req.GetOriginalRoles()
 
@@ -1187,7 +1187,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 			return trace.Wrap(err)
 		}
 
-		// build the thresholds array and role-threshold-mapping.  the rtm encodes the
+		// build the threshold array and role-threshold-mapping.  the rtm encodes the
 		// relationship between a role, and the thresholds which must pass in order
 		// for that role to be considered approved.  when building the validator we
 		// recorded the relationship between the various allow matchers and their associated
@@ -1211,15 +1211,18 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 		// RBAC system propagates sideband information to plugins.
 		req.SetSystemAnnotations(m.SystemAnnotations())
 
-		// if no suggested reviewers were provided by the user then
+		// if no suggested reviewers were provided by the user, then
 		// use the defaults suggested by the user's static roles.
 		if len(req.GetSuggestedReviewers()) == 0 {
 			req.SetSuggestedReviewers(apiutils.Deduplicate(m.SuggestedReviewers))
 		}
 
+		// Pin the time to the current time to prevent time drift.
+		now := m.clock.Now().UTC()
+
 		// 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)
+		sessionTTL, err := m.sessionTTL(ctx, identity, req, now)
 		if err != nil {
 			return trace.Wrap(err)
 		}
@@ -1231,7 +1234,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 
 		// 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)))
 			maxAccessDuration = maxDuration
@@ -1246,13 +1249,13 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
 		// Adjusted max access duration is equal to the access expiry time.
 		req.SetMaxDuration(accessExpiry)
 
-		// Setting access expiry before calling `calculatePendingRequesetTTL`
+		// Setting access expiry before calling `calculatePendingRequestTTL`
 		// 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)
+		requestTTL, err := m.calculatePendingRequestTTL(req, now)
 		if err != nil {
 			return trace.Wrap(err)
 		}
@@ -1324,36 +1327,36 @@ func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest, s
 }
 
 // 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
+// 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) (time.Duration, error) {
-	accessExpiryTTL := r.GetAccessExpiry().Sub(m.clock.Now().UTC())
+func (m *RequestValidator) calculatePendingRequestTTL(r types.AccessRequest, now time.Time) (time.Duration, error) {
+	accessExpiryTTL := r.GetAccessExpiry().Sub(now)
 
 	// If no expiration provided, use default.
 	expiry := r.Expiry()
 	if expiry.IsZero() {
 		// Guard against the default expiry being greater than access expiry.
 		if requestTTL < accessExpiryTTL {
-			expiry = m.clock.Now().UTC().Add(requestTTL)
+			expiry = now.Add(requestTTL)
 		} else {
-			expiry = m.clock.Now().UTC().Add(accessExpiryTTL)
+			expiry = now.Add(accessExpiryTTL)
 		}
 	}
 
-	if expiry.Before(m.clock.Now().UTC()) {
+	if expiry.Before(now) {
 		return 0, trace.BadParameter("invalid request TTL: Access Request can not be created in the past")
 	}
 
 	// Before returning the TTL, validate that the value requested was smaller
 	// than the maximum value allowed. Used to return a sensible error to the
 	// user.
-	requestedTTL := expiry.Sub(m.clock.Now().UTC())
+	requestedTTL := expiry.Sub(now)
 	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))
+			return 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL, requestTTL)
 		}
 		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 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL, accessExpiryTTL)
 		}
 	}
 
@@ -1362,8 +1365,8 @@ func (m *RequestValidator) calculatePendingRequestTTL(r types.AccessRequest) (ti
 
 // sessionTTL calculates the TTL of the elevated certificate that will be issued
 // if the Access Request is approved.
-func (m *RequestValidator) sessionTTL(ctx context.Context, identity tlsca.Identity, r types.AccessRequest) (time.Duration, error) {
-	ttl, err := m.truncateTTL(ctx, identity, r.GetAccessExpiry(), r.GetRoles())
+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)
 	if err != nil {
 		return 0, trace.BadParameter("invalid session TTL: %v", err)
 	}
@@ -1371,9 +1374,9 @@ func (m *RequestValidator) sessionTTL(ctx context.Context, identity tlsca.Identi
 	// Before returning the TTL, validate that the value requested was smaller
 	// than the maximum value allowed. Used to return a sensible error to the
 	// user.
-	requestedTTL := r.GetAccessExpiry().Sub(m.clock.Now().UTC())
+	requestedTTL := r.GetAccessExpiry().Sub(now)
 	if !r.GetAccessExpiry().IsZero() && requestedTTL > ttl {
-		return 0, trace.BadParameter("invalid session TTL: %v greater than maximum allowed (%v)", requestedTTL.Round(time.Minute), ttl.Round(time.Minute))
+		return 0, trace.BadParameter("invalid session TTL: %v greater than maximum allowed (%v)", requestedTTL, ttl)
 	}
 
 	return ttl, nil
@@ -1381,18 +1384,18 @@ func (m *RequestValidator) sessionTTL(ctx context.Context, identity tlsca.Identi
 
 // truncateTTL will truncate given expiration by identity expiration and
 // shortest session TTL of any role.
-func (m *RequestValidator) truncateTTL(ctx context.Context, identity tlsca.Identity, expiry time.Time, roles []string) (time.Duration, error) {
+func (m *RequestValidator) truncateTTL(ctx context.Context, identity tlsca.Identity, expiry time.Time, roles []string, now time.Time) (time.Duration, error) {
 	ttl := apidefaults.MaxCertDuration
 
 	// Reduce by remaining TTL on requesting certificate (identity).
-	identityTTL := identity.Expires.Sub(m.clock.Now())
+	identityTTL := identity.Expires.Sub(now)
 	if identityTTL > 0 && identityTTL < ttl {
 		ttl = identityTTL
 	}
 
 	// Reduce TTL further if expiration time requested is shorter than that
 	// identity.
-	expiryTTL := expiry.Sub(m.clock.Now())
+	expiryTTL := expiry.Sub(now)
 	if expiryTTL > 0 && expiryTTL < ttl {
 		ttl = expiryTTL
 	}
@@ -1415,7 +1418,7 @@ func (m *RequestValidator) truncateTTL(ctx context.Context, identity tlsca.Ident
 }
 
 // getResourceViewingRoles gets the subset of the user's roles that could be used
-// to view resources (i.e. base roles + search as roles).
+// to view resources (i.e., base roles + search as roles).
 func (m *RequestValidator) getResourceViewingRoles() []string {
 	roles := slices.Clone(m.userState.GetRoles())
 	for _, role := range m.Roles.AllowSearch {
@@ -1428,8 +1431,8 @@ func (m *RequestValidator) getResourceViewingRoles() []string {
 
 // GetRequestableRoles gets the list of all existent roles which the user is
 // able to request.  This operation is expensive since it loads all existent
-// roles in order to determine the role list.  Prefer calling CanRequestRole
-// when checking against a known role list. If resource IDs or a login hint
+// roles to determine the role list.  Prefer calling CanRequestRole
+// when checking against a known role list. If resource IDs or a login hints
 // are provided, roles will be filtered to only include those that would
 // allow access to the given resource with the given login.
 func (m *RequestValidator) GetRequestableRoles(ctx context.Context, identity tlsca.Identity, resourceIDs []types.ResourceID, loginHint string) ([]string, error) {
@@ -1562,7 +1565,7 @@ func (m *RequestValidator) push(role types.Role) error {
 }
 
 // setRolesForResourceRequest determines if the given access request is
-// resource-based, and if so it determines which underlying roles are necessary
+// resource-based, and if so, it determines which underlying roles are necessary
 // and adds them to the request.
 func (m *RequestValidator) setRolesForResourceRequest(ctx context.Context, req types.AccessRequest) error {
 	if !m.opts.expandVars {
@@ -1588,7 +1591,7 @@ func (m *RequestValidator) setRolesForResourceRequest(ctx context.Context, req t
 	return nil
 }
 
-// thresholdCollector is a helper which assembles the Thresholds array for a request.
+// thresholdCollector is a helper that assembles the Thresholds array for a request.
 // the push() method is used to insert groups of related thresholds and calculate their
 // corresponding index set.
 type thresholdCollector struct {

lib/services/access_request_test.go

@@ -1774,9 +1774,9 @@ func TestGetRequestableRoles(t *testing.T) {
 	}
 }
 
-// TestCalculatePendingRequesTTL verifies that the TTL for the Access Request is capped to the
+// TestCalculatePendingRequestTTL verifies that the TTL for the Access Request is capped to the
 // request's access expiry or capped to the default const requestTTL, whichever is smaller.
-func TestCalculatePendingRequesTTL(t *testing.T) {
+func TestCalculatePendingRequestTTL(t *testing.T) {
 	clock := clockwork.NewFakeClock()
 	now := clock.Now().UTC()
 
@@ -1869,7 +1869,7 @@ func TestCalculatePendingRequesTTL(t *testing.T) {
 			request.SetExpiry(tt.requestPendingExpiryTTL)
 			request.SetAccessExpiry(now.Add(tt.accessExpiryTTL))
 
-			ttl, err := validator.calculatePendingRequestTTL(request)
+			ttl, err := validator.calculatePendingRequestTTL(request, now)
 			tt.assertion(t, err)
 			if err == nil {
 				require.Equal(t, tt.expectedDuration, ttl)
@@ -1943,7 +1943,7 @@ func TestSessionTTL(t *testing.T) {
 			request.SetAccessExpiry(tt.accessExpiry)
 			require.NoError(t, err)
 
-			ttl, err := validator.sessionTTL(context.Background(), tt.identity, request)
+			ttl, err := validator.sessionTTL(context.Background(), tt.identity, request, now)
 			tt.assertion(t, err)
 			if err == nil {
 				require.Equal(t, tt.expectedTTL, ttl)

tool/tsh/common/kube_test.go

@@ -37,7 +37,6 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/uuid"
 	"github.com/gravitational/trace"
-	"github.com/jonboulle/clockwork"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/sync/errgroup"
@@ -366,7 +365,7 @@ func TestKubeSelection(t *testing.T) {
 			cfg.Kube.ResourceMatchers = []services.ResourceMatcher{{
 				Labels: map[string]apiutils.Strings{"*": {"*"}},
 			}}
-			cfg.Clock = clockwork.NewFakeClock()
+			// Do not use a fake clock to better imitate real-world behavior.
 		}),
 	)
 	kubeBarEKS := "bar-eks-us-west-1-123456789012"

tool/tsh/common/tsh_test.go

@@ -47,7 +47,6 @@ import (
 
 	"github.com/ghodss/yaml"
 	"github.com/gravitational/trace"
-	"github.com/jonboulle/clockwork"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	otlp "go.opentelemetry.io/proto/otlp/trace/v1"
@@ -1823,9 +1822,8 @@ func TestSSHAccessRequest(t *testing.T) {
 
 	rootAuth, rootProxy := makeTestServers(t,
 		withBootstrap(requester, searchOnlyRequester, nodeAccessRole, emptyRole, connector, alice),
-		withConfig(func(cfg *servicecfg.Config) {
-			cfg.Clock = clockwork.NewFakeClock()
-		}))
+		// Do not use a fake clock to better imitate real-world behavior.
+	)
 
 	authAddr, err := rootAuth.AuthAddr()
 	require.NoError(t, err)