diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..9e9ac9e --- /dev/null +++ b/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. diff --git a/internal/controllers/localresource/fromref_namespace_test.go b/internal/controllers/localresource/fromref_namespace_test.go new file mode 100644 index 0000000..e1ada36 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/internal/controllers/localresource/localresource.go b/internal/controllers/localresource/localresource.go index 5f9f240..ddb2b95 100644 --- a/internal/controllers/localresource/localresource.go +++ b/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,