Fix Assume Start Time Validation #39008

kimlisa · 2024-03-06T07:22:58Z

While testing i ran into some issues. Fixes the following:

Set modified start time from tctl requests approve (we allowed modifying in client, but was not being set in the backend)
Removed incorrect validation in the client, and placed validation near backend (this also avoids us having to repeat validation for other clients like teleterm and web)
Correct calculation of maxest start time allowed in the error message when user goes too far
Correct start time validation
- Start time should be after or equal to request creation time
  - Note that creation time will always be set before validation starts so it’ll never be a zero value
    - When creating, we validate after the creation time was set
    - When reviewing or updating, we validate after we retrieve the created request
- Start time should be before access expiry
- Start time cannot exceed constants.MaxAssumeStartTimeDuration FROM the creation date (eg: if the const was 1 week max, then the start time cannot exceed one week from creation time, even if access expiry is set for 2 weeks)
When printing tsh request, use local date time format to display start time, since access expiry also uses the same format (it was confusing to display two different time formats)

Extra context, we allow modifying assume start time in the following places and is where the validations are placed:

CreateAccessRequestV2
SubmitAccessReview
SetAccessRequestState (used with tctl)

Manual Testing:

# Starting profile:
> Profile URL:        https://proxy.0.0.0.0.nip.io:3080
  Logged in as:       sevy
  Cluster:            im-a-cluster-name
  Roles:              requester
  Kubernetes:         disabled
  Valid until:        2024-03-06 10:59:38 -0800 PST [valid for 12h0m0s]
  Extensions:         login-ip, permit-port-forwarding, permit-pty, private-key-policy

# requesting pass the expiry
$ ./tsh request create --roles=access --assume-start-time=2024-03-09T23:20:50.52Z 
Creating request...
ERROR: assume start time cannot equal or exceed access expiry time at: "2024-03-06T18:59:37Z"

# requesting before the creation time
$ ./tsh request create --roles=access --assume-start-time=2023-03-09T23:20:50.52Z 
Creating request...
ERROR: assume start time has to be greater than: "2024-03-06T07:00:06Z"

# requesting too far into future (currently the const limits to 1 week)
# the max duration was increased in the role to 14 days
$ ./tsh request create --roles=access --assume-start-time=2024-03-13T23:20:50.52Z --max-duration=12d
Creating request...
ERROR: assume start time is too far in the future, latest time allowed "2024-03-13T07:04:25Z"

# valid request and consistent time display
$ ./tsh request create --roles=access --assume-start-time=2024-03-10T23:20:50.52Z --max-duration=13d
Creating request...
Request ID:        018e1294-df3d-7ae5-a00d-7a964e39a54f 
Username:          sevy                                 
Roles:             access                               
Reason:            [none]                               
Reviewers:         [none] (suggested)                   
Access Expires:    2024-03-19 00:05:30                  
Assume Start Time: 2024-03-10 16:20:50                  
Status:            PENDING  

Waiting for request approval...

Approval received, getting updated certificates...

# approved with `tctl requests approve` with a modified start time
ERROR: access request "018e1294-df3d-7ae5-a00d-7a964e39a54f" can not be assumed until 2024-03-07 23:20:50.52 +0000 UTC

changelog: Fixes allowing invalid access request start time date to be set

kimlisa · 2024-03-06T07:33:51Z

my changelog is lame... i'm still thinking on making it clearer, or should I even add one? i highly doubt anyone's using this feature yet

api/types/access_request.go

zmb3 · 2024-03-06T15:16:52Z

api/types/access_request_test.go

 )

 func TestAssertAccessRequestImplementsResourceWithLabels(t *testing.T) {
 	ar, err := NewAccessRequest("test", "test", "test")
 	require.NoError(t, err)
 	require.Implements(t, (*ResourceWithLabels)(nil), ar)
 }
