From 71dd71133a9e2dc7fe08ca14828f5f7e4433c6fb Mon Sep 17 00:00:00 2001 From: Mike Wilson Date: Fri, 15 Dec 2023 16:08:54 -0500 Subject: [PATCH 1/2] Set default access request TTL to 1 week. The TTL for a request now defaults to 1 week. This will allow reviewers more time to review an access request before it disappears. Co-authored-by: Lisa Kim --- lib/services/access_request.go | 116 +++++--- lib/services/access_request_test.go | 277 ++++++++++++------ .../teleport/src/AccessRequests/utils.ts | 16 +- 3 files changed, 262 insertions(+), 147 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 33fe3ccebb98b..5ae13c5081c8a 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -33,23 +33,29 @@ import ( "github.com/gravitational/teleport/api/accessrequest" "github.com/gravitational/teleport/api/client" + "github.com/gravitational/teleport/api/client/proto" 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 { @@ -75,7 +81,7 @@ type ClusterGetter interface { // GetClusterName returns the local cluster name GetClusterName(opts ...MarshalOption) (types.ClusterName, error) // GetRemoteCluster returns a remote cluster by name - GetRemoteCluster(clusterName string) (types.RemoteCluster, error) + GetRemoteCluster(ctx context.Context, clusterName string) (types.RemoteCluster, error) } // ValidateAccessRequestClusterNames checks that the clusters in the access request exist @@ -92,7 +98,7 @@ func ValidateAccessRequestClusterNames(cg ClusterGetter, ar types.AccessRequest) if resourceID.ClusterName == localClusterName.GetClusterName() { continue } - _, err := cg.GetRemoteCluster(resourceID.ClusterName) + _, err := cg.GetRemoteCluster(context.TODO(), resourceID.ClusterName) if err != nil && !trace.IsNotFound(err) { return trace.Wrap(err, "failed to fetch remote cluster %q", resourceID.ClusterName) } @@ -163,6 +169,9 @@ func (r *RequestIDs) IsEmpty() bool { type AccessRequestGetter interface { // GetAccessRequests gets all currently active access requests. GetAccessRequests(ctx context.Context, filter types.AccessRequestFilter) ([]types.AccessRequest, error) + + // ListAccessRequests is an access request getter with pagination and sorting options. + ListAccessRequests(ctx context.Context, req *proto.ListAccessRequestsRequest) (*proto.ListAccessRequestsResponse, error) } // DynamicAccessCore is the core functionality common to all DynamicAccess implementations. @@ -1178,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) } } @@ -1228,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() { @@ -1268,36 +1282,52 @@ 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(defaults.PendingAccessDuration) + // 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()) { 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() { + 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 ttl, nil + return requestedTTL, nil } // sessionTTL calculates the TTL of the elevated certificate that will be issued @@ -1635,7 +1665,7 @@ func ValidateAccessRequestForUser(ctx context.Context, clock clockwork.Clock, ge } // UnmarshalAccessRequest unmarshals the AccessRequest resource from JSON. -func UnmarshalAccessRequest(data []byte, opts ...MarshalOption) (types.AccessRequest, error) { +func UnmarshalAccessRequest(data []byte, opts ...MarshalOption) (*types.AccessRequestV3, error) { cfg, err := CollectOptions(opts) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 4b54ad7301726..3c99d28114f74 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -1608,58 +1608,71 @@ 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) { +// TestCalculatePendingRequesTTL 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) { 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 string + // accessExpiryTTL == max access duration. + accessExpiryTTL time.Duration + // when the access request expires in the PENDING state. + requestPendingExpiryTTL time.Time + assertion require.ErrorAssertionFunc + expectedDuration time.Duration }{ { - 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: "valid: requested ttl < access expiry", + accessExpiryTTL: requestTTL - (3 * day), + requestPendingExpiryTTL: now.Add(requestTTL - (4 * day)), + expectedDuration: requestTTL - (4 * day), + 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: "valid: requested ttl == access expiry", + accessExpiryTTL: requestTTL - (3 * day), + requestPendingExpiryTTL: now.Add(requestTTL - (3 * day)), + expectedDuration: requestTTL - (3 * day), + assertion: require.NoError, }, { - 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: "valid: requested ttl == default request ttl", + accessExpiryTTL: requestTTL, + requestPendingExpiryTTL: now.Add(requestTTL), + expectedDuration: requestTTL, + 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: "valid: no TTL request defaults to the const requestTTL if access expiry is larger", + accessExpiryTTL: requestTTL + (3 * day), + expectedDuration: requestTTL, + 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, + desc: "valid: no TTL request defaults to accessExpiry if const requestTTL is larger", + accessExpiryTTL: requestTTL - (3 * day), + expectedDuration: requestTTL - (3 * day), + assertion: require.NoError, + }, + { + desc: "invalid: requested ttl > access expiry", + accessExpiryTTL: requestTTL - (3 * day), + requestPendingExpiryTTL: now.Add(requestTTL - (2 * day)), + assertion: require.Error, + }, + { + desc: "invalid: requested ttl > default request TTL", + accessExpiryTTL: requestTTL + (1 * day), + requestPendingExpiryTTL: now.Add(requestTTL + (1 * day)), + assertion: require.Error, + }, + { + desc: "invalid: requested ttl < now", + accessExpiryTTL: requestTTL - (3 * day), + requestPendingExpiryTTL: now.Add(-(3 * day)), + assertion: require.Error, }, } @@ -1674,11 +1687,7 @@ func TestRequestTTL(t *testing.T) { }) require.NoError(t, err) - role, err := types.NewRole("bar", types.RoleSpecV6{ - Options: types.RoleOptions{ - MaxSessionTTL: types.NewDuration(tt.maxSessionTTL), - }, - }) + role, err := types.NewRole("bar", types.RoleSpecV6{}) require.NoError(t, err) getter := &mockGetter{ @@ -1690,13 +1699,14 @@ func TestRequestTTL(t *testing.T) { require.NoError(t, err) request, err := types.NewAccessRequest("some-id", "foo", "bar") - request.SetExpiry(tt.expiry) require.NoError(t, err) + request.SetExpiry(tt.requestPendingExpiryTTL) + request.SetAccessExpiry(now.Add(tt.accessExpiryTTL)) - ttl, err := validator.requestTTL(context.Background(), tt.identity, request) + ttl, err := validator.calculatePendingRequestTTL(request) tt.assertion(t, err) if err == nil { - require.Equal(t, tt.expectedTTL, ttl) + require.Equal(t, tt.expectedDuration, ttl) } }) } @@ -1897,7 +1907,7 @@ func (mcg mockClusterGetter) GetClusterName(opts ...MarshalOption) (types.Cluste return mcg.localCluster, nil } -func (mcg mockClusterGetter) GetRemoteCluster(clusterName string) (types.RemoteCluster, error) { +func (mcg mockClusterGetter) GetRemoteCluster(ctx context.Context, clusterName string) (types.RemoteCluster, error) { if cluster, ok := mcg.remoteClusters[clusterName]; ok { return cluster, nil } @@ -1961,7 +1971,10 @@ func TestValidateAccessRequestClusterNames(t *testing.T) { } } -func TestMaxDuration(t *testing.T) { +// TestValidate_RequestedMaxDuration tests requested max duration +// and the default values for session and pending TTL as a result +// of requested max duration. +func TestValidate_RequestedMaxDuration(t *testing.T) { // describes a collection of roles and their conditions roleDesc := roleTestSet{ "requestedRole": { @@ -2023,6 +2036,8 @@ func TestMaxDuration(t *testing.T) { "david": {"maxDurationReqRole"}, } + defaultSessionTTL := 8 * time.Hour + g := getMockGetter(t, roleDesc, userDesc) tts := []struct { @@ -2032,101 +2047,122 @@ func TestMaxDuration(t *testing.T) { requestor string // the roles to be requested (defaults to "dictator") roles []string - // maxDuration is the requested maxDuration duration - maxDuration time.Duration + // requestedMaxDuration is the requested requestedMaxDuration duration + requestedMaxDuration time.Duration // expectedAccessDuration is the expected access duration expectedAccessDuration time.Duration // expectedSessionTTL is the expected session TTL expectedSessionTTL time.Duration + // expectedPendingTTL is the time when request expires in PENDING state + expectedPendingTTL time.Duration // DryRun is true if the request is a dry run dryRun bool }{ { - desc: "role maxDuration is respected", + desc: "role max_duration is respected and sessionTTL does not exceed the calculated max duration", requestor: "alice", - roles: []string{"requestedRole"}, - maxDuration: 7 * day, + roles: []string{"requestedRole"}, // role max_duration capped to 3 days + requestedMaxDuration: 7 * day, // ignored b/c it's > role max_duration expectedAccessDuration: 3 * day, - expectedSessionTTL: 8 * time.Hour, + expectedSessionTTL: 8 * time.Hour, // caps to defaultSessionTTL b/c it's < than the expectedAccessDuration + expectedPendingTTL: 3 * day, // caps to expectedAccessDuration b/c it's < than the const default TTL }, { - desc: "dry run allows for longer maxDuration then 7d", + desc: "role max_duration is still respected even with dry run (which requests for longer maxDuration)", requestor: "alice", - roles: []string{"requestedRole"}, - maxDuration: 10 * day, + roles: []string{"requestedRole"}, // role max_duration capped to 3 days + requestedMaxDuration: 10 * day, // ignored b/c it's > role max_duration expectedAccessDuration: 3 * day, + expectedPendingTTL: 3 * day, expectedSessionTTL: 8 * time.Hour, dryRun: true, }, { - desc: "maxDuration not set, default maxTTL (8h)", - requestor: "bob", - roles: []string{"requestedRole"}, - expectedAccessDuration: 8 * time.Hour, + desc: "role max_duration is ignored when requestedMaxDuration is not set", + requestor: "alice", + roles: []string{"requestedRole"}, // role max_duration capped to 3 days + expectedAccessDuration: 8 * time.Hour, // caps to defaultSessionTTL since requestedMaxDuration was not set + expectedPendingTTL: 8 * time.Hour, expectedSessionTTL: 8 * time.Hour, }, { - desc: "maxDuration inside request is respected", + desc: "when role max_duration is not set: default to defaultSessionTTL when requestedMaxDuration is not set", requestor: "bob", - roles: []string{"requestedRole"}, - maxDuration: 5 * time.Hour, - expectedAccessDuration: 8 * time.Hour, + roles: []string{"requestedRole"}, // role max_duration is not set (0) + expectedAccessDuration: 8 * time.Hour, // caps to defaultSessionTTL since requestedMaxDuration was not set + expectedPendingTTL: 8 * time.Hour, expectedSessionTTL: 8 * time.Hour, }, { - desc: "users with no MaxDuration are constrained by normal maxTTL logic", + desc: "when role max_duration is not set: requestedMaxDuration is respected when < defaultSessionTTL", requestor: "bob", - roles: []string{"requestedRole"}, - maxDuration: 2 * day, - expectedAccessDuration: 8 * time.Hour, + roles: []string{"requestedRole"}, // role max_duration is not set (0) + requestedMaxDuration: 5 * time.Hour, + expectedAccessDuration: 5 * time.Hour, + expectedPendingTTL: 5 * time.Hour, + expectedSessionTTL: 5 * time.Hour, // capped to expectedAccessDuration because it's < defaultSessionTTL (8h) + }, + { + desc: "when role max_duration is not set: requestedMaxDuration is ignored if > defaultSessionTTL", + requestor: "bob", + roles: []string{"requestedRole"}, // role max_duration is not set (0) + requestedMaxDuration: 10 * time.Hour, + expectedAccessDuration: 8 * time.Hour, // caps to defaultSessionTTL (8h) which is < requestedMaxDuration + expectedPendingTTL: 8 * time.Hour, expectedSessionTTL: 8 * time.Hour, }, { - desc: "maxDuration can't exceed maxTTL by default", + desc: "when role max_duration is not set: requestedMaxDuration is ignored if > role defined sesssionTTL (6h)", requestor: "bob", - roles: []string{"setMaxTTLRole"}, - maxDuration: day, - expectedAccessDuration: 6 * time.Hour, + roles: []string{"setMaxTTLRole"}, // role max_duration is not set (0), caps sessionTTL to 6 hours + requestedMaxDuration: day, + expectedAccessDuration: 6 * time.Hour, // capped to the lowest sessionTTL found in role (6h) which is < requestedMaxDuration + expectedPendingTTL: 6 * time.Hour, expectedSessionTTL: 6 * time.Hour, }, { - desc: "maxDuration is ignored if max_duration is not set in role", + desc: "when role max_duration is not set: requestedMaxDuration is respected when < role defined sessionTTL (6h)", requestor: "bob", - roles: []string{"setMaxTTLRole"}, - maxDuration: 2 * time.Hour, - expectedAccessDuration: 6 * time.Hour, - expectedSessionTTL: 6 * time.Hour, + roles: []string{"setMaxTTLRole"}, // role max_duration is not set (0), caps sessionTTL to 6 hours + requestedMaxDuration: 5 * time.Hour, + expectedAccessDuration: 5 * time.Hour, // caps to requestedMaxDuration which is < role defined sessionTTL (6h) + expectedPendingTTL: 5 * time.Hour, + expectedSessionTTL: 5 * time.Hour, }, { - desc: "maxDuration can exceed maxTTL if max_duration is set in role", + desc: "requestedMaxDuration is respected if it's < the max_duration set in role", requestor: "david", - roles: []string{"setMaxTTLRole"}, - maxDuration: day, + roles: []string{"setMaxTTLRole"}, // role max_duration capped to default MaxAccessDuration, caps sessionTTL to 6 hours + requestedMaxDuration: day, // respected because it's < default const MaxAccessDuration expectedAccessDuration: day, - expectedSessionTTL: 6 * time.Hour, + expectedPendingTTL: day, + expectedSessionTTL: 6 * time.Hour, // capped to the lowest sessionTTL found in role which is < requestedMaxDuration }, { - desc: "maxDuration shorter than maxTTL if max_duration is set in role", + desc: "expectedSessionTTL does not exceed requestedMaxDuration", requestor: "david", - roles: []string{"setMaxTTLRole"}, - maxDuration: 2 * time.Hour, + roles: []string{"setMaxTTLRole"}, // caps max_duration to default MaxAccessDuration, caps sessionTTL to 6 hours + requestedMaxDuration: 2 * time.Hour, // respected because it's < default const MaxAccessDuration expectedAccessDuration: 2 * time.Hour, - expectedSessionTTL: 2 * time.Hour, + expectedPendingTTL: 2 * time.Hour, + expectedSessionTTL: 2 * time.Hour, // capped to requestedMaxDuration because it's < role defined sessionTTL (6h) }, { - desc: "only required roles are considered for maxDuration", - requestor: "carol", - roles: []string{"requestedRole"}, - maxDuration: 5 * day, + desc: "only the assigned role that allows the requested roles are considered for maxDuration", + requestor: "carol", // has multiple roles assigned + roles: []string{"requestedRole"}, // caps max_duration to 3 days + requestedMaxDuration: 5 * day, expectedAccessDuration: 3 * day, + expectedPendingTTL: 3 * day, expectedSessionTTL: 8 * time.Hour, }, { - desc: "only required roles are considered for maxDuration #2", - requestor: "carol", - roles: []string{"requestedRole2"}, - maxDuration: 6 * day, + desc: "only the assigned role that allows the requested roles are considered for maxDuration #2", + requestor: "carol", // has multiple roles assigned + roles: []string{"requestedRole2"}, // caps max_duration to 1 day + requestedMaxDuration: 6 * day, expectedAccessDuration: day, + expectedPendingTTL: day, expectedSessionTTL: 8 * time.Hour, }, } @@ -2142,24 +2178,73 @@ func TestMaxDuration(t *testing.T) { clock := clockwork.NewFakeClock() now := clock.Now().UTC() identity := tlsca.Identity{ - Expires: now.Add(8 * time.Hour), + Expires: now.Add(defaultSessionTTL), } validator, err := NewRequestValidator(context.Background(), clock, g, tt.requestor, ExpandVars(true)) require.NoError(t, err) req.SetCreationTime(now) - req.SetMaxDuration(now.Add(tt.maxDuration)) + req.SetMaxDuration(now.Add(tt.requestedMaxDuration)) req.SetDryRun(tt.dryRun) require.NoError(t, validator.Validate(context.Background(), req, identity)) 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()) }) } } +// TestValidate_RequestedPendingTTLAndMaxDuration tests that both requested +// max duration and pending TTL is respected (given within limits). +func TestValidate_RequestedPendingTTLAndMaxDuration(t *testing.T) { + // describes a collection of roles and their conditions + roleDesc := roleTestSet{ + "requestRole": { + condition: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"requestRole"}, + MaxDuration: types.Duration(5 * day), + }, + }, + }, + } + + // describes a collection of users with various roles + userDesc := map[string][]string{ + "alice": {"requestRole"}, + } + + g := getMockGetter(t, roleDesc, userDesc) + req, err := types.NewAccessRequest("some-id", "alice", []string{"requestRole"}...) + require.NoError(t, err) + + clock := clockwork.NewFakeClock() + now := clock.Now().UTC() + defaultSessionTTL := 8 * time.Hour + identity := tlsca.Identity{ + Expires: now.Add(defaultSessionTTL), + } + + validator, err := NewRequestValidator(context.Background(), clock, g, "alice", ExpandVars(true)) + require.NoError(t, err) + + requestedMaxDuration := 4 * day + requestedPendingTTL := 2 * day + + req.SetCreationTime(now) + req.SetMaxDuration(now.Add(requestedMaxDuration)) + req.SetExpiry(now.Add(requestedPendingTTL)) + + require.NoError(t, validator.Validate(context.Background(), req, identity)) + require.Equal(t, now.Add(requestedMaxDuration), 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()) +} + type roleTestSet map[string]struct { condition types.RoleConditions options types.RoleOptions 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); From 4885dabf7e6025116bc328140232e27bd7f00140 Mon Sep 17 00:00:00 2001 From: Lisa Kim Date: Fri, 22 Mar 2024 14:47:32 -0700 Subject: [PATCH 2/2] Fix setting pending request TTL pass the access expiry time (#39548) * Prevent setting pending request TTL pass the access expiry time * Allow setting max duration less than sessionTTL when role max_duration is not set * Add more test, clarify comment * Remove unused params * Add a test for respecting both pending and max duration request * Add a fakeclock to fix access request time diff check differences --- lib/services/access_request.go | 8 ++------ lib/services/access_request_test.go | 2 +- tool/tsh/common/kube_test.go | 2 ++ tool/tsh/common/tsh_test.go | 7 ++++++- 4 files changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 5ae13c5081c8a..11a33990f26cf 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -33,7 +33,6 @@ import ( "github.com/gravitational/teleport/api/accessrequest" "github.com/gravitational/teleport/api/client" - "github.com/gravitational/teleport/api/client/proto" apidefaults "github.com/gravitational/teleport/api/defaults" "github.com/gravitational/teleport/api/types" apiutils "github.com/gravitational/teleport/api/utils" @@ -81,7 +80,7 @@ type ClusterGetter interface { // GetClusterName returns the local cluster name GetClusterName(opts ...MarshalOption) (types.ClusterName, error) // GetRemoteCluster returns a remote cluster by name - GetRemoteCluster(ctx context.Context, clusterName string) (types.RemoteCluster, error) + GetRemoteCluster(clusterName string) (types.RemoteCluster, error) } // ValidateAccessRequestClusterNames checks that the clusters in the access request exist @@ -98,7 +97,7 @@ func ValidateAccessRequestClusterNames(cg ClusterGetter, ar types.AccessRequest) if resourceID.ClusterName == localClusterName.GetClusterName() { continue } - _, err := cg.GetRemoteCluster(context.TODO(), resourceID.ClusterName) + _, err := cg.GetRemoteCluster(resourceID.ClusterName) if err != nil && !trace.IsNotFound(err) { return trace.Wrap(err, "failed to fetch remote cluster %q", resourceID.ClusterName) } @@ -169,9 +168,6 @@ func (r *RequestIDs) IsEmpty() bool { type AccessRequestGetter interface { // GetAccessRequests gets all currently active access requests. GetAccessRequests(ctx context.Context, filter types.AccessRequestFilter) ([]types.AccessRequest, error) - - // ListAccessRequests is an access request getter with pagination and sorting options. - ListAccessRequests(ctx context.Context, req *proto.ListAccessRequestsRequest) (*proto.ListAccessRequestsResponse, error) } // DynamicAccessCore is the core functionality common to all DynamicAccess implementations. diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index 3c99d28114f74..4f2f931e2655d 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -1907,7 +1907,7 @@ func (mcg mockClusterGetter) GetClusterName(opts ...MarshalOption) (types.Cluste return mcg.localCluster, nil } -func (mcg mockClusterGetter) GetRemoteCluster(ctx context.Context, clusterName string) (types.RemoteCluster, error) { +func (mcg mockClusterGetter) GetRemoteCluster(clusterName string) (types.RemoteCluster, error) { if cluster, ok := mcg.remoteClusters[clusterName]; ok { return cluster, nil } diff --git a/tool/tsh/common/kube_test.go b/tool/tsh/common/kube_test.go index ac19165591d30..c35d380fc830c 100644 --- a/tool/tsh/common/kube_test.go +++ b/tool/tsh/common/kube_test.go @@ -37,6 +37,7 @@ 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" @@ -365,6 +366,7 @@ func TestKubeSelection(t *testing.T) { cfg.Kube.ResourceMatchers = []services.ResourceMatcher{{ Labels: map[string]apiutils.Strings{"*": {"*"}}, }} + cfg.Clock = clockwork.NewFakeClock() }), ) kubeBarEKS := "bar-eks-us-west-1-123456789012" diff --git a/tool/tsh/common/tsh_test.go b/tool/tsh/common/tsh_test.go index 884381cef96e2..5f10a8b2ce689 100644 --- a/tool/tsh/common/tsh_test.go +++ b/tool/tsh/common/tsh_test.go @@ -47,6 +47,7 @@ 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" @@ -1853,7 +1854,11 @@ func TestSSHAccessRequest(t *testing.T) { } alice.SetTraits(traits) - rootAuth, rootProxy := makeTestServers(t, withBootstrap(requester, nodeAccessRole, connector, alice)) + rootAuth, rootProxy := makeTestServers(t, + withBootstrap(requester, nodeAccessRole, connector, alice), + withConfig(func(cfg *servicecfg.Config) { + cfg.Clock = clockwork.NewFakeClock() + })) authAddr, err := rootAuth.AuthAddr() require.NoError(t, err)