Skip to content

Commit

Permalink
fix: fleet guard rail allows member agent to CREATE/UPDATE/DELETE (#655)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Arvindthiru
Arvindthiru authored Jan 16, 2024
1 parent a3d45e8 commit 0a45072
Show file tree
Hide file tree
Showing 9 changed files with 284 additions and 236 deletions.
18 changes: 10 additions & 8 deletions pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,13 @@ func (v *fleetResourceValidator) Handle(ctx context.Context, req admission.Reque
klog.V(2).InfoS("handling namespace resource", "name", req.Name, "operation", req.Operation, "subResource", req.SubResource)
response = v.handleNamespace(req)
case req.Kind == validation.V1Alpha1IMCGVK || req.Kind == validation.V1Alpha1WorkGVK || req.Kind == validation.IMCGVK || req.Kind == validation.WorkGVK || req.Kind == validation.EndpointSliceExportGVK || req.Kind == validation.EndpointSliceImportGVK || req.Kind == validation.InternalServiceExportGVK || req.Kind == validation.InternalServiceImportGVK:
klog.V(2).InfoS("handling fleet owned namespaced resource in system namespace", "GVK", req.RequestKind, "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
response = v.handleFleetMemberNamespacedResource(ctx, req)
klog.V(2).InfoS("handling fleet owned namespaced resource in fleet reserved namespaces", "GVK", req.RequestKind, "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
response = v.handleFleetReservedNamespacedResource(ctx, req)
case req.Kind == validation.EventGVK:
klog.V(3).InfoS("handling event resource", "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
response = v.handleEvent(ctx, req)
case req.Namespace != "":
klog.V(2).InfoS("handling namespaced resource created in user namespaces", "GVK", req.RequestKind, "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
klog.V(2).InfoS("handling namespaced resource in fleet reserved namespaces", "GVK", req.RequestKind, "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
response = validation.ValidateUserForResource(req, v.whiteListedUsers)
default:
klog.V(3).InfoS("resource is not monitored by fleet resource validator webhook", "GVK", req.RequestKind, "namespacedName", namespacedName, "operation", req.Operation, "subResource", req.SubResource)
Expand Down Expand Up @@ -127,23 +127,25 @@ func (v *fleetResourceValidator) handleMemberCluster(req admission.Request) admi
return validation.ValidateUserForResource(req, v.whiteListedUsers)
}

// handleFleetMemberNamespacedResource allows/denies the request to modify object after validation.
func (v *fleetResourceValidator) handleFleetMemberNamespacedResource(ctx context.Context, req admission.Request) admission.Response {
// handleFleetReservedNamespacedResource allows/denies the request to modify object after validation.
func (v *fleetResourceValidator) handleFleetReservedNamespacedResource(ctx context.Context, req admission.Request) admission.Response {
var response admission.Response
if strings.HasPrefix(req.Namespace, fleetMemberNamespacePrefix) {
// check to see if valid users other than member agent is making the request.
response = validation.ValidateUserForResource(req, v.whiteListedUsers)
// check to see if member agent is making the request only on Update.
if !response.Allowed && req.Operation == admissionv1.Update {
if !response.Allowed {
// if namespace name is just "fleet-member", mcName variable becomes empty and the request is allowed since that namespaces is not watched by member agents.
mcName := parseMemberClusterNameFromNamespace(req.Namespace)
return validation.ValidateMCIdentity(ctx, v.client, req, mcName, v.isFleetV1Beta1API)
}
return response
} else if strings.HasPrefix(req.Namespace, fleetNamespacePrefix) || strings.HasPrefix(req.Namespace, kubeNamespacePrefix) {
return validation.ValidateUserForResource(req, v.whiteListedUsers)
}
klog.V(3).InfoS("namespace name doesn't begin with fleet-member prefix so we allow all operations on these namespaces",
klog.V(3).InfoS("namespace name doesn't begin with fleet/kube prefix so we allow all operations on these namespaces",
"user", req.UserInfo.Username, "groups", req.UserInfo.Groups, "operation", req.Operation, "kind", req.RequestKind.Kind, "subResource", req.SubResource, "namespacedName", types.NamespacedName{Name: req.Name, Namespace: req.Namespace})
return admission.Allowed("namespace name doesn't begin with fleet-member prefix so we allow all operations on these namespaces for the request object")
return admission.Allowed("namespace name doesn't begin with fleet/kube prefix so we allow all operations on these namespaces for the request object")
}

// handleEvent allows/denies request to modify event after validation.
Expand Down
133 changes: 85 additions & 48 deletions pkg/webhook/fleetresourcehandler/fleetresourcehandler_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,7 @@ import (
)

const (
mcName = "test-mc"
v1Beta1MCCRDName = "memberclusters.cluster.kubernetes-fleet.io"
mcName = "test-mc"
)

func TestHandleCRD(t *testing.T) {
Expand Down Expand Up @@ -680,7 +679,7 @@ func TestHandleMemberCluster(t *testing.T) {
}
}

func TestHandleFleetMemberNamespacedResource(t *testing.T) {
func TestHandleFleetReservedNamespacedResource(t *testing.T) {
v1Alpha1MockClient := &test.MockClient{
MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
if key.Name == mcName {
Expand Down Expand Up @@ -737,29 +736,47 @@ func TestHandleFleetMemberNamespacedResource(t *testing.T) {
Operation: admissionv1.Create,
},
},
wantResponse: admission.Allowed("namespace name doesn't begin with fleet-member prefix so we allow all operations on these namespaces for the request object"),
wantResponse: admission.Allowed("namespace name doesn't begin with fleet/kube prefix so we allow all operations on these namespaces for the request object"),
},
"allow user in system:masters group with update in fleet member cluster namespace with v1alpha1 Work": {
"allow hub-agent-sa in MC identity with create with v1alpha1 IMC": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-work",
Name: "test-mc",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1WorkGVK,
RequestKind: &validation.V1Alpha1IMCGVK,
UserInfo: authenticationv1.UserInfo{
Username: "testUser",
Groups: []string{"system:masters"},
Username: "system:serviceaccount:fleet-system:hub-agent-sa",
Groups: []string{"system:serviceaccounts"},
},
Operation: admissionv1.Update,
Operation: admissionv1.Create,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "testUser", utils.GenerateGroupString([]string{"system:masters"}), admissionv1.Update, &validation.V1Alpha1WorkGVK, "", types.NamespacedName{Name: "test-work", Namespace: "fleet-member-test-mc"})),
resourceValidator: fleetResourceValidator{
client: &test.MockClient{
MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
o := obj.(*fleetv1alpha1.MemberCluster)
*o = fleetv1alpha1.MemberCluster{
ObjectMeta: metav1.ObjectMeta{
Name: mcName,
},
Spec: fleetv1alpha1.MemberClusterSpec{
Identity: rbacv1.Subject{
Name: "hub-agent-sa",
},
},
}
return nil
},
},
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "system:serviceaccount:fleet-system:hub-agent-sa", utils.GenerateGroupString([]string{"system:serviceaccounts"}), admissionv1.Create, &validation.V1Alpha1IMCGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
},
"deny user in MC identity with create in fleet member cluster namespace with v1alpha1 IMC": {
"allow user in MC identity with create in fleet member cluster namespace with internalServiceExport with v1alpha1 client": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-mc",
Name: "test-ise",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1IMCGVK,
RequestKind: &validation.InternalServiceExportGVK,
UserInfo: authenticationv1.UserInfo{
Username: "test-identity",
Groups: []string{"system:authenticated"},
Expand All @@ -770,32 +787,48 @@ func TestHandleFleetMemberNamespacedResource(t *testing.T) {
resourceValidator: fleetResourceValidator{
client: v1Alpha1MockClient,
},
wantResponse: admission.Denied(fmt.Sprintf(validation.ResourceDeniedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Create, &validation.V1Alpha1IMCGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Create, &validation.InternalServiceExportGVK, "", types.NamespacedName{Name: "test-ise", Namespace: "fleet-member-test-mc"})),
},
"allow user in MC identity with update in fleet member cluster namespace with v1alpha1 IMC": {
"allow user in MC identity with create in fleet member cluster namespace with internalServiceExport with v1beta1 client": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-mc",
Name: "test-ise",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1IMCGVK,
RequestKind: &validation.InternalServiceExportGVK,
UserInfo: authenticationv1.UserInfo{
Username: "test-identity",
Groups: []string{"system:authenticated"},
},
Operation: admissionv1.Update,
Operation: admissionv1.Create,
},
},
resourceValidator: fleetResourceValidator{
client: v1Alpha1MockClient,
client: mockClient,
isFleetV1Beta1API: true,
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &validation.V1Alpha1IMCGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Create, &validation.InternalServiceExportGVK, "", types.NamespacedName{Name: "test-ise", Namespace: "fleet-member-test-mc"})),
},
"allow user in MC identity with update in fleet member cluster namespace with v1alpha1 Work": {
"allow user in system:masters group with update in fleet member cluster namespace with v1alpha1 Work": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-mc",
Name: "test-work",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1WorkGVK,
UserInfo: authenticationv1.UserInfo{
Username: "testUser",
Groups: []string{"system:masters"},
},
Operation: admissionv1.Update,
},
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "testUser", utils.GenerateGroupString([]string{"system:masters"}), admissionv1.Update, &validation.V1Alpha1WorkGVK, "", types.NamespacedName{Name: "test-work", Namespace: "fleet-member-test-mc"})),
},
"allow user in MC identity with update in fleet member cluster namespace with v1alpha1 IMC": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-mc",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1IMCGVK,
UserInfo: authenticationv1.UserInfo{
Username: "test-identity",
Groups: []string{"system:authenticated"},
Expand All @@ -806,40 +839,25 @@ func TestHandleFleetMemberNamespacedResource(t *testing.T) {
resourceValidator: fleetResourceValidator{
client: v1Alpha1MockClient,
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &validation.V1Alpha1WorkGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &validation.V1Alpha1IMCGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
},
"allow hub-agent-sa in MC identity with create with v1alpha1 IMC": {
"allow user in MC identity with update in fleet member cluster namespace with v1alpha1 Work": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-mc",
Namespace: "fleet-member-test-mc",
RequestKind: &validation.V1Alpha1IMCGVK,
RequestKind: &validation.V1Alpha1WorkGVK,
UserInfo: authenticationv1.UserInfo{
Username: "system:serviceaccount:fleet-system:hub-agent-sa",
Groups: []string{"system:serviceaccounts"},
Username: "test-identity",
Groups: []string{"system:authenticated"},
},
Operation: admissionv1.Create,
Operation: admissionv1.Update,
},
},
resourceValidator: fleetResourceValidator{
client: &test.MockClient{
MockGet: func(ctx context.Context, key client.ObjectKey, obj client.Object) error {
o := obj.(*fleetv1alpha1.MemberCluster)
*o = fleetv1alpha1.MemberCluster{
ObjectMeta: metav1.ObjectMeta{
Name: mcName,
},
Spec: fleetv1alpha1.MemberClusterSpec{
Identity: rbacv1.Subject{
Name: "hub-agent-sa",
},
},
}
return nil
},
},
client: v1Alpha1MockClient,
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "system:serviceaccount:fleet-system:hub-agent-sa", utils.GenerateGroupString([]string{"system:serviceaccounts"}), admissionv1.Create, &validation.V1Alpha1IMCGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedFormat, "test-identity", utils.GenerateGroupString([]string{"system:authenticated"}), admissionv1.Update, &validation.V1Alpha1WorkGVK, "", types.NamespacedName{Name: "test-mc", Namespace: "fleet-member-test-mc"})),
},
"allow request if get MC failed with internal server error with v1alpha1 Work": {
req: admission.Request{
Expand Down Expand Up @@ -930,15 +948,34 @@ func TestHandleFleetMemberNamespacedResource(t *testing.T) {
},
},
resourceValidator: fleetResourceValidator{
client: v1Alpha1MockClient,
client: mockClient,
isFleetV1Beta1API: true,
},
wantResponse: admission.Allowed(fmt.Sprintf(validation.ResourceAllowedGetMCFailed, "testUser", utils.GenerateGroupString([]string{"testGroup"}), admissionv1.Update, &validation.V1Alpha1WorkGVK, "", types.NamespacedName{Name: "test-work", Namespace: "fleet-member-test-mc1"})),
},
"deny request to create in fleet-system if user is not validated user with endPointSliceExport": {
req: admission.Request{
AdmissionRequest: admissionv1.AdmissionRequest{
Name: "test-net-eps",
Namespace: "fleet-system",
RequestKind: &validation.EndpointSliceExportGVK,
UserInfo: authenticationv1.UserInfo{
Username: "testUser",
Groups: []string{"testGroup"},
},
Operation: admissionv1.Create,
},
},
resourceValidator: fleetResourceValidator{
client: mockClient,
isFleetV1Beta1API: true,
},
wantResponse: admission.Denied(fmt.Sprintf(validation.ResourceDeniedFormat, "testUser", utils.GenerateGroupString([]string{"testGroup"}), admissionv1.Create, &validation.EndpointSliceExportGVK, "", types.NamespacedName{Name: "test-net-eps", Namespace: "fleet-system"})),
},
}
for testName, testCase := range testCases {
t.Run(testName, func(t *testing.T) {
gotResult := testCase.resourceValidator.handleFleetMemberNamespacedResource(context.Background(), testCase.req)
gotResult := testCase.resourceValidator.handleFleetReservedNamespacedResource(context.Background(), testCase.req)
assert.Equal(t, testCase.wantResponse, gotResult, utils.TestCaseMsg, testName)
})
}
Expand Down
7 changes: 3 additions & 4 deletions pkg/webhook/validation/uservalidation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,9 @@ const (
allowedModifyResource = "user in groups is allowed to modify resource"
deniedModifyResource = "user in groups is not allowed to modify resource"

ResourceAllowedFormat = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v"
ResourceDeniedFormat = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v"
ResourceAllowedGetMCFailed = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v because we failed to get MC"
ResourceAllowedGetFleetAPIFailed = "user: '%s' in groups: '%s' is allowed to %s resource %+v/%s: %+v because we failed to get current Fleet API version"
ResourceAllowedFormat = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v"
ResourceDeniedFormat = "user: '%s' in '%s' is not allowed to %s resource %+v/%s: %+v"
ResourceAllowedGetMCFailed = "user: '%s' in '%s' is allowed to %s resource %+v/%s: %+v because we failed to get MC"
)

var (
Expand Down
2 changes: 1 addition & 1 deletion pkg/webhook/webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ func (w *Config) buildFleetGuardRailValidatingWebhooks() []admv1.ValidatingWebho
}
// we don't monitor lease to prevent the deadlock issue, we also don't monitor events.
namespacedResourcesRules := []admv1.RuleWithOperations{
// we want to monitor delete operations on all namespaced resources.
// we want to monitor delete operations on all fleet/kube pre-fixed namespaced resources.
{
Operations: []admv1.OperationType{admv1.Delete},
Rule: createRule([]string{"*"}, []string{"*"}, []string{"*/*"}, &namespacedScope),
Expand Down
Loading

0 comments on commit 0a45072

Please sign in to comment.