gravitational/teleport
1
0
Fork 0
mirror of https://github.com/gravitational/teleport.git synced 2026-10-12 02:15:01 +00:00
Code Issues Packages Projects Releases Wiki Activity

Make resolved access requests immutable (#66736)

Browse Source 
Prevent access requests in APPROVED, DENIED, or PROMOTED states from
being updated through SetAccessRequestState, including same-state
updates that could otherwise mutate resolution metadata.
This commit is contained in:
rosstimothy
2026-05-28 14:11:41 +00:00
committed by GitHub
parent 520546d031
commit e82eceb456
4 changed files with 269 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files
api/types/access_request.go
+3 -5
View File
@@ -291,12 +291,10 @@ func (r *AccessRequestV3) GetState() RequestState {
// SetState sets State
func (r *AccessRequestV3) SetState(state RequestState) error {
if r.Spec.State.IsDenied() {
if state.IsDenied() {
return nil
}
return trace.BadParameter("cannot set request-state %q (already denied)", state.String())
if r.GetState() != state && r.GetState().IsResolved() {
return trace.BadParameter("cannot set request-state %q (already %s)", state.String(), r.GetState().String())
}
r.Spec.State = state
return nil
}
lib/auth/access_request_test.go
+49 -33
View File
@@ -784,9 +784,6 @@ func testAccessRequestDenyRules(t *testing.T, testPack *accessRequestTestPack) {
func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
t.Parallel()
ctx, cancel := context.WithCancel(context.Background())
t.Cleanup(cancel)
testCases := []struct {
desc string
requester string
@@ -870,7 +867,7 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
// generateCerts executes a GenerateUserCerts request, optionally applying
// one or more access-requests to the certificate.
generateCerts := func(reqIDs ...string) (*proto.Certs, error) {
return requesterClient.GenerateUserCerts(ctx, proto.UserCertsRequest{
return requesterClient.GenerateUserCerts(t.Context(), proto.UserCertsRequest{
SSHPublicKey: testPack.sshPubKey,
TLSPublicKey: testPack.tlsPubKey,
Username: tc.requester,
@@ -888,12 +885,12 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
checkCerts(t, certs, testPack.users[tc.requester], nil, nil, nil)
// should not be able to list any nodes
nodes, err := requesterClient.GetNodes(ctx, defaults.Namespace)
nodes, err := requesterClient.GetNodes(t.Context(), defaults.Namespace)
require.NoError(t, err)
require.Empty(t, nodes)
// requestable roles should be correct
caps, err := requesterClient.GetAccessCapabilities(ctx, types.AccessCapabilitiesRequest{
caps, err := requesterClient.GetAccessCapabilities(t.Context(), types.AccessCapabilitiesRequest{
RequestableRoles: true,
})
require.NoError(t, err)
@@ -914,7 +911,7 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
require.NoError(t, err)
// send the request to the auth server
req, err = requesterClient.CreateAccessRequestV2(ctx, req)
req, err = requesterClient.CreateAccessRequestV2(t.Context(), req)
require.ErrorIs(t, err, tc.expectRequestError)
if tc.expectRequestError != nil {
return
@@ -929,7 +926,7 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
require.NoError(t, err)
// approve the request
req, err = reviewerClient.SubmitAccessReview(ctx, types.AccessReviewSubmission{
req, err = reviewerClient.SubmitAccessReview(t.Context(), types.AccessReviewSubmission{
RequestID: req.GetName(),
Review: types.AccessReview{
ProposedState: types.RequestState_APPROVED,
@@ -962,9 +959,9 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
require.NoError(t, err)
// should be able to list the expected nodes
nodes, err = elevatedClient.GetNodes(ctx, defaults.Namespace)
nodes, err = elevatedClient.GetNodes(t.Context(), defaults.Namespace)
require.NoError(t, err)
gotNodes := []string{}
gotNodes := make([]string, 0, len(nodes))
for _, node := range nodes {
gotNodes = append(gotNodes, node.GetName())
}
@@ -972,7 +969,7 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
require.Equal(t, tc.expectNodes, gotNodes)
// renew elevated certs
newCerts, err := elevatedClient.GenerateUserCerts(ctx, proto.UserCertsRequest{
newCerts, err := elevatedClient.GenerateUserCerts(t.Context(), proto.UserCertsRequest{
SSHPublicKey: testPack.sshPubKey,
TLSPublicKey: testPack.tlsPubKey,
Username: tc.requester,
@@ -983,7 +980,7 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
require.NoError(t, err)
// in spite of providing no access requests, we still have elevated
// roles and the certicate shows the original access request
// roles and the certificate shows the original access request
checkCerts(t,
newCerts,
tc.expectRoles,
@@ -991,36 +988,55 @@ func testSingleAccessRequests(t *testing.T, testPack *accessRequestTestPack) {
[]string{req.GetName()},
requestResourceIDs)
// attempt to apply request in DENIED state (should fail)
require.NoError(t, testPack.tlsServer.Auth().SetAccessRequestState(ctx, types.AccessRequestUpdate{
// ensure that once in the APPROVED state, a request cannot be set to DENIED.
err = testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: req.GetName(),
State: types.RequestState_DENIED,
}))
_, err = generateCerts(req.GetName())
require.ErrorIs(t, err, trace.AccessDenied("access request %q has been denied", req.GetName()))
})
require.True(t, trace.IsBadParameter(err), "unexpected error: %v", err)
// ensure that once in the DENIED state, a request cannot be set back to PENDING state.
require.Error(t, testPack.tlsServer.Auth().SetAccessRequestState(ctx, types.AccessRequestUpdate{
RequestID: req.GetName(),
State: types.RequestState_PENDING,
}))
// ensure that once in the DENIED state, a request cannot be set back to APPROVED state.
require.Error(t, testPack.tlsServer.Auth().SetAccessRequestState(ctx, types.AccessRequestUpdate{
// ensure that once in the APPROVED state, a request cannot be updated again.
err = testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: req.GetName(),
State: types.RequestState_APPROVED,
})
require.True(t, trace.IsBadParameter(err), "unexpected error: %v", err)
deniedReq, err := services.NewAccessRequestWithResources(tc.requester, tc.requestRoles, requestResourceIDs)
require.NoError(t, err)
deniedReq, err = requesterClient.CreateAccessRequestV2(t.Context(), deniedReq)
require.NoError(t, err)
// attempt to use a request in DENIED state (should fail)
require.NoError(t, testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: deniedReq.GetName(),
State: types.RequestState_DENIED,
}))
// ensure that identities with requests in the DENIED state can't reissue new certs.
_, err = elevatedClient.GenerateUserCerts(ctx, proto.UserCertsRequest{
SSHPublicKey: testPack.sshPubKey,
TLSPublicKey: testPack.tlsPubKey,
Username: tc.requester,
Expires: time.Now().Add(time.Hour).UTC(),
// no new access requests
AccessRequests: nil,
_, err = generateCerts(deniedReq.GetName())
require.ErrorIs(t, err, trace.AccessDenied("access request %q has been denied", deniedReq.GetName()))
// ensure that once in the DENIED state, a request cannot be set back to PENDING state.
err = testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: deniedReq.GetName(),
State: types.RequestState_PENDING,
})
require.ErrorIs(t, err, trace.AccessDenied("access request %q has been denied", req.GetName()))
require.True(t, trace.IsBadParameter(err), "unexpected error: %v", err)
// ensure that once in the DENIED state, a request cannot be set back to APPROVED state.
err = testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: deniedReq.GetName(),
State: types.RequestState_APPROVED,
})
require.True(t, trace.IsBadParameter(err), "unexpected error: %v", err)
// ensure that once in the DENIED state, a request cannot be updated again.
err = testPack.tlsServer.Auth().SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: deniedReq.GetName(),
State: types.RequestState_DENIED,
})
require.True(t, trace.IsBadParameter(err), "unexpected error: %v", err)
})
}
}
lib/services/local/dynamic_access.go
+8 -3
View File
@@ -93,9 +93,8 @@ func (s *DynamicAccessService) SetAccessRequestState(ctx context.Context, params
return nil, trace.Wrap(err)
}
// Setting state is attempted multiple times in the event of concurrent writes.
// The reason we bother to re-attempt is because state updates aren't meant
// to be "first come first serve". Denials should overwrite approvals, but
// approvals should not overwrite denials.
// The reason we bother to re-attempt is that state updates aren't meant
// to be "first come, first served". Approved, denied, and promoted states are terminal.
for range maxCmpAttempts {
item, err := s.Get(ctx, accessRequestKey(params.RequestID))
if err != nil {
@@ -108,9 +107,15 @@ func (s *DynamicAccessService) SetAccessRequestState(ctx context.Context, params
if err != nil {
return nil, trace.Wrap(err)
}
if req.GetState() == params.State && req.GetState().IsResolved() {
return nil, trace.BadParameter("cannot update access request in state %q", req.GetState().String())
}
if err := req.SetState(params.State); err != nil {
return nil, trace.Wrap(err)
}
req.SetResolveReason(params.Reason)
req.SetResolveAnnotations(params.Annotations)
if len(params.Roles) > 0 {
lib/services/local/dynamic_access_test.go
+209
View File
@@ -21,6 +21,7 @@ import (
"time"
"github.com/google/uuid"
"github.com/gravitational/trace"
"github.com/stretchr/testify/require"
"github.com/gravitational/teleport/api/client/proto"
@@ -32,6 +33,214 @@ import (
"github.com/gravitational/teleport/lib/services"
)
func TestSetAccessRequestState(t *testing.T) {
service, _ := setupDynamicAccessService(t)
cases := []struct {
name string
initialState types.RequestState
updatedState types.RequestState
wantState types.RequestState
wantErr bool
}{
{
name: "none to pending",
initialState: types.RequestState_NONE,
updatedState: types.RequestState_PENDING,
wantState: types.RequestState_PENDING,
},
{
name: "none to approved",
initialState: types.RequestState_NONE,
updatedState: types.RequestState_APPROVED,
wantState: types.RequestState_APPROVED,
},
{
name: "none to denied",
initialState: types.RequestState_NONE,
updatedState: types.RequestState_DENIED,
wantState: types.RequestState_DENIED,
},
{
name: "none to promoted",
initialState: types.RequestState_NONE,
updatedState: types.RequestState_PROMOTED,
wantState: types.RequestState_PROMOTED,
},
{
name: "pending to approved",
initialState: types.RequestState_PENDING,
updatedState: types.RequestState_APPROVED,
wantState: types.RequestState_APPROVED,
},
{
name: "pending to denied",
initialState: types.RequestState_PENDING,
updatedState: types.RequestState_DENIED,
wantState: types.RequestState_DENIED,
},
{
name: "pending to promoted",
initialState: types.RequestState_PENDING,
updatedState: types.RequestState_PROMOTED,
wantState: types.RequestState_PROMOTED,
},
{
name: "approved to pending",
initialState: types.RequestState_APPROVED,
updatedState: types.RequestState_PENDING,
wantState: types.RequestState_APPROVED,
wantErr: true,
},
{
name: "approved to approved",
initialState: types.RequestState_APPROVED,
updatedState: types.RequestState_APPROVED,
wantState: types.RequestState_APPROVED,
wantErr: true,
},
{
name: "approved to denied",
initialState: types.RequestState_APPROVED,
updatedState: types.RequestState_DENIED,
wantState: types.RequestState_APPROVED,
wantErr: true,
},
{
name: "approved to promoted",
initialState: types.RequestState_APPROVED,
updatedState: types.RequestState_PROMOTED,
wantState: types.RequestState_APPROVED,
wantErr: true,
},
{
name: "denied to denied",
initialState: types.RequestState_DENIED,
updatedState: types.RequestState_DENIED,
wantState: types.RequestState_DENIED,
wantErr: true,
},
{
name: "promoted to pending",
initialState: types.RequestState_PROMOTED,
updatedState: types.RequestState_PENDING,
wantState: types.RequestState_PROMOTED,
wantErr: true,
},
{
name: "promoted to approved",
initialState: types.RequestState_PROMOTED,
updatedState: types.RequestState_APPROVED,
wantState: types.RequestState_PROMOTED,
wantErr: true,
},
{
name: "promoted to denied",
initialState: types.RequestState_PROMOTED,
updatedState: types.RequestState_DENIED,
wantState: types.RequestState_PROMOTED,
wantErr: true,
},
{
name: "promoted to promoted",
initialState: types.RequestState_PROMOTED,
updatedState: types.RequestState_PROMOTED,
wantState: types.RequestState_PROMOTED,
wantErr: true,
},
{
name: "none to none",
initialState: types.RequestState_NONE,
updatedState: types.RequestState_NONE,
wantState: types.RequestState_PENDING,
wantErr: true,
},
{
name: "pending to none",
initialState: types.RequestState_PENDING,
updatedState: types.RequestState_NONE,
wantState: types.RequestState_PENDING,
wantErr: true,
},
{
name: "pending to pending",
initialState: types.RequestState_PENDING,
updatedState: types.RequestState_PENDING,
wantState: types.RequestState_PENDING,
},
{
name: "approved to none",
initialState: types.RequestState_APPROVED,
updatedState: types.RequestState_NONE,
wantState: types.RequestState_APPROVED,
wantErr: true,
},
{
name: "denied to none",
initialState: types.RequestState_DENIED,
updatedState: types.RequestState_NONE,
wantState: types.RequestState_DENIED,
wantErr: true,
},
{
name: "denied to pending",
initialState: types.RequestState_DENIED,
updatedState: types.RequestState_PENDING,
wantState: types.RequestState_DENIED,
wantErr: true,
},
{
name: "denied to approved",
initialState: types.RequestState_DENIED,
updatedState: types.RequestState_APPROVED,
wantState: types.RequestState_DENIED,
wantErr: true,
},
{
name: "denied to promoted",
initialState: types.RequestState_DENIED,
updatedState: types.RequestState_PROMOTED,
wantState: types.RequestState_DENIED,
wantErr: true,
},
{
name: "promoted to none",
initialState: types.RequestState_PROMOTED,
updatedState: types.RequestState_NONE,
wantState: types.RequestState_PROMOTED,
wantErr: true,
},
}
for _, test := range cases {
t.Run(test.name, func(t *testing.T) {
req, err := types.NewAccessRequest(uuid.NewString(), "alice", "test_role_1")
require.NoError(t, err)
require.NoError(t, req.SetState(test.initialState))
err = service.CreateAccessRequest(t.Context(), req)
require.NoError(t, err)
final, err := service.SetAccessRequestState(t.Context(), types.AccessRequestUpdate{
RequestID: req.GetName(),
State: test.updatedState,
})
if test.wantErr {
require.Error(t, err)
require.True(t, trace.IsBadParameter(err))
final, err = service.GetAccessRequest(t.Context(), req.GetName())
require.NoError(t, err)
require.Equal(t, test.wantState, final.GetState())
} else {
require.NoError(t, err)
require.Equal(t, test.wantState, final.GetState())
final, err = service.GetAccessRequest(t.Context(), req.GetName())
require.NoError(t, err)
require.Equal(t, test.wantState, final.GetState())
}
})
}
}
func Test_DynamicAccessService_ListExpiredAccessRequests(t *testing.T) {
t.Parallel()
synctest.Test(t, func(t *testing.T) {