diff --git a/lib/services/access_request.go b/lib/services/access_request.go index 2d5c407f05e..ba88ed32318 100644 --- a/lib/services/access_request.go +++ b/lib/services/access_request.go @@ -1393,17 +1393,14 @@ func (v *RequestValidator) isReasonRequired(ctx context.Context, requestedRoles 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() { - return 0, nil - } - maxDuration := maxDurationTime.Sub(req.GetCreationTime()) // For dry run requests, use the maximum possible duration. // This prevents the time drift that can occur as the value is set on the client side. if req.GetDryRun() { maxDuration = MaxAccessDuration - } else if maxDuration < 0 { + // maxDuration may end up < 0 even if maxDurationTime is set + } else if !maxDurationTime.IsZero() && maxDuration < 0 { return 0, trace.BadParameter("invalid maxDuration: must be greater than creation time") } @@ -1411,26 +1408,19 @@ func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest, s return 0, trace.BadParameter("max_duration must be less than or equal to %v", MaxAccessDuration) } - minAdjDuration := maxDuration + var minAdjDuration time.Duration // Adjust the expiration time if the max_duration value is set on the request role. for _, roleName := range req.GetRoles() { - var maxDurationForRole time.Duration - for _, tms := range m.MaxDurationMatchers { - for _, matcher := range tms.Matchers { - if matcher.Match(roleName) { - if tms.MaxDuration > maxDurationForRole { - maxDurationForRole = tms.MaxDuration - } - } - } - } - - if maxDurationForRole < minAdjDuration { + maxDurationForRole := m.maxDurationForRole(roleName) + if minAdjDuration == 0 || maxDurationForRole < minAdjDuration { minAdjDuration = maxDurationForRole } } + if !maxDurationTime.IsZero() && maxDuration < minAdjDuration { + minAdjDuration = maxDuration + } - // minAdjDuration can end up being 0, if any role does not have + // minAdjDuration can end up being 0, if no role has a // field `max_duration` defined. // In this case, return the smaller value between the sessionTTL // and the requested max duration. @@ -1441,6 +1431,20 @@ func (m *RequestValidator) calculateMaxAccessDuration(req types.AccessRequest, s return minAdjDuration, nil } +func (m *RequestValidator) maxDurationForRole(roleName string) time.Duration { + var maxDurationForRole time.Duration + for _, tms := range m.MaxDurationMatchers { + for _, matcher := range tms.Matchers { + if matcher.Match(roleName) { + if tms.MaxDuration > maxDurationForRole { + maxDurationForRole = tms.MaxDuration + } + } + } + } + return maxDurationForRole +} + // 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 requestTTL and the // access request access expiry. diff --git a/lib/services/access_request_test.go b/lib/services/access_request_test.go index ea819059e5e..3e5cd903861 100644 --- a/lib/services/access_request_test.go +++ b/lib/services/access_request_test.go @@ -2673,6 +2673,14 @@ func TestValidate_RequestedMaxDuration(t *testing.T) { }, }, }, + "shortMaxDurationReqRole3": { + condition: types.RoleConditions{ + Request: &types.AccessRequestConditions{ + Roles: []string{"requestedRole"}, + MaxDuration: types.Duration(day), + }, + }, + }, } // describes a collection of users with various roles @@ -2681,6 +2689,7 @@ func TestValidate_RequestedMaxDuration(t *testing.T) { "bob": {"defaultRole"}, "carol": {"shortMaxDurationReqRole", "shortMaxDurationReqRole2"}, "david": {"maxDurationReqRole"}, + "erin": {"shortMaxDurationReqRole", "shortMaxDurationReqRole3", "maxDurationReqRole"}, } defaultSessionTTL := 8 * time.Hour @@ -2724,14 +2733,6 @@ func TestValidate_RequestedMaxDuration(t *testing.T) { expectedSessionTTL: 8 * time.Hour, dryRun: true, }, - { - 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: "when role max_duration is not set: default to defaultSessionTTL when requestedMaxDuration is not set", requestor: "bob", @@ -2812,6 +2813,25 @@ func TestValidate_RequestedMaxDuration(t *testing.T) { expectedPendingTTL: day, expectedSessionTTL: 8 * time.Hour, }, + { + desc: "role max_duration is respected when requestedMaxDuration is not set", + requestor: "alice", // has shortMaxDurationReqRole role assigned + roles: []string{"requestedRole"}, // caps max_duration to 3 days + expectedAccessDuration: 3 * day, + expectedPendingTTL: 3 * day, + expectedSessionTTL: 8 * time.Hour, + }, + { + desc: "multiple roles with different max durations assigned", + // erin has the maxDurationReqRole (7 days) role assigned + // along with 2 other roles with shorter durations + // shortMaxDurationReqRole (3 days) and shortMaxDurationReqRole3 (1 day) + requestor: "erin", + roles: []string{"requestedRole"}, // caps max_duration to 7 days + expectedAccessDuration: 7 * day, + expectedPendingTTL: 7 * day, + expectedSessionTTL: 8 * time.Hour, + }, } for _, tt := range tts { @@ -2832,7 +2852,9 @@ func TestValidate_RequestedMaxDuration(t *testing.T) { require.NoError(t, err) req.SetCreationTime(now) - req.SetMaxDuration(now.Add(tt.requestedMaxDuration)) + if tt.requestedMaxDuration != 0 { + req.SetMaxDuration(now.Add(tt.requestedMaxDuration)) + } req.SetDryRun(tt.dryRun) require.NoError(t, validator.Validate(context.Background(), req, identity))