Skip to content

Commit

Permalink
fix logic
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru committed Jun 13, 2023
1 parent 906dbb2 commit 8321d01
Show file tree
Hide file tree
Showing 5 changed files with 157 additions and 153 deletions.
32 changes: 15 additions & 17 deletions pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ const (
// ValidationPath is the webhook service path which admission requests are routed to for validating custom resource definition resources.
ValidationPath = "/validate-v1-fleetresourcehandler"
groupMatch = `^[^.]*\.(.*)`
crdGVK = "apiextensions.k8s.io/v1, Kind=CustomResourceDefinition"
)

// Add registers the webhook for K8s bulit-in object types.
Expand All @@ -36,23 +35,22 @@ type fleetResourceValidator struct {
decoder *admission.Decoder
}

func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Request) admission.Response {
func (v *fleetResourceValidator) Handle(_ context.Context, req admission.Request) admission.Response {
var response admission.Response
klog.V(2).InfoS("GVKs", "request GVK", req.Kind.String(), "crd GVK", crdGVK)
if req.Operation == admissionv1.Create || req.Operation == admissionv1.Update || req.Operation == admissionv1.Delete {
switch req.Kind.String() {
case crdGVK:
klog.V(2).InfoS("handling CRD resource", "crdGVK", crdGVK)
response = v.handleCRD(ctx, req)
case retrieveCRDGVK():
klog.V(2).InfoS("handling CRD resource", "crdGVK", retrieveCRDGVK())
response = v.handleCRD(req)
default:
klog.V(2).InfoS("resource is not monitored by fleet resource validator webhook")
klog.V(2).InfoS(fmt.Sprintf("resource with GVK: %s is not monitored by fleet resource validator webhook", req.Kind.String()))
response = admission.Allowed("")
}
}
return response
}

func (v *fleetResourceValidator) handleCRD(ctx context.Context, req admission.Request) admission.Response {
func (v *fleetResourceValidator) handleCRD(req admission.Request) admission.Response {
var crd v1.CustomResourceDefinition
if req.Operation == admissionv1.Delete {
// req.Object is not populated for delete: https://github.com/kubernetes-sigs/controller-runtime/issues/1762.
Expand All @@ -67,20 +65,20 @@ func (v *fleetResourceValidator) handleCRD(ctx context.Context, req admission.Re
}
}

// Need to check to see if the user is authenticated to do the operation.
if err := validation.ValidateUser(ctx, v.Client, req.UserInfo); err != nil {
return admission.Denied(fmt.Sprintf("failed to validate user %s in groups: %v to modify CRD", req.UserInfo.Username, req.UserInfo.Groups))
}
klog.V(2).InfoS("successfully validated the user", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)

group := regexp.MustCompile(groupMatch).FindStringSubmatch(crd.Name)[1]
if validation.CheckCRDGroup(group) {
return admission.Denied(fmt.Sprintf("user: %s in groups: %v cannot modify fleet CRD %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
if validation.CheckCRDGroup(group) && !validation.ValidateUserForCRD(req.UserInfo) {
return admission.Denied(fmt.Sprintf("failed to validate user: %s in groups: %v to modify fleet CRD: %s", req.UserInfo.Username, req.UserInfo.Groups, crd.Name))
}
klog.V(2).InfoS("successfully validated the CRD group", "userName", req.UserInfo.Username, "groups", req.UserInfo.Groups)
return admission.Allowed("")
}

func retrieveCRDGVK() string {
var crd v1.CustomResourceDefinition
crd.APIVersion = v1.SchemeGroupVersion.Group + "/" + v1.SchemeGroupVersion.Version
crd.Kind = "CustomResourceDefinition"
return crd.GroupVersionKind().String()
}

func (v *fleetResourceValidator) InjectDecoder(d *admission.Decoder) error {
v.decoder = d
return nil
Expand Down
51 changes: 5 additions & 46 deletions pkg/webhook/validation/uservalidation.go
View file
Original file line number Diff line number Diff line change
@@ -1,58 +1,17 @@
package validation

import (
"context"
"fmt"
"regexp"

authenticationv1 "k8s.io/api/authentication/v1"
"k8s.io/klog/v2"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/client"

fleetv1alpha1 "go.goms.io/fleet/apis/v1alpha1"
)

const (
authenticatedGroup = "system:authenticated"
mastersGroup = "system:masters"
serviceAccountGroup = "system:serviceaccounts"
bootstrapGroup = "system:bootstrappers"

serviceAccountUser = "system:serviceaccount"
mastersGroup = "system:masters"
)

// TODO: Get valid user names as flag and check to validate those user names.
// TODO:(Arvindthiru) Get valid usernames as flag and allow those usernames.

// ValidateUser checks to see if user is authenticated to make a request to the hub cluster's api-server.
func ValidateUser(ctx context.Context, client client.Client, userInfo authenticationv1.UserInfo) error {
// special case where users belong to the masters group.
if slices.Contains(userInfo.Groups, mastersGroup) {
return nil
}
if slices.Contains(userInfo.Groups, bootstrapGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) {
return nil
}
// this ensures all internal service accounts are validated.
if slices.Contains(userInfo.Groups, serviceAccountGroup) && slices.Contains(userInfo.Groups, authenticatedGroup) {
match := regexp.MustCompile(serviceAccountUser).FindStringSubmatch(userInfo.Username)[1]
if match != "" {
return nil
}
}
// list all the member clusters
var memberClusterList fleetv1alpha1.MemberClusterList
if err := client.List(ctx, &memberClusterList); err != nil {
klog.V(2).ErrorS(err, "failed to list member clusters")
return err
}
identities := make([]string, len(memberClusterList.Items))
for i, memberCluster := range memberClusterList.Items {
identities[i] = memberCluster.Spec.Identity.Name
}
// this ensures will allow all member agents are validated.
if slices.Contains(identities, userInfo.Username) && slices.Contains(userInfo.Groups, authenticatedGroup) {
return nil
}
return fmt.Errorf("failed to validate user %s in groups %v", userInfo.Username, userInfo.Groups)
// ValidateUserForCRD checks to see if user is authenticated to make a request to modify fleet CRDs.
func ValidateUserForCRD(userInfo authenticationv1.UserInfo) bool {
return slices.Contains(userInfo.Groups, mastersGroup)
}
79 changes: 8 additions & 71 deletions pkg/webhook/validation/uservalidation_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,24 +2,21 @@ package validation

import (
"context"
"errors"
"testing"

"github.com/crossplane/crossplane-runtime/pkg/test"
"github.com/stretchr/testify/assert"
v1 "k8s.io/api/authentication/v1"
rbacv1 "k8s.io/api/rbac/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

"go.goms.io/fleet/apis/v1alpha1"
"go.goms.io/fleet/pkg/utils"
)

func TestValidateUser(t *testing.T) {
func TestValidateUserForCRD(t *testing.T) {
testCases := map[string]struct {
client client.Client
userInfo v1.UserInfo
wantErr error
client client.Client
userInfo v1.UserInfo
wantResult bool
}{
"allow user in system:masters group": {
client: &test.MockClient{
Expand All @@ -31,67 +28,7 @@ func TestValidateUser(t *testing.T) {
Username: "test-user",
Groups: []string{"system:masters"},
},
wantErr: nil,
},
"allow user in system:bootstrappers group": {
client: &test.MockClient{
MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
return nil
},
},
userInfo: v1.UserInfo{
Username: "test-user",
Groups: []string{"system:bootstrappers", "system:authenticated"},
},
wantErr: nil,
},
"allow user in system:serviceaccounts group, has service account prefix in name": {
client: &test.MockClient{
MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
return nil
},
},
userInfo: v1.UserInfo{
Username: "service:account:test-user",
Groups: []string{"system:masters", "system:serviceaccounts"},
},
wantErr: nil,
},
"allow member cluster identity": {
client: &test.MockClient{
MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
o := list.(*v1alpha1.MemberClusterList)
*o = v1alpha1.MemberClusterList{
Items: []v1alpha1.MemberCluster{
{
Spec: v1alpha1.MemberClusterSpec{
Identity: rbacv1.Subject{
Name: "member-cluster-identity",
},
},
},
},
}
return nil
},
},
userInfo: v1.UserInfo{
Username: "member-cluster-identity",
Groups: []string{"system:authenticated"},
},
wantErr: nil,
},
"fail to list member clusters": {
client: &test.MockClient{
MockList: func(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
return errors.New("failed to member clusters")
},
},
userInfo: v1.UserInfo{
Username: "test-user",
Groups: []string{"system:authenticated"},
},
wantErr: errors.New("failed to member clusters"),
wantResult: true,
},
"fail to validate user with invalid username, groups": {
client: &test.MockClient{
Expand All @@ -103,14 +40,14 @@ func TestValidateUser(t *testing.T) {
Username: "test-user",
Groups: []string{"test-group"},
},
wantErr: errors.New("failed to validate user test-user in groups [test-group]"),
wantResult: false,
},
}

for testName, testCase := range testCases {
t.Run(testName, func(t *testing.T) {
gotErr := ValidateUser(context.Background(), testCase.client, testCase.userInfo)
assert.Equal(t, testCase.wantErr, gotErr, utils.TestCaseMsg, testName)
gotResult := ValidateUserForCRD(testCase.userInfo)
assert.Equal(t, testCase.wantResult, gotResult, utils.TestCaseMsg, testName)
})
}
}
35 changes: 29 additions & 6 deletions test/e2e/framework/cluster.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/dynamic"
"k8s.io/client-go/tools/clientcmd"
"k8s.io/client-go/tools/clientcmd/api"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/client/apiutil"
)
Expand All @@ -22,12 +23,13 @@ var (

// Cluster object defines the required clients based on the kubeconfig of the test cluster.
type Cluster struct {
Scheme *runtime.Scheme
KubeClient client.Client
DynamicClient dynamic.Interface
ClusterName string
HubURL string
RestMapper meta.RESTMapper
Scheme *runtime.Scheme
KubeClient client.Client
ImpersonateKubeClient client.Client
DynamicClient dynamic.Interface
ClusterName string
HubURL string
RestMapper meta.RESTMapper
}

func NewCluster(name string, scheme *runtime.Scheme) *Cluster {
Expand All @@ -40,12 +42,18 @@ func NewCluster(name string, scheme *runtime.Scheme) *Cluster {
// GetClusterClient returns a Cluster client for the cluster.
func GetClusterClient(cluster *Cluster) {
clusterConfig := GetClientConfig(cluster)
impersonateClusterConfig := GetImpersonateClientConfig(cluster)

restConfig, err := clusterConfig.ClientConfig()
if err != nil {
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up rest config")
}

impersonateRestConfig, err := impersonateClusterConfig.ClientConfig()
if err != nil {
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up impersonate rest config")
}

cluster.KubeClient, err = client.New(restConfig, client.Options{Scheme: cluster.Scheme})
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client")

Expand All @@ -54,6 +62,9 @@ func GetClusterClient(cluster *Cluster) {

cluster.RestMapper, err = apiutil.NewDynamicRESTMapper(restConfig, apiutil.WithLazyDiscovery)
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Rest Mapper")

cluster.ImpersonateKubeClient, err = client.New(impersonateRestConfig, client.Options{Scheme: cluster.Scheme})
gomega.Expect(err).Should(gomega.Succeed(), "Failed to set up Kube Client")
}

func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig {
Expand All @@ -63,3 +74,15 @@ func GetClientConfig(cluster *Cluster) clientcmd.ClientConfig {
CurrentContext: cluster.ClusterName,
})
}

func GetImpersonateClientConfig(cluster *Cluster) clientcmd.ClientConfig {
return clientcmd.NewNonInteractiveDeferredLoadingClientConfig(
&clientcmd.ClientConfigLoadingRules{ExplicitPath: kubeconfigPath},
&clientcmd.ConfigOverrides{
CurrentContext: cluster.ClusterName,
AuthInfo: api.AuthInfo{
Impersonate: "test-user",
ImpersonateGroups: []string{"system:authenticated"},
},
})
}
Loading

0 comments on commit 8321d01

Please sign in to comment.