Add max duration option to access request (#29042)

* "Add 'persist' attribute to time-limited access requests

A 'persist' attribute was added to access requests, specifying how long the access will be granted for.

* Add comments
Remove unused code

* Add new role and refactor testing for persist durations

Included a new role, 'requestedRole2' to increase the diversity of the tests. Also refactored the testing descriptions for 'persist can exceed maxTTL' and 'persist shorter than maxTTL' to the user 'bob'. Added new test situations considering the role's maximum time to live. The purpose of these edits is to ensure that only the required roles are taken into account for persist duration, and this needs to be properly reflected in our tests.

* Initialize role persist cache

This change allows the initialization of the role persist cache under the local user role map.
This is necessary because previously, if the map was not initialized, it could result in null pointer exceptions when attempting to add entries. Now, the map is guaranteed to be initialized prior to adding any roles.

* Replace "Persist" with "MaxDuration" in various files

Renamed the "Persist" field in a number of auth and tsh related files to "MaxDuration". This name change more accurately reflects the nature of these fields, namely how long certain types of access should be granted for.

* "Rename 'persist' to 'maxDuration' in access request logic"

This commit renames all variables, parameters, and functions called 'persist' to 'maxDuration' and modifies the relevant comments and error messages in the access request logic in the services library. The aim is to make the naming more descriptive and meaningful, which could lead to more maintainable code. Specifically, 'maxDuration' is a more accurate description of the maximum duration an access request can be active and granted for. 'persist' may be easily misconstrued as relating to data persistence, which is not the intention.

* Refactor 'persist' to 'maxDuration' in access request code

Changes have been made to the variable name 'persist' in the access request code, renaming it to 'maxDuration'. This change is aimed towards improving code readability and determining the function of the variable more clearly. The adjusted name 'maxDuration' reflects its actual purpose - limiting the maximum duration of access requests. This change affects the 'access_request.go' file and the respective tests in 'tsh_test.go'.

* Apply code review suggestions

* Refactor max duration calculation in access requests

Add more tests
This commit is contained in:
Jakub Nyckowski
2023-07-20 22:36:35 +00:00
committed by GitHub
parent 5adceac922
commit 7845b979d5
11 changed files with 2828 additions and 2363 deletions
@@ -30,7 +30,7 @@ option (gogoproto.unmarshaler_all) = true;
// Metadata is a common event metadata
message Metadata {
// Index is a monotonicaly incremented index in the event sequence
// Index is a monotonically incremented index in the event sequence
int64 Index = 1 [(gogoproto.jsontag) = "ei"];
// Type is the event type
@@ -53,7 +53,7 @@ message Metadata {
string ClusterName = 6 [(gogoproto.jsontag) = "cluster_name,omitempty"];
}
// SesssionMetadata is a common session event metadata
// SessionMetadata is a common session event metadata
message SessionMetadata {
// SessionID is a unique UUID of the session.
string SessionID = 1 [(gogoproto.jsontag) = "sid"];
@@ -328,7 +328,7 @@ message SessionJoin {
}
// SessionPrint event happens every time a write occurs to
// temirnal I/O during a session
// terminal I/O during a session
message SessionPrint {
// Metadata is a common event metadata
Metadata Metadata = 1 [
@@ -337,7 +337,7 @@ message SessionPrint {
(gogoproto.jsontag) = ""
];
// ChunkIndex is a monotonicaly incremented index for ordering print events
// ChunkIndex is a monotonically incremented index for ordering print events
int64 ChunkIndex = 2 [(gogoproto.jsontag) = "ci"];
// Data is data transferred, it is not marshaled to JSON format
@@ -1089,7 +1089,7 @@ message SessionLeave {
];
}
// UserLogin records a successfull or failed user login event
// UserLogin records a successfully or failed user login event
message UserLogin {
// Metadata is a common event metadata
Metadata Metadata = 1 [
@@ -1277,7 +1277,7 @@ message AccessRequestCreate {
string Reason = 8 [(gogoproto.jsontag) = "reason,omitempty"];
// Annotations is an optional set of attributes supplied by a plugin during
// approval/denail of the request.
// approval/denial of the request.
google.protobuf.Struct Annotations = 9 [
(gogoproto.jsontag) = "annotations,omitempty",
(gogoproto.casttype) = "Struct"
@@ -1295,6 +1295,13 @@ message AccessRequestCreate {
(gogoproto.jsontag) = "resource_ids,omitempty",
(gogoproto.nullable) = false
];
// MaxDuration indicates how long the access should be granted for.
google.protobuf.Timestamp MaxDuration = 13 [
(gogoproto.stdtime) = true,
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "max_duration,omitempty"
];
}
// ResourceID is a unique identifier for a teleport resource. This is duplicated
@@ -2207,6 +2207,13 @@ message AccessRequestSpecV3 {
// DryRun indicates that the request should not actually be created, the
// auth server should only validate the access request.
bool DryRun = 16 [(gogoproto.jsontag) = "dry_run,omitempty"];
// MaxDuration indicates how long the access should be granted for.
google.protobuf.Timestamp MaxDuration = 17 [
(gogoproto.stdtime) = true,
(gogoproto.nullable) = false,
(gogoproto.jsontag) = "max_duration,omitempty"
];
}
// AccessRequestFilter encodes filter params for access requests.
@@ -2861,6 +2868,13 @@ message AccessRequestConditions {
// defines the underlying roles which will be requested as part of any
// Resource Access Request.
repeated string SearchAsRoles = 6 [(gogoproto.jsontag) = "search_as_roles,omitempty"];
// MaxDuration is the amount of time the access will be granted for.
// If this is zero, the default duration is used.
int64 MaxDuration = 7 [
(gogoproto.jsontag) = "max_duration,omitempty",
(gogoproto.casttype) = "Duration"
];
}
// AccessReviewConditions is a matcher for allow/deny restrictions on
+14
View File
@@ -98,6 +98,10 @@ type AccessRequest interface {
GetLoginHint() string
// SetLoginHint sets the requested login hint.
SetLoginHint(string)
// GetMaxDuration gets the maximum time at which the access should be approved for.
GetMaxDuration() time.Time
// SetMaxDuration sets the maximum time at which the access should be approved for.
SetMaxDuration(time.Time)
// GetDryRun returns true if this request should not be created and is only
// a dry run to validate request capabilities.
GetDryRun() bool
@@ -414,6 +418,16 @@ func (r *AccessRequestV3) GetDryRun() bool {
return r.Spec.DryRun
}
// GetMaxDuration gets the maximum time at which the access should be approved for.
func (r *AccessRequestV3) GetMaxDuration() time.Time {
return r.Spec.MaxDuration
}
// SetMaxDuration sets the maximum time at which the access should be approved for.
func (r *AccessRequestV3) SetMaxDuration(t time.Time) {
r.Spec.MaxDuration = t
}
// SetDryRun sets the dry run flag on the request.
func (r *AccessRequestV3) SetDryRun(dryRun bool) {
r.Spec.DryRun = dryRun
File diff suppressed because it is too large Load Diff
+1654 -1574
View File
File diff suppressed because it is too large Load Diff
+2
View File
@@ -4093,6 +4093,7 @@ func (a *Server) CreateAccessRequest(ctx context.Context, req types.AccessReques
RequestID: req.GetName(),
RequestState: req.GetState().String(),
Reason: req.GetRequestReason(),
MaxDuration: req.GetMaxDuration(),
})
if err != nil {
log.WithError(err).Warn("Failed to emit access request create event.")
@@ -4194,6 +4195,7 @@ func (a *Server) SubmitAccessReview(ctx context.Context, params types.AccessRevi
ProposedState: params.Review.ProposedState.String(),
Reason: params.Review.Reason,
Reviewer: params.Review.Author,
MaxDuration: req.GetMaxDuration(),
}
if len(params.Review.Annotations) > 0 {
+91 -13
View File
@@ -42,6 +42,13 @@ import (
const maxAccessRequestReasonSize = 4096
// A day is sometimes 23 hours, sometimes 25 hours, usually 24 hours.
const day = 24 * time.Hour
// maxAccessDuration is the maximum duration that an access request can be
// granted for.
const maxAccessDuration = 7 * day
// ValidateAccessRequest validates the AccessRequest and sets default values
func ValidateAccessRequest(ar types.AccessRequest) error {
if err := ar.CheckAndSetDefaults(); err != nil {
@@ -353,6 +360,11 @@ func ValidateAccessPredicates(role types.Role) error {
}
}
if maxDuration := role.GetAccessRequestConditions(types.Allow).MaxDuration; maxDuration.Duration() != 0 &&
maxDuration.Duration() > maxAccessDuration {
return trace.BadParameter("max access duration must be less or equal 7 days")
}
return nil
}
@@ -531,7 +543,7 @@ type requestResolution struct {
}
// calculateReviewBasedResolution calculates the request resolution based upon
// a request's reviews. Returns (nil,nil) in the event no resolution has been reached.
// a request's reviews. Returns (nil,nil) in the event no resolution has been reached.
func calculateReviewBasedResolution(req types.AccessRequest) (*requestResolution, error) {
// thresholds and reviews must be populated before state-transitions are possible
thresholds, reviews := req.GetThresholds(), req.GetReviews()
@@ -741,7 +753,7 @@ func insertAnnotations(annotations map[string][]string, conditions types.AccessR
}
}
// ReviewPermissionChecker is a helper for validating whether or not a user
// ReviewPermissionChecker is a helper for validating whether a user
// is allowed to review specific access requests.
type ReviewPermissionChecker struct {
User types.User
@@ -914,9 +926,9 @@ func (c *ReviewPermissionChecker) push(role types.Role) error {
}
// RequestValidator a helper for validating access requests.
// a user's statically assigned roles are are "added" to the
// a user's statically assigned roles are "added" to the
// validator via the push() method, which extracts all the
// relevant rules, peforms variable substitutions, and builds
// relevant rules, performs variable substitutions, and builds
// a set of simple Allow/Deny datastructures. These, in turn,
// are used to validate and expand the access request.
type RequestValidator struct {
@@ -940,10 +952,14 @@ type RequestValidator struct {
Matchers []parse.Matcher
Thresholds []types.AccessReviewThreshold
}
SuggestedReviewers []string
SuggestedReviewers []string
MaxDurationMatchers []struct {
Matchers []parse.Matcher
MaxDuration time.Duration
}
}
// NewRequestValidator configures a new RequestValidor for the specified user.
// NewRequestValidator configures a new RequestValidator for the specified user.
func NewRequestValidator(ctx context.Context, clock clockwork.Clock, getter RequestValidatorGetter, username string, opts ...ValidateRequestOption) (RequestValidator, error) {
user, err := getter.GetUser(username, false)
if err != nil {
@@ -1026,7 +1042,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
if !m.CanSearchAsRole(roleName) {
// Roles are normally determined automatically for resource
// access requests, this role must have been explicitly
// requested or a new deny rule has since been added.
// requested, or a new deny rule has since been added.
return trace.BadParameter("user %q can not request role %q", req.GetUser(), roleName)
}
} else {
@@ -1075,7 +1091,7 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
now := m.clock.Now().UTC()
// Calculate expiration time of the Access Request (how long it
// Calculate the expiration time of the Access Request (how long it
// will await approval).
ttl, err := m.requestTTL(ctx, identity, req)
if err != nil {
@@ -1083,18 +1099,70 @@ func (m *RequestValidator) Validate(ctx context.Context, req types.AccessRequest
}
req.SetExpiry(now.Add(ttl))
// Calculate expiration time of the elevated certificate that will
// be issued if the Access Request is approved.
ttl, err = m.sessionTTL(ctx, identity, req)
maxDuration, err := m.calculateMaxAccessDuration(req)
if err != nil {
return trace.Wrap(err)
}
req.SetAccessExpiry(now.Add(ttl))
// If the maxDuration flag is set, use it instead of the session TTL.
if maxDuration > 0 {
ttl = maxDuration
} else {
// Calculate the expiration time of the elevated certificate that will
// be issued if the Access Request is approved.
ttl, err = m.sessionTTL(ctx, identity, req)
if err != nil {
return trace.Wrap(err)
}
}
accessTTL := now.Add(ttl)
req.SetAccessExpiry(accessTTL)
}
return nil
}
// 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) {
// Check if the maxDuration time is set.
maxDurationTime := req.GetMaxDuration()
if maxDurationTime.IsZero() {
return 0, nil
}
maxDuration := maxDurationTime.Sub(req.GetCreationTime())
if maxDuration < 0 {
return 0, trace.BadParameter("invalid maxDuration: must be greater than creation time")
}
if maxDuration > maxAccessDuration {
return 0, trace.BadParameter("max_duration must be less or equal 7 days")
}
minAdjDuration := maxDuration
// 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 {
minAdjDuration = maxDurationForRole
}
}
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) {
@@ -1181,7 +1249,7 @@ func (m *RequestValidator) truncateTTL(ctx context.Context, identity tlsca.Ident
// 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 againt a known role list.
// when checking against a known role list.
func (m *RequestValidator) GetRequestableRoles() ([]string, error) {
allRoles, err := m.getter.GetRoles(context.TODO())
if err != nil {
@@ -1247,6 +1315,16 @@ func (m *RequestValidator) push(role types.Role) error {
})
}
if allow.MaxDuration != 0 {
m.MaxDurationMatchers = append(m.MaxDurationMatchers, struct {
Matchers []parse.Matcher
MaxDuration time.Duration
}{
Matchers: newMatchers,
MaxDuration: allow.MaxDuration.Duration(),
})
}
// validation process for incoming access requests requires
// generating system annotations to be attached to the request
// before it is inserted into the backend.
+212 -2
View File
@@ -333,7 +333,7 @@ func TestReviewThresholds(t *testing.T) {
author: g.user(t, "proletariat"),
propose: approve,
},
{ // 1 of 2 required denials (does not triger threshold)
{ // 1 of 2 required denials (does not trigger a threshold)
author: g.user(t, "proletariat"),
propose: deny,
},
@@ -368,7 +368,7 @@ func TestReviewThresholds(t *testing.T) {
author: g.user(t, "proletariat"),
noReview: true,
},
{ // 1 of 2 required denials for "coup" (does not triger threshold)
{ // 1 of 2 required denials for "coup" (does not trigger a threshold)
author: g.user(t, "military"),
propose: deny,
},
@@ -1869,6 +1869,216 @@ func TestGetResourceDetails(t *testing.T) {
require.Equal(t, "friendly group 1", details[types.ResourceIDToString(resourceIDs[4])].FriendlyName)
}
func TestMaxDuration(t *testing.T) {
// describes a collection of roles and their conditions
roleDesc := roleTestSet{
"requestedRole": {
// ...
},
"requestedRole2": {
// ...
},
"setMaxTTLRole": {
options: types.RoleOptions{
MaxSessionTTL: types.Duration(8 * time.Hour),
},
},
"defaultRole": {
condition: types.RoleConditions{
Request: &types.AccessRequestConditions{
Roles: []string{"requestedRole", "setMaxTTLRole"},
},
},
},
"defaultShortRole": {
condition: types.RoleConditions{
Request: &types.AccessRequestConditions{
Roles: []string{"requestedRole", "setMaxTTLRole"},
},
},
},
"maxDurationReqRole": {
condition: types.RoleConditions{
Request: &types.AccessRequestConditions{
Roles: []string{"requestedRole", "setMaxTTLRole"},
MaxDuration: types.Duration(7 * day),
},
},
},
"shortMaxDurationReqRole": {
condition: types.RoleConditions{
Request: &types.AccessRequestConditions{
Roles: []string{"requestedRole"},
MaxDuration: types.Duration(3 * day),
},
},
},
"shortMaxDurationReqRole2": {
condition: types.RoleConditions{
Request: &types.AccessRequestConditions{
Roles: []string{"requestedRole2"},
MaxDuration: types.Duration(day),
},
},
},
}
// describes a collection of users with various roles
userDesc := map[string][]string{
"alice": {"shortMaxDurationReqRole"},
"bob": {"defaultRole"},
"carol": {"shortMaxDurationReqRole", "shortMaxDurationReqRole2"},
"david": {"maxDurationReqRole"},
}
g := getMockGetter(t, roleDesc, userDesc)
tts := []struct {
// desc is a short description of the test scenario (should be unique)
desc string
// requestor is the name of the requesting user
requestor string
// the roles to be requested (defaults to "dictator")
roles []string
// maxDuration is the requested maxDuration duration
maxDuration time.Duration
// expectedAccessDuration is the expected access duration
expectedAccessDuration time.Duration
}{
{
desc: "role maxDuration is respected",
requestor: "alice",
roles: []string{"requestedRole"},
maxDuration: 7 * day,
expectedAccessDuration: 3 * day,
},
{
desc: "maxDuration not set, default maxTTL (8h)",
requestor: "bob",
roles: []string{"requestedRole"},
expectedAccessDuration: 8 * time.Hour,
},
{
desc: "maxDuration inside request is respected",
requestor: "bob",
roles: []string{"requestedRole"},
maxDuration: 5 * time.Hour,
expectedAccessDuration: 8 * time.Hour,
},
{
desc: "users with no MaxDuration are constrained by normal maxTTL logic",
requestor: "bob",
roles: []string{"requestedRole"},
maxDuration: 2 * day,
expectedAccessDuration: 8 * time.Hour,
},
{
desc: "maxDuration can't exceed maxTTL by default",
requestor: "bob",
roles: []string{"setMaxTTLRole"},
maxDuration: day,
expectedAccessDuration: 8 * time.Hour,
},
{
desc: "maxDuration is ignored if max_duration is not set in role",
requestor: "bob",
roles: []string{"setMaxTTLRole"},
maxDuration: 2 * time.Hour,
expectedAccessDuration: 8 * time.Hour,
},
{
desc: "maxDuration can exceed maxTTL if max_duration is set in role",
requestor: "david",
roles: []string{"setMaxTTLRole"},
maxDuration: day,
expectedAccessDuration: day,
},
{
desc: "maxDuration shorter than maxTTL if max_duration is set in role",
requestor: "david",
roles: []string{"setMaxTTLRole"},
maxDuration: 2 * time.Hour,
expectedAccessDuration: 2 * time.Hour,
},
{
desc: "only required roles are considered for maxDuration",
requestor: "carol",
roles: []string{"requestedRole"},
maxDuration: 5 * day,
expectedAccessDuration: 3 * day,
},
{
desc: "only required roles are considered for maxDuration #2",
requestor: "carol",
roles: []string{"requestedRole2"},
maxDuration: 6 * day,
expectedAccessDuration: day,
},
}
for _, tt := range tts {
t.Run(tt.desc, func(t *testing.T) {
require.GreaterOrEqual(t, len(tt.roles), 1, "at least one role must be specified")
// create a request for the specified author
req, err := types.NewAccessRequest("some-id", tt.requestor, tt.roles...)
require.NoError(t, err)
clock := clockwork.NewFakeClock()
now := clock.Now().UTC()
identity := tlsca.Identity{
Expires: now.Add(8 * time.Hour),
}
validator, err := NewRequestValidator(context.Background(), clock, g, tt.requestor, ExpandVars(true))
require.NoError(t, err)
req.SetCreationTime(now)
req.SetMaxDuration(now.Add(tt.maxDuration))
require.NoError(t, validator.Validate(context.Background(), req, identity))
require.Equal(t, now.Add(tt.expectedAccessDuration), req.GetAccessExpiry())
})
}
}
type roleTestSet map[string]struct {
condition types.RoleConditions
options types.RoleOptions
}
func getMockGetter(t *testing.T, roleDesc roleTestSet, userDesc map[string][]string) *mockGetter {
t.Helper()
roles := make(map[string]types.Role)
for name, desc := range roleDesc {
role, err := types.NewRole(name, types.RoleSpecV6{
Allow: desc.condition,
Options: desc.options,
})
require.NoError(t, err)
roles[name] = role
}
users := make(map[string]types.User)
for name, roles := range userDesc {
user, err := types.NewUser(name)
require.NoError(t, err)
user.SetRoles(roles)
users[name] = user
}
g := &mockGetter{
roles: roles,
users: users,
}
return g
}
func newNode(t *testing.T, name, hostname string) types.Server {
node, err := types.NewServer(name, types.KindNode,
types.ServerSpecV2{
+4
View File
@@ -212,6 +212,10 @@ func printRequest(cf *CLIConf, req types.AccessRequest) error {
}
table.AddRow([]string{"Reason:", reason})
table.AddRow([]string{"Reviewers:", reviewers + " (suggested)"})
if !req.GetAccessExpiry().IsZero() {
// Display the expiry time in the local timezone. UTC is confusing.
table.AddRow([]string{"Access Expires:", req.GetAccessExpiry().Local().Format(time.DateTime)})
}
table.AddRow([]string{"Status:", req.GetState().String()})
_, err := table.AsBuffer().WriteTo(cf.Stdout())
+9 -1
View File
@@ -321,6 +321,9 @@ type CLIConf struct {
// be issued if the Access Request is approved.
SessionTTL time.Duration
// MaxDuration specifies how long the access will be granted for.
MaxDuration time.Duration
// executablePath is the absolute path to the current executable.
executablePath string
@@ -1000,6 +1003,7 @@ func Run(ctx context.Context, args []string, opts ...CliOption) error {
reqCreate.Flag("resource", "Resource ID to be requested").StringsVar(&cf.RequestedResourceIDs)
reqCreate.Flag("request-ttl", "Expiration time for the access request").DurationVar(&cf.RequestTTL)
reqCreate.Flag("session-ttl", "Expiration time for the elevated certificate").DurationVar(&cf.SessionTTL)
reqCreate.Flag("max-duration", "How long the the access should be granted for").DurationVar(&cf.MaxDuration)
reqReview := req.Command("review", "Review an access request.")
reqReview.Arg("request-id", "ID of target request").Required().StringVar(&cf.RequestID)
@@ -2409,7 +2413,7 @@ func createAccessRequest(cf *CLIConf) (types.AccessRequest, error) {
req.SetSuggestedReviewers(reviewers)
// Only set RequestTTL and SessionTTL if values are greater than zero.
// Otherwise leave defaults and the server will take the zero values and
// Otherwise, leave defaults, and the server will take the zero values and
// transform them into default expirations accordingly.
if cf.RequestTTL > 0 {
req.SetExpiry(time.Now().UTC().Add(cf.RequestTTL))
@@ -2417,6 +2421,10 @@ func createAccessRequest(cf *CLIConf) (types.AccessRequest, error) {
if cf.SessionTTL > 0 {
req.SetAccessExpiry(time.Now().UTC().Add(cf.SessionTTL))
}
if cf.MaxDuration > 0 {
// Time will be relative to the approval time instead of the request time.
req.SetMaxDuration(time.Now().UTC().Add(cf.MaxDuration))
}
return req, nil
}
+2 -1
View File
@@ -4123,7 +4123,8 @@ func TestSerializeAccessRequests(t *testing.T) {
],
"state": 1,
"created": "0001-01-01T00:00:00Z",
"expires": "0001-01-01T00:00:00Z"
"expires": "0001-01-01T00:00:00Z",
"max_duration": "0001-01-01T00:00:00Z"
}
}
`