From 93c89b56b3b5185f6d08c70e1d139cbedf3d25b9 Mon Sep 17 00:00:00 2001 From: Sergey Smolnikov Date: Fri, 22 Sep 2023 14:42:44 +0200 Subject: [PATCH] Validate user is member of one of existing admin groups (#538) * Added validation to create a config map * Validate creating a config map * Validate get a config map * Fixed deleting of temp role and rolebinding * Fixed has access * Corrected prop name case * Added validation to create app method. Removed from playground case * Added warning * Corrected * Fixed unit-tests * Fixed unit-tests * Added unit-tests --- .vscode/launch.json | 2 + api/applications/applications_controller.go | 30 ++-- .../applications_controller_test.go | 155 +++++++++++++++++- api/applications/applications_handler.go | 151 +++++++++++++++-- .../applications_handler_config.go | 8 +- .../applications_handler_factory.go | 32 +++- api/applications/errors.go | 7 + api/applications/get_applications_handler.go | 29 +--- .../models/application_registration_patch.go | 2 +- api/utils/access/access.go | 29 ++++ 10 files changed, 379 insertions(+), 66 deletions(-) create mode 100644 api/applications/errors.go create mode 100644 api/utils/access/access.go diff --git a/.vscode/launch.json b/.vscode/launch.json index 65367744..ed088e52 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -20,6 +20,8 @@ "RADIX_ACTIVE_CLUSTER_EGRESS_IPS": "104.45.84.1", "REQUIRE_APP_CONFIGURATION_ITEM": "true", "REQUIRE_APP_AD_GROUPS": "true", + "RADIX_ENVIRONMENT":"qa", + "RADIX_APP":"radix-api" }, "args": [ "--useOutClusterClient=false" diff --git a/api/applications/applications_controller.go b/api/applications/applications_controller.go index e21dfcf0..89389964 100644 --- a/api/applications/applications_controller.go +++ b/api/applications/applications_controller.go @@ -178,7 +178,7 @@ func (ac *applicationController) ShowApplications(accounts models.Accounts, w ht matcher = applicationModels.MatchBySSHRepoFunc(sshRepo) } - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrations, err := handler.GetApplications(r.Context(), matcher, ac.hasAccessToRR, GetApplicationsOptions{}) if err != nil { @@ -243,7 +243,7 @@ func (ac *applicationController) SearchApplications(accounts models.Accounts, w return } - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) matcher := applicationModels.MatchByNamesFunc(appNamesRequest.Names) appRegistrations, err := handler.GetApplications( @@ -304,7 +304,7 @@ func (ac *applicationController) GetApplication(accounts models.Accounts, w http appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) application, err := handler.GetApplication(r.Context(), appName) @@ -404,7 +404,7 @@ func (ac *applicationController) RegenerateMachineUserTokenHandler(accounts mode // description: "Internal server error" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) machineUser, err := handler.RegenerateMachineUserToken(r.Context(), appName) if err != nil { @@ -453,7 +453,7 @@ func (ac *applicationController) RegenerateDeployKeyHandler(accounts models.Acco // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) var sharedSecretAndPrivateKey applicationModels.RegenerateDeployKeyAndSecretData if err := json.NewDecoder(r.Body).Decode(&sharedSecretAndPrivateKey); err != nil { radixhttp.ErrorResponse(w, r, err) @@ -501,7 +501,7 @@ func (ac *applicationController) GetDeployKeyAndSecret(accounts models.Accounts, // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) deployKeyAndSecret, err := handler.GetDeployKeyAndSecret(r.Context(), appName) if err != nil { @@ -555,7 +555,7 @@ func (ac *applicationController) RegisterApplication(accounts models.Accounts, w } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.RegisterApplication(r.Context(), applicationRegistrationRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -616,7 +616,7 @@ func (ac *applicationController) ChangeRegistrationDetails(accounts models.Accou } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.ChangeRegistrationDetails(r.Context(), appName, applicationRegistrationRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -677,7 +677,7 @@ func (ac *applicationController) ModifyRegistrationDetails(accounts models.Accou } // Need in cluster Radix client in order to validate registration using sufficient privileges - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) appRegistrationUpsertResponse, err := handler.ModifyRegistrationDetails(r.Context(), appName, applicationRegistrationPatchRequest) if err != nil { radixhttp.ErrorResponse(w, r, err) @@ -721,7 +721,7 @@ func (ac *applicationController) DeleteApplication(accounts models.Accounts, w h // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) err := handler.DeleteApplication(r.Context(), appName) if err != nil { @@ -752,7 +752,7 @@ func (ac *applicationController) ListPipelines(accounts models.Accounts, w http. // type: string // It was suggested to keep this under /applications/{appName} endpoint, but for now this will be the same for all applications - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) supportedPipelines := handler.GetSupportedPipelines() radixhttp.JSONResponse(w, r, supportedPipelines) } @@ -796,7 +796,7 @@ func (ac *applicationController) TriggerPipelineBuild(accounts models.Accounts, // "404": // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineBuild(r.Context(), appName, r) if err != nil { @@ -847,7 +847,7 @@ func (ac *applicationController) TriggerPipelineBuildDeploy(accounts models.Acco // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineBuildDeploy(r.Context(), appName, r) if err != nil { @@ -898,7 +898,7 @@ func (ac *applicationController) TriggerPipelineDeploy(accounts models.Accounts, // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelineDeploy(r.Context(), appName, r) if err != nil { @@ -947,7 +947,7 @@ func (ac *applicationController) TriggerPipelinePromote(accounts models.Accounts // description: "Not found" appName := mux.Vars(r)["appName"] - handler := ac.applicationHandlerFactory(accounts) + handler := ac.applicationHandlerFactory.Create(accounts) jobSummary, err := handler.TriggerPipelinePromote(r.Context(), appName, r) if err != nil { diff --git a/api/applications/applications_controller_test.go b/api/applications/applications_controller_test.go index 1f3bcb1b..a532b716 100644 --- a/api/applications/applications_controller_test.go +++ b/api/applications/applications_controller_test.go @@ -5,6 +5,8 @@ import ( "context" "encoding/json" "fmt" + "github.com/equinor/radix-api/models" + "github.com/equinor/radix-common/utils/pointers" "net/http" "net/url" "os" @@ -46,6 +48,15 @@ const ( ) func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontest.Utils, *controllertest.Utils, *kubefake.Clientset, *fake.Clientset, prometheusclient.Interface, secretsstorevclient.Interface) { + return setupTestWithFactory(newTestApplicationHandlerFactory( + ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }, + )) +} + +func setupTestWithFactory(handlerFactory ApplicationHandlerFactory) (*commontest.Utils, *controllertest.Utils, *kubefake.Clientset, *fake.Clientset, prometheusclient.Interface, secretsstorevclient.Interface) { // Setup kubeclient := kubefake.NewSimpleClientset() radixclient := fake.NewSimpleClientset() @@ -66,9 +77,7 @@ func setupTest(requireAppConfigurationItem, requireAppADGroups bool) (*commontes func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil }, - NewApplicationHandlerFactory( - ApplicationHandlerConfig{RequireAppConfigurationItem: requireAppConfigurationItem, RequireAppADGroups: requireAppADGroups}, - ), + handlerFactory, ), ) @@ -91,7 +100,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -104,7 +116,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, rr v1.RadixRegistration) (bool, error) { return rr.GetName() == "my-second-app", nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -117,7 +132,10 @@ func TestGetApplications_HasAccessToSomeRR(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) responseChannel := controllerTestUtils.ExecuteRequest("GET", "/api/v1/applications") response := <-responseChannel @@ -185,7 +203,10 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return true, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) // Tests t.Run("search for "+appNames[0], func(t *testing.T) { @@ -259,7 +280,10 @@ func TestSearchApplications(t *testing.T) { controllerTestUtils := controllertest.NewTestUtils(kubeclient, radixclient, secretproviderclient, NewApplicationController( func(_ context.Context, _ kubernetes.Interface, _ v1.RadixRegistration) (bool, error) { return false, nil - }, NewApplicationHandlerFactory(ApplicationHandlerConfig{true, true}))) + }, newTestApplicationHandlerFactory(ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: true}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return true, nil + }))) searchParam := applicationModels.ApplicationsSearchRequest{Names: []string{appNames[0]}} responseChannel := controllerTestUtils.ExecuteRequestWithParameters("POST", "/api/v1/applications/_search", &searchParam) response := <-responseChannel @@ -1139,6 +1163,104 @@ func TestModifyApplication_IgnoreRequireADGroupValidationWhenRequiredButCurrentI assert.Equal(t, http.StatusOK, response.Code) } +func TestModifyApplication_UpdateADGroupValidation(t *testing.T) { + type scenario struct { + requireAppADGroups bool + name string + hasAccessToAdGroups bool + expectedResponseCode int + adminAdGroups []string + } + scenarios := []scenario{ + { + name: "Require ADGroups, has groups and has access to them", + requireAppADGroups: true, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Require ADGroups, has no groups and has access to them", + requireAppADGroups: true, + adminAdGroups: []string{}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Require ADGroups, has groups and has no access to them", + requireAppADGroups: true, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Require ADGroups, has no groups and has no access to them", + requireAppADGroups: true, + adminAdGroups: []string{}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Not require ADGroups, has groups and has access to them", + requireAppADGroups: false, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Not require ADGroups, has no groups and has access to them", + requireAppADGroups: false, + adminAdGroups: []string{}, + hasAccessToAdGroups: true, + expectedResponseCode: http.StatusOK, + }, + { + name: "Not require ADGroups, has groups and has no access to them", + requireAppADGroups: false, + adminAdGroups: []string{"e654757d-c789-11e8-bbad-045000000001"}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusBadRequest, + }, + { + name: "Not require ADGroups, has no groups and has no access to them", + requireAppADGroups: false, + adminAdGroups: []string{}, + hasAccessToAdGroups: false, + expectedResponseCode: http.StatusOK, + }, + } + + for _, ts := range scenarios { + t.Run(ts.name, func(t *testing.T) { + _, controllerTestUtils, _, radixClient, _, _ := setupTestWithFactory(newTestApplicationHandlerFactory( + ApplicationHandlerConfig{RequireAppConfigurationItem: true, RequireAppADGroups: ts.requireAppADGroups}, + func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) { + return ts.hasAccessToAdGroups, nil + }, + )) + + rr, err := anApplicationRegistration(). + WithName("any-name"). + WithAdGroups([]string{"e654757d-c789-11e8-bbad-045007777777"}). + BuildRR() + require.NoError(t, err) + _, err = radixClient.RadixV1().RadixRegistrations().Create(context.Background(), rr, metav1.CreateOptions{}) + require.NoError(t, err) + + // Test + patchRequest := applicationModels.ApplicationRegistrationPatchRequest{ + ApplicationRegistrationPatch: &applicationModels.ApplicationRegistrationPatch{ + AdGroups: pointers.Ptr(ts.adminAdGroups), + }, + } + + responseChannel := controllerTestUtils.ExecuteRequestWithParameters("PATCH", fmt.Sprintf("/api/v1/applications/%s", "any-name"), patchRequest) + response := <-responseChannel + assert.Equal(t, ts.expectedResponseCode, response.Code, fmt.Sprintf("Expected response code %d but got %d", ts.expectedResponseCode, response.Code)) + }) + } +} + func TestHandleTriggerPipeline_ForNonMappedAndMappedAndMagicBranchEnvironment_JobIsNotCreatedForUnmapped(t *testing.T) { // Setup commonTestUtils, controllerTestUtils, _, _, _, _ := setupTest(true, true) @@ -1606,3 +1728,20 @@ func buildApplicationRegistrationRequest(applicationRegistration applicationMode AcknowledgeWarnings: acknowledgeWarnings, } } + +type testApplicationHandlerFactory struct { + config ApplicationHandlerConfig + hasAccessToGetConfigMap hasAccessToGetConfigMapFunc +} + +func newTestApplicationHandlerFactory(config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) *testApplicationHandlerFactory { + return &testApplicationHandlerFactory{ + config: config, + hasAccessToGetConfigMap: hasAccessToGetConfigMap, + } +} + +// Create creates a new ApplicationHandler +func (f *testApplicationHandlerFactory) Create(accounts models.Accounts) ApplicationHandler { + return NewApplicationHandler(accounts, f.config, f.hasAccessToGetConfigMap) +} diff --git a/api/applications/applications_handler.go b/api/applications/applications_handler.go index 960e7f86..e1d3b756 100644 --- a/api/applications/applications_handler.go +++ b/api/applications/applications_handler.go @@ -4,7 +4,6 @@ import ( "context" "encoding/json" "fmt" - "k8s.io/client-go/kubernetes/fake" "net/http" "strings" "time" @@ -23,16 +22,20 @@ import ( "github.com/equinor/radix-common/utils/slice" "github.com/equinor/radix-operator/pkg/apis/applicationconfig" "github.com/equinor/radix-operator/pkg/apis/defaults" + "github.com/equinor/radix-operator/pkg/apis/defaults/k8s" jobPipeline "github.com/equinor/radix-operator/pkg/apis/pipeline" v1 "github.com/equinor/radix-operator/pkg/apis/radix/v1" "github.com/equinor/radix-operator/pkg/apis/radixvalidators" crdUtils "github.com/equinor/radix-operator/pkg/apis/utils" log "github.com/sirupsen/logrus" - corev1auth "k8s.io/api/authorization/v1" + authorizationapi "k8s.io/api/authorization/v1" corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/kubernetes/fake" ) type patch struct { @@ -41,22 +44,35 @@ type patch struct { Value interface{} `json:"value"` } +type hasAccessToGetConfigMapFunc func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) + // ApplicationHandler Instance variables type ApplicationHandler struct { - jobHandler job.JobHandler - environmentHandler environments.EnvironmentHandler - accounts models.Accounts - config ApplicationHandlerConfig + jobHandler job.JobHandler + environmentHandler environments.EnvironmentHandler + accounts models.Accounts + config ApplicationHandlerConfig + namespace string + hasAccessToGetConfigMap func(ctx context.Context, kubeClient kubernetes.Interface, namespace string, configMapName string) (bool, error) } // NewApplicationHandler Constructor -func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerConfig) ApplicationHandler { +func NewApplicationHandler(accounts models.Accounts, config ApplicationHandlerConfig, hasAccessToGetConfigMap hasAccessToGetConfigMapFunc) ApplicationHandler { return ApplicationHandler{ - accounts: accounts, - jobHandler: job.Init(accounts, deployments.Init(accounts)), - environmentHandler: environments.Init(environments.WithAccounts(accounts)), - config: config, + accounts: accounts, + jobHandler: job.Init(accounts, deployments.Init(accounts)), + environmentHandler: environments.Init(environments.WithAccounts(accounts)), + config: config, + namespace: getApiNamespace(config), + hasAccessToGetConfigMap: hasAccessToGetConfigMap, + } +} + +func getApiNamespace(config ApplicationHandlerConfig) string { + if namespace := crdUtils.GetEnvironmentNamespace(config.AppName, config.EnvironmentName); len(namespace) > 0 { + return namespace } + panic("missing RADIX_APP or RADIX_ENVIRONMENT environment variables") } func (ah *ApplicationHandler) getUserAccount() models.Account { @@ -179,6 +195,10 @@ func (ah *ApplicationHandler) RegisterApplication(ctx context.Context, applicati return upsertResponse, err } } + err = ah.validateUserIsMemberOfAdGroups(ctx, applicationRegistrationRequest.ApplicationRegistration.Name, applicationRegistrationRequest.ApplicationRegistration.AdGroups) + if err != nil { + return nil, err + } radixRegistration, err = ah.getUserAccount().RadixClient.RadixV1().RadixRegistrations().Create(ctx, radixRegistration, metav1.CreateOptions{}) if err != nil { @@ -277,13 +297,17 @@ func (ah *ApplicationHandler) ModifyRegistrationDetails(ctx context.Context, app return nil, err } - payload := []patch{} + payload := make([]patch, 0) runUpdate := false updatedRegistration := currentRegistration.DeepCopy() // Only these fields can change over time patchRequest := applicationRegistrationPatchRequest.ApplicationRegistrationPatch if patchRequest.AdGroups != nil && !radixutils.ArrayEqualElements(currentRegistration.Spec.AdGroups, *patchRequest.AdGroups) { + err := ah.validateUserIsMemberOfAdGroups(ctx, appName, *patchRequest.AdGroups) + if err != nil { + return nil, err + } updatedRegistration.Spec.AdGroups = *patchRequest.AdGroups payload = append(payload, patch{Op: "replace", Path: "/spec/adGroups", Value: *patchRequest.AdGroups}) runUpdate = true @@ -554,7 +578,7 @@ func (ah *ApplicationHandler) getRegistrationUpdateWarnings(radixRegistration *v } func (ah *ApplicationHandler) isValidRegistrationInsert(radixRegistration *v1.RadixRegistration) error { - // Need to use in-cluster client of the API server, because the user might not have enough priviledges + // Need to use in-cluster client of the API server, because the user might not have enough privileges // to run a full validation return radixvalidators.CanRadixRegistrationBeInserted(ah.getServiceAccount().RadixClient, radixRegistration, ah.getAdditionalRadixRegistrationInsertValidators()...) } @@ -713,9 +737,9 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string case *fake.Clientset: return true, nil default: - review, err := ah.accounts.UserAccount.Client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &corev1auth.SelfSubjectAccessReview{ - Spec: corev1auth.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &corev1auth.ResourceAttributes{ + review, err := ah.accounts.UserAccount.Client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &authorizationapi.SelfSubjectAccessReview{ + Spec: authorizationapi.SelfSubjectAccessReviewSpec{ + ResourceAttributes: &authorizationapi.ResourceAttributes{ Verb: "patch", Group: "radix.equinor.com", Resource: "radixregistrations", @@ -727,6 +751,47 @@ func (ah *ApplicationHandler) userIsAppAdmin(ctx context.Context, appName string } } +func (ah *ApplicationHandler) validateUserIsMemberOfAdGroups(ctx context.Context, appName string, adGroups []string) error { + if len(adGroups) == 0 { + if ah.config.RequireAppADGroups { + return userShouldBeMemberOfAdminAdGroupError() + } + return nil + } + name := fmt.Sprintf("access-validation-%s", appName) + labels := map[string]string{"radix-access-validation": "true"} + configMapName := fmt.Sprintf("%s-%s", name, strings.ToLower(crdUtils.RandString(6))) + role, err := createRoleToGetConfigMap(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, name, labels, configMapName) + if err != nil { + return err + } + defer func() { + err = deleteRole(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, role.GetName()) + if err != nil { + log.Warnf("Failed to delete role %s: %v", role.GetName(), err) + } + }() + roleBinding, err := createRoleBindingForRole(ctx, ah.accounts.ServiceAccount.Client, ah.namespace, role, name, adGroups, labels) + if err != nil { + return err + } + defer func() { + err = deleteRoleBinding(context.Background(), ah.accounts.ServiceAccount.Client, ah.namespace, roleBinding.GetName()) + if err != nil { + log.Warnf("Failed to delete role binding %s: %v", roleBinding.GetName(), err) + } + }() + + valid, err := ah.hasAccessToGetConfigMap(ctx, ah.accounts.UserAccount.Client, ah.namespace, configMapName) + if err != nil { + return err + } + if !valid { + return userShouldBeMemberOfAdminAdGroupError() + } + return nil +} + func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistration) bool { // HACK ConfigBranch is required, so we set it to "master" if empty to support existing apps registered before ConfigBranch was introduced if len(strings.TrimSpace(existingRegistration.Spec.ConfigBranch)) > 0 { @@ -735,3 +800,57 @@ func setConfigBranchToFallbackWhenEmpty(existingRegistration *v1.RadixRegistrati existingRegistration.Spec.ConfigBranch = applicationconfig.ConfigBranchFallback return true } + +func createRoleToGetConfigMap(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string, labels map[string]string, configMapName string) (*rbacv1.Role, error) { + return kubeClient.RbacV1().Roles(namespace).Create(ctx, &rbacv1.Role{ + ObjectMeta: metav1.ObjectMeta{GenerateName: roleName, Labels: labels}, + Rules: []rbacv1.PolicyRule{{ + Verbs: []string{"get"}, + APIGroups: []string{""}, + Resources: []string{"configmaps"}, + ResourceNames: []string{configMapName}, + }}, + }, metav1.CreateOptions{}) +} + +func deleteRole(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleName string) error { + deletionPropagation := metav1.DeletePropagationBackground + return kubeClient.RbacV1().Roles(namespace).Delete(ctx, roleName, metav1.DeleteOptions{ + PropagationPolicy: &deletionPropagation, + }) +} + +func createRoleBindingForRole(ctx context.Context, kubeClient kubernetes.Interface, namespace string, role *rbacv1.Role, roleBindingName string, adGroups []string, labels map[string]string) (*rbacv1.RoleBinding, error) { + var subjects []rbacv1.Subject + for _, adGroup := range adGroups { + subjects = append(subjects, rbacv1.Subject{ + Kind: k8s.KindGroup, + Name: adGroup, + APIGroup: k8s.RbacApiGroup, + }) + } + newRoleBinding := &rbacv1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + GenerateName: roleBindingName, + Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: k8s.RbacApiVersion, + Kind: k8s.KindRole, + Name: role.GetName(), + UID: role.GetUID(), + }, + }, + }, + RoleRef: rbacv1.RoleRef{ + APIGroup: k8s.RbacApiGroup, + Kind: k8s.KindRole, + Name: role.GetName(), + }, Subjects: subjects, + } + return kubeClient.RbacV1().RoleBindings(namespace).Create(ctx, newRoleBinding, metav1.CreateOptions{}) +} + +func deleteRoleBinding(ctx context.Context, kubeClient kubernetes.Interface, namespace, roleBindingName string) error { + return kubeClient.RbacV1().RoleBindings(namespace).Delete(ctx, roleBindingName, metav1.DeleteOptions{}) +} diff --git a/api/applications/applications_handler_config.go b/api/applications/applications_handler_config.go index f8abffce..cbb74b58 100644 --- a/api/applications/applications_handler_config.go +++ b/api/applications/applications_handler_config.go @@ -27,8 +27,10 @@ func LoadApplicationHandlerConfig(args []string) (ApplicationHandlerConfig, erro } type ApplicationHandlerConfig struct { - RequireAppConfigurationItem bool `cfg:"require_app_configuration_item" flag:"require-app-configuration-item"` - RequireAppADGroups bool `cfg:"require_app_ad_groups" flag:"require-app-ad-groups"` + RequireAppConfigurationItem bool `cfg:"require_app_configuration_item" flag:"require-app-configuration-item"` + RequireAppADGroups bool `cfg:"require_app_ad_groups" flag:"require-app-ad-groups"` + AppName string `cfg:"radix_app" flag:"radix-app"` + EnvironmentName string `cfg:"radix_environment" flag:"radix-environment"` } func ApplicationHandlerConfigFlagSet() *pflag.FlagSet { @@ -36,5 +38,7 @@ func ApplicationHandlerConfigFlagSet() *pflag.FlagSet { flagset.ParseErrorsWhitelist = pflag.ParseErrorsWhitelist{UnknownFlags: true} flagset.Bool("require-app-configuration-item", true, "Require configuration item for application") flagset.Bool("require-app-ad-groups", true, "Require AD groups for application") + flagset.String("radix-app", "", "Application name") + flagset.String("radix-environment", "", "Environment name") return flagset } diff --git a/api/applications/applications_handler_factory.go b/api/applications/applications_handler_factory.go index ab0fddc9..d54a24a9 100644 --- a/api/applications/applications_handler_factory.go +++ b/api/applications/applications_handler_factory.go @@ -1,15 +1,41 @@ package applications import ( + "context" + "github.com/equinor/radix-api/api/utils/access" "github.com/equinor/radix-api/models" + authorizationapi "k8s.io/api/authorization/v1" + "k8s.io/client-go/kubernetes" ) // ApplicationHandlerFactory defines a factory function for creating an ApplicationHandler -type ApplicationHandlerFactory func(accounts models.Accounts) ApplicationHandler +type ApplicationHandlerFactory interface { + Create(accounts models.Accounts) ApplicationHandler +} + +type applicationHandlerFactory struct { + config ApplicationHandlerConfig +} // NewApplicationHandlerFactory creates a new ApplicationHandlerFactory func NewApplicationHandlerFactory(config ApplicationHandlerConfig) ApplicationHandlerFactory { - return func(accounts models.Accounts) ApplicationHandler { - return NewApplicationHandler(accounts, config) + return &applicationHandlerFactory{ + config: config, } } + +// Create creates a new ApplicationHandler +func (f *applicationHandlerFactory) Create(accounts models.Accounts) ApplicationHandler { + return NewApplicationHandler(accounts, f.config, hasAccessToGetConfigMap) +} + +func hasAccessToGetConfigMap(ctx context.Context, kubeClient kubernetes.Interface, namespace, configMapName string) (bool, error) { + return access.HasAccess(ctx, kubeClient, &authorizationapi.ResourceAttributes{ + Verb: "get", + Group: "", + Resource: "configmaps", + Version: "*", + Namespace: namespace, + Name: configMapName, + }) +} diff --git a/api/applications/errors.go b/api/applications/errors.go new file mode 100644 index 00000000..ace8f507 --- /dev/null +++ b/api/applications/errors.go @@ -0,0 +1,7 @@ +package applications + +import radixhttp "github.com/equinor/radix-common/net/http" + +func userShouldBeMemberOfAdminAdGroupError() error { + return radixhttp.ValidationError("Radix Registration", "User should be a member of at least one admin AD group or their sub-members") +} diff --git a/api/applications/get_applications_handler.go b/api/applications/get_applications_handler.go index ead6ab67..2b5eb56e 100644 --- a/api/applications/get_applications_handler.go +++ b/api/applications/get_applications_handler.go @@ -10,6 +10,7 @@ import ( deploymentModels "github.com/equinor/radix-api/api/deployments/models" environmentModels "github.com/equinor/radix-api/api/environments/models" jobModels "github.com/equinor/radix-api/api/jobs/models" + "github.com/equinor/radix-api/api/utils/access" "golang.org/x/sync/errgroup" authorizationapi "k8s.io/api/authorization/v1" @@ -212,25 +213,11 @@ func (ah *ApplicationHandler) filterRadixRegByAccess(ctx context.Context, radixr // cannot run as test - does not return correct values func hasAccess(ctx context.Context, client kubernetes.Interface, rr v1.RadixRegistration) (bool, error) { - sar := authorizationapi.SelfSubjectAccessReview{ - Spec: authorizationapi.SelfSubjectAccessReviewSpec{ - ResourceAttributes: &authorizationapi.ResourceAttributes{ - Verb: "get", - Group: "radix.equinor.com", - Resource: "radixregistrations", - Version: "*", - Name: rr.GetName(), - }, - }, - } - - r, err := postSelfSubjectAccessReviews(ctx, client, sar) - if err != nil { - return false, err - } - return r.Status.Allowed, nil -} - -func postSelfSubjectAccessReviews(ctx context.Context, client kubernetes.Interface, sar authorizationapi.SelfSubjectAccessReview) (*authorizationapi.SelfSubjectAccessReview, error) { - return client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) + return access.HasAccess(ctx, client, &authorizationapi.ResourceAttributes{ + Verb: "get", + Group: "radix.equinor.com", + Resource: "radixregistrations", + Version: "*", + Name: rr.GetName(), + }) } diff --git a/api/applications/models/application_registration_patch.go b/api/applications/models/application_registration_patch.go index 6003f0a6..cd0d3acc 100644 --- a/api/applications/models/application_registration_patch.go +++ b/api/applications/models/application_registration_patch.go @@ -11,7 +11,7 @@ type ApplicationRegistrationPatch struct { // ReaderAdGroups the groups that should be able to read the application // // required: false - ReaderAdGroups *[]string `json:"ReaderAdGroups,omitempty"` + ReaderAdGroups *[]string `json:"readerAdGroups,omitempty"` // Owner of the application - should be an email // diff --git a/api/utils/access/access.go b/api/utils/access/access.go new file mode 100644 index 00000000..e642db00 --- /dev/null +++ b/api/utils/access/access.go @@ -0,0 +1,29 @@ +package access + +import ( + "context" + + authorizationapi "k8s.io/api/authorization/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// HasAccess checks if user has access to a resource +// cannot run as test - does not return correct values +func HasAccess(ctx context.Context, client kubernetes.Interface, resourceAttributes *authorizationapi.ResourceAttributes) (bool, error) { + sar := authorizationapi.SelfSubjectAccessReview{ + Spec: authorizationapi.SelfSubjectAccessReviewSpec{ + ResourceAttributes: resourceAttributes, + }, + } + + r, err := postSelfSubjectAccessReviews(ctx, client, sar) + if err != nil { + return false, err + } + return r.Status.Allowed, nil +} + +func postSelfSubjectAccessReviews(ctx context.Context, client kubernetes.Interface, sar authorizationapi.SelfSubjectAccessReview) (*authorizationapi.SelfSubjectAccessReview, error) { + return client.AuthorizationV1().SelfSubjectAccessReviews().Create(ctx, &sar, metav1.CreateOptions{}) +}