fix(localresource): confine fromRef to the referencing resource's own namespace (#10)

SECURITY.md

@@ -0,0 +1,34 @@
+# Security Policy
+
+## Reporting a vulnerability
+
+**Please do not open a public issue for a suspected vulnerability.**
+
+Use GitHub's private vulnerability reporting, which is enabled on this repository:
+**[Report a vulnerability](https://github.com/krateo-platformops/git-provider/security/advisories/new)**
+(Security → Advisories → Report a vulnerability).
+
+A report is most useful when it says what an actor who holds a given permission can end up doing —
+the permission they start from, the resource they reach, and what leaves the cluster. A runnable
+manifest is welcome but never required; please hold it back from anywhere public.
+
+## Supported versions
+
+Fixes land on `main` and ship in the next tagged release. Only the latest minor is supported.
+
+## What this provider can reach, by design
+
+git-provider reconciles `Repo` and `LocalResource` custom resources. Two properties are worth
+knowing when assessing a report, because they shape most of the interesting reports:
+
+- **It acts with its own identity.** Reads and git pushes use the provider's ServiceAccount, not the
+  identity of whoever authored the custom resource. The provider holds a cluster-wide `get`, because
+  a `LocalResource` may reference arbitrary resource kinds.
+- **It is an egress path.** A `LocalResource` names the git remote it pushes to, so anything the
+  provider can read and serialise can leave the cluster.
+
+Those two together mean that "who may create a `LocalResource` in namespace X" is a meaningful
+privilege boundary. `fromRef` is confined to the referencing resource's own namespace for exactly
+this reason (#10). A same-namespace read is still performed with the provider's identity rather than
+the author's; authorizing it against the author (`SubjectAccessReview`) remains open work, and a
+report that sharpens that case is welcome.

internal/controllers/localresource/fromref_namespace_test.go

@@ -0,0 +1,59 @@
+package localresource
+
+import (
+	"strings"
+	"testing"
+)
+
+// krateo-platformops/git-provider#10: fromRef is resolved with the PROVIDER's ServiceAccount, which
+// holds a cluster-wide get, and the fetched object is serialised in full and pushed to a git remote
+// the same CR names. An author-chosen namespace therefore turned "may create a LocalResource" into
+// "may read anything in the cluster, and publish it".
+//
+// This pins the confinement. The rule is kept pure precisely so it is testable — SyncLocalResources
+// clones a repository long before it reaches this point.
+func TestValidateFromRefNamespace(t *testing.T) {
+	cases := []struct {
+		name        string
+		ref, cr     string
+		wantErr     bool
+		wantMention string
+	}{
+		{
+			name: "same namespace is allowed",
+			ref:  "team-a", cr: "team-a", wantErr: false,
+		},
+		{
+			name: "empty namespace is allowed (cluster-scoped read path, not the escalation)",
+			ref:  "", cr: "team-a", wantErr: false,
+		},
+		{
+			name: "another namespace is REFUSED — this is the confused deputy",
+			ref:  "kube-system", cr: "team-a", wantErr: true, wantMention: "kube-system",
+		},
+		{
+			name: "krateo-system is not special-cased",
+			ref:  "krateo-system", cr: "team-a", wantErr: true, wantMention: "krateo-system",
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			err := validateFromRefNamespace(c.ref, c.cr)
+			if c.wantErr != (err != nil) {
+				t.Fatalf("validateFromRefNamespace(%q, %q) error = %v, wantErr %v", c.ref, c.cr, err, c.wantErr)
+			}
+			if !c.wantErr {
+				return
+			}
+			// The operator has to be able to act on this, so the message must name the rejected
+			// namespace and the one that was permitted — not just "forbidden".
+			if !strings.Contains(err.Error(), c.wantMention) {
+				t.Errorf("error must name the rejected namespace %q, got: %v", c.wantMention, err)
+			}
+			if !strings.Contains(err.Error(), c.cr) {
+				t.Errorf("error must name the permitted namespace %q, got: %v", c.cr, err)
+			}
+		})
+	}
+}

internal/controllers/localresource/localresource.go

@@ -363,6 +363,24 @@ func (e *external) syncWithRetry(ctx context.Context, cr *localResourcev1alpha1.
 	return err
 }
 
+// validateFromRefNamespace rejects a fromRef that names a namespace other than the referencing
+// LocalResource's own (#10). Pure so the rule is testable without a git server — SyncLocalResources
+// clones a repository before it reaches this point.
+//
+// An empty namespace is permitted: that is the cluster-scoped read path, not the author-directed
+// namespace escalation this closes.
+func validateFromRefNamespace(refNamespace, crNamespace string) error {
+	if refNamespace == "" || refNamespace == crNamespace {
+		return nil
+	}
+	return fmt.Errorf(
+		"fromResource.fromRef.namespace %q is not permitted: a LocalResource may only reference "+
+			"resources in its own namespace (%q). This reference is resolved with the provider's "+
+			"identity, so permitting another namespace would let it read and publish objects its "+
+			"author may not be entitled to",
+		refNamespace, crNamespace)
+}
+
 func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1alpha1.LocalResource, commitMessage string) error {
 	spec := cr.Spec.DeepCopy()
 
@@ -418,8 +436,28 @@ func (e *external) SyncLocalResources(ctx context.Context, cr *localResourcev1al
 			return fmt.Errorf("parsing group version from fromResource.FromRef: %w", err)
 		}
 
+		// fromRef is confined to THIS LocalResource's own namespace (#10).
+		//
+		// The read below uses the PROVIDER's ServiceAccount, not the author's — and that account
+		// holds a cluster-wide get on every resource, because cross-namespace refs were the point.
+		// The fetched object is then serialised in full (Secret.data included) and pushed to a git
+		// remote this same CR names. So an author-chosen namespace turned "may create a
+		// LocalResource" into "may read anything in the cluster, and publish it". Confused deputy.
+		//
+		// Refusing an out-of-namespace ref removes the author's ability to AIM that identity. It does
+		// not make the read authorized: a same-namespace read still happens as the provider, so
+		// whoever may create a LocalResource in a namespace can still read what the provider can read
+		// THERE. Closing that needs a SubjectAccessReview against the CR's author, which the CR does
+		// not record — left as an explicit follow-up rather than half-built here.
+		if err := validateFromRefNamespace(spec.FromResource.FromRef.Namespace, cr.GetNamespace()); err != nil {
+			return err
+		}
+
 		var cli dynamic.ResourceInterface
 		if spec.FromResource.FromRef.Namespace == "" {
+			// Cluster-scoped target (the kind has no namespace). Still the provider's identity, so it
+			// is in scope for the SubjectAccessReview follow-up above; left working because it is not
+			// the author-directed namespace escalation this change closes.
 			cli = e.dynamic.Resource(schema.GroupVersionResource{
 				Group:    gv.Group,
 				Version:  gv.Version,