Skip to content
This repository was archived by the owner on Nov 14, 2024. It is now read-only.

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

Merged
merged 7 commits into from
Sep 1, 2021
Merged

feature: use SelfSubjectRulesReview for RBAC, fallback to SelfSubjectAccessRequest #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
56ce81b
add api.WarnResult
johnstcn Aug 30, 2021
c49a547
feat: use SSRR for RBAC instead of SSAR
johnstcn Aug 30, 2021
dcb95d4
chore: Makefile: add test target
johnstcn Sep 1, 2021
a98c45b
fix: RBAC: use SSRR by default, fallback to SSAR
johnstcn Sep 1, 2021
7e8bfdb
fix: ssrr checks
johnstcn Sep 1, 2021
b0f4120
rbac_test: add more tests
johnstcn Sep 1, 2021
65da237
fixup! feat: use SSRR for RBAC instead of SSAR
johnstcn Sep 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,3 +27,11 @@ lint/shellcheck: $(shell scripts/depfind/sh.sh)

lint: lint/go lint/shellcheck
.PHONY: lint

test: test/go
.PHONY: test

test/go:
@echo "--- go test"
go test -parallel=$(shell nproc) ./...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: use go test -parallel ./...

I believe go test -parallel (no options) should auto-detect available processors, per go help testflag:

        -parallel n
            Allow parallel execution of test functions that call t.Parallel.
            The value of this flag is the maximum number of tests to run
            simultaneously; by default, it is set to the value of GOMAXPROCS.
            Note that -parallel only applies within a single test binary.
            The 'go test' command may run tests for different packages
            in parallel as well, according to the setting of the -p flag
            (see 'go help build')

in the future, we might want to consider using gotestsum as the output is nicer

.PHONY: test/go
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ require (
k8s.io/apimachinery v0.19.14
k8s.io/client-go v0.19.14
k8s.io/klog/v2 v2.10.0 // indirect
k8s.io/utils v0.0.0-20210305010621-2afb4311ab10 // indirect
k8s.io/kubectl v0.19.14
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment: tough call, seems we're either:

  1. doing something upstream tells us not to do (import kubectl); or
  2. signing on to maintain a copy of some code

Maybe this one in particular is OK and it's just stuff in k/k that we shouldn't import:

To use Kubernetes code as a library in other applications, see the list of published components. Use of the k8s.io/kubernetes module or k8s.io/kubernetes/... packages as libraries is not supported.

I'm unsure and inclined to just merge what you have 🤷‍♂️

)
150 changes: 148 additions & 2 deletions go.sum
View file
Open in desktop

Large diffs are not rendered by default.

10 changes: 10 additions & 0 deletions internal/api/warn.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
package api

// WarnResult returns a CheckResult when a warning occurs.
func WarnResult(name string, summary string) *CheckResult {
return &CheckResult{
Name: name,
State: StateWarning,
Summary: summary,
}
}
17 changes: 17 additions & 0 deletions internal/api/warn_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package api_test

import (
"testing"

"cdr.dev/slog/sloggers/slogtest/assert"
"github.com/cdr/coder-doctor/internal/api"
)

func TestWarnResult(t *testing.T) {
t.Parallel()
res := api.WarnResult("check-name", "something bad happened")

assert.Equal(t, "name matches", "check-name", res.Name)
assert.Equal(t, "state matches", api.StateWarning, res.State)
assert.Equal(t, "summary matches", "something bad happened", res.Summary)
}
165 changes: 162 additions & 3 deletions internal/checks/kube/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,164 @@ package kube
import (
"context"
"fmt"
"sort"
"strings"

"golang.org/x/xerrors"

"cdr.dev/slog"

"github.com/Masterminds/semver/v3"

"github.com/cdr/coder-doctor/internal/api"

authorizationv1 "k8s.io/api/authorization/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
authorizationclientv1 "k8s.io/client-go/kubernetes/typed/authorization/v1"
rbacutil "k8s.io/kubectl/pkg/util/rbac"
"k8s.io/kubectl/pkg/util/slice"
)

