Add support for requiring reason for access requests #49124
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
6 times, most recently
from
880bc0e to
1e9fe39
Compare
kopiczko
commented
Nov 18, 2024
Comment on lines
+312
to
+325
| // | ||
| // If loginHint is provided, it will attempt to prune the list to a single role. |
There was a problem hiding this comment.
I copied this godoc from pruneResourceRequestRoles which applicableSearchAsRoles is calling to surface it a bit.
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
from
1e9fe39 to
c5b9345
Compare
kopiczko
commented
Nov 18, 2024
Comment on lines
+2293
to
+2344
| { | ||
| name: "resource request: but require reason when another role allowing _role_ access requires reason for the role", | ||
| currentRoles: []string{"fork-node-requester", "fork-access-requester-with-reason"}, | ||
| requestResourceIDs: []types.ResourceID{ | ||
| {ClusterName: clusterName, Kind: types.KindNode, Name: "fork-node"}, | ||
| }, | ||
| expectError: trace.BadParameter(`request reason must be specified (required for role "fork-access")`), | ||
| }, |
There was a problem hiding this comment.
This is the situation discussed here https://gravitational.slack.com/archives/C07QJQNGY2J/p1731620401319179
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
from
c5b9345 to
adeaa39
Compare
r0mant
requested review from
fspmarshall
and removed request for
ryanclark and
flyinghermit
smallinsky
reviewed
Nov 19, 2024
There was a problem hiding this comment.
First pass.
Could you take a look at UT failing on the AccessRequest test cases;
=== FAIL: lib/auth TestAccessRequestNonGreedyAnnotations/identity-resource-requester_requests_resource,_receives_identity_annotations (0.03s)
{"caller":"services/access_checker.go:1269","component":null,"level":"warning","message":"Failed to find roles in x509 identity for test-user. Fetching from backend. If the identity provider allows username changes, this can potentially allow an attacker to change the role of the existing user.","timestamp":"2024-11-18T19:32:40Z"}
{"timestamp":"2024-11-18T19:32:40Z","level":"debug","caller":"events/discard.go:147","message":"Discarding event","event_id":"","event_type":"role.created","event_time":"0001-01-01T00:00:00Z","event_index":0}
{"caller":"auth/auth.go:5216","component":"auth","level":"debug","message":"Creating Access Request 019340c3-4a76-7f2a-bf1e-70abfac19a85 with expiry 2024-11-18 20:32:39 +0000 UTC.","timestamp":"2024-11-18T19:32:40Z"}
auth_with_roles_test.go:8628:
Error Trace: /__w/teleport/teleport/lib/auth/auth_with_roles_test.go:8628
Error: Received unexpected error:
no roles configured in the "search_as_roles" for this user allow access to any requested resources. The user may already have access to all requested resources with their existing roles. resources: ["/localhost/node/server-identity"] roles: [identity-access] login: ""
Test: TestAccessRequestNonGreedyAnnotations/identity-resource-requester_requests_resource,_receives_identity_annotations
```
a2e458c to
98728f7
Compare
|
@smallinsky @r0mant @fspmarshall I extracted and merged the dependencies, adapted to request.mode change and consider this ready to review now. |
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
from
98728f7 to
033826f
Compare
smallinsky
approved these changes
Nov 25, 2024
r0mant
approved these changes
Nov 25, 2024
lib/services/access_request.go
Outdated
| } | ||
|
|
||
| if m.requireReasonForAllRoles { | ||
| return trace.BadParameter("request reason must be specified (required request_access option in one of the roles)") |
There was a problem hiding this comment.
Would returning trace.AccessDenied here be more appropriate?
There was a problem hiding this comment.
I think it's BadParameter. It's says reason is not given in the request, but it's required. I'd even say AccessDenied would be slightly confusing, as it may look as the user is not allowed to create Access Requests.
Comment on lines
+1366
to
+1377
| if len(allApplicableRoles) == 0 { | ||
| allApplicableRoles = roles | ||
| } else { | ||
| allApplicableRoles = append(allApplicableRoles, roles...) | ||
| } |
There was a problem hiding this comment.
Nit: I think this can be simplified to just:
Suggested change
| if len(allApplicableRoles) == 0 { | |
| allApplicableRoles = roles | |
| } else { | |
| allApplicableRoles = append(allApplicableRoles, roles...) | |
| } | |
| allApplicableRoles = append(allApplicableRoles, roles...) |
There was a problem hiding this comment.
Yes, I wanted to avoid allocation, but it doesn't matter much.
public-teleport-github-review-bot
bot
removed request for
fspmarshall,
gzdunek and
ryanclark
github-actions bot
pushed a commit
that referenced
this pull request
Nov 26, 2024
This got exposed while working on Access Request reason required PR: #49124
kopiczko
added a commit
that referenced
this pull request
Nov 27, 2024
This changes the proto type (+validation) only to declutter the original PR #49124 The real changes are in - api/proto/teleport/legacy/types/types.proto - api/types/access_request.go - lib/auth/auth_with_roles.go - lib/auth/auth_with_roles_test.go The rest is all generated.
kopiczko
added a commit
that referenced
this pull request
Nov 27, 2024
This changes the proto type (+validation) only to declutter the original PR #49124 The real changes are in - api/proto/teleport/legacy/types/types.proto - api/types/access_request.go - lib/auth/auth_with_roles.go - lib/auth/auth_with_roles_test.go The rest is all generated.
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
from
a5165aa to
0148abe
Compare
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 27, 2024
This changes the proto type (+validation) only to declutter the original PR #49124 The real changes are in - api/proto/teleport/legacy/types/types.proto - api/types/access_request.go - lib/auth/auth_with_roles.go - lib/auth/auth_with_roles_test.go The rest is all generated.
kopiczko
force-pushed
the
kopiczko/add-role.spec.allow.request.reason.required
branch
from
f1e5833 to
8a56526
Compare
kopiczko
deleted the
kopiczko/add-role.spec.allow.request.reason.required
branch
github-merge-queue bot
pushed a commit
that referenced
this pull request
Nov 27, 2024
This got exposed while working on Access Request reason required PR: #49124
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #20164
TODO:
changelog: Added support for requiring reason for access requests (with a new
role.spec.allow.request.reason.modesetting).