NETOBSERV-972 check if cluster admin via namespaces

Follow-up on netobserv#320, which relaxed the permission checks performed when lokiAuth is DISABLED: after discussion, we roll back to a more strict approach; however to mitigate the limitation of TokenReview (it doesn't provide a reliable way to check for cluster admins right), we verify that the user can list namespaces, assuming this is a cluster admin capability.
jotak · May 22, 2023 · 512759c · 512759c
1 parent 99b3a5f
commit 512759c
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 46 deletions.
diff --git a/cmd/plugin-backend.go b/cmd/plugin-backend.go
@@ -93,12 +93,9 @@ func main() {
 		if *lokiForwardUserToken {
 			// FORWARD lokiAuth mode
 			checkType = auth.CheckAuthenticated
-		} else if *lokiTokenPath != "" {
-			// HOST lokiAuth mode
-			checkType = auth.CheckAdmin
 		} else {
-			// DISABLED lokiAuth mode
-			checkType = auth.CheckAuthenticated
+			// HOST or DISABLED lokiAuth mode
+			checkType = auth.CheckAdmin
 		}
 		log.Info(fmt.Sprintf("auth-check 'auto' resolved to '%s'", checkType))
 	} else {

diff --git a/pkg/kubernetes/auth/check_auth.go b/pkg/kubernetes/auth/check_auth.go
@@ -33,9 +33,9 @@ func NewChecker(typez CheckType, apiProvider client.APIProvider) (Checker, error
 	case CheckNone:
 		return &NoopChecker{}, nil
 	case CheckAuthenticated:
-		return &BearerTokenChecker{apiProvider: apiProvider, predicates: []tokenReviewPredicate{mustBeAuthenticated}}, nil
+		return &BearerTokenChecker{apiProvider: apiProvider, predicates: []authPredicate{mustBeAuthenticated}}, nil
 	case CheckAdmin:
-		return &BearerTokenChecker{apiProvider: apiProvider, predicates: []tokenReviewPredicate{mustBeAuthenticated, mustBeClusterAdmin}}, nil
+		return &BearerTokenChecker{apiProvider: apiProvider, predicates: []authPredicate{mustBeAuthenticated, mustBeClusterAdmin}}, nil
 	}
 	return nil, fmt.Errorf("auth checker type unknown: %s. Must be one of %s, %s, %s", typez, CheckAdmin, CheckAuthenticated, CheckNone)
 }
@@ -61,50 +61,47 @@ func getUserToken(header http.Header) (string, error) {
 	return "", errors.New("missing Authorization header")
 }
 
