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 1 commit
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
Prev Previous commit
Next Next commit
feat: use SSRR for RBAC instead of SSAR
  • Loading branch information
@johnstcn
johnstcn committed Aug 30, 2021
commit c49a547764b4b862224d0a308bb80dec09309bf6
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.

4 changes: 2 additions & 2 deletions internal/checks/kube/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ func (k *KubernetesChecker) Run(ctx context.Context) error {
}
}

for _, res := range k.CheckRBAC(ctx) {
for _, res := range k.CheckRBACSSRR(ctx) {
if err := k.writer.WriteResult(res); err != nil {
return xerrors.Errorf("check RBAC: %w", err)
return xerrors.Errorf("check SSAR: %w", err)
}
}
return nil
Expand Down
120 changes: 120 additions & 0 deletions internal/checks/kube/rbac.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package kube
import (
"context"
"fmt"
"sort"
"strings"

"golang.org/x/xerrors"
Expand All @@ -12,8 +13,11 @@ import (
"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"
)

func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
Expand Down Expand Up @@ -50,6 +54,100 @@ func (k *KubernetesChecker) CheckRBAC(ctx context.Context) []*api.CheckResult {
return results
}

func (k *KubernetesChecker) CheckRBACSSRR(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

results = append(results, api.WarnResult(checkName, "SelfSubjectRulesReview response incomplete - results may not be correct"))
}

// 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))

// TODO: optimise this
for req, reqVerbs := range k.reqs.ResourceRequirements {
if err := satisfies(req, reqVerbs, compactRules); err != nil {
summary := fmt.Sprintf("missing permissions on resource %s: %s", req.Resource, err)
results = append(results, api.ErrorResult(checkName, summary, err))
continue
}
summary := fmt.Sprintf("%s: can %s", req.Resource, 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
}

func (k *KubernetesChecker) checkOneRBAC(ctx context.Context, authClient authorizationclientv1.AuthorizationV1Interface, req *ResourceRequirement, reqVerbs ResourceVerbs) error {
have := make([]string, 0, len(reqVerbs))
for _, verb := range reqVerbs {
Expand Down Expand Up @@ -92,3 +190,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
}
111 changes: 111 additions & 0 deletions internal/checks/kube/rbac_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

authorizationv1 "k8s.io/api/authorization/v1"
rbacv1 "k8s.io/api/rbac/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/client-go/rest"

Expand Down Expand Up @@ -91,3 +92,113 @@ var selfSubjectAccessReviewDenied authorizationv1.SelfSubjectAccessReview = auth
Allowed: false,
},
}

func Test_CheckRBACSSRR(t *testing.T) {
t.Parallel()

tests := []struct {
Name string
Response *authorizationv1.SelfSubjectRulesReview
F func(*testing.T, []*api.CheckResult)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: just a personal preference and maybe anathema to Go's philosophy, I think it's clearer to use full names for things instead of one-letter variables, e.g. CheckFunc or something

}{
{
Name: "nothing allowed",
Response: &selfSubjectRulesReviewEmpty,
F: func(t *testing.T, results []*api.CheckResult) {
assert.False(t, "results should not be empty", len(results) == 0)
for _, result := range results {
assert.True(t, result.Name+" should have an error", result.Details["error"] != nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion/nitpick: we might want to consider a helper func that does this, similar to what we do in slog

assert.True(t, result.Name+" should fail", result.State == api.StateFailed)
}
},
},
}

for _, test := range tests {
test := test
t.Run(test.Name, func(t *testing.T) {
t.Parallel()

server := newTestHTTPServer(t, http.StatusOK, test.Response)
defer server.Close()

client, err := kubernetes.NewForConfig(&rest.Config{Host: server.URL})
assert.Success(t, "failed to create client", err)

checker := NewKubernetesChecker(client)
results := checker.CheckRBACSSRR(context.Background())
test.F(t, results)
})
}
}

func Test_Satisfies(t *testing.T) {
t.Parallel()

testCases := []struct {
Name string
Requirement *ResourceRequirement
Verbs ResourceVerbs
Rules []rbacv1.PolicyRule
Expected *string
}{
{
Name: "allowed: get create pods",
Requirement: NewResourceRequirement("", "v1", "pods"),
Verbs: verbsGetCreate,
Rules: []rbacv1.PolicyRule{testV1WildcardRule},
Expected: nil,
},
{
Name: "not allowed: get create pods",
Requirement: NewResourceRequirement("", "v1", "pods"),
Verbs: verbsGetCreate,
Rules: []rbacv1.PolicyRule{testNoPermRule},
Expected: strptr("not satisfied"),
},
// TODO(cian): add many, many, more.
}

for _, testCase := range testCases {
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: personally I use tests for the array and test for the loop var, not a strong feeling, might be nice to be consistent

testCase := testCase
t.Run(testCase.Name, func(t *testing.T) {
actual := satisfies(testCase.Requirement, testCase.Verbs, testCase.Rules)
if testCase.Expected == nil {
assert.Success(t, testCase.Name+"- should not error", actual)
} else {
assert.ErrorContains(t, testCase.Name+" - expected error contains", actual, *testCase.Expected)
}
})
}
}

var testNoPermRule = makeTestPolicyRule(ss(), ss(), ss(), ss(), ss())
var testV1WildcardRule = makeTestPolicyRule(verbsAll, ss(""), ss("*"), ss(), ss())

func makeTestPolicyRule(verbs, groups, resources, resourceNames, nonResourceURLs []string) rbacv1.PolicyRule {
return rbacv1.PolicyRule{
Verbs: verbs,
APIGroups: groups,
Resources: resources,
ResourceNames: resourceNames,
NonResourceURLs: nonResourceURLs,
}
}

var selfSubjectRulesReviewEmpty = authorizationv1.SelfSubjectRulesReview{
Spec: authorizationv1.SelfSubjectRulesReviewSpec{
Namespace: "default",
},
Status: authorizationv1.SubjectRulesReviewStatus{
ResourceRules: []authorizationv1.ResourceRule{},
NonResourceRules: []authorizationv1.NonResourceRule{},
},
}

func strptr(s string) *string {
Copy link
Contributor

Choose a reason for hiding this comment

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

kubernetes has a pointer.String() util for this

return &s
}

func ss(s ...string) []string {
return s
}
3 changes: 3 additions & 0 deletions internal/cmd/check/kubernetes/kubernetes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ func run(cmd *cobra.Command, _ []string) error {
}

currentContext := rawConfig.Contexts[rawConfig.CurrentContext]
if currentContext.Namespace == "" {
currentContext.Namespace = "default"
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess Kubernetes doesn't do this automatically, for some reason?

I don't know if we have a default namespace, if we did I'd assume it to be coder rather than default, so if we need a namespace we might want to consider using coder here?

}

log.Info(cmd.Context(), "kubernetes config:",
slog.F("context", rawConfig.CurrentContext),
Expand Down