Skip to content

Commit

Permalink
fix: simplify checker for access to namespaces
Browse files Browse the repository at this point in the history
  • Loading branch information
erikgb committed Feb 1, 2025
1 parent 9722058 commit 9030f4c
Show file tree
Hide file tree
Showing 6 changed files with 196 additions and 5 deletions.
2 changes: 1 addition & 1 deletion cmd/gitops-server/cmd/cmd.go
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.NewSimplerChecker(), log)
clustersManager.Start(ctx)

healthChecker := health.NewHealthChecker()
Expand Down
4 changes: 2 additions & 2 deletions core/clustersmngr/factory_test.go
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.NewSimplerChecker()
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.NewSimplerChecker()
clustersFetcher := new(clustersmngrfakes.FakeClusterFetcher)

clustersManager := clustersmngr.NewClustersManager([]clustersmngr.ClusterFetcher{clustersFetcher}, nsChecker, logger)
Expand Down
75 changes: 75 additions & 0 deletions core/nsaccess/simpler_nsaccess.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package nsaccess

import (
"context"
"fmt"

Check failure on line 5 in core/nsaccess/simpler_nsaccess.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

File is not properly formatted (gci)
authv1 "k8s.io/api/authorization/v1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
typedauth "k8s.io/client-go/kubernetes/typed/authorization/v1"
)

type simplerChecker struct {
}

func NewSimplerChecker() Checker {
return simplerChecker{}
}

func (sc simplerChecker) FilterAccessibleNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface, namespaces []corev1.Namespace) ([]corev1.Namespace, error) {
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 := hasAccessToNamespace(ctx, auth, ns)
if err != nil {
return nil, fmt.Errorf("user namespace access: %v", err)

Check failure on line 33 in core/nsaccess/simpler_nsaccess.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
}

if ok {
result = append(result, ns)
}
}

return result, nil
}

func hasAccessToNamespace(ctx context.Context, auth typedauth.AuthorizationV1Interface, ns corev1.Namespace) (bool, error) {
ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Verb: "get",
Resource: "configmaps",
Namespace: ns.Name,
},
},
}
res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
if err != nil {
return false, err
}
return res.Status.Allowed, nil
}

func hasAccessToAllNamespaces(ctx context.Context, auth typedauth.AuthorizationV1Interface) (bool, error) {
ssar := &authv1.SelfSubjectAccessReview{
Spec: authv1.SelfSubjectAccessReviewSpec{
ResourceAttributes: &authv1.ResourceAttributes{
Verb: "list",
Resource: "namespaces",
},
},
}
res, err := auth.SelfSubjectAccessReviews().Create(ctx, ssar, metav1.CreateOptions{})
if err != nil {
return false, err
}
return res.Status.Allowed, nil
}
116 changes: 116 additions & 0 deletions core/nsaccess/simpler_nsaccess_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package nsaccess

import (
"context"

Check failure on line 4 in core/nsaccess/simpler_nsaccess_test.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

File is not properly formatted (gci)
rbacv1 "k8s.io/api/rbac/v1"
v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/client-go/kubernetes"
"k8s.io/kubectl/pkg/scheme"
"testing"

Check failure on line 9 in core/nsaccess/simpler_nsaccess_test.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

File is not properly formatted (gci)

. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
)

