Skip to content

fix: simplify checker for access to namespaces #4666

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: simplify checker for access to namespaces #4666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion cmd/gitops-server/cmd/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ func runCmd(cmd *cobra.Command, args []string) error {

fetcher := fetcher.NewSingleClusterFetcher(cl)

clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{fetcher}, nsaccess.NewChecker(nsaccess.DefautltWegoAppRules), log)
clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{fetcher}, nsaccess.NewChecker(), log)
clustersManager.Start(ctx)

healthChecker := health.NewHealthChecker()
Expand Down
4 changes: 2 additions & 2 deletions core/clustersmngr/factory_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ func TestUpdateUserNamespacesFailsToConnect(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

nsChecker := nsaccess.NewChecker(nil)
nsChecker := nsaccess.NewChecker()
clustersFetcher := new(clustersmngrfakes.FakeClusterFetcher)

clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{clustersFetcher}, nsChecker, logger)
Expand Down Expand Up @@ -302,7 +302,7 @@ func TestGetClusters(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()

nsChecker := nsaccess.NewChecker(nil)
nsChecker := nsaccess.NewChecker()
clustersFetcher := new(clustersmngrfakes.FakeClusterFetcher)

clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{clustersFetcher}, nsChecker, logger)
Expand Down
228 changes: 35 additions & 193 deletions core/nsaccess/nsaccess.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,48 +6,12 @@ import (

authorizationv1 "k8s.io/api/authorization/v1"
corev1 "k8s.io/api/core/v1"
rbacv1 "k8s.io/api/rbac/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
typedauth "k8s.io/client-go/kubernetes/typed/authorization/v1"
)

//go:generate go run github.com/maxbrunsfeld/counterfeiter/v6 -generate

// DefautltWegoAppRules is the minimun set of permissions a user will need to use the wego-app in a given namespace
var DefautltWegoAppRules = []rbacv1.PolicyRule{
{
APIGroups: []string{""},
Resources: []string{"pods", "secrets"},
Verbs: []string{"get", "list"},
},
{
APIGroups: []string{""},
Resources: []string{"events"},
Verbs: []string{"get", "list", "watch"},
},
{
APIGroups: []string{"apps"},
Resources: []string{"deployments", "replicasets"},
Verbs: []string{"get", "list"},
},
{
APIGroups: []string{"kustomize.toolkit.fluxcd.io"},
Resources: []string{"kustomizations"},
Verbs: []string{"get", "list"},
},
{
APIGroups: []string{"helm.toolkit.fluxcd.io"},
Resources: []string{"helmreleases"},
Verbs: []string{"get", "list"},
},
{
APIGroups: []string{"source.toolkit.fluxcd.io"},
Resources: []string{"buckets", "helmcharts", "helmrepositories", "gitrepositories", "ocirepositories"},
Verbs: []string{"get", "list"},
},
}

// Checker contains methods for validing user access to Kubernetes namespaces, based on a set of PolicyRules
//
//counterfeiter:generate . Checker
Expand All @@ -56,19 +20,25 @@ type Checker interface {
FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error)
}

type simpleChecker struct {
rules []rbacv1.PolicyRule
}
type simpleChecker struct{}

