p/d/server: try SAR for unauthenticated

With AUTH-509 the ability for the unauthenticated group to do selfsubjectacessreviews (ssar) has been removed. In case that the ssar fails, we attempt a subjectaccessreview for system:anonymous or the system:unauthenticated group.
2026-02-05 09:45:55 +01:00 · 2024-05-22 21:55:02 +02:00
parent 9b3550d810
commit 64b7965e70
4 changed files with 86 additions and 31 deletions
--- a/pkg/dockerregistry/server/auth.go
+++ b/pkg/dockerregistry/server/auth.go
@@ -191,6 +191,10 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 		return nil, ac.wrapErr(ctx, err)
 	}

+	irClient, err := ac.registryClient.Client()
+	if err != nil {
+		return nil, ac.wrapErr(ctx, err)
+	}
 	osClient, err := ac.registryClient.ClientFromToken(bearerToken)
 	if err != nil {
 		return nil, ac.wrapErr(ctx, err)
@@ -243,7 +247,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 					verb = "update"
 				} else {
 					if !verifiedPrune {
-						if err := verifyPruneAccess(ctx, osClient); err != nil {
+						if err := verifyPruneAccess(ctx, osClient, irClient); err != nil {
 							return nil, ac.wrapErr(ctx, err)
 						}
 						verifiedPrune = true
@@ -254,7 +258,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 				return nil, ac.wrapErr(ctx, ErrUnsupportedAction)
 			}

-			if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient); err != nil {
+			if err := verifyImageStreamAccess(ctx, imageStreamNS, imageStreamName, verb, osClient, irClient); err != nil {
 				if access.Action != "pull" {
 					return nil, ac.wrapErr(ctx, err)
 				}
@@ -268,11 +272,11 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 			}
 			switch access.Action {
 			case "get":
-				if err := verifyImageStreamAccess(ctx, namespace, name, access.Action, osClient); err != nil {
+				if err := verifyImageStreamAccess(ctx, namespace, name, access.Action, osClient, irClient); err != nil {
 					return nil, ac.wrapErr(ctx, err)
 				}
 			case "put":
-				if err := verifyImageSignatureAccess(ctx, namespace, name, osClient); err != nil {
+				if err := verifyImageSignatureAccess(ctx, namespace, name, osClient, irClient); err != nil {
 					return nil, ac.wrapErr(ctx, err)
 				}
 			default:
@@ -282,7 +286,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 		case "metrics":
 			switch access.Action {
 			case "get":
-				if err := verifyMetricsAccess(ctx, ac.metricsConfig, bearerToken, osClient); err != nil {
+				if err := verifyMetricsAccess(ctx, ac.metricsConfig, bearerToken, osClient, irClient); err != nil {
 					return nil, ac.wrapErr(ctx, err)
 				}
 			default:
@@ -295,7 +299,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 				if verifiedPrune {
 					continue
 				}
-				if err := verifyPruneAccess(ctx, osClient); err != nil {
+				if err := verifyPruneAccess(ctx, osClient, irClient); err != nil {
 					return nil, ac.wrapErr(ctx, err)
 				}
 				verifiedPrune = true
@@ -309,7 +313,7 @@ func (ac *AccessController) Authorized(ctx context.Context, accessRecords ...reg
 				if access.Action != "*" {
 					return nil, ac.wrapErr(ctx, ErrUnsupportedAction)
 				}
-				if err := verifyCatalogAccess(ctx, osClient); err != nil {
+				if err := verifyCatalogAccess(ctx, osClient, irClient); err != nil {
 					return nil, ac.wrapErr(ctx, err)
 				}
 			default:
@@ -416,16 +420,22 @@ func sarStatus(sar *authorizationapi.SelfSubjectAccessReview) string {
 func verifyWithSAR(
 	ctx context.Context,
 	attrs *authorizationapi.ResourceAttributes,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	response, err := c.SelfSubjectAccessReviews().Create(ctx, &authorizationapi.SelfSubjectAccessReview{
+	response, err := remoteClient.SelfSubjectAccessReviews().Create(ctx, &authorizationapi.SelfSubjectAccessReview{
 		Spec: authorizationapi.SelfSubjectAccessReviewSpec{
 			ResourceAttributes: attrs,
 		},
 	}, metav1.CreateOptions{})
 	if err != nil {
 		dcontext.GetLogger(ctx).Errorf("OpenShift client error: %s", err)
-		if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) {
+
+		if kerrors.IsForbidden(err) {
+			return verifyWithAnonSAR(ctx, attrs, internalClient)
+		}
+
+		if kerrors.IsUnauthorized(err) {
 			return ErrOpenShiftAccessDenied
 		}
 		return err
@@ -439,72 +449,102 @@ func verifyWithSAR(
 	return nil
 }

+func verifyWithAnonSAR(
+	ctx context.Context,
+	attrs *authorizationapi.ResourceAttributes,
+	internalClient client.SubjectAccessReviewsNamespacer,
+) error {
+	response, err := internalClient.SubjectAccessReviews().Create(ctx, &authorizationapi.SubjectAccessReview{
+		Spec: authorizationapi.SubjectAccessReviewSpec{
+			User:               "system:anonymous",
+			Groups:             []string{"system:unauthenticated"},
+			ResourceAttributes: attrs,
+		},
+	}, metav1.CreateOptions{})
+	if err != nil {
+		dcontext.GetLogger(ctx).Errorf("OpenShift internal client error: %s", err)
+		if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) {
+			return ErrOpenShiftAccessDenied
+		}
+		return err
+	}
+
+	if !response.Status.Allowed {
+		return ErrOpenShiftAccessDenied
+	}
+
+	return nil
+}
+
 func verifyWithGlobalSAR(
 	ctx context.Context,
 	resource, subresource, verb string,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	attrs := &authorizationapi.ResourceAttributes{
+	return verifyWithSAR(ctx, &authorizationapi.ResourceAttributes{
 		Verb:        verb,
 		Group:       imageapi.GroupName,
 		Resource:    resource,
 		Subresource: subresource,
-	}
-
-	return verifyWithSAR(ctx, attrs, c)
+	}, remoteClient, internalClient)
 }

 func verifyWithLocalSAR(
 	ctx context.Context,
 	resource, namespace, name, verb string,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	attrs := &authorizationapi.ResourceAttributes{
+	return verifyWithSAR(ctx, &authorizationapi.ResourceAttributes{
 		Namespace: namespace,
 		Verb:      verb,
 		Group:     imageapi.GroupName,
 		Resource:  resource,
 		Name:      name,
-	}
-
-	return verifyWithSAR(ctx, attrs, c)
+	}, remoteClient, internalClient)
 }

 func verifyImageStreamAccess(
 	ctx context.Context,
 	namespace, imageRepo, verb string,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	return verifyWithLocalSAR(ctx, "imagestreams/layers", namespace, imageRepo, verb, c)
+	return verifyWithLocalSAR(ctx, "imagestreams/layers", namespace, imageRepo, verb, remoteClient, internalClient)
 }

 func verifyImageSignatureAccess(
 	ctx context.Context,
 	namespace, imageRepo string,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	return verifyWithLocalSAR(ctx, "imagesignatures", namespace, imageRepo, "create", c)
+	return verifyWithLocalSAR(ctx, "imagesignatures", namespace, imageRepo, "create", remoteClient, internalClient)
 }

 func verifyPruneAccess(
 	ctx context.Context,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	return verifyWithGlobalSAR(ctx, "images", "", "delete", c)
+	return verifyWithGlobalSAR(ctx, "images", "", "delete", remoteClient, internalClient)
 }

 func verifyCatalogAccess(
 	ctx context.Context,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
-	return verifyWithGlobalSAR(ctx, "imagestreams", "", "list", c)
+	return verifyWithGlobalSAR(ctx, "imagestreams", "", "list", remoteClient, internalClient)
 }

 func verifyMetricsAccess(
 	ctx context.Context,
 	metrics configuration.Metrics,
 	token string,
-	c client.SelfSubjectAccessReviewsNamespacer,
+	remoteClient client.SelfSubjectAccessReviewsNamespacer,
+	internalClient client.SubjectAccessReviewsNamespacer,
 ) error {
 	if !metrics.Enabled {
 		return ErrOpenShiftAccessDenied
@@ -517,7 +557,7 @@ func verifyMetricsAccess(
 		return nil
 	}

-	if err := verifyWithGlobalSAR(ctx, "registry", "metrics", "get", c); err != nil {
+	if err := verifyWithGlobalSAR(ctx, "registry", "metrics", "get", remoteClient, internalClient); err != nil {
 		return err
 	}

--- a/pkg/dockerregistry/server/auth_test.go
+++ b/pkg/dockerregistry/server/auth_test.go
@@ -91,7 +91,7 @@ func TestVerifyImageStreamAccess(t *testing.T) {
 		if err != nil {
 			t.Fatal(err)
 		}
-		err = verifyImageStreamAccess(ctx, "foo", "bar", "create", osclient)
+		err = verifyImageStreamAccess(ctx, "foo", "bar", "create", osclient, osclient)
 		if err == nil || test.expectedError == nil {
 			if err != test.expectedError {
 				t.Fatalf("verifyImageStreamAccess did not get expected error - got %s - expected %s", err, test.expectedError)
--- a/pkg/dockerregistry/server/client/client.go
+++ b/pkg/dockerregistry/server/client/client.go
@@ -33,6 +33,7 @@ type Interface interface {
 	LimitRangesGetter
 	LocalSubjectAccessReviewsNamespacer
 	SelfSubjectAccessReviewsNamespacer
+	SubjectAccessReviewsNamespacer
 	UsersInterfacer
 	ImageContentSourcePolicyInterfacer
 }
@@ -120,6 +121,10 @@ func (c *apiClient) SelfSubjectAccessReviews() SelfSubjectAccessReviewInterface
 	return c.auth.SelfSubjectAccessReviews()
 }

+func (c *apiClient) SubjectAccessReviews() SubjectAccessReviewInterface {
+	return c.auth.SubjectAccessReviews()
+}
+
 type registryClient struct {
 	kubeConfig *restclient.Config
 }
--- a/pkg/dockerregistry/server/client/interfaces.go
+++ b/pkg/dockerregistry/server/client/interfaces.go
@@ -69,6 +69,10 @@ type SelfSubjectAccessReviewsNamespacer interface {
 	SelfSubjectAccessReviews() SelfSubjectAccessReviewInterface
 }

+type SubjectAccessReviewsNamespacer interface {
+	SubjectAccessReviews() SubjectAccessReviewInterface
+}
+
 var _ ImageSignatureInterface = imageclientv1.ImageSignatureInterface(nil)

 type ImageSignatureInterface interface {
@@ -140,3 +144,9 @@ var _ SelfSubjectAccessReviewInterface = authclientv1.SelfSubjectAccessReviewInt
 type SelfSubjectAccessReviewInterface interface {
 	Create(ctx context.Context, selfSubjectAccessReview *authapiv1.SelfSubjectAccessReview, opts metav1.CreateOptions) (*authapiv1.SelfSubjectAccessReview, error)
 }
+
+var _ SubjectAccessReviewInterface = authclientv1.SubjectAccessReviewInterface(nil)
+
+type SubjectAccessReviewInterface interface {
+	Create(ctx context.Context, subjectAccessReview *authapiv1.SubjectAccessReview, opts metav1.CreateOptions) (*authapiv1.SubjectAccessReview, error)
+}