From aa77570d65f0242acc8bc5249706d98af1bcb6bf Mon Sep 17 00:00:00 2001 From: Michael Wilson Date: Thu, 22 Feb 2024 16:03:10 -0500 Subject: [PATCH] Set default access request TTL to 1 week. (#35799) The TTL for a request now defaults to 1 week. This will allow reviewers more time to review an access request before it disappears. --- lib/services/access_request.go | 32 +++---- lib/services/access_request_test.go | 94 ------------------- .../teleport/src/AccessRequests/utils.ts | 16 ++-- 3 files changed, 24 insertions(+), 118 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index d42eb8e9c27b3..e5101149a272c 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -36,20 +36,25 @@ import ( apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" - "github.com/gravitational/teleport/lib/defaults" "github.com/gravitational/teleport/lib/tlsca" "github.com/gravitational/teleport/lib/utils" "github.com/gravitational/teleport/lib/utils/parse" ) -const maxAccessRequestReasonSize = 4096 +const ( + maxAccessRequestReasonSize = 4096 -// A day is sometimes 23 hours, sometimes 25 hours, usually 24 hours. -const day = 24 * time.Hour + // A day is sometimes 23 hours, sometimes 25 hours, usually 24 hours. + day = 24 * time.Hour -// maxAccessDuration is the maximum duration that an access request can be -// granted for. -const maxAccessDuration = 14 * day + // maxAccessDuration is the maximum duration that an access request can be + // granted for. + maxAccessDuration = 14 * day + + // requestTTL is the the TTL for an access request, i.e. the amount of time that + // the access request can be reviewed. Defaults to 1 week. + requestTTL = 7 * day +) // ValidateAccessRequest validates the AccessRequest and sets default values func ValidateAccessRequest(ar types.AccessRequest) error { @@ -1270,27 +1275,22 @@ func (m *RequestValidator) requestTTL(ctx context.Context, identity tlsca.Identi // If no expiration provided, use default. expiry := r.Expiry() if expiry.IsZero() { - expiry = m.clock.Now().UTC().Add(defaults.PendingAccessDuration) + expiry = m.clock.Now().UTC().Add(requestTTL) } if expiry.Before(m.clock.Now().UTC()) { return 0, trace.BadParameter("invalid request TTL: Access Request can not be created in the past") } - ttl, err := m.truncateTTL(ctx, identity, expiry, r.GetRoles()) - if err != nil { - return 0, trace.BadParameter("invalid request TTL: %v", err) - } - // 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()) - if !r.Expiry().IsZero() && requestedTTL > ttl { - return 0, trace.BadParameter("invalid request TTL: %v greater than maximum allowed (%v)", requestedTTL.Round(time.Minute), ttl.Round(time.Minute)) + 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)) } - return ttl, nil + return requestedTTL, nil } // sessionTTL calculates the TTL of the elevated certificate that will be issued diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 54b4027393240..6f1e23bf8b424 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -1608,100 +1608,6 @@ func TestPruneRequestRoles(t *testing.T) { } } -// TestRequestTTL verifies that the TTL for the Access Request gets reduced by -// requested access time and lifetime of the requesting certificate. -func TestRequestTTL(t *testing.T) { - clock := clockwork.NewFakeClock() - now := clock.Now().UTC() - - tests := []struct { - desc string - expiry time.Time - identity tlsca.Identity - maxSessionTTL time.Duration - expectedTTL time.Duration - assertion require.ErrorAssertionFunc - }{ - { - desc: "access request with ttl, below limit", - expiry: now.Add(8 * time.Hour), - identity: tlsca.Identity{Expires: now.Add(10 * time.Hour)}, - maxSessionTTL: 10 * time.Hour, - expectedTTL: 8 * time.Hour, - assertion: require.NoError, - }, - { - desc: "access request with ttl, above limit", - expiry: now.Add(11 * time.Hour), - identity: tlsca.Identity{Expires: now.Add(10 * time.Hour)}, - maxSessionTTL: 10 * time.Hour, - assertion: require.Error, - }, - { - desc: "access request without ttl (default ttl)", - expiry: time.Time{}, - identity: tlsca.Identity{Expires: now.Add(10 * time.Hour)}, - maxSessionTTL: 10 * time.Hour, - expectedTTL: defaults.PendingAccessDuration, - assertion: require.NoError, - }, - { - desc: "access request without ttl (default ttl), truncation by identity expiration", - expiry: time.Time{}, - identity: tlsca.Identity{Expires: now.Add(12 * time.Minute)}, - maxSessionTTL: 13 * time.Minute, - expectedTTL: 12 * time.Minute, - assertion: require.NoError, - }, - { - desc: "access request without ttl (default ttl), truncation by role max session ttl", - expiry: time.Time{}, - identity: tlsca.Identity{Expires: now.Add(14 * time.Hour)}, - maxSessionTTL: 13 * time.Minute, - expectedTTL: 13 * time.Minute, - assertion: require.NoError, - }, - } - - for _, tt := range tests { - t.Run(tt.desc, func(t *testing.T) { - // Setup test user "foo" and "bar" and the mock auth server that - // will return users and roles. - uls, err := userloginstate.New(header.Metadata{ - Name: "foo", - }, userloginstate.Spec{ - Roles: []string{"bar"}, - }) - require.NoError(t, err) - - role, err := types.NewRole("bar", types.RoleSpecV6{ - Options: types.RoleOptions{ - MaxSessionTTL: types.NewDuration(tt.maxSessionTTL), - }, - }) - require.NoError(t, err) - - getter := &mockGetter{ - userStates: map[string]*userloginstate.UserLoginState{"foo": uls}, - roles: map[string]types.Role{"bar": role}, - } - - validator, err := NewRequestValidator(context.Background(), clock, getter, "foo", ExpandVars(true)) - require.NoError(t, err) - - request, err := types.NewAccessRequest("some-id", "foo", "bar") - request.SetExpiry(tt.expiry) - require.NoError(t, err) - - ttl, err := validator.requestTTL(context.Background(), tt.identity, request) - tt.assertion(t, err) - if err == nil { - require.Equal(t, tt.expectedTTL, ttl) - } - }) - } -} - // TestSessionTTL verifies that the TTL for elevated access gets reduced by // requested access time, lifetime of certificate, and strictest session TTL on // any role. diff --git a/web/packages/teleport/src/AccessRequests/utils.ts b/web/packages/teleport/src/AccessRequests/utils.ts index 9f3fabe26f305..ecc69c4cf1bbd 100644 --- a/web/packages/teleport/src/AccessRequests/utils.ts +++ b/web/packages/teleport/src/AccessRequests/utils.ts @@ -119,10 +119,10 @@ export function middleValues( })); } -// Generate a list of middle values between now and the session TTL. +// Generate a list of middle values between now and the request TTL. export function requestTtlMiddleValues( created: Date, - sessionTTL: Date + requestTtl: Date ): TimeDuration[] { const getInterval = (d: Date) => roundToNearestTenMinutes( @@ -132,22 +132,22 @@ export function requestTtlMiddleValues( }) ); - if (isAfter(addHours(created, 1), sessionTTL)) { + if (isAfter(addHours(created, 1), requestTtl)) { return [ { - timestamp: sessionTTL.getTime(), - duration: getInterval(sessionTTL), + timestamp: requestTtl.getTime(), + duration: getInterval(requestTtl), }, ]; } const points: Date[] = []; - // Staggered hour options, up to the maximum possible session TTL. - const hourOptions = [1, 2, 3, 4, 6, 8, 12, 18, 24, 30]; + // Staggered hour options, up to 1 week. + const hourOptions = [1, 2, 3, 4, 6, 8, 12, 18, 24, 30, 168]; for (const h of hourOptions) { const t = addHours(created, h); - if (isAfter(t, sessionTTL)) { + if (isAfter(t, requestTtl)) { break; } points.push(t);