func NewChecker(rules []rbacv1.PolicyRule) Checker {
return simpleChecker{rules: rules}
func NewChecker() Checker {
return simpleChecker{}
}

func (sc simpleChecker) FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error) {
result := []corev1.Namespace{}
accessToAllNamespace, err := hasAccessToAllNamespaces(ctx, auth)
if err != nil {
return nil, err
}

if accessToAllNamespace {
return namespaces, nil
}

var result []corev1.Namespace
for _, ns := range namespaces {
ok, err := userCanUseNamespace(ctx, auth, ns, sc.rules)
ok, err := hasAccessToNamespace(ctx, auth, ns)
if err != nil {
return nil, fmt.Errorf("user namespace access: %w", err)
}
Expand All @@ -81,163 +51,35 @@ func (sc simpleChecker) FilterAccessibleNamespaces(ctx context.Context, auth typ
return result, nil
}

func userCanUseNamespace(ctx context.Context, auth typedauth.AuthorizationV1Interface, ns corev1.Namespace, rules []rbacv1.PolicyRule) (bool, error) {
sar := &authorizationv1.SelfSubjectRulesReview{
Spec: authorizationv1.SelfSubjectRulesReviewSpec{
Namespace: ns.Name,
func hasAccessToNamespace(ctx context.Context, auth typedauth.AuthorizationV1Interface, ns corev1.Namespace) (bool, error) {
ssar := &authorizationv1.SelfSubjectAccessReview{
Spec: authorizationv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authorizationv1.ResourceAttributes{
Verb: "get",
Resource: "configmaps",
Namespace: ns.Name,
},
},
}

authRes, err := auth.SelfSubjectRulesReviews().Create(ctx, sar, metav1.CreateOptions{})
res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
if err != nil {
return false, err
}

return hasAllRules(authRes.Status, rules), nil
return res.Status.Allowed, nil
}

var allK8sVerbs = []string{"create", "get", "list", "watch", "patch", "delete", "deletecollection"}

func allResources(rules []rbacv1.PolicyRule) []string {
resources := sets.NewString()
for _, r := range rules {
resources.Insert(r.Resources...)
}

return resources.List()
}

func allAPIGroups(rules []rbacv1.PolicyRule) []string {
apiGroups := sets.NewString()
for _, r := range rules {
apiGroups.Insert(r.APIGroups...)
}

return apiGroups.List()
}

// hasAll rules determines if a set of SubjectRulesReview rules match a minimum set of policy rules
//
// We need to understand the "sum" of all the rules for a role.
// Convert to a hash lookup to make it easier to tell what a user can do.
// Looks like { "apps": { "deployments": { get: true, list: true } } }
//
// We handle wildcards by expanding them out to all the types of resources/APIGroups
// that are relevant to the provided rules.
// This creates slightly nonsensical sets sometimes, for example given:
//
// PolicyRules:
//
// {
// APIGroups: []string{""},
// Resources: []string{"secrets"},
// Verbs: []string{"get"},
// },
//
// {
// APIGroups: []string{"apps"},
// Resources: []string{"deployment"}",
// Verbs: []string{"get"},
// },
//
// SubjectRulesReviewStatus:
// ["*", "*", "get"]
//
// We get:
//
// {
// "": {
// "secrets": { get: true }
// "deployments": { get: true }
// },
// "apps": {
// "secrets": { get: true }
// "deployments", "secrets": { get: true }
// }
// }
//
// Secrets don't exist in "apps" according to k8s,
// but the "index checker" that checks this struct does not mind.
func hasAllRules(status authorizationv1.SubjectRulesReviewStatus, rules []rbacv1.PolicyRule) bool {
derivedAccess := map[string]map[string]map[string]bool{}

allResourcesInRules := allResources(rules)
allAPIGroupsInRules := allAPIGroups(rules)

for _, statusRule := range status.ResourceRules {
apiGroups := statusRule.APIGroups
if containsWildcard(apiGroups) {
apiGroups = allAPIGroupsInRules
}

for _, apiGroup := range apiGroups {
if _, ok := derivedAccess[apiGroup]; !ok {
derivedAccess[apiGroup] = map[string]map[string]bool{}
}

resources := statusRule.Resources
if containsWildcard(resources) {
resources = allResourcesInRules
}

for _, resource := range resources {
if _, ok := derivedAccess[apiGroup][resource]; !ok {
derivedAccess[apiGroup][resource] = map[string]bool{}
}

verbs := statusRule.Verbs
if containsWildcard(verbs) {
verbs = allK8sVerbs
}

for _, v := range verbs {
derivedAccess[apiGroup][resource][v] = true
}
}
}
}

hasAccess := true

Rules:
for _, rule := range rules {
for _, apiGroup := range rule.APIGroups {
g, ok := derivedAccess[apiGroup]

if !ok {
hasAccess = false
break Rules
}
for _, resource := range rule.Resources {
r, ok := g[resource]
if !ok {
// A resource is not present for this apiGroup.
hasAccess = false
break Rules
}

for _, verb := range rule.Verbs {
_, ok := r[verb]
if !ok {
// A verb is not present for this resource,
// no need to check the rest of the verbs.
hasAccess = false
break Rules
}
}
}
}
func hasAccessToAllNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface) (bool, error) {
ssar := &authorizationv1.SelfSubjectAccessReview{
Spec: authorizationv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authorizationv1.ResourceAttributes{
Verb: "list",
Resource: "namespaces",
},
},
}

return hasAccess
}

func containsWildcard(permissions []string) bool {
for _, p := range permissions {
if p == "*" {
return true
}
res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
if err != nil {
return false, err
}

return false
return res.Status.Allowed, nil
}
Loading