func Test_simplerChecker_FilterAccessibleNamespaces(t *testing.T) {
g := NewGomegaWithT(t)
ctx := context.Background()

testEnv := &envtest.Environment{}

testCfg, err := testEnv.Start()
g.Expect(err).NotTo(HaveOccurred())

t.Cleanup(func() {
err := testEnv.Stop()
if err != nil {
t.Error(err)
}
})

adminClient, err := client.New(testCfg, client.Options{
Scheme: scheme.Scheme,
})
g.Expect(err).NotTo(HaveOccurred())

// The aggregated cluster role controller is not running in the simplified testEnv control plane.
// Prepare the default admin user-facing role for the following tests.
cr := &rbacv1.ClusterRole{}
g.Expect(adminClient.Get(ctx, client.ObjectKey{Name: "admin"}, cr)).To(Succeed())
g.Expect(cr.Rules).To(BeEmpty())
cr.Rules = append(cr.Rules, rbacv1.PolicyRule{
APIGroups: []string{""},
Resources: []string{"configmaps"},
Verbs: []string{"get", "list", "watch"},
})
g.Expect(adminClient.Update(ctx, cr)).To(Succeed())

t.Run("cluster-admin has access to all namespaces", func(t *testing.T) {
list := &corev1.NamespaceList{}
g.Expect(adminClient.List(ctx, list)).To(Succeed())
g.Expect(list.Items).To(Not(BeEmpty()))

cs, err := kubernetes.NewForConfig(testCfg)
g.Expect(err).NotTo(HaveOccurred())

res, err := NewSimplerChecker().FilterAccessibleNamespaces(ctx, cs.AuthorizationV1(), list.Items)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(res).To(ContainElements(list.Items))
})

t.Run("standard user has access to no namespaces", func(t *testing.T) {
list := &corev1.NamespaceList{}
g.Expect(adminClient.List(ctx, list)).To(Succeed())
g.Expect(list.Items).To(Not(BeEmpty()))

user, err := testEnv.AddUser(envtest.User{Name: "no-access-user"}, nil)

Check failure on line 68 in core/nsaccess/simpler_nsaccess_test.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

ineffectual assignment to err (ineffassign)
cs, err := kubernetes.NewForConfig(user.Config())
g.Expect(err).NotTo(HaveOccurred())

res, err := NewSimplerChecker().FilterAccessibleNamespaces(ctx, cs.AuthorizationV1(), list.Items)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(res).To(BeEmpty())
})

t.Run("namespace owner has access to namespace", func(t *testing.T) {
username := "test-user"

ns := &corev1.Namespace{
ObjectMeta: v1.ObjectMeta{
GenerateName: "test-ns-",
},
}
g.Expect(adminClient.Create(ctx, ns)).To(Succeed())

rb := &rbacv1.RoleBinding{
ObjectMeta: v1.ObjectMeta{
Name: "test-ns-admins",
Namespace: ns.Name,
},
RoleRef: rbacv1.RoleRef{
APIGroup: "rbac.authorization.k8s.io",
Kind: "ClusterRole",
Name: "admin",
},
Subjects: []rbacv1.Subject{{
Kind: "User",
Name: username,
}},
}
g.Expect(adminClient.Create(ctx, rb)).To(Succeed())

list := &corev1.NamespaceList{}
g.Expect(adminClient.List(ctx, list)).To(Succeed())
g.Expect(list.Items).To(ContainElement(HaveField("Name", ns.Name)))

user, err := testEnv.AddUser(envtest.User{Name: username}, nil)

Check failure on line 108 in core/nsaccess/simpler_nsaccess_test.go

View workflow job for this annotation

GitHub Actions / CI Check Static Checks

ineffectual assignment to err (ineffassign)
cs, err := kubernetes.NewForConfig(user.Config())
g.Expect(err).NotTo(HaveOccurred())

res, err := NewSimplerChecker().FilterAccessibleNamespaces(ctx, cs.AuthorizationV1(), list.Items)
g.Expect(err).NotTo(HaveOccurred())
g.Expect(res).To(ConsistOf(HaveField("Name", ns.Name)))
})
}
2 changes: 1 addition & 1 deletion core/server/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ func NewCoreConfig(log logr.Logger, cfg *rest.Config, clusterName string, cluste
log: log.WithName("core-server"),
RestCfg: cfg,
clusterName: clusterName,
NSAccess: nsaccess.NewChecker(nsaccess.DefautltWegoAppRules),
NSAccess: nsaccess.NewSimplerChecker(),
ClustersManager: clustersManager,
PrimaryKinds: kinds,
HealthChecker: healthChecker,
Expand Down
2 changes: 1 addition & 1 deletion pkg/services/crd/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func createClient(k8sEnv *testutils.K8sTestEnv) (clustersmngr.Client, clustersmn

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

Expand Down

0 comments on commit 9030f4c

Please sign in to comment.