-func runTokenReview(ctx context.Context, apiProvider client.APIProvider, token string, preds []tokenReviewPredicate) error {
+func runTokenReview(ctx context.Context, apiProvider client.APIProvider, token string, preds []authPredicate) error {
 	client, err := apiProvider()
 	if err != nil {
 		return err
 	}
-
-	rvw, err := client.CreateTokenReview(ctx, &authv1.TokenReview{
-		Spec: authv1.TokenReviewSpec{
-			Token: token,
-		},
-	}, &metav1.CreateOptions{})
-	if err != nil {
-		return err
-	}
 	for _, predFunc := range preds {
-		if err = predFunc(rvw); err != nil {
+		if err = predFunc(ctx, client, token); err != nil {
 			return err
 		}
 	}
 	return nil
 }
 
-type tokenReviewPredicate func(*authv1.TokenReview) error
+type authPredicate func(context.Context, client.KubeAPI, string) error
 
-func mustBeAuthenticated(rvw *authv1.TokenReview) error {
+func mustBeAuthenticated(ctx context.Context, cl client.KubeAPI, token string) error {
+	rvw, err := cl.CreateTokenReview(ctx, &authv1.TokenReview{
+		Spec: authv1.TokenReviewSpec{
+			Token: token,
+		},
+	}, &metav1.CreateOptions{})
+	if err != nil {
+		return err
+	}
 	if !rvw.Status.Authenticated {
 		return errors.New("user not authenticated")
 	}
 	return nil
 }
 
-func mustBeClusterAdmin(rvw *authv1.TokenReview) error {
-	for _, group := range rvw.Status.User.Groups {
-		if group == "system:cluster-admins" {
-			return nil
-		}
+func mustBeClusterAdmin(ctx context.Context, cl client.KubeAPI, token string) error {
+	if err := cl.CheckAdmin(ctx, token); err != nil {
+		return errors.New("user not an admin")
 	}
-	return errors.New("user not in cluster-admins group")
+	return nil
 }
 
 type BearerTokenChecker struct {
 	Checker
 	apiProvider client.APIProvider
-	predicates  []tokenReviewPredicate
+	predicates  []authPredicate
 }
 
 func (c *BearerTokenChecker) CheckAuth(ctx context.Context, header http.Header) error {

diff --git a/pkg/kubernetes/auth/check_auth_test.go b/pkg/kubernetes/auth/check_auth_test.go
@@ -13,13 +13,13 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-func setupChecker(typez CheckType, m *TokenReviewMock) Checker {
+func setupChecker(typez CheckType, m *AuthCheckMock) Checker {
 	checker, _ := NewChecker(typez, func() (client.KubeAPI, error) { return m, nil })
 	return checker
 }
 
 func TestCheckAuth_NoAuth(t *testing.T) {
-	m := TokenReviewMock{}
+	m := AuthCheckMock{}
 	m.mockNoAuth()
 
 	// Any user authenticated mode
@@ -55,7 +55,7 @@ func TestCheckAuth_NoAuth(t *testing.T) {
 }
 
 func TestCheckAuth_NormalUser(t *testing.T) {
-	m := TokenReviewMock{}
+	m := AuthCheckMock{}
 	m.mockNormalUser()
 
 	// Any user authenticated mode
@@ -79,7 +79,7 @@ func TestCheckAuth_NormalUser(t *testing.T) {
 
 	err = checkerAdmin.CheckAuth(context.TODO(), http.Header{"Authorization": []string{"Bearer abcdef"}})
 	require.Error(t, err)
-	require.Equal(t, "user not in cluster-admins group", err.Error())
+	require.Equal(t, "user not an admin", err.Error())
 
 	// Noop mode
 	checkerNoop := NoopChecker{}
@@ -90,7 +90,7 @@ func TestCheckAuth_NormalUser(t *testing.T) {
 }
 
 func TestCheckAuth_Admin(t *testing.T) {
-	m := TokenReviewMock{}
+	m := AuthCheckMock{}
 	m.mockAdmin()
 
 	// Any user authenticated mode
@@ -126,7 +126,7 @@ func TestCheckAuth_Admin(t *testing.T) {
 const fakeError = "an error occured"
 
 func TestCheckAuth_APIError(t *testing.T) {
-	m := TokenReviewMock{}
+	m := AuthCheckMock{}
 	m.mockError()
 
 	// Any user authenticated mode
@@ -161,39 +161,47 @@ func TestCheckAuth_APIError(t *testing.T) {
 	require.NoError(t, err)
 }
 
-type TokenReviewMock struct {
+type AuthCheckMock struct {
 	mock.Mock
 	client.KubeAPI
 }
 
-func (m *TokenReviewMock) CreateTokenReview(ctx context.Context, tr *authv1.TokenReview, opts *metav1.CreateOptions) (*authv1.TokenReview, error) {
+func (m *AuthCheckMock) CreateTokenReview(ctx context.Context, tr *authv1.TokenReview, opts *metav1.CreateOptions) (*authv1.TokenReview, error) {
 	args := m.Called(ctx, tr, opts)
 	return args.Get(0).(*authv1.TokenReview), args.Error(1)
 }
 
-func (m *TokenReviewMock) mockError() {
+func (m *AuthCheckMock) CheckAdmin(ctx context.Context, token string) error {
+	args := m.Called(ctx, token)
+	return args.Error(0)
+}
+
+func (m *AuthCheckMock) mockError() {
 	m.On("CreateTokenReview", mock.Anything, mock.Anything, mock.Anything).Return(&authv1.TokenReview{}, errors.New(fakeError))
 }
 
-func (m *TokenReviewMock) mockNoAuth() {
+func (m *AuthCheckMock) mockNoAuth() {
 	m.On("CreateTokenReview", mock.Anything, mock.Anything, mock.Anything).Return(&authv1.TokenReview{
-		Status: mockedTokenReviewStatus(false, true),
+		Status: mockedTokenReviewStatus(false),
 	}, nil)
+	m.On("CheckAdmin", mock.Anything, mock.Anything).Return(errors.New("User is not an admin"))
 }
 
-func (m *TokenReviewMock) mockNormalUser() {
+func (m *AuthCheckMock) mockNormalUser() {
 	m.On("CreateTokenReview", mock.Anything, mock.Anything, mock.Anything).Return(&authv1.TokenReview{
-		Status: mockedTokenReviewStatus(true, false),
+		Status: mockedTokenReviewStatus(true),
 	}, nil)
+	m.On("CheckAdmin", mock.Anything, mock.Anything).Return(errors.New("User is not an admin"))
 }
 
-func (m *TokenReviewMock) mockAdmin() {
+func (m *AuthCheckMock) mockAdmin() {
 	m.On("CreateTokenReview", mock.Anything, mock.Anything, mock.Anything).Return(&authv1.TokenReview{
-		Status: mockedTokenReviewStatus(true, true),
+		Status: mockedTokenReviewStatus(true),
 	}, nil)
+	m.On("CheckAdmin", mock.Anything, mock.Anything).Return(nil)
 }
 
-func mockedTokenReviewStatus(isAuth, isAdmin bool) authv1.TokenReviewStatus {
+func mockedTokenReviewStatus(isAuth bool) authv1.TokenReviewStatus {
 	st := authv1.TokenReviewStatus{
 		Authenticated: isAuth,
 		User: authv1.UserInfo{
@@ -205,8 +213,5 @@ func mockedTokenReviewStatus(isAuth, isAdmin bool) authv1.TokenReviewStatus {
 	if isAuth {
 		st.User.Groups = append(st.User.Groups, "system:authenticated")
 	}
-	if isAdmin {
-		st.User.Groups = append(st.User.Groups, "system:cluster-admins")
-	}
 	return st
 }
diff --git a/pkg/kubernetes/client/client.go b/pkg/kubernetes/client/client.go
@@ -12,6 +12,7 @@ import (
 // Interface for mocking
 type KubeAPI interface {
 	CreateTokenReview(ctx context.Context, tr *authv1.TokenReview, opts *metav1.CreateOptions) (*authv1.TokenReview, error)
+	CheckAdmin(ctx context.Context, token string) error
 }
 
 type APIProvider func() (KubeAPI, error)
@@ -25,6 +26,22 @@ func (c *InCluster) CreateTokenReview(ctx context.Context, tr *authv1.TokenRevie
 	return c.client.AuthenticationV1().TokenReviews().Create(ctx, tr, *opts)
 }
 
+func (c *InCluster) CheckAdmin(ctx context.Context, token string) error {
+	config, err := rest.InClusterConfig()
+	if err != nil {
+		return err
+	}
+	config.BearerToken = token
+	config.BearerTokenFile = ""
+
+	client, err := kubernetes.NewForConfig(config)
+	if err != nil {
+		return err
+	}
+	_, err = client.CoreV1().Namespaces().List(ctx, metav1.ListOptions{})
+	return err
+}
+
 func NewInCluster() (KubeAPI, error) {
 	config, err := rest.InClusterConfig()
 	if err != nil {