Uh oh!
There was an error while loading. Please reload this page.
✨ Check known required permissions for install before installing with the helm applier - #1716
✨ Check known required permissions for install before installing with the helm applier#1716trgeiger wants to merge 5 commits into
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
65bc862 to
27e8d4fCompared767e20 to
678687cComparec114d4c to
74c93c7CompareCodecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #1716 +/- ##
==========================================
- Coverage 68.34% 67.73% -0.61%
==========================================
Files 63 64 +1 Lines 5117 5228 +111 ==========================================
+ Hits 3497 3541 +44 - Misses 1390 1446 +56 - Partials 230 241 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| clusterScopedErrs := []error{} | ||
| requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} | ||
| for _, o := range objects { |
There was a problem hiding this comment.
If the object is a ClusterRole or Role, we need to check that we have its rules at the appropriate scope (or the escalate verb on clusterroles or roles)
If the object is a ClusterRoleBinding or a RoleBinding, we need to check that we have the rules present in the referenced ClusterRole/Role (or the bind verb on clusterrolesbindings or rolebindings)
Right?
There was a problem hiding this comment.
That does sound right, yes. We can try do to that work here or have that be a future iteration/improvement in another PR. I'm fine with either. This PR started as just implementing the quick "get" check but along the way realized we can and should just go ahead and check all the verbs above while we check for get so that's why nothing more complicated is happening yet.
There was a problem hiding this comment.
I think we'll have to have this basically immediately because every single bundle we support is going to have RBAC objects in the manifest by virtue of all of this stuff having CRDs and controllers.
There was a problem hiding this comment.
It looks like using the RBACAuthorizer I linked to in another thread would handle all this for you, fwiw.
There was a problem hiding this comment.
I feel like I've seen libraries in k/k for this. Looking now.
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh yeah I actually was using the RulesAllow in an earlier version of this but it was causing some import issue @bentito was noticing
There was a problem hiding this comment.
I'll fix that up and use those functions again
There was a problem hiding this comment.
Looking around, I also see:
- https://github.com/kubernetes/kubernetes/blob/f0077a3689669018557de723a0416fda267d132f/staging/src/k8s.io/component-helpers/auth/rbac/validation/policy_comparator.go#L27
- https://github.com/kubernetes/kubernetes/blob/f0077a3689669018557de723a0416fda267d132f/pkg/registry/rbac/validation/rule.go#L53
Ultimately though, it seems like the RBACAuthorizer encapsulates everything we'd want to do and is the actual authorizer the apiserver uses. The catch is that we'd have to give ourselves permission to list/watch all RBAC objects in the cluster. Is that a dealbreaker?
Maybe something we should go ask the sig-auth folks: what's better for best effort authz checks? use an in-process RBAC authorizer with cached RBAC from informers, or a flood of SSAR requests?
There was a problem hiding this comment.
By give ourselves, do you mean operator-controller's manager would need list/watch on all RBAC?
There was a problem hiding this comment.
@joelanford@trgeiger was getting at that the point that the import adding RBACAuthorizer brings in seems to normally be k8s.io/kubernetes which is undesirable b/c that depends on several staging-only deps at v0.0.0. I am trying to figure out a more granular import for it to avoid the kitchen sink import?
There was a problem hiding this comment.
In considering RBACAuthorizer, I think we should probably get more team input on the addition of all the list/watch on all rbac objects in the cluster since that would be a pretty significant expansion of what OLMv1 sees, right? I'll link this thread to the olm-dev chat as well
There was a problem hiding this comment.
The more I dive into the RBACAuthorizer, the more I understand the complexity of doing this check ourselves. There are a lot of edge cases. I think we may need to at least start out with using SelfSubjectAccessReview so that we don't have to take on the complexity of handling all of the nuance.
There was a problem hiding this comment.
And it looks like SSAR is not sufficient for checking bindings and role escalations... So for RBAC objects we probably need to do a dry-run create.
Adds a check that makes sure the installer service account has the necessary permissions to manage all the resources in the ClusterExtension payload and returns errors detailing all the missing permissions. This prevents a hung server-connected dry-run getting caught on individual missing get permissions as well returns many other missing required permissions needed for the actual installation or upgrade. Signed-off-by: Tayler Geiger <tayler@redhat.com>
Signed-off-by: Brett Tofel <btofel@redhat.com>
Signed-off-by: Brett Tofel <btofel@redhat.com>
37c4e34 to
ade3961Compare| clusterScopedErrs := []error{} | ||
| requiredVerbs := []string{"get", "create", "update", "list", "watch", "delete", "patch"} | ||
| for _, o := range objects { |
There was a problem hiding this comment.
I think we'll have to have this basically immediately because every single bundle we support is going to have RBAC objects in the manifest by virtue of all of this stuff having CRDs and controllers.
| // SelfSubjectRulesReview formats the resource names as lowercase and plural | ||
| func sanitizeResourceName(resourceName string) string { | ||
| return strings.ToLower(resourceName) + "s" |
There was a problem hiding this comment.
This is not a safe way to handle this because pluralization is complicated, and CRDs get to define their resource name however they want. Seems like we would need to use a rest mapper to get the actual resource name for the object.
| type NewForConfigFunc func(*rest.Config) (authorizationv1client.AuthorizationV1Interface, error) | ||
| type AuthorizationClientMapper struct { |
There was a problem hiding this comment.
would it make sense for AuthorizationClientMapper and its related types and methods to be part of authorization package?
To me they don't seem helm specific and we'll be encapsulating authorization into a separate package.
Also, to avoid situation where we the caller needs to know about authorizationv1client.AuthorizationV1Interface and first obtain the authorization client and then pass it to CheckObjectPermissions I was thinking we could have CheckContentPermissions be a public method of the custom authorization client that we return, which would internally call more generic checkObjectPermissions.
For that, the client in authorization package would have to wrap authorizationv1client.AuthorizationV1Interface and then in this package, we'd just define an interface of the custom authorization client with whatever we need to use here similar to this:
typeauthorizationClientinterface {
CheckContentPermissions(...)
}With that we would be encapsulating internals of authorization within a dedicated single package and exposing a clean interface to the callers with the added benefit of having a simple interface to mock on the callers side.
WDYT?
Obviously those are just suggestions, the second one especially.
| err = h.checkContentPermissions(ctx, contentFS, authclient, ext) | ||
| if err != nil { | ||
| return nil, "", err |
There was a problem hiding this comment.
might be a good idea to wrap with context, ie. `fmt.Errorf(err, "failed checking content permissions") for better debuggability. Here and other places.
There was a problem hiding this comment.
I'll make this change in the bits I introduce in this PR and we can maybe edit the other error returns in helm.go in a separate enhancement PR
There was a problem hiding this comment.
sure, that's actually what I meant - other places introduced by this PR
| }) | ||
| } | ||
| resAttrs := []authorizationv1.ResourceAttributes{} |
There was a problem hiding this comment.
nit: you could probably initialize it with specific length or at least capacity equal to len(requiredVerbs) * len(objects)
| for _, resAttr := range resAttrs { | ||
| if !canI(resAttr, rules) { | ||
| if resAttr.Namespace != "" { | ||
| namespacedErrs = append(namespacedErrs, fmt.Errorf("cannot %s %s %s in namespace %s", |
There was a problem hiding this comment.
nit: maybe it would make sense to put the name of the resource and namespace or even all of those string values in quotes by using %q?
| } | ||
| errs := append(namespacedErrs, clusterScopedErrs...) | ||
| if len(errs) > 0 { | ||
| errs = append([]error{fmt.Errorf("installer service account %s is missing required permissions", ext.Spec.ServiceAccount.Name)}, errs...) |
There was a problem hiding this comment.
nit: that's not ideal from performance perspective, maybe just do return fmt.Errorf("installer service..., %s, errors.Join(errs...))?
There was a problem hiding this comment.
Wait, can you re-explain this suggestion? I thought I understood at first glance. Are you suggesting I sub in the errs into the "installer service account %s..." error message with Errorf? Like nested fmt.Errorf(fmt.Sprintf("installer SA %s is missing perms", ext.Spec.ServiceAccountName)) + " %s", errs...)? or something like that
There was a problem hiding this comment.
sure, I meant something like fmt.Errorf("installer SA... perms, %w", ext.Spec.ServiceAccountName, errors.Join(errs...))
Alternatively you could start with errs slice already having this "install service account %s" error at first position and then at the end when you see there were no actual errors, simply return a nil error
Move all authorization-related code to the authorization package, rename anything using 'auth' to 'authorization' to avoid confusion with authentication.
ade3961 to
7c9a05eCompareAdds authorization client to authorization package which helps with testing. Also makes errors more descriptive. Signed-off-by: Tayler Geiger <tayler@redhat.com> Co-authored-by: Brett Tofel <btofel@redhat.com>
openshift-merge-robot
commented
Feb 28, 2025
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
trgeiger
commented
Mar 6, 2025
Closing as we are going to implement this functionality differently |
Adds a check that makes sure the installer service account has the necessary get permissions to get all the resources it will need to inspect for a server-connected dry-run and returns errors detailing all the missing get permissions. This prevents a hung server-connected dry-run getting caught on individual missing get permissions.
Description
Reviewer Checklist