// CheckRBAC checks the cluster for the RBAC permissions required by Coder.
// It will attempt to first use a SelfSubjectRulesReview to determine the capabilities
// of the user. If this fails (notably on GKE), fall back to using SelfSubjectAccessRequests
// which is slower but is more likely to work.
func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
if ssrrResults := k.checkRBACDefault(ctx); ssrrResults != nil {
return ssrrResults
}

k.log.Warn(ctx, "SelfSubjectRulesReview response incomplete, falling back to SelfSubjectAccessRequests (slow)")
return k.checkRBACFallback(ctx)
}

func (k *KubernetesChecker) checkRBACDefault(ctx context.Context) []*api.CheckResult {
const checkName = "kubernetes-rbac-ssrr"
authClient := k.client.AuthorizationV1()
results := make([]*api.CheckResult, 0)

sar := &authorizationv1.SelfSubjectRulesReview{
Spec: authorizationv1.SelfSubjectRulesReviewSpec{
Namespace: k.namespace,
},
}

response, err := authClient.SelfSubjectRulesReviews().Create(ctx, sar, metav1.CreateOptions{})
if err != nil {
return []*api.CheckResult{api.SkippedResult(checkName, "unable to create SelfSubjectRulesReview request: "+err.Error())}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: consider adding the error to the Details field, since it's structured data.

the longer-term idea is that someone should be able to submit an output log (JSON dump or something) of their CheckResult logs, and we should be able to ingest those into BigQuery or something as a form of cluster telemetry, to help guide our product decisions -- parsing text out is possible but I think it's better to avoid that

}

if response.Status.Incomplete {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: does this mean that callers need to treat a nil slice return differently from an empty slice? I'd suggest returning a typed error for this condition instead, for example:

const errSelfSubjectRulesReviewNotSupported = errors.New("cluster does not support SelfSubjectRulesReview")

if response.Status.Incomplete {
return nil, errSelfSubjectRulesReviewNotSupported
}

and then in the caller, you can check if the err == errSelfSubjectRulesReviewNotSupported

return nil // In this case, we should fall back to using SelfSubjectAccessRequests.
}

// convert the list of rules from the server to PolicyRules and dedupe/compact
breakdownRules := []rbacv1.PolicyRule{}
for _, rule := range convertToPolicyRule(response.Status) {
breakdownRules = append(breakdownRules, rbacutil.BreakdownRule(rule)...)
}

compactRules, _ := rbacutil.CompactRules(breakdownRules) //nolint:errcheck - err is always nil
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question/suggestion: is err always guaranteed by the API contract to be nil, or is that just the current implementation? if the latter, then I think we should still check, otherwise we can have panics if they change things later

it would seem unusual to me for them to return an error that is always guaranteed to be nil, but I'm not too familiar with this API

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current implementation upstream always returns nil, weirdly enough 🤷
It's probably not guaranteed though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if it's not guaranteed, then IMO we should check, otherwise it's just a ticking time bomb :)

sort.Stable(rbacutil.SortableRuleSlice(compactRules))
for _, r := range compactRules {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: I don't know if we can check if Debug logging is enabled; if not, we could avoid the for loop/method call overhead

k.log.Debug(ctx, "Got SSRR PolicyRule", slog.F("rule", r))
}

// TODO: optimise this
for req, reqVerbs := range k.reqs.ResourceRequirements {
if err := satisfies(req, reqVerbs, compactRules); err != nil {
summary := fmt.Sprintf("resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(checkName, summary, err))
continue
}
resourceName := req.Resource
if req.Group != "" {
resourceName = req.Group + "/" + req.Resource
}
summary := fmt.Sprintf("resource %s: can %s", resourceName, strings.Join(reqVerbs, ", "))
results = append(results, api.PassResult(checkName, summary))
}

// TODO: remove this when the helm chart is fixed
for req, reqVerbs := range k.reqs.RoleOnlyResourceRequirements {
if err := satisfies(req, reqVerbs, compactRules); err != nil {
summary := fmt.Sprintf("resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(checkName, summary, err))
continue
}
resourceName := req.Resource
if req.Group != "" {
resourceName = req.Group + "/" + req.Resource
}
summary := fmt.Sprintf("resource %s: can %s", resourceName, strings.Join(reqVerbs, ", "))
results = append(results, api.PassResult(checkName, summary))
}

return results
}

func satisfies(req *ResourceRequirement, verbs ResourceVerbs, rules []rbacv1.PolicyRule) error {
for _, rule := range rules {
if apiGroupsMatch(req.Group, rule.APIGroups) &&
apiResourceMatch(req.Resource, rule.Resources) &&
verbsMatch(verbs, rule.Verbs) {
return nil
}
}
return xerrors.Errorf("not satisfied: group:%q resource:%q version:%q verbs:%+v", req.Group, req.Resource, req.Version, verbs)
}

// The below adapted from k8s.io/pkg/apis/rbac/v1/evaluation_helpers.go
func verbsMatch(want, have ResourceVerbs) bool {
if slice.ContainsString(have, "*", nil) {
return true
}

for _, v := range want {
if !slice.ContainsString(have, v, nil) {
return false
}
}
return true
}

func apiGroupsMatch(want string, have []string) bool {
for _, g := range have {
if g == "*" {
return true
}

if g == want {
return true
}
}
return false
}

func apiResourceMatch(want string, have []string) bool {
for _, r := range have {
if r == "*" {
return true
}

if r == want {
return true
}
}
return false
}

// checkRBACFallback uses a SelfSubjectAccessRequest to check the cluster for the required
// accesses. This requires a number of checks and is relatively slow.
func (k *KubernetesChecker) checkRBACFallback(ctx context.Context) []*api.CheckResult {
const checkName = "kubernetes-rbac"
authClient := k.client.AuthorizationV1()
results := make([]*api.CheckResult, 0)

for req, reqVerbs := range k.reqs.ResourceRequirements {
resName := fmt.Sprintf("%s-%s", checkName, req.Resource)
if err := k.checkOneRBAC(ctx, authClient, req, reqVerbs); err != nil {
if err := k.checkOneRBACSSAR(ctx, authClient, req, reqVerbs); err != nil {
summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(resName, summary, err))
continue
Expand All @@ -37,7 +174,7 @@ func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
// that don't exist.
for req, reqVerbs := range k.reqs.RoleOnlyResourceRequirements {
resName := fmt.Sprintf("%s-%s", checkName, req.Resource)
if err := k.checkOneRBAC(ctx, authClient, req, reqVerbs); err != nil {
if err := k.checkOneRBACSSAR(ctx, authClient, req, reqVerbs); err != nil {
summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(resName, summary, err))
continue
Expand All @@ -50,7 +187,7 @@ func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
return results
}

func (k *KubernetesChecker) checkOneRBAC(ctx context.Context, authClient authorizationclientv1.AuthorizationV1Interface, req *ResourceRequirement, reqVerbs ResourceVerbs) error {
func (k *KubernetesChecker) checkOneRBACSSAR(ctx context.Context, authClient authorizationclientv1.AuthorizationV1Interface, req *ResourceRequirement, reqVerbs ResourceVerbs) error {
have := make([]string, 0, len(reqVerbs))
for _, verb := range reqVerbs {
sar := &authorizationv1.SelfSubjectAccessReview{
Expand Down Expand Up @@ -92,3 +229,25 @@ func findClosestVersionRequirements(v *semver.Version) *VersionedResourceRequire
}
return nil
}

// lifted from kubectl/pkg/cmd/auth/cani.go
func convertToPolicyRule(status authorizationv1.SubjectRulesReviewStatus) []rbacv1.PolicyRule {
ret := []rbacv1.PolicyRule{}
for _, resource := range status.ResourceRules {
ret = append(ret, rbacv1.PolicyRule{
Verbs: resource.Verbs,
APIGroups: resource.APIGroups,
Resources: resource.Resources,
ResourceNames: resource.ResourceNames,
})
}

for _, nonResource := range status.NonResourceRules {
ret = append(ret, rbacv1.PolicyRule{
Verbs: nonResource.Verbs,
NonResourceURLs: nonResource.NonResourceURLs,
})
}

return ret
}
Loading