+
+func TestValidateAssumeStartTime(t *testing.T) {
+	clock := clockwork.NewFakeClock()


I don't think you really need a fake clock here, as the test is really about working with arbitrary time.Time values.

api/types/access_request_test.go

zmb3 · 2024-03-06T15:19:12Z

lib/auth/access_request_test.go

+
+	t.Cleanup(func() { require.NoError(t, requesterClient.Close()) })
+
+	clock := clockwork.NewFakeClock()


Do you need this clock? The clock that your fake server is using is going to be a different clock anyway.

lib/auth/access_request_test.go

lib/auth/auth.go

tool/tsh/common/access_request.go

jakule

A few nits. Overall LGTM

jakule · 2024-03-11T15:39:44Z

api/types/access_request_test.go

+
+func TestValidateAssumeStartTime(t *testing.T) {
+	creation := time.Now().UTC()
+	day := 24 * time.Hour


nit:

Suggested change

day := 24 * time.Hour

const day = 24 * time.Hour

jakule · 2024-03-11T15:40:14Z

api/types/access_request_test.go

+		{
+			name:      "start time too far in the future",
+			startTime: creation.Add(constants.MaxAssumeStartDuration + (1 * day)),
+			errCheck: func(tt require.TestingT, err error, i ...interface{}) {


nit:

Suggested change

errCheck: func(tt require.TestingT, err error, i ...interface{}) {

errCheck: func(tt require.TestingT, err error, i ...any) {

jakule · 2024-03-11T15:40:37Z

api/types/access_request_test.go

+	}{
+		{
+			name:      "start time too far in the future",
+			startTime: creation.Add(constants.MaxAssumeStartDuration + (1 * day)),


nit:

Suggested change

startTime: creation.Add(constants.MaxAssumeStartDuration + (1 * day)),

startTime: creation.Add(constants.MaxAssumeStartDuration + day),

jakule · 2024-03-11T15:42:44Z

lib/auth/access_request_test.go

+	createdRequest                types.AccessRequest
+}
+
+func createAccessRequestWithStartTime(t *testing.T) accessRequestWithStartTime {


nit: You can add t.Helper() to mark this as a helper function.

jakule · 2024-03-11T15:46:00Z

lib/services/access_request_test.go

@@ -644,7 +644,7 @@ func TestReviewThresholds(t *testing.T) {
 					propose:         approve,
 					assumeStartTime: clock.Now().UTC().Add(10000 * time.Hour),
 					errCheck: func(tt require.TestingT, err error, i ...interface{}) {
-						require.ErrorIs(tt, err, trace.BadParameter("request start time is after expiry"), i...)
+						require.Contains(tt, err.Error(), "assume start time must be prior to access expiry time", i...)


Use require.ErrorContains(t, err, "Error message...") as nil passed here will panic at e.Error() without any sensible error message.

zmb3

Approved with suggestions.

zmb3 · 2024-03-13T19:25:56Z

api/types/access_request.go

+func ValidateAssumeStartTime(assumeStartTime time.Time, accessExpiry time.Time, creationTime time.Time) error {
+	// Guard against requesting a start time before the request creation time.
+	if assumeStartTime.Before(creationTime) {
+		return trace.BadParameter("assume start time has to be after: %q", creationTime.Format(time.RFC3339))


Suggested change

return trace.BadParameter("assume start time has to be after: %q", creationTime.Format(time.RFC3339))

return trace.BadParameter("assume start time has to be after %v", creationTime.Format(time.RFC3339))

I don't know that quoting the creation time is necessary. The quote helps if we want to be able to identify an empty string, but formatting a time will always product a non-empty string.

I also don't think : is necessary - just make it read like a normal sentence.

(don't forget to update your tests if you decide to accept these suggestions)

zmb3 · 2024-03-13T19:26:05Z

api/types/access_request.go

+	}
+	// Guard against requesting a start time after access expiry.
+	if assumeStartTime.After(accessExpiry) || assumeStartTime.Equal(accessExpiry) {
+		return trace.BadParameter("assume start time must be prior to access expiry time at: %q",


Suggested change

return trace.BadParameter("assume start time must be prior to access expiry time at: %q",

return trace.BadParameter("assume start time must be prior to access expiry time at %v",

zmb3 · 2024-03-13T19:26:16Z

api/types/access_request.go

+	// should be on or before constants.MaxAssumeStartDuration.
+	maxAssumableStartTime := creationTime.Add(constants.MaxAssumeStartDuration)
+	if maxAssumableStartTime.Before(accessExpiry) && assumeStartTime.After(maxAssumableStartTime) {
+		return trace.BadParameter("assume start time is too far in the future, latest time allowed %q",


Suggested change

return trace.BadParameter("assume start time is too far in the future, latest time allowed %q",

return trace.BadParameter("assume start time is too far in the future, latest time allowed is %v",

zmb3 · 2024-03-13T19:27:41Z

api/types/access_request_test.go

+			name:      "start time too far in the future",
+			startTime: creation.Add(constants.MaxAssumeStartDuration + day),
+			errCheck: func(tt require.TestingT, err error, i ...any) {
+				require.True(tt, trace.IsBadParameter(err), "expected bad parameter, got %v", err)


Suggested change

require.True(tt, trace.IsBadParameter(err), "expected bad parameter, got %v", err)

The test still holds if you remove this line, right? (same below)

zmb3 · 2024-03-13T19:29:01Z

lib/auth/access_request_test.go

+		},
+	}
+	for _, tc := range testCases {
+		review.Review.AssumeStartTime = &tc.startTime


Can you use t.Run() here to run proper subtests? Otherwise if this test fails we will just see that there was an error on line 1603 and won't be obvious which of the test cases actually failed.

(same goes for below)

The following allows modifying the start time and is where validation is placed: - CreateAccessRequestV2 - SubmitAccessReview - SetAccessRequestState (used with tctl)

public-teleport-github-review-bot · 2024-03-14T03:57:05Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v14	Failed
branch/v15	Failed

* Fix Assume Start Time Validation (#39008) * Remove checking for max assume start time test (same as max duration)

github-actions bot requested a review from capnspacehook March 6, 2024 07:23

kimlisa requested a review from jakule March 6, 2024 07:23

github-actions bot requested a review from ryanclark March 6, 2024 07:23

github-actions bot added size/md tctl tctl - Teleport admin tool tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Mar 6, 2024

kimlisa requested review from fspmarshall and removed request for ryanclark and capnspacehook March 6, 2024 07:23

kimlisa changed the title ~~Fix Assume Start Time~~ Fix Assume Start Time Validation Mar 6, 2024

gravitational deleted a comment from github-actions bot Mar 6, 2024

kimlisa force-pushed the lisa/fix-assume-start-time branch from b3d57f8 to 14faaba Compare March 6, 2024 07:54

zmb3 reviewed Mar 6, 2024

View reviewed changes

kimlisa requested a review from zmb3 March 6, 2024 19:33

kimlisa force-pushed the lisa/fix-assume-start-time branch 2 times, most recently from 4d04132 to 9852ae8 Compare March 6, 2024 20:04

jakule approved these changes Mar 11, 2024

View reviewed changes

fspmarshall approved these changes Mar 13, 2024

View reviewed changes

zmb3 approved these changes Mar 13, 2024

View reviewed changes

kimlisa added 5 commits March 13, 2024 20:18

Remove incorrect client validation

fb4f261

Use consistent date time display

e0ace0a

Create start time validation helper

a8197de

Add validation checks

3629b2a

The following allows modifying the start time and is where validation is placed: - CreateAccessRequestV2 - SubmitAccessReview - SetAccessRequestState (used with tctl)

Address Crs

0e0c0d1

kimlisa force-pushed the lisa/fix-assume-start-time branch from 63f53fa to 0e0c0d1 Compare March 14, 2024 03:18

kimlisa added backport/branch/v14 backport/branch/v15 labels Mar 14, 2024

kimlisa enabled auto-merge March 14, 2024 03:19

kimlisa added this pull request to the merge queue Mar 14, 2024

Merged via the queue into master with commit c04ea7f Mar 14, 2024
38 checks passed

kimlisa deleted the lisa/fix-assume-start-time branch March 14, 2024 03:55

kimlisa added a commit that referenced this pull request Mar 14, 2024

Fix Assume Start Time Validation (#39008)

a712385

kimlisa mentioned this pull request Mar 14, 2024

[v15] Fix Assume Start Time Validation #39322

Merged

kimlisa added a commit that referenced this pull request Mar 14, 2024

Fix Assume Start Time Validation (#39008)

2163296

kimlisa mentioned this pull request Mar 14, 2024

[v14] Fix Assume Start Time Validation #39324

Merged

github-merge-queue bot pushed a commit that referenced this pull request Mar 14, 2024

Fix Assume Start Time Validation (#39008) (#39322)

de8b1e5

github-merge-queue bot pushed a commit that referenced this pull request Mar 23, 2024

[v14] Fix Assume Start Time Validation (#39324)

880ae37

* Fix Assume Start Time Validation (#39008) * Remove checking for max assume start time test (same as max duration)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Assume Start Time Validation #39008

Fix Assume Start Time Validation #39008

kimlisa commented Mar 6, 2024 •

edited

kimlisa commented Mar 6, 2024

zmb3 Mar 6, 2024

zmb3 Mar 6, 2024

jakule left a comment

jakule Mar 11, 2024

jakule Mar 11, 2024

jakule Mar 11, 2024

jakule Mar 11, 2024

jakule Mar 11, 2024

zmb3 left a comment

zmb3 Mar 13, 2024

zmb3 Mar 13, 2024

zmb3 Mar 13, 2024

zmb3 Mar 13, 2024

zmb3 Mar 13, 2024

public-teleport-github-review-bot bot commented Mar 14, 2024


		t.Cleanup(func() { require.NoError(t, requesterClient.Close()) })

		clock := clockwork.NewFakeClock()

	errCheck: func(tt require.TestingT, err error, i ...interface{}) {
	errCheck: func(tt require.TestingT, err error, i ...any) {

	startTime: creation.Add(constants.MaxAssumeStartDuration + (1 * day)),
	startTime: creation.Add(constants.MaxAssumeStartDuration + day),

	return trace.BadParameter("assume start time has to be after: %q", creationTime.Format(time.RFC3339))
	return trace.BadParameter("assume start time has to be after %v", creationTime.Format(time.RFC3339))

Fix Assume Start Time Validation #39008

Fix Assume Start Time Validation #39008

Conversation

kimlisa commented Mar 6, 2024 • edited

kimlisa commented Mar 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakule left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmb3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

public-teleport-github-review-bot bot commented Mar 14, 2024

kimlisa commented Mar 6, 2024 •

edited