-
Notifications
You must be signed in to change notification settings - Fork 1.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Custom Approval Conditions #5441
Conversation
e5e8e23
to
90b61ca
Compare
@fspmarshall I think it is a very thoughtful design, could not find anything outstanding on the first pass. I liked the suggested reviewers part as well, this is exactly what customers were asking for. I will do a second pass and will look in the code. |
41c454c
to
66a6adb
Compare
705ce77
to
41327cc
Compare
@awly @a-palchikov @andrejtokarcik Do you mind reviewing this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note to @russjones, @andrejtokarcik and @fspmarshall. This access/review/threshold system is very expressive. This raises the importance of:
without tracing and role scenario testers, it will be extremely hard to troubleshoot and develop
api/types/access_request.go
Outdated
|
||
// matches collects the results of checking all relevant thresholds | ||
// to see if their filters match this review. | ||
matches := make(map[uint32]bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any specific reason we use uint32 for state vs signed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Negative threshold IDs (indexes) don't make sense.
api/types/access_request.go
Outdated
} | ||
|
||
// check for roles that can be transitioned to an approved state | ||
CheckRoleApprovals: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may be I'm missing something, but it seems that we are doing the same check after processing every review.
what if we split this loop into two parts (to make reading a bit easier) The first pass will process counts only (and short-circuit if the deny matches). The second pass will look for approved roles with all thresholdSets accumulated. As an advantage, you can record more votes even if the threshold matches.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're not missing anything, but IMO the current layout is superior because it is deterministic regardless of the length of the review list. If we tally all votes and then determined the state-transition as separate steps, then the result that would be produced by this logic continues to change as new reviews are added, even after the initial state-transition condition is reached.
Currently, this property is irrelevant since we skip this entire function once we've exited the PENDING
state, but the fact that the function is deterministic means that it will be much easier to extend in the future. Say that we wanted to add an integration with the upcoming "locking" feature which automatically generated locks if a denial theshold was triggered after an approval was initially triggered. The current logic could be left basically untouched, and we could just add a new loop below that starts processing where the previous loop halted. Basically, "its not a bug, its a feature" lol.
} | ||
|
||
// pushThreshold pushes a threshold to the main threshold list and returns its index | ||
// as a uint32 for compatibility with grpc types. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
grpc supports int64, may be use it instead?
https://developers.google.com/protocol-buffers/docs/proto3#scalar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason? We certainly don't need to support threshold counts so large that they would overflow a uint32
.
@@ -66,6 +72,21 @@ type AccessRequest interface { | |||
GetSystemAnnotations() map[string][]string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General note to @russjones, @andrejtokarcik and @fspmarshall. This access/review/threshold system is very expressive. This raises the importance of:
without tracing and role scenario testers, it will be extremely hard to troubleshoot and develop
@fspmarshall can you also please give an example of how to prevent access requestor from approving their own request (or if this is done automatically)? This is for the docs |
Another general observation: I would explore if this code could be improved if nested loops are replaced with some auxiliary data structures, like maps, filters, or trees. |
41327cc
to
76ea3e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I concur with @a-palchikov's comment on the RFD. We could most probably get rid of all the RoleThresholdMapping
complexity if we conceded to not supporting actual multi-role requests. The tsh
UI could still accept multiple specified roles but such commands would be resolved into a series of atomic requests/commands, one for each role. The issue of role subselection would immediately disappear.
This would actually facilitate one of the goals (I suppose) of introducing multi-party approvals -- to enable setups where as many requests can be reviewed by as many people as possible. With the current logic, as far as I understand, a request for roles a,b,c
would require any potential reviewer to:
- not be denied from reviewing any of
a
orb
orc
; and - be allowed to review all of
a
andb
andc
.
True, this is a result of the prudent decision to err on the side of strictness, yet it also implies that an inappropriately chosen request role could make the set of available reviewers de facto empty.
Complex role approval scenarios should be handled by external plugins that are well-understood or even developed by their users, not within Teleport itself. It's bound to be rather difficult to even document and communicate such complex solutions with all their edge cases.
Finally, I find the use of nested loops and code labels a bit too excessive here. It seems that many of the functions could be better structured by means of smaller helper functions.
api/types/access_request.go
Outdated
for _, existingReview := range r.Spec.Reviews { | ||
if existingReview.Author == rev.Author { | ||
return trace.AccessDenied("user %q has already reviewed this request", rev.Author) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really necessary to exclude the ability to change or take back one's review decision? Allowing this should be non-controversial for requests still in the PENDING
state.
However, I'd perhaps go as far as to allow users to re-set their review even after their contribution has caused the review to transition to DENIED
or APPROVED
. In that case the review state should be re-evaluated and possibly go back to PENDING
. This could help in dealing with accidental approvals including those cases when the requestor hasn't yet managed to obtain the certs with the elevated privileges.
Naturally, care must be taken so that a single user isn't able to contribute multiple times towards the thresholds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our old model allowed changing request state after it was already approved, and that ended up being a bad decision in retrospect. The access request system grants privileges by allowing the user to provision certificates with additional roles. If a request is changed from approved to some other non-approved state, it gives the false impression that the privileges were never assumed. Once a request has been in the approved state for even a few milliseconds, you cannot discount the possibility that the user has assumed elevated privileges. Since our caching layer is fairly asynchronous, re-checking the request state on every user action isn't a great solution to this. Its better to have the request continue to reflect the fact that the user likely did successfully escalate privilege.
In terms of cancelling reviews before the request's state-transition occurs, I'm not fundamentally opposed. I don't see any pressing need to add it on the initial release of this feature though.
// don't bother double-storing equivalent thresholds | ||
for i, threshold := range c.Thresholds { | ||
if t.Equals(threshold) { | ||
return uint32(i), nil | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Equals
appears to take the threshold name into account. In order to really prevent double-storing it might be more appropriate for that field to be ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't actually any harm in double-storing (I just figure its good practice not to). Since threshold names are displayed in the web UI, I don't want teleport to make the decision about which of two descriptions for a given requirement is the one that should be displayed.
This looks pertinent: teleport/lib/services/access_request.go Lines 385 to 388 in 76ea3e1
|
@andrejtokarcik @fspmarshall @a-palchikov re: single request to multiple request suggestion: This would make the processing much more complex though - right now the request is a transaction with a state, it's either approved or not. With your proposal, the request would be split into multiple object, each with it's own state. This would be quite hard to reason about, as request could be partially approved/denied. |
In addition to @klizhentas's points:
|
76ea3e1
to
acadfdb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fspmarshall @russjones almost forgot, because it's a dual authz feature, please restrict it's usage to
https://github.com/gravitational/teleport/blob/master/lib/modules/modules.go#L45
AdvancedAccessWorkflows
Similar to this:
teleport/lib/auth/auth_with_roles.go
Line 1875 in 5e9ea1c
case features.AdvancedAccessWorkflows == false && |
bc4ca76
to
ea81929
Compare
@@ -185,6 +211,71 @@ func (r *AccessRequestV3) SetSystemAnnotations(annotations map[string][]string) | |||
r.Spec.SystemAnnotations = annotations | |||
} | |||
|
|||
func (r *AccessRequestV3) GetOriginalRoles() []string { | |||
if l := len(r.Spec.RoleThresholdMapping); l == 0 || l == len(r.Spec.Roles) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry if that was already explained but I don't see it explained here hence the question. Is it guaranteed that l == r.Spec.Roles
implies that RTMs encode for the same roles as specified in r.Spec.Roles
?
@@ -195,6 +286,11 @@ func (r *AccessRequestV3) CheckAndSetDefaults() error { | |||
return trace.Wrap(err) | |||
} | |||
} | |||
|
|||
// dedupe and sort roles to simplify comparing role lists | |||
r.Spec.Roles = utils.Deduplicate(r.Spec.Roles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's important to note but I'd find it useful to warn if the spec contains duplicate roles. Is it also complying to some old behavior and trying to keep backwards-compatible?
tool/tsh/access_request.go
Outdated
req.GetName(), | ||
req.GetUser(), | ||
strings.Join(req.GetRoles(), ","), | ||
req.GetCreationTime().Format(time.RFC822), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
req.GetCreationTime().Format(time.RFC822), | |
req.GetCreationTime().UTC().Format(time.RFC822), |
@@ -76,6 +76,23 @@ type CLIConf struct { | |||
DesiredRoles string | |||
// RequestReason indicates the reason for an access request. | |||
RequestReason string | |||
// SuggestedReviewers is a list of suggested request reviewers. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: aren't these better grouped in an AccessRrequest
block in CLIConf
?
tool/tsh/tsh.go
Outdated
) | ||
|
||
// special case: --request-roles=no disables auto-request behavior. | ||
if cf.DesiredRoles == "no" { | ||
noAutoRequest = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoRequest = false
would be more readable in if autoRequest && cf.DesiredRoles == ""
// verify that all roles are present within the request | ||
for _, role := range rev.Roles { | ||
if _, ok := rtm[role]; !ok { | ||
return trace.BadParameter("role %q is not a member of this request", role) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: if this built the map for all missing roles - it would be a better UX
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree in theory, but the difference is negligible IMO and this function is already more busy than I'd like it to be, so I think I'm gonna leave as-is for now.
lib/services/access_request.go
Outdated
|
||
tid := uint32(i) | ||
if int(tid) != i { | ||
// sanity-check. we disallow extremely large threshold lists elsewhere, but its always |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// sanity-check. we disallow extremely large threshold lists elsewhere, but its always | |
// sanity-check. we disallow extremely large threshold lists elsewhere, but it's always |
|
||
switch { | ||
case lastReview.ProposedState.IsApproved(): | ||
if len(approved) != len(req.GetRoleThresholdMapping()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"not equals" comparison is suspect unless it is guaranteed that len(approved) <= len(req.GetRoleThresholdMapping())
but it's hard to say. Would be safer to say if len(approved) < len(req.GetRoleThresholdMapping())
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The approved
mapping's keys are taken from the rtm, so I'm pretty confident in saying that its guaranteed.
lib/services/access_request.go
Outdated
return len(needsAllow) == 0, nil | ||
} | ||
|
||
func NewReviewPermissionChecker(getter UserAndRoleGetter, username string) (ReviewPermissionChecker, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accept the context as parameter instead of using context.TODO
lib/services/access_request.go
Outdated
// incoming requests must have system annotations attached | ||
// before being inserted into the backend. this is how the | ||
// RBAC system propagates sideband information to plugins. | ||
req.SetSystemAnnotations(m.SystemAnnotations()) | ||
|
||
// if no suggested reviewers were provided by the user then | ||
// use the defaults sugested by the user's static roles. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// use the defaults sugested by the user's static roles. | |
// use the defaults suggested by the user's static roles. |
ea81929
to
12b2760
Compare
<<<<<<< HEAD | ||
golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9 | ||
======= | ||
github.com/vulcand/predicate v1.1.0 | ||
>>>>>>> custom approval conditions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A little merge leftover.
NOTE: This is an MVP feature. Evaluation of permissions errs on the side of strictness, and some planned features (e.g. role subselection) are not yet supported.
Based upon RFD/0014 (#5071)
Key Features
Introduces the concept of access request "reviews" which can be used to approve/deny access requests. Unlike directly approving/denying via resource-level permissions, reviews support granular permissions and flexible multi-party approval scenarios.
Here is an example of a simple review-based configuration:
In the above example, users with the role
populist
are able to request the roledictator
. When generated, this request will automatically be configured with a reviewthreshold
requiring three approvals or two denials. In this scenario, a user with rolepopulist
will therefore be able to assume roledictator
if three separate users all submit approving reviews. Users with roleproletariat
have permission to review requests fordictator
, so three users with roleproletariat
will be able to approve a request for roledictator
if they all agree.Note that
thresholds
is a list. It is possible to specify multiple thresholds. Lets look at a more full-featured use of thresholds:In the above scenario, reviewers with the role
proletariat
count toward therevolution
threshold, while reviewers with the rolemilitary
count toward thecoup
threshold (reviewers with both roles count toward both thresholds). Say, for example, thatalice
andbob
both hold roleproletariat
and submit approving reviews. Therevolution
threshold is not yet met, so the request is still pending. Now say thatcarol
holds rolemilitary
and also submits an approval. We have more approvals than either threshold requires, but neither threshold has been reached since we have one approval that matches thecoup
threshold and two approvals that match therevolution
threshold.NOTE: A filter which matches against a reviewing user's roles (e.g.
contains(reviewer.roles, "some-role")
) effectively "leaks" the fact that the user holds that role to all other users who are allowed to view the request. It does not, however, leak what other roles might be held by the reviewer, as the actual evaluation of the filter is performed only by the auth server. The access request resource simply records a mapping of which reviews match which filters.Reviewer permissions can also be fairly granular. Ex:
The above permits a user to review any access request, so long as that request does not include the role
elected-representative
and does have the system annotationmechanism: popular-uprising
.In addition to new RBAC options,
tsh
now supports a series ofrequest
subcommands.Users are always allowed to see their own requests, and requests that they are allowed to review:
As indicated by the above
hint
test,tsh request show
can be used to print out detailed information about a request:Reviews can be submitted via
tsh request review
:A new
access_request.review
audit event has been added:Note that in the first two events,
state
isPENDING
butproposed_state
isAPPROVED
. This indicates that the review being submitted proposes approval, but the state of the request post-application is stillPENDING
. The third event shows stateAPPROVED
indicating that the post-application state of the request is nowAPPROVED
. This is consistent with a request with an approval threshold of 3. Subsequent reviews have no effect, but still generate events. If, for example, the next review was a denial, we would see an event like this:Gotchas
In order to preserve compatibility with existing configurations,
update
permissions for theaccess_request
resource still allow users to approve any request. Going forward this will not be recommended.Role subselection is not currently implemented for reviews. Care has been taken to ensure that this system will be compatible with role subselection, but getting role subselection to work in a correct and intuitive manner without overly complicating the implementation is tricky. I've opted not to let it be a blocker on the rest of the features.
The current implementation is a little overly-strict when it comes to deciding which thresholds need to pass in order for a request to be approved, and a little over-eager to deny requests. In particular, it requires one threshold from each relevant allow directive, and denies the entire request if any threshold from any statically assigned role is met. In theory, this isn't always necessary, but I opted to start with the simpler and restrictive model.
Only approval permissions from statically-assigned roles are taken into consideration. This may become a configurable behavior in the future, but once again we're erring on the side of strictness.
Other Stuff
suggested_reviewers
list, andtsh request ls
supports a--suggested
flag which lists only requests for which the current user is a suggested reviewer. The names in thesuggested_reviewer
list don't actually need to be teleport usernames. They could just as easily be slack usernames, emails, etc. This feature is purely for convenience and has no bearing on permissions.