Validate GitRepo defaults from GitRepoRestriction and Policy against allow-lists

internal/cmd/controller/gitops/reconciler/restrictions.go

@@ -60,24 +60,26 @@ func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *f
 		return errors.New("empty targetNamespace denied, because allowedTargetNamespaceSelector restriction is present")
 	}
 
-	targetNamespace, err := isAllowed(gitrepo.Spec.TargetNamespace, "", allowedTargetNS)
-	if err != nil {
+	// Apply defaults before validation, so a default that is not in the allow-list
+	// is rejected rather than silently accepted. This keeps the GitRepo path
+	// consistent with the HelmOp and Bundle paths.
+	serviceAccount := firstNonEmpty(gitrepo.Spec.ServiceAccount, defaultSA)
+	clientSecretName := firstNonEmpty(gitrepo.Spec.ClientSecretName, defaultClientSecret)
+
+	if err := isAllowed(gitrepo.Spec.TargetNamespace, allowedTargetNS); err != nil {
 		return fmt.Errorf("disallowed targetNamespace %s: %w", gitrepo.Spec.TargetNamespace, err)
 	}
 
-	serviceAccount, err := isAllowed(gitrepo.Spec.ServiceAccount, defaultSA, allowedSAs)
-	if err != nil {
-		return fmt.Errorf("disallowed serviceAccount %s: %w", gitrepo.Spec.ServiceAccount, err)
+	if err := isAllowed(serviceAccount, allowedSAs); err != nil {
+		return fmt.Errorf("disallowed serviceAccount %s: %w", serviceAccount, err)
 	}
 
-	repo, err := isAllowedByRepo(gitrepo.Spec.Repo, grr.AllowedRepoPatterns, pol.GitAllowedRepoPatterns)
-	if err != nil {
+	if _, err := isAllowedByRepo(gitrepo.Spec.Repo, grr.AllowedRepoPatterns, pol.GitAllowedRepoPatterns); err != nil {
 		return fmt.Errorf("disallowed repo %s: %w", gitrepo.Spec.Repo, err)
 	}
 
-	clientSecretName, err := isAllowed(gitrepo.Spec.ClientSecretName, defaultClientSecret, allowedClientSecrets)
-	if err != nil {
-		return fmt.Errorf("disallowed clientSecretName %s: %w", gitrepo.Spec.ClientSecretName, err)
+	if err := isAllowed(clientSecretName, allowedClientSecrets); err != nil {
+		return fmt.Errorf("disallowed clientSecretName %s: %w", clientSecretName, err)
 	}
 
 	// Policy: RequireServiceAccount is checked after all defaulting.
@@ -86,9 +88,7 @@ func AuthorizeAndAssignDefaults(ctx context.Context, c client.Client, gitrepo *f
 	}
 
 	// Write resolved values back to the GitRepo.
-	gitrepo.Spec.TargetNamespace = targetNamespace
 	gitrepo.Spec.ServiceAccount = serviceAccount
-	gitrepo.Spec.Repo = repo
 	gitrepo.Spec.ClientSecretName = clientSecretName
 
 	return nil
@@ -123,18 +123,15 @@ func aggregate(restrictions []fleet.GitRepoRestriction) (result fleet.GitRepoRes
 	return result
 }
 
-func isAllowed(currentValue, defaultValue string, allowedValues []string) (string, error) {
-	if currentValue == "" {
-		return defaultValue, nil
-	}
-	if len(allowedValues) == 0 {
-		return currentValue, nil
+func isAllowed(currentValue string, allowedValues []string) error {
+	if currentValue == "" || len(allowedValues) == 0 {
+		return nil
 	}
 	if slices.Contains(allowedValues, currentValue) {
-		return currentValue, nil
+		return nil
 	}
 
-	return currentValue, fmt.Errorf("%s not in allowed set %v", currentValue, allowedValues)
+	return fmt.Errorf("%s not in allowed set %v", currentValue, allowedValues)
 }
 
 func isAllowedByRegex(currentValue, defaultValue string, patterns []string) (string, error) {

internal/cmd/controller/gitops/reconciler/restrictions_test.go

@@ -192,9 +192,9 @@ func TestAuthorizeAndAssignDefaults(t *testing.T) {
 			restrictions: &fleet.GitRepoRestrictionList{
 				Items: []fleet.GitRepoRestriction{
 					{
-						AllowedClientSecretNames: []string{"csn"},
+						AllowedClientSecretNames: []string{"dcsn"},
 						AllowedRepoPatterns:      []string{".*foo.bar.*"},
-						AllowedServiceAccounts:   []string{"sacc"},
+						AllowedServiceAccounts:   []string{"dsacc"},
 						AllowedTargetNamespaces:  []string{"tns"},
 						DefaultClientSecretName:  "dcsn",
 						DefaultServiceAccount:    "dsacc",
@@ -210,6 +210,57 @@ func TestAuthorizeAndAssignDefaults(t *testing.T) {
 				},
 			},
 		},
+		{
+			// A default that is not in the allow-list must be rejected, not
+			// silently applied. This mirrors the HelmOp and Bundle behavior.
+			name: "deny defaultServiceAccount that is not in allowedServiceAccounts",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedRepoPatterns:    []string{".*foo.bar.*"},
+						AllowedServiceAccounts: []string{"sacc"},
+						DefaultServiceAccount:  "dsacc",
+					},
+				},
+			},
+			// The GitRepo is left unmutated: validation fails before the
+			// resolved defaults are written back.
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+			expectedErr: "disallowed serviceAccount.*",
+		},
+		{
+			// Same for a defaultClientSecretName outside the allow-list.
+			name: "deny defaultClientSecretName that is not in allowedClientSecretNames",
+			inputGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+			restrictions: &fleet.GitRepoRestrictionList{
+				Items: []fleet.GitRepoRestriction{
+					{
+						AllowedRepoPatterns:      []string{".*foo.bar.*"},
+						AllowedClientSecretNames: []string{"csn"},
+						DefaultClientSecretName:  "dcsn",
+					},
+				},
+			},
+			expectedGr: fleet.GitRepo{
+				Spec: fleet.GitRepoSpec{
+					Repo: "http://foo.bar/baz",
+				},
+			},
+			expectedErr: "disallowed clientSecretName.*",
+		},
 		{
 			// Policy patterns are anchored: a pattern without .* must match the full URL.
 			name: "Policy repo pattern: reject URL that matches only as substring",