From fff92866823a9f20089ffa2fe12e206d9ca726b1 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 07:47:52 +0000 Subject: [PATCH 1/8] fix(watch): stop an API-surface trigger informer RBAC denies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ServesWatchable asks what the API server SERVES. It is not what this ServiceAccount may READ. An ordinary apiserver serves apiregistration.k8s.io whether or not the operator's ClusterRole names apiservices, so a least-privilege install sails past the "is it served?" gate, starts the informer, and takes a 403 on the reflector's first LIST — which client-go then retries forever. That is the same benign, endlessly repeated noise the unserved-resource gate exists to remove, and exactly how a real error gets missed. Both trigger informers now install a watch error handler. A Forbidden stops that informer and logs once; every other error stays with the reflector's own backoff, because a 500 or a reset connection is transient and tearing the informer down would turn a blip into a lost trigger. The reflector routes LIST errors through the same handler, so the denial is caught where it actually happens. A 403 is authoritative and will not resolve by retrying, but it CAN be granted later, so the resource is un-started rather than blacklisted: the next successful catalog refresh re-arms it, no restart. That required giving each trigger its own informer and context. The shared dynamicSharedInformerFactory records an informer as started forever, so a resource stopped for any reason could never be re-armed through it. These two resources are conveniences — they only make the catalog notice a new CRD or aggregated API sooner than its periodic tick would. Failing closed and quiet is the honest answer to "you may not read this". Co-Authored-By: Claude Opus 4.8 (1M context) --- .../api_surface_triggers_forbidden_test.go | 170 ++++++++++++++++++ internal/watch/api_surface_triggers_test.go | 2 +- internal/watch/manager.go | 32 +++- internal/watch/manager_catalog.go | 133 +++++++++++--- 4 files changed, 305 insertions(+), 32 deletions(-) create mode 100644 internal/watch/api_surface_triggers_forbidden_test.go diff --git a/internal/watch/api_surface_triggers_forbidden_test.go b/internal/watch/api_surface_triggers_forbidden_test.go new file mode 100644 index 000000000..bc5cc37a7 --- /dev/null +++ b/internal/watch/api_surface_triggers_forbidden_test.go @@ -0,0 +1,170 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kwatch "k8s.io/apimachinery/pkg/watch" + dynamicfake "k8s.io/client-go/dynamic/fake" + clienttesting "k8s.io/client-go/testing" +) + +func triggerListKinds() map[schema.GroupVersionResource]string { + return map[schema.GroupVersionResource]string{ + crdTriggerGVR(): "CustomResourceDefinitionList", + apiServiceTriggerGVR(): "APIServiceList", + } +} + +// forbiddenTriggerClient serves the CRD trigger normally and denies the given resource with a +// 403 on both list and watch — an ordinary apiserver seen by a ServiceAccount whose ClusterRole +// does not name that resource. Flip the returned flag to grant the permission. +// +// The denial is gated on an atomic rather than by rewriting the client's reaction chains: a +// reflector on another resource reads those chains concurrently, so mutating them races. +func forbiddenTriggerClient(denied schema.GroupVersionResource) (*dynamicfake.FakeDynamicClient, *atomic.Bool) { + client := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), triggerListKinds()) + + forbidden := apierrors.NewForbidden( + schema.GroupResource{Group: denied.Group, Resource: denied.Resource}, "", + errors.New("RBAC: no rule grants list on this resource")) + + var denying atomic.Bool + denying.Store(true) + + client.PrependReactor("list", denied.Resource, + func(clienttesting.Action) (bool, runtime.Object, error) { + if denying.Load() { + return true, nil, forbidden + } + return false, nil, nil // fall through to the object tracker + }) + client.PrependWatchReactor(denied.Resource, + func(clienttesting.Action) (bool, kwatch.Interface, error) { + if denying.Load() { + return true, nil, forbidden + } + return false, nil, nil + }) + return client, &denying +} + +// managerWithTriggerClient wires a manager whose discovery serves BOTH triggers (so the +// ServesWatchable gate cannot be what stops anything) and whose dynamic client is the fake. +func managerWithTriggerClient(t *testing.T, client *dynamicfake.FakeDynamicClient) *Manager { + t.Helper() + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + + catalog := NewAPIResourceCatalog() + _, err := catalog.Refresh(aggregatingDiscovery()) + require.NoError(t, err) + + m := &Manager{Log: logr.Discard()} + m.triggerCtx = ctx + m.triggerClient = client + m.resourceCatalog = catalog + return m +} + +func (m *Manager) triggerIsStarted(gvr schema.GroupVersionResource) bool { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + _, started := m.triggersStarted[gvr] + return started +} + +func (m *Manager) triggerForbiddenLogged(gvr schema.GroupVersionResource) bool { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + _, logged := m.triggersForbiddenLogged[gvr] + return logged +} + +// Discovery reports what the SERVER serves, not what the CALLER may read. On an ordinary +// apiserver apiregistration.k8s.io is served, so ServesWatchable says yes and the informer +// starts — then the reflector's LIST is denied and client-go retries it forever. That is the +// same endless, benign noise the unserved-resource gate exists to remove. +// +// The informer must stop, and stop counting as started, so a later refresh can re-arm it. +func TestEnsureAPISurfaceTriggerInformers_StopsForbiddenInformer(t *testing.T) { + denied := apiServiceTriggerGVR() + client, _ := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + // Discovery serves both, so both start; only the 403 may take one down. + require.True(t, m.triggerIsStarted(crdTriggerGVR())) + + require.Eventually(t, func() bool { return !m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, + "a forbidden trigger informer must be stopped and un-started, not retried forever") + + // One denial must not disarm the whole API surface. + require.True(t, m.triggerIsStarted(crdTriggerGVR()), + "the permitted trigger must keep running") +} + +// A denial that never clears must be recorded once, not once per reflector retry. +func TestEnsureAPISurfaceTriggerInformers_ForbiddenLogsOncePerDenial(t *testing.T) { + denied := apiServiceTriggerGVR() + client, _ := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Eventually(t, func() bool { return m.triggerForbiddenLogged(denied) }, + 5*time.Second, 10*time.Millisecond, "the denial must be recorded so it logs once") +} + +// Granting the RBAC later must be picked up on the next catalog refresh, with no restart — +// the same promise the unserved path makes for an aggregation layer installed later. +func TestEnsureAPISurfaceTriggerInformers_ReArmsAfterForbiddenIsGranted(t *testing.T) { + denied := apiServiceTriggerGVR() + client, denying := forbiddenTriggerClient(denied) + m := managerWithTriggerClient(t, client) + + m.ensureAPISurfaceTriggerInformers(m.Log) + require.Eventually(t, func() bool { return !m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, "forbidden informer should have been stopped") + + // Grant the permission. The reactors stay in place and start falling through. + denying.Store(false) + + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Eventually(t, func() bool { return m.triggerIsStarted(denied) }, + 5*time.Second, 10*time.Millisecond, + "a trigger denied earlier must re-arm once RBAC allows it, without a restart") +} + +// A non-RBAC error is transient — a connection reset, a 500 — and the reflector's own +// backoff owns it. Tearing the informer down there would turn a blip into a lost trigger. +func TestEnsureAPISurfaceTriggerInformers_KeepsInformerOnTransientError(t *testing.T) { + gvr := apiServiceTriggerGVR() + client := dynamicfake.NewSimpleDynamicClientWithCustomListKinds(runtime.NewScheme(), triggerListKinds()) + client.PrependReactor("list", gvr.Resource, + func(clienttesting.Action) (bool, runtime.Object, error) { + return true, nil, apierrors.NewInternalError(errors.New("etcd leader changed")) + }) + + m := managerWithTriggerClient(t, client) + m.ensureAPISurfaceTriggerInformers(m.Log) + + require.Never(t, func() bool { return !m.triggerIsStarted(gvr) }, + 500*time.Millisecond, 25*time.Millisecond, + "a 500 is not a permission problem; the reflector must keep retrying it") + require.False(t, m.triggerForbiddenLogged(gvr)) +} diff --git a/internal/watch/api_surface_triggers_test.go b/internal/watch/api_surface_triggers_test.go index 319806bcb..f7e8d7368 100644 --- a/internal/watch/api_surface_triggers_test.go +++ b/internal/watch/api_surface_triggers_test.go @@ -156,6 +156,6 @@ func TestEnsureAPISurfaceTriggerInformers_NoOpBeforeStart(t *testing.T) { m := &Manager{Log: logr.Discard()} m.ensureAPISurfaceTriggerInformers(m.Log) - require.Nil(t, m.triggerFactory) + require.Nil(t, m.triggerClient) require.Empty(t, m.triggersStarted) } diff --git a/internal/watch/manager.go b/internal/watch/manager.go index 101d9bad0..be721a2b6 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -19,7 +19,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/watch" "k8s.io/client-go/dynamic" - "k8s.io/client-go/dynamic/dynamicinformer" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -30,8 +29,20 @@ import ( "github.com/ConfigButler/gitops-reverser/internal/typeset" ) -// RBAC permissions for dynamic watch manager - read-only access to watch all (also future ones!) resource types -// +kubebuilder:rbac:groups=*,resources=*,verbs=get;list;watch +// The API-resource catalog reads the two resources that describe the API surface itself, so +// it can tell a type that vanished from one the operator was never allowed to see. These are +// the trigger informers' resources too; without them a least-privilege install gets a 403 on +// every reflector retry, which is why the trigger error handler stops on Forbidden. +// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch +// +kubebuilder:rbac:groups=apiregistration.k8s.io,resources=apiservices,verbs=get;list;watch + +// A WatchRule may name ANY type, so the watch manager's read access cannot be derived from +// code. It is deliberately NOT a marker: controller-gen would fold `groups=*,resources=*` +// into this ClusterRole, and RBAC is additive — a wildcard read grants cluster-wide Secret +// list/watch no matter how narrow the Secret rule beside it is. The wildcard therefore lives +// in its own opt-out ClusterRole (config/rbac/watch-any-resource-role.yaml, +// `rbac.watchAnyResource` in the chart), so an operator that mirrors two CRDs can be denied +// every other object in the cluster. See docs/rbac.md. // Manager is a controller-runtime Runnable that keeps the followability registry and the // demand-driven materialization axis fresh and drives the per-type splice reconcile. It @@ -101,16 +112,23 @@ type Manager struct { // (re-)evaluated after every catalog refresh — which controllers drive, not just the // manager's own loop — so this is not a startup-only structure. triggersMu sync.Mutex - // triggerCtx is the manager's lifetime context, the stop channel every trigger informer - // is started with. Set once by Start; nil before then, which defers informer creation. + // triggerCtx is the manager's lifetime context, the parent of every trigger informer's + // own context. Set once by Start; nil before then, which defers informer creation. triggerCtx context.Context - // triggerFactory is the shared dynamic informer factory backing the trigger informers. - triggerFactory dynamicinformer.DynamicSharedInformerFactory + // triggerClient is the dynamic client the trigger informers list and watch through. + // Built once from the REST config, or injected by tests. + triggerClient dynamic.Interface // triggersStarted is the set of trigger resources whose informer is already running. triggersStarted map[schema.GroupVersionResource]struct{} + // triggerStops cancels one trigger informer without touching the others. Each informer + // gets its own context so a single forbidden resource can be stopped and later re-armed. + triggerStops map[schema.GroupVersionResource]context.CancelFunc // triggersSkipLogged records which unserved trigger resources have already been logged, // so a permanently absent aggregation layer produces one line, not one per refresh. triggersSkipLogged map[schema.GroupVersionResource]struct{} + // triggersForbiddenLogged records which trigger resources RBAC has already denied, so a + // permanently unauthorized resource produces one line per denial, not one per retry. + triggersForbiddenLogged map[schema.GroupVersionResource]struct{} // catalogReadyOnce guards the one-time "catalog ready" log line, matching the // firstMessage/firstGroupReady sync.Once pattern used by the audit consumer. catalogReadyOnce sync.Once diff --git a/internal/watch/manager_catalog.go b/internal/watch/manager_catalog.go index 8b5cc2f7b..26ee01233 100644 --- a/internal/watch/manager_catalog.go +++ b/internal/watch/manager_catalog.go @@ -13,6 +13,8 @@ import ( "github.com/go-logr/logr" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/metric" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" @@ -388,7 +390,7 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { log.V(1).Info("deferring API surface trigger informers - catalog not ready") return } - if m.triggerFactory == nil { + if m.triggerClient == nil { cfg := m.restConfig() if cfg == nil { log.V(1).Info("skipping API surface trigger informers - no REST config available") @@ -399,9 +401,13 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { log.Error(err, "failed to create API surface trigger client") return } - m.triggerFactory = dynamicinformer.NewDynamicSharedInformerFactory(dynamicClient, 0) + m.triggerClient = dynamicClient + } + if m.triggersStarted == nil { m.triggersStarted = map[schema.GroupVersionResource]struct{}{} m.triggersSkipLogged = map[schema.GroupVersionResource]struct{}{} + m.triggersForbiddenLogged = map[schema.GroupVersionResource]struct{}{} + m.triggerStops = map[schema.GroupVersionResource]context.CancelFunc{} } handler := cache.ResourceEventHandlerFuncs{ @@ -420,25 +426,100 @@ func (m *Manager) ensureAPISurfaceTriggerInformers(log logr.Logger) { "the catalog refreshes on its periodic tick instead", "gvr", gvr.String()) } - var fresh []cache.SharedIndexInformer for _, gvr := range start { - informer := m.triggerFactory.ForResource(gvr).Informer() - if _, addErr := informer.AddEventHandler(handler); addErr != nil { - log.Error(addErr, "failed to add API surface trigger handler", "gvr", gvr.String()) - continue - } - m.triggersStarted[gvr] = struct{}{} - delete(m.triggersSkipLogged, gvr) - fresh = append(fresh, informer) - log.Info("API surface trigger informer started", "gvr", gvr.String()) + m.startAPISurfaceTriggerInformer(ctx, log, gvr, handler) + } +} + +// startAPISurfaceTriggerInformer runs one trigger informer under its own context. Each gets +// a private informer rather than a share of a dynamicSharedInformerFactory: the factory +// records an informer as started forever, so a resource stopped for one reason could never +// be re-armed through it. Own informer, own context, own stop. +func (m *Manager) startAPISurfaceTriggerInformer( + ctx context.Context, + log logr.Logger, + gvr schema.GroupVersionResource, + handler cache.ResourceEventHandlerFuncs, +) { + informer := dynamicinformer.NewFilteredDynamicInformer( + m.triggerClient, gvr, metav1.NamespaceAll, 0, cache.Indexers{}, nil, + ).Informer() + + if _, addErr := informer.AddEventHandler(handler); addErr != nil { + log.Error(addErr, "failed to add API surface trigger handler", "gvr", gvr.String()) + return } - if len(fresh) == 0 { + // Must precede Run. A forbidden LIST reaches this handler too: the reflector routes every + // ListAndWatch error through it, not only watch errors. + if setErr := informer.SetWatchErrorHandlerWithContext(m.triggerWatchErrorHandler(gvr, log)); setErr != nil { + log.Error(setErr, "failed to install API surface trigger error handler", "gvr", gvr.String()) return } - // Start is idempotent per informer: it launches only the ones not yet running. - m.triggerFactory.Start(ctx.Done()) - go waitForAPISurfaceTriggerSync(ctx, log, fresh) + gvrCtx, cancel := context.WithCancel(ctx) + m.triggersStarted[gvr] = struct{}{} + m.triggerStops[gvr] = cancel + delete(m.triggersSkipLogged, gvr) + // A denial that clears and returns must be logged again; the once-only guard covers a + // single denial, not the resource's whole lifetime. + delete(m.triggersForbiddenLogged, gvr) + log.Info("API surface trigger informer started", "gvr", gvr.String()) + + // cancel is also held in triggerStops so a forbidden reflector can stop just this one; + // releasing it here as well keeps the context from outliving the informer it belongs to. + go func() { + defer cancel() + informer.RunWithContext(gvrCtx) + }() + go waitForAPISurfaceTriggerSync(gvrCtx, log, gvr, informer) +} + +// triggerWatchErrorHandler tears down a trigger informer the operator is not authorized to +// read, and leaves every other error to the reflector's own backoff. +// +// Discovery answers what the API server SERVES, which is not what this ServiceAccount may +// READ: a cluster can serve apiregistration.k8s.io while a least-privilege ClusterRole omits +// apiservices. The informer then starts and its LIST is denied on every retry, forever — +// the same benign, endlessly repeated noise the unserved-resource gate exists to remove, +// and exactly how a real error gets missed. These resources are conveniences (they only +// make the catalog refresh sooner than its periodic tick), so failing closed and quiet is +// the honest response to "you may not read this". +// +// A 403 is authoritative and will not resolve by retrying, but it CAN be granted later, so +// the resource is un-started rather than blacklisted: the next catalog refresh re-arms it. +func (m *Manager) triggerWatchErrorHandler( + gvr schema.GroupVersionResource, + log logr.Logger, +) cache.WatchErrorHandlerWithContext { + return func(ctx context.Context, r *cache.Reflector, err error) { + if !apierrors.IsForbidden(err) { + cache.DefaultWatchErrorHandler(ctx, r, err) + return + } + m.stopForbiddenTrigger(gvr, log, err) + } +} + +// stopForbiddenTrigger cancels the informer's context and forgets it was started, so the +// next successful catalog refresh can try again. Called from the reflector's goroutine. +func (m *Manager) stopForbiddenTrigger(gvr schema.GroupVersionResource, log logr.Logger, err error) { + m.triggersMu.Lock() + defer m.triggersMu.Unlock() + + if cancel, ok := m.triggerStops[gvr]; ok { + cancel() + delete(m.triggerStops, gvr) + } + delete(m.triggersStarted, gvr) + + if _, logged := m.triggersForbiddenLogged[gvr]; logged { + // The reflector can report the denial more than once before its context unwinds. + return + } + m.triggersForbiddenLogged[gvr] = struct{}{} + log.Info("not authorized to watch an API surface trigger; informer stopped and the catalog "+ + "refreshes on its periodic tick instead. Grant get/list/watch to re-arm it without a restart", + "gvr", gvr.String(), "reason", err.Error()) } // apiSurfaceTriggerGVRs are the resources whose changes mean the API surface moved: a CRD @@ -479,14 +560,18 @@ func (m *Manager) setTriggerContext(ctx context.Context) { m.triggerCtx = ctx } -func waitForAPISurfaceTriggerSync(ctx context.Context, log logr.Logger, informers []cache.SharedIndexInformer) { - syncFns := make([]cache.InformerSynced, 0, len(informers)) - for _, informer := range informers { - syncFns = append(syncFns, informer.HasSynced) - } - if !cache.WaitForCacheSync(ctx.Done(), syncFns...) { - log.Info("API surface trigger informer sync stopped before completion") +// waitForAPISurfaceTriggerSync watches one informer's initial sync. It takes that informer's +// own context, so a trigger stopped for being forbidden ends this wait instead of leaving a +// goroutine blocked on a cache that will never sync. +func waitForAPISurfaceTriggerSync( + ctx context.Context, + log logr.Logger, + gvr schema.GroupVersionResource, + informer cache.SharedIndexInformer, +) { + if !cache.WaitForCacheSync(ctx.Done(), informer.HasSynced) { + log.V(1).Info("API surface trigger informer sync stopped before completion", "gvr", gvr.String()) return } - log.V(1).Info("API surface trigger informers synced") + log.V(1).Info("API surface trigger informer synced", "gvr", gvr.String()) } From 1a12159b01bcdd7fb1a10e13522672ef1c761028 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 07:48:09 +0000 Subject: [PATCH 2/8] feat(rbac): make the wildcard cluster read an opt-out ClusterRole MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit config/rbac/role.yaml asked for `apiGroups: ["*"], resources: ["*"], verbs: [get,list,watch]` and, beside it, `secrets: [get,list,watch,create,update,patch]`. The wildcard is honest about the general case — a WatchRule may name any type, including one installed later — but RBAC is ADDITIVE, so while it sat in the manager role no chart value could take back the cluster-wide Secret read it implied. A reverser asked to mirror two CRDs on a management cluster held read access to every credential in it. It does not need them. The manager has held no Secret informer since Secret values stopped being retained in the control plane: Cache.DisableFor excludes them, so every read is a direct get of a Secret a GitProvider or GitTarget names by name. Nothing calls list, watch or patch on a Secret. The marker was never updated to match the binary, so the shipped role still asked for what the code had stopped using. Three changes, none of which alter what a default install can do: - The wildcard moves to its own ClusterRole, gitops-reverser-watch-any-resource, created by default (chart: rbac.watchAnyResource, kustomize: watch-any-resource-role.yaml) and droppable whole. It is deliberately not a kubebuilder marker: controller-gen would fold it straight back into the role it must stay out of. - The manager role's secrets rule narrows to get;create;update — what the binary calls. - customresourcedefinitions and apiservices become explicit rules. They were reachable only through the wildcard, and the API-resource catalog and its trigger informers read both, so a narrowed install would have 403'd on them. Set rbac.watchAnyResource=false, grant get/list/watch on the types your WatchRules name, and the reverser runs with zero cluster-wide Secret access. Pinned by chart-render tests that assert no rendered ClusterRole grants Secret list/watch once the wildcard is off, and that the generated manager role never carries the wildcard itself. Documented in docs/rbac.md. Co-Authored-By: Claude Opus 4.8 (1M context) --- charts/gitops-reverser/templates/rbac.yaml | 38 +++++ charts/gitops-reverser/values.yaml | 11 ++ cmd/main_chart_rbac_test.go | 159 ++++++++++++++++++ config/rbac/kustomization.yaml | 4 + config/rbac/role.yaml | 13 +- config/rbac/watch-any-resource-role.yaml | 24 +++ .../rbac/watch-any-resource-rolebinding.yaml | 15 ++ docs/README.md | 1 + docs/UPGRADING.md | 38 +++++ docs/rbac.md | 85 ++++++++++ internal/controller/gitprovider_controller.go | 2 +- internal/controller/gittarget_controller.go | 2 +- 12 files changed, 386 insertions(+), 6 deletions(-) create mode 100644 cmd/main_chart_rbac_test.go create mode 100644 config/rbac/watch-any-resource-role.yaml create mode 100644 config/rbac/watch-any-resource-rolebinding.yaml create mode 100644 docs/rbac.md diff --git a/charts/gitops-reverser/templates/rbac.yaml b/charts/gitops-reverser/templates/rbac.yaml index 786f1d73c..95562d64d 100644 --- a/charts/gitops-reverser/templates/rbac.yaml +++ b/charts/gitops-reverser/templates/rbac.yaml @@ -28,4 +28,42 @@ subjects: - kind: ServiceAccount name: {{ include "gitops-reverser.serviceAccountName" . }} namespace: {{ .Release.Namespace }} +{{- if .Values.rbac.watchAnyResource }} +--- +# A WatchRule may name any type, including types installed later, so this cannot be derived +# from the code. It is a separate ClusterRole because RBAC is additive: folded into the +# manager role, this wildcard would grant cluster-wide Secret list/watch however narrow the +# Secret rule beside it is. Set rbac.watchAnyResource=false and grant the types you actually +# mirror. See docs/rbac.md. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource + labels: + {{- include "gitops-reverser.labels" . | nindent 4 }} +rules: +- apiGroups: + - '*' + resources: + - '*' + verbs: + - get + - list + - watch +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource + labels: + {{- include "gitops-reverser.labels" . | nindent 4 }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource +subjects: +- kind: ServiceAccount + name: {{ include "gitops-reverser.serviceAccountName" . }} + namespace: {{ .Release.Namespace }} +{{- end }} {{- end }} diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 36be158c4..b67f73cdb 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -211,6 +211,17 @@ attribution: # RBAC configuration rbac: create: true + # Grant cluster-wide read (get/list/watch) on every resource, in a ClusterRole of its own. + # A WatchRule may name any type, including one installed after this release, so the set + # cannot be derived from the code — and RBAC is additive, so this wildcard grants read on + # Secrets too, whatever the manager role says. The operator itself never lists or watches + # Secrets; it reads only the ones a GitProvider/GitTarget names. + # + # Set false once you know which types you mirror, and grant get/list/watch on exactly + # those. The reverser then runs with zero cluster-wide Secret access. It still needs + # namespaces, customresourcedefinitions and apiservices, which the manager role already + # carries. See docs/rbac.md. + watchAnyResource: true # Resource limits and requests resources: diff --git a/cmd/main_chart_rbac_test.go b/cmd/main_chart_rbac_test.go new file mode 100644 index 000000000..59d38526d --- /dev/null +++ b/cmd/main_chart_rbac_test.go @@ -0,0 +1,159 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "os" + "os/exec" + "strings" + "testing" + + "github.com/stretchr/testify/require" + rbacv1 "k8s.io/api/rbac/v1" + "sigs.k8s.io/yaml" +) + +// rbacChartInputs are read in-process so `go test` treats them as cache inputs; helm reads +// the chart in a subprocess the test log cannot see. +var rbacChartInputs = []string{ + "../charts/gitops-reverser/values.yaml", + "../charts/gitops-reverser/templates/rbac.yaml", + "../charts/gitops-reverser/config/role.yaml", +} + +// renderClusterRoles renders the chart's RBAC and returns every ClusterRole it produces. +func renderClusterRoles(t *testing.T, setValues ...string) []rbacv1.ClusterRole { + t.Helper() + + for _, path := range rbacChartInputs { + _, err := os.ReadFile(path) + require.NoErrorf(t, err, "%s missing; `task helm-sync` generates it", path) + } + + args := []string{ + "template", "gitops-reverser", "../charts/gitops-reverser", + "--show-only", "templates/rbac.yaml", + } + for _, sv := range setValues { + args = append(args, "--set", sv) + } + out, err := exec.Command("helm", args...).CombinedOutput() + require.NoErrorf(t, err, "helm template failed: %s", out) + + var roles []rbacv1.ClusterRole + for _, doc := range strings.Split(string(out), "\n---") { + if strings.TrimSpace(doc) == "" { + continue + } + var obj rbacv1.ClusterRole + require.NoError(t, yaml.Unmarshal([]byte(doc), &obj)) + if obj.Kind == "ClusterRole" { + roles = append(roles, obj) + } + } + require.NotEmpty(t, roles, "chart rendered no ClusterRole") + return roles +} + +func covers(values []string, want string) bool { + for _, v := range values { + if v == want || v == "*" { + return true + } + } + return false +} + +// grantsSecretListOrWatch reports the rules that let the holder enumerate Secrets cluster-wide. +// `get` is fine: the operator reads only the Secrets a GitProvider or GitTarget names. +func grantsSecretListOrWatch(role rbacv1.ClusterRole) []rbacv1.PolicyRule { + var bad []rbacv1.PolicyRule + for _, rule := range role.Rules { + if !covers(rule.APIGroups, "") || !covers(rule.Resources, "secrets") { + continue + } + if covers(rule.Verbs, "list") || covers(rule.Verbs, "watch") { + bad = append(bad, rule) + } + } + return bad +} + +func roleNamed(t *testing.T, roles []rbacv1.ClusterRole, suffix string) (rbacv1.ClusterRole, bool) { + t.Helper() + for _, r := range roles { + if strings.HasSuffix(r.Name, suffix) { + return r, true + } + } + return rbacv1.ClusterRole{}, false +} + +// The whole point of splitting the wildcard out: turning it off must leave a role that +// cannot enumerate Secrets. A reverser mirroring two CRDs on a management cluster would +// otherwise hold read access to every credential in it. +func TestChartRBAC_WithoutWildcardTheOperatorCannotEnumerateSecrets(t *testing.T) { + roles := renderClusterRoles(t, "rbac.watchAnyResource=false") + + _, found := roleNamed(t, roles, "-watch-any-resource") + require.False(t, found, "the wildcard ClusterRole must not render when disabled") + + for _, role := range roles { + require.Emptyf(t, grantsSecretListOrWatch(role), + "ClusterRole %q still grants Secret list/watch", role.Name) + } +} + +// The manager role is generated from the kubebuilder markers. It must never carry the +// wildcard itself: RBAC is additive, so a wildcard there would re-grant Secret list/watch +// and no chart value could take it away. +func TestChartRBAC_ManagerRoleNeverCarriesTheWildcard(t *testing.T) { + for _, values := range [][]string{{"rbac.watchAnyResource=true"}, {"rbac.watchAnyResource=false"}} { + roles := renderClusterRoles(t, values...) + manager, found := roleNamed(t, roles, "-manager-role") + require.True(t, found, "manager ClusterRole must always render") + + for _, rule := range manager.Rules { + require.NotContains(t, rule.Resources, "*", + "manager role must not grant wildcard resources (%v)", rule.APIGroups) + } + require.Empty(t, grantsSecretListOrWatch(manager), + "the manager role must never enumerate Secrets; it reads the ones it is pointed at") + } +} + +// The API-resource catalog and its trigger informers read these two. Without them a +// least-privilege install 403s on every reflector retry. +func TestChartRBAC_ManagerRoleGrantsTheAPISurfaceReads(t *testing.T) { + roles := renderClusterRoles(t, "rbac.watchAnyResource=false") + manager, found := roleNamed(t, roles, "-manager-role") + require.True(t, found) + + for _, want := range []struct{ group, resource string }{ + {"apiextensions.k8s.io", "customresourcedefinitions"}, + {"apiregistration.k8s.io", "apiservices"}, + {"", "namespaces"}, + } { + granted := false + for _, rule := range manager.Rules { + if covers(rule.APIGroups, want.group) && covers(rule.Resources, want.resource) && + covers(rule.Verbs, "list") && covers(rule.Verbs, "watch") { + granted = true + break + } + } + require.Truef(t, granted, "manager role must grant list+watch on %s/%s", want.group, want.resource) + } +} + +// Default installs keep working: a WatchRule may name any type, so the wildcard ships on. +func TestChartRBAC_WildcardRoleShipsByDefault(t *testing.T) { + roles := renderClusterRoles(t) + + wildcard, found := roleNamed(t, roles, "-watch-any-resource") + require.True(t, found, "the wildcard ClusterRole must render by default") + require.Len(t, wildcard.Rules, 1) + require.Equal(t, []string{"*"}, wildcard.Rules[0].APIGroups) + require.Equal(t, []string{"*"}, wildcard.Rules[0].Resources) + require.ElementsMatch(t, []string{"get", "list", "watch"}, wildcard.Rules[0].Verbs) +} diff --git a/config/rbac/kustomization.yaml b/config/rbac/kustomization.yaml index 8f754254b..9ce8b7e17 100644 --- a/config/rbac/kustomization.yaml +++ b/config/rbac/kustomization.yaml @@ -3,6 +3,10 @@ kind: Kustomization resources: - sa.yaml - role.yaml + # The wildcard read a WatchRule may need, kept out of role.yaml so it can be dropped whole. + # See docs/rbac.md; the Helm chart gates the same pair on `rbac.watchAnyResource`. + - watch-any-resource-role.yaml + - watch-any-resource-rolebinding.yaml - cluster-role-jane-aggregated.yaml - cluster-rolebinding.yaml - cluster-rolebinding-jane.yaml # E2E-only helper: allows impersonated user jane@acme.com writes in tests. diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 40ca498db..b1f41a800 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -19,14 +19,19 @@ rules: verbs: - create - get - - list - - patch - update +- apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - get + - list - watch - apiGroups: - - '*' + - apiregistration.k8s.io resources: - - '*' + - apiservices verbs: - get - list diff --git a/config/rbac/watch-any-resource-role.yaml b/config/rbac/watch-any-resource-role.yaml new file mode 100644 index 000000000..ba213a582 --- /dev/null +++ b/config/rbac/watch-any-resource-role.yaml @@ -0,0 +1,24 @@ +# The read access the watch manager needs for the types a WatchRule names. +# +# A WatchRule may name any type in the cluster, including types installed after this role, so +# this cannot be derived from the code the way every other rule in config/rbac/role.yaml is. +# It is kept OUT of that role on purpose: RBAC is additive, so a wildcard read here would +# grant cluster-wide Secret list/watch no matter how narrow the Secret rule beside it is. +# +# Drop this role (Helm: `rbac.watchAnyResource: false`) when you know which types the +# reverser will mirror, and grant get/list/watch on exactly those instead. The operator +# needs nothing else from it: it holds no long-lived informers on watched types, and it +# never reads a Secret it was not pointed at by name. +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: gitops-reverser-watch-any-resource +rules: + - apiGroups: + - '*' + resources: + - '*' + verbs: + - get + - list + - watch diff --git a/config/rbac/watch-any-resource-rolebinding.yaml b/config/rbac/watch-any-resource-rolebinding.yaml new file mode 100644 index 000000000..cde3a3903 --- /dev/null +++ b/config/rbac/watch-any-resource-rolebinding.yaml @@ -0,0 +1,15 @@ +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + labels: + app.kubernetes.io/managed-by: kustomize + app.kubernetes.io/name: gitops-reverser + name: gitops-reverser-watch-any-resource +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: gitops-reverser-watch-any-resource +subjects: +- kind: ServiceAccount + name: gitops-reverser + namespace: sut diff --git a/docs/README.md b/docs/README.md index a7d738b2a..ab768e920 100644 --- a/docs/README.md +++ b/docs/README.md @@ -20,6 +20,7 @@ If you only want the supported product docs, start with the files below. - [`sops-age-guide.md`](sops-age-guide.md): Secret encryption with SOPS + age - [`security-model.md`](security-model.md): controller access, trust boundaries, and the Git credentials Secret shape +- [`rbac.md`](rbac.md): the two ClusterRoles, and how to run with zero cluster-wide Secret access - [`bi-directional.md`](bi-directional.md): safe shared-path and handoff patterns - [`alternatives.md`](alternatives.md): nearby tools and when another approach fits better - [`UPGRADING.md`](UPGRADING.md): breaking changes and migration steps, newest first diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index 69a972442..b05410276 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -7,6 +7,44 @@ guidance that the changelog's breaking-change entries link to. We are pre-1.0, so breaking changes bump the **minor** version (release-please is configured with `bump-minor-pre-major`) rather than the major. Read the relevant entry before upgrading across it. +## Unreleased — the wildcard cluster read moved to its own, droppable ClusterRole (next minor; behavior change) + +The shipped RBAC now says what the binary actually does. Nothing is taken away from a default +install; what changes is that the parts can be separated. + +**The manager ClusterRole no longer contains `apiGroups: ["*"], resources: ["*"]`.** That rule now +lives in a ClusterRole of its own, `-watch-any-resource`, created by default +(`rbac.watchAnyResource: true`) and bound to the same ServiceAccount. A `WatchRule` may name any +type, so the default install keeps the same effective permissions. The split exists because RBAC is +additive: while the wildcard sat in the manager role, no chart value could remove the cluster-wide +Secret read it implied. + +**The manager role's `secrets` rule narrowed from `get,list,watch,create,update,patch` to +`get,create,update`.** The operator has held no Secret informer since `v0.31.0` — Secrets are +excluded from the manager cache, so every read is a direct `get` of a Secret a `GitProvider` or +`GitTarget` names — but the marker was never updated to match. It never used `list`, `watch` or +`patch`. + +**The manager role gained explicit `get,list,watch` on `customresourcedefinitions` and +`apiservices`.** They were previously reachable only through the wildcard. The API-resource catalog +and its trigger informers read both. + +**Migration** + +- Default installs (`rbac.watchAnyResource` unset): no action. Helm creates the extra ClusterRole + and binding; `kubectl apply` of `dist/install.yaml` includes them. +- To run least-privilege, set `rbac.watchAnyResource: false` and grant `get,list,watch` on the types + your `WatchRule`s name. See [`rbac.md`](rbac.md). +- If you hand-wrote a role because the shipped one was too broad, you can now drop the parts that + duplicate `-manager-role`. + +**Related behavior change.** A trigger informer (`customresourcedefinitions`, `apiservices`) that +the API server serves but RBAC denies is now **stopped** after the first `403`, logged once, and +re-armed automatically on a later catalog refresh if the permission is granted. Previously the +reflector retried the denial forever. Discovery reports what the server serves, not what the caller +may read, so a narrowed role reached this path — which is exactly the path this release makes easy +to enter. + ## Unreleased — `manifest-analyzer` scan modes renamed, and `--format json` now emits a versioned contract (next minor; breaking) The analyzer's machine-readable output moved to the new public diff --git a/docs/rbac.md b/docs/rbac.md new file mode 100644 index 000000000..57ced4b5f --- /dev/null +++ b/docs/rbac.md @@ -0,0 +1,85 @@ +# RBAC: running the reverser without trusting it with the cluster + +The reverser ships able to read every object in the cluster, because a `WatchRule` may name +any type — including a type installed after the operator. That default is honest, not +required. This page shows how to run it with the read access it actually uses, which on a +management cluster is the difference between "mirrors two CRDs" and "can read every +credential we own". + +## The two ClusterRoles + +| ClusterRole | Contents | Where it comes from | +|---|---|---| +| `-manager-role` | Exactly what the binary calls: its own CRs, `namespaces`, `customresourcedefinitions`, `apiservices`, and `secrets` with **`get`, `create`, `update`** | Generated from the kubebuilder markers into [`config/rbac/role.yaml`](../config/rbac/role.yaml). Never edit by hand. | +| `-watch-any-resource` | `apiGroups: ["*"], resources: ["*"], verbs: ["get","list","watch"]` | Hand-maintained: [`config/rbac/watch-any-resource-role.yaml`](../config/rbac/watch-any-resource-role.yaml), or the chart's `rbac.watchAnyResource` (default `true`). | + +They are separate because **RBAC is additive**. A wildcard read folded into the manager role +would grant cluster-wide Secret `list`/`watch` no matter how narrow the Secret rule beside +it is, and no chart value could take it back. Split out, the wildcard can be dropped whole. + +## The operator does not read Secrets wholesale + +It holds **no Secret informer**: Secrets are excluded from the manager's cache +([`cmd/main.go`](../cmd/main.go), `Cache.DisableFor`), so every read is a direct `get` of a +Secret a `GitProvider` or `GitTarget` names. Nothing in the operator calls `list` or `watch` +on Secrets. It creates and updates only the Secrets it owns (the generated signing key). + +That is why the manager role asks for `get`, `create`, `update` and nothing more. + +## Running least-privilege + +Set `rbac.watchAnyResource: false` and grant read on the types you actually mirror: + +```yaml +# helm values +rbac: + create: true + watchAnyResource: false +``` + +```yaml +# your own ClusterRole, bound to the reverser's ServiceAccount +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: gitops-reverser-watched-types +rules: + # exactly the types your WatchRules name + - apiGroups: ["gitops-api.configbutler.ai"] + resources: ["tenants", "reposelections"] + verbs: ["get", "list", "watch"] +``` + +The manager role already carries `namespaces`, `customresourcedefinitions` and `apiservices`, +so you do not restate them. Bind your role to the same ServiceAccount, alongside the manager +role's binding. + +Result: `GitProvider` Ready, `ClusterWatchRule` Ready with its streams, commits flowing, and +**zero cluster-wide Secret access**. + +If the reverser needs a Git credentials Secret in its own namespace, that is a namespaced +`Role`, not a `ClusterRole` — the manager role's `get` on `secrets` is cluster-scoped only +because a `GitProvider` may point at any namespace. A namespaced binding is enough when it +does not. + +## What happens if a read is denied + +Discovery answers what the API **server serves**, not what this ServiceAccount may **read**. +An ordinary apiserver serves `apiregistration.k8s.io` whether or not your ClusterRole names +`apiservices`, so a narrowed role sails past the "is it served?" check and then gets a `403` +on the informer's first `LIST`. + +For the two **API-surface trigger** informers (`customresourcedefinitions`, `apiservices`) +the operator treats a `403` as authoritative: it stops that informer, logs once, and falls +back to refreshing its API-resource catalog on the periodic tick. It does **not** retry the +denial forever, which would bury real errors under benign noise. Grant the permission later +and the next catalog refresh re-arms the informer — no restart. + +These two are conveniences: they only make the catalog notice a new CRD or aggregated API +sooner than the periodic refresh would. Everything else keeps working without them, which is +why failing closed and quiet is the right answer to "you may not read this". + +A denied **watched type** is different: that is the job you asked for, not a convenience, so +the operator keeps retrying it rather than quietly dropping it. Its watch stream errors and +the manager retries with backoff. If you narrow the role, make sure every type your +`WatchRule`s name is granted, and check the operator logs after the change. diff --git a/internal/controller/gitprovider_controller.go b/internal/controller/gitprovider_controller.go index 3ac2146f6..cbd19464e 100644 --- a/internal/controller/gitprovider_controller.go +++ b/internal/controller/gitprovider_controller.go @@ -54,7 +54,7 @@ type gitProviderLogFirsts struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders/finalizers,verbs=update -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update // Reconcile is part of the main kubernetes reconciliation loop which aims to // move the current state of the cluster closer to the desired state. diff --git a/internal/controller/gittarget_controller.go b/internal/controller/gittarget_controller.go index 96a757fd2..9e68dda54 100644 --- a/internal/controller/gittarget_controller.go +++ b/internal/controller/gittarget_controller.go @@ -107,7 +107,7 @@ type GitTargetReconciler struct { // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=configbutler.ai,resources=gittargets/status,verbs=get;update;patch // +kubebuilder:rbac:groups=configbutler.ai,resources=gitproviders,verbs=get;list;watch -// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;create;update // Reconcile validates GitTarget references and drives startup lifecycle gates. // From ccd1cddaf8d3c9522e7c8ec35cbb9eb941864da3 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 07:57:54 +0000 Subject: [PATCH 3/8] fix(watch): a watch closed by shutdown is not a watch error MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Teardown closes the watch's result channel AND cancels the context, so both arms of the streaming select are ready at once — and Go picks among ready select cases at random. Roughly half of all clean shutdowns therefore returned "target watch result channel closed" instead of nil. Nothing retried on it, but every teardown could log a failure that had not happened, and a log that cries wolf on every shutdown is a log nobody reads. The context is the authority: the channel closing while ctx is done means we closed it. A channel that closes while the context is still live is still a real error — the watch died under us and the caller must reopen — so that path is unchanged, and pinned by its own test. Both streaming loops (the replay session and the live stream) shared the bug and now share targetWatchClosedErr. Found while stress-testing the API-surface trigger informers: this surfaced as an intermittent failure of TestTargetWatchReplayAndStream_FallsBackWhenReplayWatchIsForbidden under -race, which reproduces on main and is not caused by that change. `task test` does not pass -race, so CI never saw it. The regression test repeats 200 times because a coin flip proves nothing once. Co-Authored-By: Claude Opus 4.8 (1M context) --- .coverage-baseline | 2 +- internal/watch/target_watch.go | 17 +++++- internal/watch/target_watch_shutdown_test.go | 58 ++++++++++++++++++++ 3 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 internal/watch/target_watch_shutdown_test.go diff --git a/.coverage-baseline b/.coverage-baseline index add296677..641c13bc5 100644 --- a/.coverage-baseline +++ b/.coverage-baseline @@ -1 +1 @@ -75.4 +75.5 diff --git a/internal/watch/target_watch.go b/internal/watch/target_watch.go index 919c5ead5..4421c6da2 100644 --- a/internal/watch/target_watch.go +++ b/internal/watch/target_watch.go @@ -38,6 +38,19 @@ var ( errTargetWatchExpired = errors.New("target watch resourceVersion expired") ) +// targetWatchClosedErr distinguishes a watch that died under us from one that closed because +// we are shutting down. Teardown closes the result channel and cancels the context, leaving +// BOTH select cases ready — and select picks among ready cases at random, so the shutdown +// path returned a spurious error roughly half the time. The context is the authority. +func targetWatchClosedErr(ctx context.Context) error { + select { + case <-ctx.Done(): + return nil + default: + return errTargetWatchClosed + } +} + type targetWatchSet struct { cancel context.CancelFunc specs map[targetWatchKey]string @@ -346,7 +359,7 @@ func (m *Manager) targetWatchReplayAndStream( return nil case ev, ok := <-w.ResultChan(): if !ok { - return errTargetWatchClosed + return targetWatchClosedErr(ctx) } nextReplaying, err := m.handleTargetWatchSessionEvent( ctx, log, gitDest, key, ops, ev, replaying, &replay, @@ -605,7 +618,7 @@ func (m *Manager) streamLiveTargetWatchEvents( return nil case ev, ok := <-events: if !ok { - return errTargetWatchClosed + return targetWatchClosedErr(ctx) } if targetWatchEventAtOrBeforeFloor(ev, floor) { continue diff --git a/internal/watch/target_watch_shutdown_test.go b/internal/watch/target_watch_shutdown_test.go new file mode 100644 index 000000000..d6c24df38 --- /dev/null +++ b/internal/watch/target_watch_shutdown_test.go @@ -0,0 +1,58 @@ +// SPDX-License-Identifier: Apache-2.0 + +package watch + +import ( + "context" + "testing" + + "github.com/go-logr/logr" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/watch" + + "github.com/ConfigButler/gitops-reverser/internal/types" +) + +// A watch whose result channel closes because the operator is shutting down has not failed. +// Both the ctx.Done() case and the closed-channel case are ready at once, and Go's select +// picks between ready cases at random — so a clean shutdown reported a spurious +// "target watch result channel closed" about half the time. Nothing retried on it, but it +// surfaced as an error on every teardown, and it is the kind of noise that trains an +// operator to ignore the log. +// +// Repeated because the bug is a coin flip per iteration: one run proves nothing. +func TestStreamLiveTargetWatchEvents_CanceledContextIsNotAnError(t *testing.T) { + t.Parallel() + + m := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("target", "default") + key := targetWatchKey{GVR: configmapsGVR, Namespace: "apps"} + + for i := range 200 { + ctx, cancel := context.WithCancel(context.Background()) + cancel() // shutting down + + events := make(chan watch.Event) + close(events) // ...which is why the watch closed + + err := m.streamLiveTargetWatchEvents(ctx, logr.Discard(), gitDest, key, OperationSet{}, events) + require.NoErrorf(t, err, "iteration %d: a closed watch during shutdown is not an error", i) + } +} + +// The same channel closing while the operator is still running IS an error: the watch died +// under it, and the caller must reopen rather than silently stop mirroring the type. +func TestStreamLiveTargetWatchEvents_ClosedChannelWhileRunningIsAnError(t *testing.T) { + t.Parallel() + + m := &Manager{Log: logr.Discard()} + gitDest := types.NewResourceReference("target", "default") + key := targetWatchKey{GVR: configmapsGVR, Namespace: "apps"} + + events := make(chan watch.Event) + close(events) + + err := m.streamLiveTargetWatchEvents( + context.Background(), logr.Discard(), gitDest, key, OperationSet{}, events) + require.ErrorIs(t, err, errTargetWatchClosed) +} From cd3feb3089b355c59056f75ec118dda2bfe5580e Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 08:18:15 +0000 Subject: [PATCH 4/8] docs(review): align help text, chart docs and the f8 doc with what the code does Review polish from the merged #221, plus two things it turned up. --redis-addr's help said an empty value was "Incompatible with --author-attribution=true or --admission-webhook". Only the first half is true. validateAuditConfig requires Redis for attribution and returns nil otherwise, and validateAdmissionWebhookConfig says so in a comment: the admission webhook stays enabled without Redis and no-ops command-author capture, so CommitRequests commit as the configured committer. The chart README already documented the real behavior; the flag help was the drift. Pinned now by a flag-combination test, which also caught that --admission-webhook-cert-path has no default and is required whenever the webhook is on -- a TLS requirement, not a Redis one. watchCursorKey built its key from the raw s.keyPrefix while every other key family resolves it at use. Only NewRedisStore constructs a RedisStore today, so nothing was mis-keyed, but the point of resolveKeyPrefix is that a store built any other way still writes under the default root rather than into an unprefixed keyspace indistinguishable from another tool's keys. Made consistent before the key contract freezes, with a zero-value-store test that fails without it. f8's closing line linked ../multi-tenant/README.md, which does not exist here -- those design docs live on the multi-tenant branch. Dropped the reference rather than pointing at a file this repo does not have. docs/README.md indexed docs/serious-bug/, deleted in 7bd1f32 and never unindexed: same bug, removed too. Also: the chart README gains queue.redis.keyPrefix, queue.redis.db, rbac.create and rbac.watchAnyResource rows; docs/configuration.md explains the prefix after the Redis example; and the chart-args contract test gains attribution-enabled cases, so the audit flag block the default path never renders is parsed by the real flag parser too. Co-Authored-By: Claude Opus 4.8 (1M context) --- charts/gitops-reverser/README.md | 4 ++ cmd/main.go | 5 +- cmd/main_chart_args_test.go | 39 +++++++++++-- cmd/main_redis_flags_test.go | 56 +++++++++++++++++++ docs/README.md | 1 - docs/configuration.md | 5 ++ .../f8-repo-discovery-and-onboarding-scan.md | 2 +- internal/queue/key_prefix_test.go | 14 +++++ internal/queue/redis_store.go | 2 +- 9 files changed, 118 insertions(+), 10 deletions(-) create mode 100644 cmd/main_redis_flags_test.go diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index a52c8191a..e06da5093 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -185,11 +185,15 @@ nodeSelector: | `queue.redis.auth.existingSecret` | Name of a pre-created Secret holding the Redis password (only used when `queue.redis.addr` is set) | `""` | | `queue.redis.auth.existingSecretKey` | Key within the Secret that holds the password | `password` | | `queue.redis.auth.username` | Optional Redis ACL username | `""` | +| `queue.redis.db` | Redis logical database index. Redis offers 16; use `queue.redis.keyPrefix` to go past that | `0` | +| `queue.redis.keyPrefix` | Root of every key this release writes (watch cursors, attribution facts, command author records). Give each reverser its own prefix to share one Redis/Valkey between more reversers than `db` can separate. Changing it orphans the previous prefix's keys: cursors cold-replay once, which is safe. Allowed: `[A-Za-z0-9]`, `-`, `_`, `.`, `:` | `gitops-reverser` | | `queue.redis.tls.enabled` | Enable TLS for Redis connection | `false` | | `attribution.enabled` | Run audit ingress and name mirrored-resource commit authors from matching kube-apiserver audit facts | `false` | | `attribution.ttl` | How long an attribution fact is retained waiting for the matching watch event to join it | `10m` | | `attribution.grace` | Bounded per-event wait for a matching audit fact before a watch event ships as the committer | `3s` | | `servers.admission.enabled` | Install the validate-operator-types admission webhook that captures CommitRequest authors (a form of author attribution). Enabled by default; a no-op until `queue.redis.addr` is set | `true` | +| `rbac.create` | Create the manager ClusterRole and its binding | `true` | +| `rbac.watchAnyResource` | Also create `-watch-any-resource`, a ClusterRole granting `get,list,watch` on every resource. A `WatchRule` may name any type, so this ships on. Set `false` and grant only the types you mirror to run with zero cluster-wide Secret access — see [`docs/rbac.md`](../../docs/rbac.md) | `true` | | `servers.metrics.bindAddress` | Metrics listener bind address | `:8080` | | `servers.metrics.tls.enabled` | Serve metrics with TLS | `false` | | `servers.metrics.tls.certPath` | Metrics TLS certificate mount path | `/tmp/k8s-metrics-server/metrics-server-certs` | diff --git a/cmd/main.go b/cmd/main.go index 6a8e4511f..4ccc8ef9e 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -423,8 +423,9 @@ func parseFlagsWithArgs(fs *flag.FlagSet, args []string) (appConfig, error) { fs.StringVar(&cfg.redisAddr, "redis-addr", "valkey:6379", "Redis/Valkey address (host:port). Holds each GitTarget's watch resume cursors (state continuity) "+ "and, when author attribution is enabled, the attribution facts. Leave empty to run without "+ - "Redis: watches cold-replay on restart instead of resuming. Incompatible with "+ - "--author-attribution=true or --admission-webhook.") + "Redis: watches cold-replay on restart instead of resuming. Required by "+ + "--author-attribution=true. --admission-webhook still runs without it, but command-author "+ + "capture is a no-op: CommitRequests commit as the configured committer.") fs.BoolVar(&cfg.authorAttribution, "author-attribution", true, "Name the real actor (human or service account) who caused each change as the Git commit author, "+ "resolved from matching audit facts; this runs the audit webhook ingress (default true). When "+ diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index a4fc88675..bc13f440a 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -60,11 +60,13 @@ func renderManagerArgs(t *testing.T, setValues ...string) []string { // lints and packages cleanly. Parse what the chart actually emits. func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { tests := map[string]struct { - setValues []string - wantKeyPrefix string - wantRedisAddr string - wantRedisDBSet int - wantUsername string + setValues []string + wantKeyPrefix string + wantRedisAddr string + wantRedisDBSet int + wantUsername string + wantAttribution bool + wantAuditTLSCert string }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", @@ -85,6 +87,29 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantRedisDBSet: 3, wantUsername: "reverser", }, + // Attribution renders a whole block of audit-server flags the default path never emits, + // and it is the mode that validateAuditConfig scrutinises. Render it and parse it too. + "attribution enabled with audit TLS": { + setValues: []string{ + "attribution.enabled=true", + "queue.redis.addr=redis.example.com:6379", + }, + wantKeyPrefix: "gitops-reverser", + wantRedisAddr: "redis.example.com:6379", + wantAttribution: true, + wantAuditTLSCert: "/tmp/k8s-audit-server/audit-server-certs", + }, + // Attribution without audit TLS takes the --audit-insecure branch instead. + "attribution enabled without audit TLS": { + setValues: []string{ + "attribution.enabled=true", + "queue.redis.addr=redis.example.com:6379", + "servers.audit.tls.enabled=false", + }, + wantKeyPrefix: "gitops-reverser", + wantRedisAddr: "redis.example.com:6379", + wantAttribution: true, + }, } for name, tc := range tests { @@ -103,6 +128,10 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { // A quoted-value render would reach Redis as `"reverser"` and fail ACL auth. require.Equal(t, tc.wantUsername, cfg.redisUsername) } + require.Equal(t, tc.wantAttribution, cfg.authorAttribution) + if tc.wantAuditTLSCert != "" { + require.Equal(t, tc.wantAuditTLSCert, cfg.auditCertPath) + } }) } } diff --git a/cmd/main_redis_flags_test.go b/cmd/main_redis_flags_test.go new file mode 100644 index 000000000..c2487750e --- /dev/null +++ b/cmd/main_redis_flags_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +package main + +import ( + "flag" + "testing" + + "github.com/stretchr/testify/require" +) + +func parseArgs(t *testing.T, args ...string) (appConfig, error) { + t.Helper() + return parseFlagsWithArgs(flag.NewFlagSet("manager", flag.ContinueOnError), args) +} + +// The --redis-addr help text is a promise about which combinations start. Keep it true. +func TestParseFlags_RedisAddrIsOnlyRequiredByAttribution(t *testing.T) { + tests := map[string]struct { + args []string + wantErr string + }{ + "attribution needs redis": { + args: []string{"--redis-addr=", "--author-attribution=true"}, + wantErr: "redis-addr is required when author-attribution is enabled", + }, + // The webhook is failurePolicy: Ignore by design and the controller is the real gate, + // so running it without Redis is a supported, degraded mode — not a usage error. + // (--admission-webhook-cert-path has no default and is required whenever the webhook + // is on; that requirement is about TLS, not about Redis.) + "admission webhook runs without redis": { + args: []string{ + "--redis-addr=", "--author-attribution=false", + "--admission-webhook", "--admission-webhook-cert-path=/tmp/certs", + }, + }, + "admission webhook still requires its cert path": { + args: []string{"--redis-addr=", "--author-attribution=false", "--admission-webhook"}, + wantErr: "admission-webhook-cert-path is required", + }, + "configured-author runs without redis": { + args: []string{"--redis-addr=", "--author-attribution=false"}, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + _, err := parseArgs(t, tc.args...) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + }) + } +} diff --git a/docs/README.md b/docs/README.md index ab768e920..9aafa5aca 100644 --- a/docs/README.md +++ b/docs/README.md @@ -38,7 +38,6 @@ docs surface: kept for historical context - [`ci/`](ci/): CI/devcontainer rationale and troubleshooting - [`future/`](future/): ideas that are intentionally deferred -- [`serious-bug/`](serious-bug/): worked bug reports against upstream projects - [`audit-setup/`](audit-setup/): cluster-specific audit delivery notes and examples These root-level files are also working notes rather than polished user docs: diff --git a/docs/configuration.md b/docs/configuration.md index c06b68ccc..0f268b6f2 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -695,6 +695,11 @@ queue: existingSecretKey: "password" ``` +Every key this operator writes is rooted at `queue.redis.keyPrefix` (`--redis-key-prefix`, default +`gitops-reverser`). Give each reverser its own prefix when several share one Redis/Valkey: `--redis-db` +separates only 16 logical databases, and one reverser per tenant or per branch environment passes that +long before it reaches any real Redis limit. + When attribution is enabled, these flags tune the join: - `--author-attribution-ttl` (default `10m`): how long an attribution fact is retained waiting for the diff --git a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md index 0bd46d55a..fa4d425ae 100644 --- a/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md +++ b/docs/design/gitops-api/f8-repo-discovery-and-onboarding-scan.md @@ -257,7 +257,7 @@ followed — the same posture as `ScanDir`, over the whole tree. It reuses `coll A product layer consuming this from Go imports [`pkg/manifestanalyzer`](../../../pkg/manifestanalyzer) — `ScanRepo` and `ScanFolder` — rather than exec'ing the binary. That package is the supported, versioned projection of the reports below; the engine above stays internal and -free to move. See [../multi-tenant/README.md](../multi-tenant/README.md) item 4. +free to move. **Report shape as shipped**, refining the sketch above. Per candidate: `path`, `layout`, `acceptedByOperator`, `refusalReasons[]` (`{code, detail}`), `renderRoot`, `readScope[]`, diff --git a/internal/queue/key_prefix_test.go b/internal/queue/key_prefix_test.go index 44d1ae2fd..6294ee95b 100644 --- a/internal/queue/key_prefix_test.go +++ b/internal/queue/key_prefix_test.go @@ -108,6 +108,20 @@ func TestRedisStore_DefaultPrefixIsUnchanged(t *testing.T) { require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) } +// A store built without NewRedisStore — a zero-value struct in a test, a future constructor +// that forgets to normalize — must still write under the default root rather than into an +// unprefixed keyspace indistinguishable from another tool's keys. Every key family resolves +// the prefix at use, not only at construction; watch cursors were the one that did not. +func TestRedisStore_ZeroValueStoreStillWritesPrefixedKeys(t *testing.T) { + t.Parallel() + + var store RedisStore // keyPrefix == "" + require.Equal(t, "gitops-reverser:watch:v1:target:gtuid-3:configmaps:cluster:last-rv", + store.watchCursorKey("gtuid-3", coreConfigmapsGVR(), "")) + require.Equal(t, "gitops-reverser:author:v1:command:cr-uid", store.CommandAuthorStore().key("cr-uid")) + require.Equal(t, "gitops-reverser:author:v1:audit:", store.AttributionIndex(0).factKeyBase("")) +} + // Two reversers sharing one Redis/Valkey and one logical database must not read each // other's cursors. Sixteen databases is the only separator Redis offers; a prefix is not // bounded that way. diff --git a/internal/queue/redis_store.go b/internal/queue/redis_store.go index cd369ff70..6a7500d88 100644 --- a/internal/queue/redis_store.go +++ b/internal/queue/redis_store.go @@ -150,6 +150,6 @@ func (s *RedisStore) watchCursorKey( if namespace != "" { scope = "namespace:" + escapeKeyField(namespace) } - return s.keyPrefix + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + + return resolveKeyPrefix(s.keyPrefix) + watchCursorKeySuffix + "target:" + escapeKeyField(gitTargetUID) + ":" + groupResourceKey(gvr.Group, gvr.Resource) + ":" + scope + ":last-rv" } From 3a49109353d5111921adc1bf410289145033225a Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 08:25:44 +0000 Subject: [PATCH 5/8] feat(rbac): choose watched types with rbac.watchTypes.mode any|selected The opt-out boolean told a user the wildcard was bad without helping them leave it: they still had to hand-write a ClusterRole, bind it to the right ServiceAccount, and know which rules the manager role already carries. Most would keep the wildcard. rbac: watchTypes: mode: any # cluster-wide read; the reverser can read every Secret selected: [] # mode: selected -> read exactly these types, nothing else The chart renders the ClusterRole and binding for both modes, so least privilege is a value change rather than a piece of RBAC to maintain and keep in sync with the ServiceAccount. `any` renders exactly the wildcard rule the boolean did, so the default install is unchanged. Verbs are never the user's to choose. A watched type is only ever read, so the chart fixes get/list/watch and refuses a `verbs:` key rather than let someone grant `create` to something that will never write. It also refuses an unknown mode, and `selected` with an empty list -- which would leave the operator authorized to watch nothing, a silent failure that looks exactly like a broken WatchRule. The values comment now names the consequence of `any` instead of describing the mechanism: the reverser can read every Secret in the cluster, including credentials that have nothing to do with it. The commented example follows configmaps and deployments, which is what someone reading it is most likely to want. Renames the kustomize pair to gitops-reverser-watch-any to match the chart's -watch-. Both never shipped -- rbac.watchAnyResource was introduced earlier on this branch -- so this replaces it outright rather than deprecating it. Co-Authored-By: Claude Opus 4.8 (1M context) --- charts/gitops-reverser/README.md | 3 +- charts/gitops-reverser/templates/rbac.yaml | 48 ++++-- charts/gitops-reverser/values.yaml | 27 ++-- cmd/main_chart_rbac_test.go | 153 ++++++++++++++---- config/rbac/kustomization.yaml | 6 +- ...resource-role.yaml => watch-any-role.yaml} | 4 +- ...inding.yaml => watch-any-rolebinding.yaml} | 4 +- docs/UPGRADING.md | 36 +++-- docs/rbac.md | 68 ++++---- internal/watch/manager.go | 6 +- 10 files changed, 255 insertions(+), 100 deletions(-) rename config/rbac/{watch-any-resource-role.yaml => watch-any-role.yaml} (86%) rename config/rbac/{watch-any-resource-rolebinding.yaml => watch-any-rolebinding.yaml} (78%) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index e06da5093..c91025daa 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -193,7 +193,8 @@ nodeSelector: | `attribution.grace` | Bounded per-event wait for a matching audit fact before a watch event ships as the committer | `3s` | | `servers.admission.enabled` | Install the validate-operator-types admission webhook that captures CommitRequest authors (a form of author attribution). Enabled by default; a no-op until `queue.redis.addr` is set | `true` | | `rbac.create` | Create the manager ClusterRole and its binding | `true` | -| `rbac.watchAnyResource` | Also create `-watch-any-resource`, a ClusterRole granting `get,list,watch` on every resource. A `WatchRule` may name any type, so this ships on. Set `false` and grant only the types you mirror to run with zero cluster-wide Secret access — see [`docs/rbac.md`](../../docs/rbac.md) | `true` | +| `rbac.watchTypes.mode` | Which types a `WatchRule` may read. `any` grants cluster-wide read on everything — convenient, but the reverser can then read every Secret in the cluster. `selected` grants read on `rbac.watchTypes.selected` only: zero cluster-wide Secret access. See [`docs/rbac.md`](../../docs/rbac.md) | `any` | +| `rbac.watchTypes.selected` | Types to grant when `mode: selected`, as `{apiGroups, resources}` entries (verbs are always `get,list,watch`). Required and non-empty in that mode; `namespaces`, `customresourcedefinitions` and `apiservices` come from the manager role and must not be restated | `[]` | | `servers.metrics.bindAddress` | Metrics listener bind address | `:8080` | | `servers.metrics.tls.enabled` | Serve metrics with TLS | `false` | | `servers.metrics.tls.certPath` | Metrics TLS certificate mount path | `/tmp/k8s-metrics-server/metrics-server-certs` | diff --git a/charts/gitops-reverser/templates/rbac.yaml b/charts/gitops-reverser/templates/rbac.yaml index 95562d64d..a5f3ed7ff 100644 --- a/charts/gitops-reverser/templates/rbac.yaml +++ b/charts/gitops-reverser/templates/rbac.yaml @@ -28,20 +28,28 @@ subjects: - kind: ServiceAccount name: {{ include "gitops-reverser.serviceAccountName" . }} namespace: {{ .Release.Namespace }} -{{- if .Values.rbac.watchAnyResource }} +{{- $watchTypes := .Values.rbac.watchTypes | default dict }} +{{- $mode := $watchTypes.mode | default "any" }} +{{- if not (has $mode (list "any" "selected")) }} +{{- fail (printf "rbac.watchTypes.mode must be \"any\" or \"selected\", got %q" $mode) }} +{{- end }} +{{- $selected := $watchTypes.selected | default list }} +{{- if and (eq $mode "selected") (not $selected) }} +{{- fail "rbac.watchTypes.mode=selected needs at least one entry in rbac.watchTypes.selected, or the reverser can watch nothing" }} +{{- end }} --- -# A WatchRule may name any type, including types installed later, so this cannot be derived -# from the code. It is a separate ClusterRole because RBAC is additive: folded into the -# manager role, this wildcard would grant cluster-wide Secret list/watch however narrow the -# Secret rule beside it is. Set rbac.watchAnyResource=false and grant the types you actually -# mirror. See docs/rbac.md. +# The read access a WatchRule needs. Separate from the manager role because RBAC is additive: +# folded in, a wildcard here would grant cluster-wide Secret list/watch however narrow the +# Secret rule beside it is. Verbs are always get/list/watch — the reverser never writes to a +# watched type. See docs/rbac.md. apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} labels: {{- include "gitops-reverser.labels" . | nindent 4 }} rules: +{{- if eq $mode "any" }} - apiGroups: - '*' resources: @@ -50,20 +58,40 @@ rules: - get - list - watch +{{- else }} +{{- range $i, $rule := $selected }} +{{- if not $rule.apiGroups }} +{{- fail (printf "rbac.watchTypes.selected[%d] needs apiGroups (use [\"\"] for the core group)" $i) }} +{{- end }} +{{- if not $rule.resources }} +{{- fail (printf "rbac.watchTypes.selected[%d] needs resources" $i) }} +{{- end }} +{{- if $rule.verbs }} +{{- fail (printf "rbac.watchTypes.selected[%d] must not set verbs; the reverser only ever reads a watched type" $i) }} +{{- end }} +- apiGroups: + {{- toYaml $rule.apiGroups | nindent 4 }} + resources: + {{- toYaml $rule.resources | nindent 4 }} + verbs: + - get + - list + - watch +{{- end }} +{{- end }} --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding metadata: - name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} labels: {{- include "gitops-reverser.labels" . | nindent 4 }} roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: {{ include "gitops-reverser.fullname" . }}-watch-any-resource + name: {{ include "gitops-reverser.fullname" . }}-watch-{{ $mode }} subjects: - kind: ServiceAccount name: {{ include "gitops-reverser.serviceAccountName" . }} namespace: {{ .Release.Namespace }} {{- end }} -{{- end }} diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index b67f73cdb..178a1bc5a 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -211,17 +211,24 @@ attribution: # RBAC configuration rbac: create: true - # Grant cluster-wide read (get/list/watch) on every resource, in a ClusterRole of its own. - # A WatchRule may name any type, including one installed after this release, so the set - # cannot be derived from the code — and RBAC is additive, so this wildcard grants read on - # Secrets too, whatever the manager role says. The operator itself never lists or watches - # Secrets; it reads only the ones a GitProvider/GitTarget names. + # Which types the reverser may read (get/list/watch). It never writes to a watched type. # - # Set false once you know which types you mirror, and grant get/list/watch on exactly - # those. The reverser then runs with zero cluster-wide Secret access. It still needs - # namespaces, customresourcedefinitions and apiservices, which the manager role already - # carries. See docs/rbac.md. - watchAnyResource: true + # any cluster-wide read on everything. Convenient — a WatchRule can name any type, + # even one installed later — but it means the reverser can read every Secret in + # the cluster, including credentials that have nothing to do with it. + # selected read only the types you list. Least privilege: zero cluster-wide Secret + # access. A WatchRule naming an ungranted type will fail to watch it. + # + # `selected` entries take apiGroups and resources; verbs are always get/list/watch. + # namespaces, customresourcedefinitions and apiservices come from the manager role, so do + # not restate them. See docs/rbac.md. + watchTypes: + mode: any # any | selected + selected: [] + # - apiGroups: [""] # core group + # resources: ["configmaps"] + # - apiGroups: ["apps"] + # resources: ["deployments"] # Resource limits and requests resources: diff --git a/cmd/main_chart_rbac_test.go b/cmd/main_chart_rbac_test.go index 59d38526d..950c6e897 100644 --- a/cmd/main_chart_rbac_test.go +++ b/cmd/main_chart_rbac_test.go @@ -21,6 +21,27 @@ var rbacChartInputs = []string{ "../charts/gitops-reverser/config/role.yaml", } +// selectedConfigMapsAndDeployments is the least-privilege example the chart ships commented +// out: the core group's configmaps plus apps/deployments, and nothing else. +var selectedConfigMapsAndDeployments = []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=", // "" is the core group + "rbac.watchTypes.selected[0].resources[0]=configmaps", + "rbac.watchTypes.selected[1].apiGroups[0]=apps", + "rbac.watchTypes.selected[1].resources[0]=deployments", +} + +func helmTemplateRBAC(setValues ...string) ([]byte, error) { + args := []string{ + "template", "gitops-reverser", "../charts/gitops-reverser", + "--show-only", "templates/rbac.yaml", + } + for _, sv := range setValues { + args = append(args, "--set", sv) + } + return exec.Command("helm", args...).CombinedOutput() +} + // renderClusterRoles renders the chart's RBAC and returns every ClusterRole it produces. func renderClusterRoles(t *testing.T, setValues ...string) []rbacv1.ClusterRole { t.Helper() @@ -30,14 +51,7 @@ func renderClusterRoles(t *testing.T, setValues ...string) []rbacv1.ClusterRole require.NoErrorf(t, err, "%s missing; `task helm-sync` generates it", path) } - args := []string{ - "template", "gitops-reverser", "../charts/gitops-reverser", - "--show-only", "templates/rbac.yaml", - } - for _, sv := range setValues { - args = append(args, "--set", sv) - } - out, err := exec.Command("helm", args...).CombinedOutput() + out, err := helmTemplateRBAC(setValues...) require.NoErrorf(t, err, "helm template failed: %s", out) var roles []rbacv1.ClusterRole @@ -89,14 +103,14 @@ func roleNamed(t *testing.T, roles []rbacv1.ClusterRole, suffix string) (rbacv1. return rbacv1.ClusterRole{}, false } -// The whole point of splitting the wildcard out: turning it off must leave a role that -// cannot enumerate Secrets. A reverser mirroring two CRDs on a management cluster would +// The whole point of splitting the wildcard out: `selected` mode must leave a role set that +// cannot enumerate Secrets. A reverser mirroring two types on a management cluster would // otherwise hold read access to every credential in it. -func TestChartRBAC_WithoutWildcardTheOperatorCannotEnumerateSecrets(t *testing.T) { - roles := renderClusterRoles(t, "rbac.watchAnyResource=false") +func TestChartRBAC_SelectedModeCannotEnumerateSecrets(t *testing.T) { + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) - _, found := roleNamed(t, roles, "-watch-any-resource") - require.False(t, found, "the wildcard ClusterRole must not render when disabled") + _, found := roleNamed(t, roles, "-watch-any") + require.False(t, found, "the wildcard ClusterRole must not render in selected mode") for _, role := range roles { require.Emptyf(t, grantsSecretListOrWatch(role), @@ -104,28 +118,57 @@ func TestChartRBAC_WithoutWildcardTheOperatorCannotEnumerateSecrets(t *testing.T } } +// selected grants exactly the listed types, read-only. Anything more is a privilege the user +// did not ask for; anything less and the WatchRule cannot do its job. +func TestChartRBAC_SelectedModeGrantsOnlyTheListedTypes(t *testing.T) { + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) + + selected, found := roleNamed(t, roles, "-watch-selected") + require.True(t, found, "selected mode must render its ClusterRole") + require.Len(t, selected.Rules, 2) + + require.Equal(t, []string{""}, selected.Rules[0].APIGroups) + require.Equal(t, []string{"configmaps"}, selected.Rules[0].Resources) + require.Equal(t, []string{"apps"}, selected.Rules[1].APIGroups) + require.Equal(t, []string{"deployments"}, selected.Rules[1].Resources) + + for _, rule := range selected.Rules { + require.ElementsMatch(t, []string{"get", "list", "watch"}, rule.Verbs, + "a watched type is only ever read") + require.NotContains(t, rule.Resources, "*") + require.NotContains(t, rule.APIGroups, "*") + } +} + // The manager role is generated from the kubebuilder markers. It must never carry the // wildcard itself: RBAC is additive, so a wildcard there would re-grant Secret list/watch // and no chart value could take it away. func TestChartRBAC_ManagerRoleNeverCarriesTheWildcard(t *testing.T) { - for _, values := range [][]string{{"rbac.watchAnyResource=true"}, {"rbac.watchAnyResource=false"}} { - roles := renderClusterRoles(t, values...) - manager, found := roleNamed(t, roles, "-manager-role") - require.True(t, found, "manager ClusterRole must always render") - - for _, rule := range manager.Rules { - require.NotContains(t, rule.Resources, "*", - "manager role must not grant wildcard resources (%v)", rule.APIGroups) - } - require.Empty(t, grantsSecretListOrWatch(manager), - "the manager role must never enumerate Secrets; it reads the ones it is pointed at") + modes := map[string][]string{ + "any": {"rbac.watchTypes.mode=any"}, + "selected": selectedConfigMapsAndDeployments, + } + for name, values := range modes { + t.Run(name, func(t *testing.T) { + roles := renderClusterRoles(t, values...) + manager, found := roleNamed(t, roles, "-manager-role") + require.True(t, found, "manager ClusterRole must always render") + + for _, rule := range manager.Rules { + require.NotContains(t, rule.Resources, "*", + "manager role must not grant wildcard resources (%v)", rule.APIGroups) + } + require.Empty(t, grantsSecretListOrWatch(manager), + "the manager role must never enumerate Secrets; it reads the ones it is pointed at") + }) } } -// The API-resource catalog and its trigger informers read these two. Without them a -// least-privilege install 403s on every reflector retry. +// The API-resource catalog and its trigger informers read these three. Without them a +// least-privilege install 403s on every reflector retry, which is why a `selected` user must +// not have to restate them. func TestChartRBAC_ManagerRoleGrantsTheAPISurfaceReads(t *testing.T) { - roles := renderClusterRoles(t, "rbac.watchAnyResource=false") + roles := renderClusterRoles(t, selectedConfigMapsAndDeployments...) manager, found := roleNamed(t, roles, "-manager-role") require.True(t, found) @@ -146,14 +189,64 @@ func TestChartRBAC_ManagerRoleGrantsTheAPISurfaceReads(t *testing.T) { } } -// Default installs keep working: a WatchRule may name any type, so the wildcard ships on. +// Default installs keep working: a WatchRule may name any type, so `any` is the default. func TestChartRBAC_WildcardRoleShipsByDefault(t *testing.T) { roles := renderClusterRoles(t) - wildcard, found := roleNamed(t, roles, "-watch-any-resource") + wildcard, found := roleNamed(t, roles, "-watch-any") require.True(t, found, "the wildcard ClusterRole must render by default") require.Len(t, wildcard.Rules, 1) require.Equal(t, []string{"*"}, wildcard.Rules[0].APIGroups) require.Equal(t, []string{"*"}, wildcard.Rules[0].Resources) require.ElementsMatch(t, []string{"get", "list", "watch"}, wildcard.Rules[0].Verbs) } + +// A mis-set watchTypes must fail the render, not silently grant the wrong thing. Granting +// nothing is as bad as granting everything: one breaks the operator, the other the cluster. +func TestChartRBAC_MisconfiguredWatchTypesFailsTheRender(t *testing.T) { + tests := map[string]struct { + setValues []string + wantErr string + }{ + "unknown mode": { + setValues: []string{"rbac.watchTypes.mode=readonly"}, + wantErr: `rbac.watchTypes.mode must be "any" or "selected"`, + }, + "selected with no entries": { + setValues: []string{"rbac.watchTypes.mode=selected"}, + wantErr: "needs at least one entry", + }, + "entry without resources": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=apps", + }, + wantErr: "selected[0] needs resources", + }, + "entry without apiGroups": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].resources[0]=deployments", + }, + wantErr: "selected[0] needs apiGroups", + }, + // Verbs are not the user's to choose: the reverser mirrors a type, it never writes one. + "entry setting verbs": { + setValues: []string{ + "rbac.watchTypes.mode=selected", + "rbac.watchTypes.selected[0].apiGroups[0]=apps", + "rbac.watchTypes.selected[0].resources[0]=deployments", + "rbac.watchTypes.selected[0].verbs[0]=create", + }, + wantErr: "must not set verbs", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + out, err := helmTemplateRBAC(tc.setValues...) + require.Errorf(t, err, "render should have failed, got:\n%s", out) + require.Contains(t, string(out), tc.wantErr) + }) + } +} diff --git a/config/rbac/kustomization.yaml b/config/rbac/kustomization.yaml index 9ce8b7e17..5907796f4 100644 --- a/config/rbac/kustomization.yaml +++ b/config/rbac/kustomization.yaml @@ -4,9 +4,9 @@ resources: - sa.yaml - role.yaml # The wildcard read a WatchRule may need, kept out of role.yaml so it can be dropped whole. - # See docs/rbac.md; the Helm chart gates the same pair on `rbac.watchAnyResource`. - - watch-any-resource-role.yaml - - watch-any-resource-rolebinding.yaml + # See docs/rbac.md; the Helm chart renders the same pair from `rbac.watchTypes`. + - watch-any-role.yaml + - watch-any-rolebinding.yaml - cluster-role-jane-aggregated.yaml - cluster-rolebinding.yaml - cluster-rolebinding-jane.yaml # E2E-only helper: allows impersonated user jane@acme.com writes in tests. diff --git a/config/rbac/watch-any-resource-role.yaml b/config/rbac/watch-any-role.yaml similarity index 86% rename from config/rbac/watch-any-resource-role.yaml rename to config/rbac/watch-any-role.yaml index ba213a582..d2ddd24e8 100644 --- a/config/rbac/watch-any-resource-role.yaml +++ b/config/rbac/watch-any-role.yaml @@ -5,14 +5,14 @@ # It is kept OUT of that role on purpose: RBAC is additive, so a wildcard read here would # grant cluster-wide Secret list/watch no matter how narrow the Secret rule beside it is. # -# Drop this role (Helm: `rbac.watchAnyResource: false`) when you know which types the +# Drop this role (Helm: `rbac.watchTypes.mode: selected`) when you know which types the # reverser will mirror, and grant get/list/watch on exactly those instead. The operator # needs nothing else from it: it holds no long-lived informers on watched types, and it # never reads a Secret it was not pointed at by name. apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole metadata: - name: gitops-reverser-watch-any-resource + name: gitops-reverser-watch-any rules: - apiGroups: - '*' diff --git a/config/rbac/watch-any-resource-rolebinding.yaml b/config/rbac/watch-any-rolebinding.yaml similarity index 78% rename from config/rbac/watch-any-resource-rolebinding.yaml rename to config/rbac/watch-any-rolebinding.yaml index cde3a3903..26b6cd389 100644 --- a/config/rbac/watch-any-resource-rolebinding.yaml +++ b/config/rbac/watch-any-rolebinding.yaml @@ -4,11 +4,11 @@ metadata: labels: app.kubernetes.io/managed-by: kustomize app.kubernetes.io/name: gitops-reverser - name: gitops-reverser-watch-any-resource + name: gitops-reverser-watch-any roleRef: apiGroup: rbac.authorization.k8s.io kind: ClusterRole - name: gitops-reverser-watch-any-resource + name: gitops-reverser-watch-any subjects: - kind: ServiceAccount name: gitops-reverser diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index b05410276..c54452894 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -12,12 +12,12 @@ We are pre-1.0, so breaking changes bump the **minor** version (release-please i The shipped RBAC now says what the binary actually does. Nothing is taken away from a default install; what changes is that the parts can be separated. -**The manager ClusterRole no longer contains `apiGroups: ["*"], resources: ["*"]`.** That rule now -lives in a ClusterRole of its own, `-watch-any-resource`, created by default -(`rbac.watchAnyResource: true`) and bound to the same ServiceAccount. A `WatchRule` may name any -type, so the default install keeps the same effective permissions. The split exists because RBAC is -additive: while the wildcard sat in the manager role, no chart value could remove the cluster-wide -Secret read it implied. +**The manager ClusterRole no longer contains `apiGroups: ["*"], resources: ["*"]`.** The types a +`WatchRule` may read now come from a ClusterRole of their own, rendered from the new +`rbac.watchTypes` and bound to the same ServiceAccount. The default (`mode: any`) reproduces the old +wildcard exactly, so the default install keeps the same effective permissions. The split exists +because RBAC is additive: while the wildcard sat in the manager role, no chart value could remove the +cluster-wide Secret read it implied. **The manager role's `secrets` rule narrowed from `get,list,watch,create,update,patch` to `get,create,update`.** The operator has held no Secret informer since `v0.31.0` — Secrets are @@ -31,12 +31,24 @@ and its trigger informers read both. **Migration** -- Default installs (`rbac.watchAnyResource` unset): no action. Helm creates the extra ClusterRole - and binding; `kubectl apply` of `dist/install.yaml` includes them. -- To run least-privilege, set `rbac.watchAnyResource: false` and grant `get,list,watch` on the types - your `WatchRule`s name. See [`rbac.md`](rbac.md). -- If you hand-wrote a role because the shipped one was too broad, you can now drop the parts that - duplicate `-manager-role`. +- Default installs (`rbac.watchTypes` unset): no action. Helm creates `-watch-any` and its + binding; `kubectl apply` of `dist/install.yaml` includes the same pair. +- To run least-privilege, set `rbac.watchTypes.mode: selected` and list the types your `WatchRule`s + name. The chart renders the ClusterRole; verbs are always `get,list,watch`. See [`rbac.md`](rbac.md). + + ```yaml + rbac: + watchTypes: + mode: selected + selected: + - apiGroups: [""] + resources: ["configmaps"] + - apiGroups: ["apps"] + resources: ["deployments"] + ``` + +- If you hand-wrote a role because the shipped one was too broad, you can now drop it along with the + parts that duplicate `-manager-role`. **Related behavior change.** A trigger informer (`customresourcedefinitions`, `apiservices`) that the API server serves but RBAC denies is now **stopped** after the first `403`, logged once, and diff --git a/docs/rbac.md b/docs/rbac.md index 57ced4b5f..ffa59df3e 100644 --- a/docs/rbac.md +++ b/docs/rbac.md @@ -11,11 +11,47 @@ credential we own". | ClusterRole | Contents | Where it comes from | |---|---|---| | `-manager-role` | Exactly what the binary calls: its own CRs, `namespaces`, `customresourcedefinitions`, `apiservices`, and `secrets` with **`get`, `create`, `update`** | Generated from the kubebuilder markers into [`config/rbac/role.yaml`](../config/rbac/role.yaml). Never edit by hand. | -| `-watch-any-resource` | `apiGroups: ["*"], resources: ["*"], verbs: ["get","list","watch"]` | Hand-maintained: [`config/rbac/watch-any-resource-role.yaml`](../config/rbac/watch-any-resource-role.yaml), or the chart's `rbac.watchAnyResource` (default `true`). | +| `-watch-any` **or** `-watch-selected` | The types a `WatchRule` may read, `get`/`list`/`watch` only | The chart's `rbac.watchTypes`; for kustomize, [`config/rbac/watch-any-role.yaml`](../config/rbac/watch-any-role.yaml). | They are separate because **RBAC is additive**. A wildcard read folded into the manager role would grant cluster-wide Secret `list`/`watch` no matter how narrow the Secret rule beside -it is, and no chart value could take it back. Split out, the wildcard can be dropped whole. +it is, and no chart value could take it back. Split out, the wildcard can be replaced. + +## Choosing what the reverser may read + +```yaml +rbac: + create: true + watchTypes: + mode: any # any | selected + selected: [] +``` + +**`mode: any`** (default) grants cluster-wide read on every resource. A `WatchRule` can name +any type, including one installed after the operator, and it will just work. The price is +that the reverser **can read every Secret in the cluster** — every git credential, every +token — even though it never asks for one it was not pointed at. + +**`mode: selected`** grants read on exactly the types you list, and nothing else: + +```yaml +rbac: + watchTypes: + mode: selected + selected: + - apiGroups: [""] # core group + resources: ["configmaps"] + - apiGroups: ["apps"] + resources: ["deployments"] +``` + +Verbs are always `get`, `list`, `watch` — the reverser never writes to a watched type, so +the chart refuses a `verbs:` key rather than let you grant one by accident. It also refuses +`mode: selected` with an empty list, which would leave the operator able to watch nothing. + +Do **not** restate `namespaces`, `customresourcedefinitions` or `apiservices`: the manager +role already carries them. A `WatchRule` naming a type you did not grant will fail to watch +it, so the list must cover every type your rules name. ## The operator does not read Secrets wholesale @@ -28,31 +64,9 @@ That is why the manager role asks for `get`, `create`, `update` and nothing more ## Running least-privilege -Set `rbac.watchAnyResource: false` and grant read on the types you actually mirror: - -```yaml -# helm values -rbac: - create: true - watchAnyResource: false -``` - -```yaml -# your own ClusterRole, bound to the reverser's ServiceAccount -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: gitops-reverser-watched-types -rules: - # exactly the types your WatchRules name - - apiGroups: ["gitops-api.configbutler.ai"] - resources: ["tenants", "reposelections"] - verbs: ["get", "list", "watch"] -``` - -The manager role already carries `namespaces`, `customresourcedefinitions` and `apiservices`, -so you do not restate them. Bind your role to the same ServiceAccount, alongside the manager -role's binding. +Set `mode: selected` and list your types. The chart renders the ClusterRole and its binding +for you — there is no hand-written role to maintain, and no way to drift from the +ServiceAccount the operator actually runs as. Result: `GitProvider` Ready, `ClusterWatchRule` Ready with its streams, commits flowing, and **zero cluster-wide Secret access**. diff --git a/internal/watch/manager.go b/internal/watch/manager.go index be721a2b6..09834f6e2 100644 --- a/internal/watch/manager.go +++ b/internal/watch/manager.go @@ -40,9 +40,9 @@ import ( // code. It is deliberately NOT a marker: controller-gen would fold `groups=*,resources=*` // into this ClusterRole, and RBAC is additive — a wildcard read grants cluster-wide Secret // list/watch no matter how narrow the Secret rule beside it is. The wildcard therefore lives -// in its own opt-out ClusterRole (config/rbac/watch-any-resource-role.yaml, -// `rbac.watchAnyResource` in the chart), so an operator that mirrors two CRDs can be denied -// every other object in the cluster. See docs/rbac.md. +// in a ClusterRole of its own (config/rbac/watch-any-role.yaml, `rbac.watchTypes.mode: any` in +// the chart), so an operator that mirrors two CRDs can be told the exact types it may read and +// be denied every other object in the cluster. See docs/rbac.md. // Manager is a controller-runtime Runnable that keeps the followability registry and the // demand-driven materialization axis fresh and drives the per-type splice reconcile. It From b6364ee5d599a46e2baf2e3df963ac0cb05544e9 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 08:32:58 +0000 Subject: [PATCH 6/8] feat(chart): validate rbac.watchTypes with values.schema.json, not template guards The five `fail` calls guarding rbac.watchTypes were a hand-rolled type system. helm has one: values.schema.json, enforced on template, lint, install and upgrade, and carried inside the packaged chart so it protects a real `helm install` and not just a render from this repo. Every rule the guards expressed is now a schema rule, and the errors name the offending path rather than a sentence someone wrote once: at '/rbac/watchTypes/mode': value must be one of 'any', 'selected' at '/rbac/watchTypes/selected': minItems: got 0, want 1 at '/rbac/watchTypes/selected/0': missing property 'apiGroups' at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed `additionalProperties: false` on rbac buys one the guards could not: an unknown key is an error rather than a silent no-op. `rbac.watchAnyResoure` -- or a leftover `watchAnyResource` from earlier on this branch -- now fails the install instead of quietly leaving the wildcard in place, which is precisely how someone believes they narrowed access and has not. The schema is deliberately partial: it constrains rbac, whose shape is load-bearing, and leaves the rest of values.yaml free-form until a value needs a rule. Everything else still validates, because unspecified properties are permitted at the top level. Trade-off, stated: `helm template --skip-schema-validation` bypasses all of it. The template then renders a ClusterRole with no rules, so the reverser is authorized to watch nothing -- it fails closed, not open. That is the right way round, and the tests cover it by asserting the schema errors rather than the template's. The one remaining `fail` stays: it reports a missing generated config/role.yaml, which is a file, not a value, and no values schema can see it. Co-Authored-By: Claude Opus 4.8 (1M context) --- charts/gitops-reverser/templates/rbac.yaml | 23 +++----- charts/gitops-reverser/values.schema.json | 64 ++++++++++++++++++++++ charts/gitops-reverser/values.yaml | 18 +++--- cmd/main_chart_rbac_test.go | 20 +++++-- docs/rbac.md | 16 +++++- 5 files changed, 107 insertions(+), 34 deletions(-) create mode 100644 charts/gitops-reverser/values.schema.json diff --git a/charts/gitops-reverser/templates/rbac.yaml b/charts/gitops-reverser/templates/rbac.yaml index a5f3ed7ff..a433ddea4 100644 --- a/charts/gitops-reverser/templates/rbac.yaml +++ b/charts/gitops-reverser/templates/rbac.yaml @@ -28,15 +28,15 @@ subjects: - kind: ServiceAccount name: {{ include "gitops-reverser.serviceAccountName" . }} namespace: {{ .Release.Namespace }} +{{/* + mode, the shape of each `selected` entry, and "selected must not be empty" are all enforced + by values.schema.json, which helm checks on template, lint, install and upgrade. Keeping the + rules there rather than as `fail` calls means one statement of the contract, and errors that + name the offending path. +*/}} {{- $watchTypes := .Values.rbac.watchTypes | default dict }} {{- $mode := $watchTypes.mode | default "any" }} -{{- if not (has $mode (list "any" "selected")) }} -{{- fail (printf "rbac.watchTypes.mode must be \"any\" or \"selected\", got %q" $mode) }} -{{- end }} {{- $selected := $watchTypes.selected | default list }} -{{- if and (eq $mode "selected") (not $selected) }} -{{- fail "rbac.watchTypes.mode=selected needs at least one entry in rbac.watchTypes.selected, or the reverser can watch nothing" }} -{{- end }} --- # The read access a WatchRule needs. Separate from the manager role because RBAC is additive: # folded in, a wildcard here would grant cluster-wide Secret list/watch however narrow the @@ -59,16 +59,7 @@ rules: - list - watch {{- else }} -{{- range $i, $rule := $selected }} -{{- if not $rule.apiGroups }} -{{- fail (printf "rbac.watchTypes.selected[%d] needs apiGroups (use [\"\"] for the core group)" $i) }} -{{- end }} -{{- if not $rule.resources }} -{{- fail (printf "rbac.watchTypes.selected[%d] needs resources" $i) }} -{{- end }} -{{- if $rule.verbs }} -{{- fail (printf "rbac.watchTypes.selected[%d] must not set verbs; the reverser only ever reads a watched type" $i) }} -{{- end }} +{{- range $rule := $selected }} - apiGroups: {{- toYaml $rule.apiGroups | nindent 4 }} resources: diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json new file mode 100644 index 000000000..6e54b68fa --- /dev/null +++ b/charts/gitops-reverser/values.schema.json @@ -0,0 +1,64 @@ +{ + "$schema": "https://json-schema.org/draft/2020-12/schema", + "title": "gitops-reverser values", + "description": "Partial schema. Only the values whose shape is load-bearing are constrained here; everything else is free-form until it needs a rule.", + "type": "object", + "properties": { + "rbac": { + "type": "object", + "properties": { + "create": { + "type": "boolean", + "description": "Create the manager ClusterRole and its binding." + }, + "watchTypes": { + "type": "object", + "description": "Which types a WatchRule may read. The reverser never writes to a watched type.", + "properties": { + "mode": { + "enum": ["any", "selected"], + "description": "any: cluster-wide read on everything, including every Secret. selected: read only the types listed below." + }, + "selected": { + "type": "array", + "description": "Types to grant when mode is selected. Verbs are always get/list/watch.", + "items": { + "type": "object", + "properties": { + "apiGroups": { + "type": "array", + "minItems": 1, + "items": { "type": "string" }, + "description": "API groups; use [\"\"] for the core group." + }, + "resources": { + "type": "array", + "minItems": 1, + "items": { "type": "string" } + } + }, + "required": ["apiGroups", "resources"], + "additionalProperties": false + } + } + }, + "additionalProperties": false, + "if": { + "properties": { "mode": { "const": "selected" } }, + "required": ["mode"] + }, + "then": { + "properties": { + "selected": { + "minItems": 1, + "description": "mode=selected with no entries would leave the reverser able to watch nothing." + } + }, + "required": ["selected"] + } + } + }, + "additionalProperties": false + } + } +} diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 178a1bc5a..45e098716 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -211,17 +211,15 @@ attribution: # RBAC configuration rbac: create: true - # Which types the reverser may read (get/list/watch). It never writes to a watched type. + # Which types the reverser may read. It never writes to a watched type, so verbs are always + # get/list/watch. Shape is enforced by values.schema.json; see docs/rbac.md. # - # any cluster-wide read on everything. Convenient — a WatchRule can name any type, - # even one installed later — but it means the reverser can read every Secret in - # the cluster, including credentials that have nothing to do with it. - # selected read only the types you list. Least privilege: zero cluster-wide Secret - # access. A WatchRule naming an ungranted type will fail to watch it. - # - # `selected` entries take apiGroups and resources; verbs are always get/list/watch. - # namespaces, customresourcedefinitions and apiservices come from the manager role, so do - # not restate them. See docs/rbac.md. + # any cluster-wide read on everything, so a WatchRule can name any type — including + # one installed later. It also means the reverser can read every Secret in the + # cluster, credentials that have nothing to do with it included. + # selected read only the types below. Zero cluster-wide Secret access. A WatchRule naming + # a type you did not grant will fail to watch it. namespaces, CRDs and + # apiservices come from the manager role — do not restate them. watchTypes: mode: any # any | selected selected: [] diff --git a/cmd/main_chart_rbac_test.go b/cmd/main_chart_rbac_test.go index 950c6e897..f43b907c3 100644 --- a/cmd/main_chart_rbac_test.go +++ b/cmd/main_chart_rbac_test.go @@ -17,6 +17,7 @@ import ( // the chart in a subprocess the test log cannot see. var rbacChartInputs = []string{ "../charts/gitops-reverser/values.yaml", + "../charts/gitops-reverser/values.schema.json", "../charts/gitops-reverser/templates/rbac.yaml", "../charts/gitops-reverser/config/role.yaml", } @@ -203,6 +204,10 @@ func TestChartRBAC_WildcardRoleShipsByDefault(t *testing.T) { // A mis-set watchTypes must fail the render, not silently grant the wrong thing. Granting // nothing is as bad as granting everything: one breaks the operator, the other the cluster. +// +// values.schema.json is what rejects these, and helm applies it to template, lint, install +// and upgrade alike. The template carries no equivalent `fail` guards, so these cases are the +// only thing standing between a typo and a ClusterRole nobody meant to create. func TestChartRBAC_MisconfiguredWatchTypesFailsTheRender(t *testing.T) { tests := map[string]struct { setValues []string @@ -210,25 +215,25 @@ func TestChartRBAC_MisconfiguredWatchTypesFailsTheRender(t *testing.T) { }{ "unknown mode": { setValues: []string{"rbac.watchTypes.mode=readonly"}, - wantErr: `rbac.watchTypes.mode must be "any" or "selected"`, + wantErr: "at '/rbac/watchTypes/mode': value must be one of 'any', 'selected'", }, "selected with no entries": { setValues: []string{"rbac.watchTypes.mode=selected"}, - wantErr: "needs at least one entry", + wantErr: "at '/rbac/watchTypes/selected': minItems: got 0, want 1", }, "entry without resources": { setValues: []string{ "rbac.watchTypes.mode=selected", "rbac.watchTypes.selected[0].apiGroups[0]=apps", }, - wantErr: "selected[0] needs resources", + wantErr: "at '/rbac/watchTypes/selected/0': missing property 'resources'", }, "entry without apiGroups": { setValues: []string{ "rbac.watchTypes.mode=selected", "rbac.watchTypes.selected[0].resources[0]=deployments", }, - wantErr: "selected[0] needs apiGroups", + wantErr: "at '/rbac/watchTypes/selected/0': missing property 'apiGroups'", }, // Verbs are not the user's to choose: the reverser mirrors a type, it never writes one. "entry setting verbs": { @@ -238,7 +243,12 @@ func TestChartRBAC_MisconfiguredWatchTypesFailsTheRender(t *testing.T) { "rbac.watchTypes.selected[0].resources[0]=deployments", "rbac.watchTypes.selected[0].verbs[0]=create", }, - wantErr: "must not set verbs", + wantErr: "at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed", + }, + // A typo in a key name is the likeliest way to think you narrowed access when you did not. + "unknown key under watchTypes": { + setValues: []string{"rbac.watchTypes.modes=selected"}, + wantErr: "additional properties 'modes' not allowed", }, } diff --git a/docs/rbac.md b/docs/rbac.md index ffa59df3e..2b8976095 100644 --- a/docs/rbac.md +++ b/docs/rbac.md @@ -45,9 +45,19 @@ rbac: resources: ["deployments"] ``` -Verbs are always `get`, `list`, `watch` — the reverser never writes to a watched type, so -the chart refuses a `verbs:` key rather than let you grant one by accident. It also refuses -`mode: selected` with an empty list, which would leave the operator able to watch nothing. +Verbs are always `get`, `list`, `watch` — the reverser never writes to a watched type, so a +`verbs:` key is rejected rather than letting you grant one by accident. So is `mode: +selected` with an empty list, which would leave the operator able to watch nothing, and an +unknown key under `rbac`, which is how you think you narrowed access when you did not. + +Those rules live in the chart's +[`values.schema.json`](../charts/gitops-reverser/values.schema.json), which Helm enforces on +`template`, `lint`, `install` and `upgrade`, naming the offending path: + +``` +Error: values don't meet the specifications of the schema(s) in the following chart(s): +- at '/rbac/watchTypes/selected/0': additional properties 'verbs' not allowed +``` Do **not** restate `namespaces`, `customresourcedefinitions` or `apiservices`: the manager role already carries them. A `WatchRule` naming a type you did not grant will fail to watch From a04b5687b9a96adb16a6af1bc35dde0ea97bf631 Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 08:46:28 +0000 Subject: [PATCH 7/8] feat(chart): a full values.schema.json, and two things it found Extends the rbac-only schema to every value. Objects the chart owns are closed (additionalProperties: false), so a typo fails the install instead of silently doing nothing: `--set replicaCounts=2` or `--set servers.metric.port=8080` now name the offending path rather than rendering the defaults and leaving the operator to wonder. Kubernetes pass-through objects -- securityContext, affinity, resources, volumes, tolerations -- stay open, because their shape belongs to Kubernetes and not to this chart. `global` stays open too: a parent chart may set it. Where the binary already had a contract, the schema states it: logging.level enum debug|info|error|panic -- `warn` is NOT a zap level. The chart renders --zap-log-level=warn happily and the pod crash-loops on it. logging.stacktraceLevel enum info|error|panic (no debug) logging.encoder enum json|console queue.redis.keyPrefix pattern + maxLength, the same character class ValidateKeyPrefix enforces. A quote in it is what CrashLoopBackOff'd the manager on this branch. servers.admission.timeoutSeconds 1..30, which is what the apiserver accepts auditService.nodePort 30000..32767 durations Go duration pattern (ttl, grace, timeouts, cert lifetimes) Two suspicious things fell out of diffing what values.yaml declares against what the templates actually read: 1. servers.enableHTTP2 was DEAD. No template referenced it, so setting it changed nothing, while its comment promised a mitigation for the HTTP/2 Rapid-Reset CVE class. The effective behavior was safe by accident -- the binary's --enable-http2 defaults to false -- but a value that reads like a security control and does nothing is worse than no value. Wired to the flag, rendered only when true, so the default install is byte-identical. Pinned by a chart-args case that fails against the unwired template. 2. env, volumes and volumeMounts are read by the Deployment but were never declared. They were invisible to anyone reading values.yaml, and a closed top-level schema would have broken them. Declared, documented, and covered. Verified against every real invocation: chart defaults, `helm lint`, the e2e `helm upgrade --install` flags, `task dist-install`, quickstart, attribution, and rbac selected mode. Co-Authored-By: Claude Opus 4.8 (1M context) --- charts/gitops-reverser/README.md | 3 + .../gitops-reverser/templates/deployment.yaml | 3 + charts/gitops-reverser/values.schema.json | 482 +++++++++++++++++- charts/gitops-reverser/values.yaml | 6 + cmd/main_chart_args_test.go | 10 + 5 files changed, 497 insertions(+), 7 deletions(-) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index c91025daa..54f205c07 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -162,6 +162,9 @@ nodeSelector: |-----------|-------------|---------| | `replicaCount` | Number of controller replicas (can't be higher than 1 for now, sorry) | `1` | | `image.repository` | Container image repository | `ghcr.io/configbutler/gitops-reverser` | +| `env` | Extra container env vars, as Kubernetes `EnvVar` entries | `[]` | +| `volumes` / `volumeMounts` | Extra pod volumes and their mounts, appended as-is | `[]` | +| `servers.enableHTTP2` | Serve HTTP/2 on the TLS servers. Off by default: disabling it mitigates the HTTP/2 Rapid-Reset CVE class | `false` | | `servers.audit.bindAddress` | host:port the audit ingress server binds to (`--audit-bind-address`) | `0.0.0.0:9444` | | `servers.audit.port` | Audit container/Service port; must match the port in `bindAddress` | `9444` | | `servers.audit.tls.enabled` | Serve audit ingress with TLS | `true` | diff --git a/charts/gitops-reverser/templates/deployment.yaml b/charts/gitops-reverser/templates/deployment.yaml index 66a9a7559..35d0fa075 100644 --- a/charts/gitops-reverser/templates/deployment.yaml +++ b/charts/gitops-reverser/templates/deployment.yaml @@ -42,6 +42,9 @@ spec: imagePullPolicy: {{ .Values.image.pullPolicy }} args: - --health-probe-bind-address={{ .Values.servers.healthProbe.bindAddress }} + {{- if .Values.servers.enableHTTP2 }} + - --enable-http2 + {{- end }} - --metrics-bind-address={{ .Values.servers.metrics.bindAddress }} {{- if not .Values.servers.metrics.tls.enabled }} - --metrics-insecure diff --git a/charts/gitops-reverser/values.schema.json b/charts/gitops-reverser/values.schema.json index 6e54b68fa..f085fa484 100644 --- a/charts/gitops-reverser/values.schema.json +++ b/charts/gitops-reverser/values.schema.json @@ -1,11 +1,286 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", "title": "gitops-reverser values", - "description": "Partial schema. Only the values whose shape is load-bearing are constrained here; everything else is free-form until it needs a rule.", + "description": "Helm validates these on template, lint, install and upgrade. Objects the chart owns are closed (additionalProperties: false) so a typo fails the install instead of silently doing nothing; Kubernetes pass-through objects (securityContext, affinity, resources, volumes, ...) stay open, because their shape belongs to Kubernetes and not to this chart.", "type": "object", + "additionalProperties": false, + "$defs": { + "duration": { + "type": "string", + "pattern": "^([0-9]+(\\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$", + "description": "Go duration, e.g. 30s, 10m, 87600h." + }, + "port": { + "type": "integer", + "minimum": 1, + "maximum": 65535 + }, + "secretNameOverride": { + "type": "string", + "description": "Leave empty to use the chart-generated Secret name." + }, + "intOrString": { + "type": ["integer", "string"] + }, + "stringMap": { + "type": "object", + "additionalProperties": { "type": "string" } + }, + "objectList": { + "type": "array", + "items": { "type": "object" } + } + }, "properties": { + "global": { + "type": "object", + "description": "Helm's cross-chart values. Free-form: this chart reads none of it, but a parent chart may set it." + }, + + "replicaCount": { "type": "integer", "minimum": 0 }, + + "deploymentStrategy": { + "type": "object", + "additionalProperties": false, + "properties": { + "type": { "enum": ["RollingUpdate", "Recreate"] }, + "rollingUpdate": { + "type": "object", + "additionalProperties": false, + "properties": { + "maxSurge": { "$ref": "#/$defs/intOrString" }, + "maxUnavailable": { "$ref": "#/$defs/intOrString" } + } + } + } + }, + + "image": { + "type": "object", + "additionalProperties": false, + "properties": { + "repository": { "type": "string", "minLength": 1 }, + "pullPolicy": { "enum": ["Always", "IfNotPresent", "Never"] }, + "tag": { "type": "string" } + } + }, + + "imagePullSecrets": { "$ref": "#/$defs/objectList" }, + "nameOverride": { "type": "string" }, + "fullnameOverride": { "type": "string" }, + "createNamespace": { "type": "boolean" }, + + "serviceAccount": { + "type": "object", + "additionalProperties": false, + "properties": { + "create": { "type": "boolean" }, + "annotations": { "$ref": "#/$defs/stringMap" }, + "name": { "type": "string" } + } + }, + + "podAnnotations": { "$ref": "#/$defs/stringMap" }, + "podSecurityContext": { "type": "object" }, + "securityContext": { "type": "object" }, + + "env": { + "$ref": "#/$defs/objectList", + "description": "Extra container env vars, as Kubernetes EnvVar entries." + }, + "volumes": { "$ref": "#/$defs/objectList" }, + "volumeMounts": { "$ref": "#/$defs/objectList" }, + + "controllerManager": { + "type": "object", + "additionalProperties": false, + "properties": { + "additionalSensitiveResources": { + "type": "array", + "items": { "type": "string" }, + "description": "Resource or group/resource, e.g. core.cozystack.io/tenantsecrets." + } + } + }, + + "certManager": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "issuer": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string", "minLength": 1 }, + "kind": { "enum": ["Issuer", "ClusterIssuer"] }, + "create": { "type": "boolean" } + } + } + } + }, + + "servers": { + "type": "object", + "additionalProperties": false, + "properties": { + "enableHTTP2": { + "type": "boolean", + "description": "Serve HTTP/2 on the TLS servers. Off by default: it mitigates the HTTP/2 Rapid-Reset CVE class." + }, + "healthProbe": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" } + } + }, + "metrics": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + }, + "admission": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "timeoutSeconds": { + "type": "integer", + "minimum": 1, + "maximum": 30, + "description": "The apiserver rejects a webhook timeout outside 1-30s." + }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + }, + "audit": { + "type": "object", + "additionalProperties": false, + "properties": { + "bindAddress": { "type": "string" }, + "port": { "$ref": "#/$defs/port" }, + "timeouts": { + "type": "object", + "additionalProperties": false, + "properties": { + "read": { "$ref": "#/$defs/duration" }, + "write": { "$ref": "#/$defs/duration" }, + "idle": { "$ref": "#/$defs/duration" } + } + }, + "maxRequestBodyBytes": { "type": "integer", "minimum": 1 }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "certManager": { "type": "boolean" }, + "certPath": { "type": "string" }, + "certName": { "type": "string" }, + "certKey": { "type": "string" }, + "clientCAPath": { "type": "string" }, + "clientCAName": { "type": "string" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" }, + "rootCA": { + "type": "object", + "additionalProperties": false, + "properties": { + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + }, + "client": { + "type": "object", + "additionalProperties": false, + "properties": { + "duration": { "$ref": "#/$defs/duration" }, + "renewBefore": { "$ref": "#/$defs/duration" }, + "secretNameOverride": { "$ref": "#/$defs/secretNameOverride" } + } + } + } + } + } + } + } + }, + + "queue": { + "type": "object", + "additionalProperties": false, + "properties": { + "redis": { + "type": "object", + "additionalProperties": false, + "properties": { + "addr": { "type": "string", "description": "host:port. Empty runs without Redis." }, + "auth": { + "type": "object", + "additionalProperties": false, + "properties": { + "existingSecret": { "type": "string" }, + "existingSecretKey": { "type": "string" }, + "username": { "type": "string" } + } + }, + "db": { "type": "integer", "minimum": 0 }, + "keyPrefix": { + "type": "string", + "maxLength": 128, + "pattern": "^[A-Za-z0-9._:-]*$", + "description": "Root of every key this release writes. The binary validates it too, and rejects a prefix that is empty once trailing ':' is trimmed." + }, + "tls": { + "type": "object", + "additionalProperties": false, + "properties": { "enabled": { "type": "boolean" } } + } + } + } + } + }, + + "attribution": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "ttl": { "$ref": "#/$defs/duration" }, + "grace": { "$ref": "#/$defs/duration" } + } + }, + "rbac": { "type": "object", + "additionalProperties": false, "properties": { "create": { "type": "boolean", @@ -13,6 +288,7 @@ }, "watchTypes": { "type": "object", + "additionalProperties": false, "description": "Which types a WatchRule may read. The reverser never writes to a watched type.", "properties": { "mode": { @@ -24,6 +300,8 @@ "description": "Types to grant when mode is selected. Verbs are always get/list/watch.", "items": { "type": "object", + "additionalProperties": false, + "required": ["apiGroups", "resources"], "properties": { "apiGroups": { "type": "array", @@ -36,29 +314,219 @@ "minItems": 1, "items": { "type": "string" } } - }, - "required": ["apiGroups", "resources"], - "additionalProperties": false + } } } }, - "additionalProperties": false, "if": { "properties": { "mode": { "const": "selected" } }, "required": ["mode"] }, "then": { + "required": ["selected"], "properties": { "selected": { "minItems": 1, "description": "mode=selected with no entries would leave the reverser able to watch nothing." } + } + } + } + } + }, + + "resources": { "type": "object" }, + "nodeSelector": { "$ref": "#/$defs/stringMap" }, + "tolerations": { "$ref": "#/$defs/objectList" }, + "affinity": { "type": "object" }, + + "podDisruptionBudget": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "minAvailable": { "$ref": "#/$defs/intOrString" }, + "maxUnavailable": { "$ref": "#/$defs/intOrString" } + } + }, + + "monitoring": { + "type": "object", + "additionalProperties": false, + "properties": { + "serviceMonitor": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "namespace": { "type": "string" }, + "labels": { "$ref": "#/$defs/stringMap" }, + "interval": { "$ref": "#/$defs/duration" }, + "scrapeTimeout": { "$ref": "#/$defs/duration" }, + "path": { "type": "string" }, + "port": { "type": "string" }, + "scheme": { + "enum": ["http", "https"], + "description": "Must match servers.metrics.tls.enabled." + } + } + } + } + }, + + "service": { + "type": "object", + "additionalProperties": false, + "properties": { + "clusterIP": { "type": "string" }, + "ports": { + "type": "object", + "additionalProperties": false, + "properties": { + "audit": { "$ref": "#/$defs/port" }, + "metrics": { "$ref": "#/$defs/port" } + } + } + } + }, + + "auditService": { + "type": "object", + "additionalProperties": false, + "properties": { + "type": { "enum": ["ClusterIP", "NodePort", "LoadBalancer"] }, + "nodePort": { "type": "integer", "minimum": 30000, "maximum": 32767 }, + "clusterIP": { "type": "string" } + } + }, + + "auditKubeconfig": { + "type": "object", + "additionalProperties": false, + "properties": { "insecureSkipTLSVerify": { "type": "boolean" } } + }, + + "quickstart": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "namespace": { "type": "string" }, + "createNamespace": { "type": "boolean" }, + "gitProvider": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "url": { "type": "string" }, + "allowedBranches": { "type": "array", "items": { "type": "string" } }, + "secretRef": { + "type": "object", + "additionalProperties": false, + "properties": { "name": { "type": "string" } } }, - "required": ["selected"] + "push": { + "type": "object", + "additionalProperties": false, + "properties": { "commitWindow": { "$ref": "#/$defs/duration" } } + } + } + }, + "gitTarget": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "branch": { "type": "string" }, + "path": { "type": "string", "minLength": 1 }, + "encryption": { + "type": "object", + "additionalProperties": false, + "properties": { + "provider": { "type": "string" }, + "age": { + "type": "object", + "additionalProperties": false, + "properties": { + "enabled": { "type": "boolean" }, + "generateWhenMissing": { "type": "boolean" } + } + }, + "secretRef": { + "type": "object", + "additionalProperties": false, + "properties": { "name": { "type": "string" } } + } + } + } + } + }, + "watchRule": { + "type": "object", + "additionalProperties": false, + "properties": { + "name": { "type": "string" }, + "rules": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "properties": { + "operations": { + "type": "array", + "minItems": 1, + "items": { "enum": ["CREATE", "UPDATE", "DELETE"] } + }, + "apiGroups": { "type": "array", "items": { "type": "string" } }, + "apiVersions": { "type": "array", "items": { "type": "string" } }, + "resources": { "type": "array", "items": { "type": "string" } } + } + } + } } } }, - "additionalProperties": false + "if": { + "properties": { "enabled": { "const": true } }, + "required": ["enabled"] + }, + "then": { + "properties": { + "gitProvider": { + "properties": { "url": { "minLength": 1 } }, + "required": ["url"] + } + }, + "required": ["gitProvider"] + } + }, + + "logging": { + "type": "object", + "additionalProperties": false, + "properties": { + "level": { + "enum": ["debug", "info", "error", "panic"], + "description": "zap levels. `warn` is NOT one of them: the binary rejects it and the pod crash-loops." + }, + "development": { "type": "boolean" }, + "encoder": { "enum": ["json", "console"] }, + "stacktraceLevel": { + "enum": ["info", "error", "panic"], + "description": "zap stacktrace levels; `debug` is not accepted here." + } + } + }, + + "labels": { + "type": "object", + "additionalProperties": false, + "properties": { + "managedBy": { + "type": "string", + "description": "Set to kubectl only for the generated install.yaml; leave as Helm otherwise." + } + } } } } diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index 45e098716..fc8d4f2a1 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -33,6 +33,12 @@ serviceAccount: podAnnotations: {} +# Extension points the Deployment appends as-is. Declared here because the template already +# reads them; without an entry they are invisible to anyone reading values.yaml. +env: [] # Kubernetes EnvVar entries, e.g. [{name: REDIS_PASSWORD, valueFrom: {...}}] +volumes: [] +volumeMounts: [] + podSecurityContext: runAsNonRoot: true runAsUser: 65532 diff --git a/cmd/main_chart_args_test.go b/cmd/main_chart_args_test.go index bc13f440a..e435bc5f1 100644 --- a/cmd/main_chart_args_test.go +++ b/cmd/main_chart_args_test.go @@ -67,10 +67,18 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { wantUsername string wantAttribution bool wantAuditTLSCert string + wantHTTP2 bool }{ "chart defaults": { wantKeyPrefix: "gitops-reverser", }, + // servers.enableHTTP2 was a value no template read: setting it changed nothing, while + // its comment promised an HTTP/2 Rapid-Reset mitigation. Assert it reaches the binary. + "http2 enabled": { + setValues: []string{"servers.enableHTTP2=true"}, + wantKeyPrefix: "gitops-reverser", + wantHTTP2: true, + }, "nested tenant prefix": { setValues: []string{"queue.redis.keyPrefix=cell-a:tenant-7"}, wantKeyPrefix: "cell-a:tenant-7", @@ -132,6 +140,8 @@ func TestChartRendersArgsTheBinaryAccepts(t *testing.T) { if tc.wantAuditTLSCert != "" { require.Equal(t, tc.wantAuditTLSCert, cfg.auditCertPath) } + require.Equal(t, tc.wantHTTP2, cfg.enableHTTP2, + "servers.enableHTTP2 must reach the binary, and default to off") }) } } From 6f1913f53d9a986a79726c41dbded750402b550c Mon Sep 17 00:00:00 2001 From: Simon Koudijs Date: Fri, 10 Jul 2026 09:00:51 +0000 Subject: [PATCH 8/8] docs: stop overclaiming, refresh the security model, and prune the stale future plans Three doc problems, one of them a correctness bug in a security claim. 1. "zero cluster-wide Secret access" was FALSE. `selected` mode removes the wildcard, so the reverser can no longer list or watch Secrets -- but the manager ClusterRole still grants secrets get/create/update, cluster-scoped, because a GitProvider may name a Secret in any namespace. A selected install cannot DISCOVER a Secret; it can still read one whose name it knows. The tests always said this ("cannot enumerate Secrets", "does not grant Secret list/watch"); four prose sites said something stronger. They now say what the tests say, and docs/rbac.md carries a verb-by-verb table of what stays. Scoping get/create/update to the referenced namespaces needs a namespaced Role the chart cannot render without the manifests -- named as remaining work rather than quietly implied. 2. docs/security-model.md contradicted this branch. It still said the Secret narrowing was "not yet an RBAC change" and that the default install grants secrets get;list;watch. Rewritten around the two ClusterRoles, with the parts of the (now deleted) retention plan that are product truth rather than plan: no Secret informer, cache DisableFor, direct GET by name, the 5-minute steady reconcile as the rotation path, and public-recipient-only SOPS. Two mermaid diagrams, both parsed with the real mermaid parser rather than eyeballed. 3. The future/ plans were describing a world this branch deleted. secret-value-retention-plan.md was implemented; scoped-rbac-least-privilege-plan.md and secret-handling-roadmap.md both still opened with "Track B: not started" and documented the wildcard-in-manager-role that no longer exists. All three removed. What survives is two short docs: the age-recipient metadata idea (with the annotation-forgery rule that makes it dangerous to get wrong), and the three genuinely open least-privilege items -- never mirror Secrets, permission-aware followability, and the namespaced-Role/RBAC-generator work. Every inbound link repointed; no link left dangling. Also: root README lists docs/rbac.md beside security-model.md, and the aggregated-apiserver design note no longer claims config/rbac/role.yaml holds the wildcard -- it moved to watch-any-role.yaml. Co-Authored-By: Claude Opus 4.8 (1M context) --- README.md | 1 + charts/gitops-reverser/README.md | 2 +- charts/gitops-reverser/values.yaml | 12 +- docs/README.md | 2 +- .../e2e-aggregated-apiserver-test-design.md | 10 +- ...pted-projection-race-and-external-drift.md | 2 +- docs/design/reconcile-triggering.md | 4 +- docs/future/least-privilege-remaining-work.md | 68 +++++ .../publish-age-recipients-as-metadata.md | 36 +++ .../scoped-rbac-least-privilege-plan.md | 253 ----------------- docs/future/secret-handling-roadmap.md | 120 -------- docs/future/secret-value-retention-plan.md | 265 ------------------ docs/rbac.md | 25 +- docs/security-model.md | 112 ++++++-- 14 files changed, 227 insertions(+), 685 deletions(-) create mode 100644 docs/future/least-privilege-remaining-work.md create mode 100644 docs/future/publish-age-recipients-as-metadata.md delete mode 100644 docs/future/scoped-rbac-least-privilege-plan.md delete mode 100644 docs/future/secret-handling-roadmap.md delete mode 100644 docs/future/secret-value-retention-plan.md diff --git a/README.md b/README.md index df96aa943..4cab32686 100644 --- a/README.md +++ b/README.md @@ -233,6 +233,7 @@ Start with the stable docs surface: - [`docs/configuration.md`](docs/configuration.md) - [`docs/attribution-setup-guide.md`](docs/attribution-setup-guide.md) - [`docs/security-model.md`](docs/security-model.md) +- [`docs/rbac.md`](docs/rbac.md): the two ClusterRoles, and how to stop the reverser enumerating Secrets - [`docs/commit-signing.md`](docs/commit-signing.md) - [`docs/github-setup-guide.md`](docs/github-setup-guide.md) - [`docs/sops-age-guide.md`](docs/sops-age-guide.md) diff --git a/charts/gitops-reverser/README.md b/charts/gitops-reverser/README.md index 54f205c07..293cea85d 100644 --- a/charts/gitops-reverser/README.md +++ b/charts/gitops-reverser/README.md @@ -196,7 +196,7 @@ nodeSelector: | `attribution.grace` | Bounded per-event wait for a matching audit fact before a watch event ships as the committer | `3s` | | `servers.admission.enabled` | Install the validate-operator-types admission webhook that captures CommitRequest authors (a form of author attribution). Enabled by default; a no-op until `queue.redis.addr` is set | `true` | | `rbac.create` | Create the manager ClusterRole and its binding | `true` | -| `rbac.watchTypes.mode` | Which types a `WatchRule` may read. `any` grants cluster-wide read on everything — convenient, but the reverser can then read every Secret in the cluster. `selected` grants read on `rbac.watchTypes.selected` only: zero cluster-wide Secret access. See [`docs/rbac.md`](../../docs/rbac.md) | `any` | +| `rbac.watchTypes.mode` | Which types a `WatchRule` may read. `any` grants cluster-wide read on everything — convenient, but the reverser can then read every Secret in the cluster. `selected` grants read on `rbac.watchTypes.selected` only, so the reverser cannot list or watch Secrets (it keeps `get` on named Secrets it is pointed at). See [`docs/rbac.md`](../../docs/rbac.md) | `any` | | `rbac.watchTypes.selected` | Types to grant when `mode: selected`, as `{apiGroups, resources}` entries (verbs are always `get,list,watch`). Required and non-empty in that mode; `namespaces`, `customresourcedefinitions` and `apiservices` come from the manager role and must not be restated | `[]` | | `servers.metrics.bindAddress` | Metrics listener bind address | `:8080` | | `servers.metrics.tls.enabled` | Serve metrics with TLS | `false` | diff --git a/charts/gitops-reverser/values.yaml b/charts/gitops-reverser/values.yaml index fc8d4f2a1..142063b26 100644 --- a/charts/gitops-reverser/values.yaml +++ b/charts/gitops-reverser/values.yaml @@ -221,11 +221,13 @@ rbac: # get/list/watch. Shape is enforced by values.schema.json; see docs/rbac.md. # # any cluster-wide read on everything, so a WatchRule can name any type — including - # one installed later. It also means the reverser can read every Secret in the - # cluster, credentials that have nothing to do with it included. - # selected read only the types below. Zero cluster-wide Secret access. A WatchRule naming - # a type you did not grant will fail to watch it. namespaces, CRDs and - # apiservices come from the manager role — do not restate them. + # one installed later. It also means the reverser can list and read every Secret + # in the cluster, credentials that have nothing to do with it included. + # selected read only the types below, so the reverser cannot list or watch Secrets at all. + # (The manager role keeps `get` on Secrets: it still reads the git-creds and + # age-key Secrets a GitProvider/GitTarget names.) A WatchRule naming a type you + # did not grant will fail to watch it. namespaces, CRDs and apiservices come from + # the manager role — do not restate them. watchTypes: mode: any # any | selected selected: [] diff --git a/docs/README.md b/docs/README.md index 9aafa5aca..76823f3d6 100644 --- a/docs/README.md +++ b/docs/README.md @@ -20,7 +20,7 @@ If you only want the supported product docs, start with the files below. - [`sops-age-guide.md`](sops-age-guide.md): Secret encryption with SOPS + age - [`security-model.md`](security-model.md): controller access, trust boundaries, and the Git credentials Secret shape -- [`rbac.md`](rbac.md): the two ClusterRoles, and how to run with zero cluster-wide Secret access +- [`rbac.md`](rbac.md): the two ClusterRoles, and how to stop the reverser enumerating Secrets - [`bi-directional.md`](bi-directional.md): safe shared-path and handoff patterns - [`alternatives.md`](alternatives.md): nearby tools and when another approach fits better - [`UPGRADING.md`](UPGRADING.md): breaking changes and migration steps, newest first diff --git a/docs/design/e2e-aggregated-apiserver-test-design.md b/docs/design/e2e-aggregated-apiserver-test-design.md index 3137dc51e..e8a47b2b5 100644 --- a/docs/design/e2e-aggregated-apiserver-test-design.md +++ b/docs/design/e2e-aggregated-apiserver-test-design.md @@ -280,7 +280,10 @@ are published per Kubernetes release; newer k3s versions may require building fr ### gitops-reverser RBAC: no changes needed -The existing ClusterRole at `config/rbac/role.yaml` already grants wildcard read access: +The default kustomize install still binds a wildcard read, but it no longer sits in +`config/rbac/role.yaml`: it moved to a ClusterRole of its own, +[`config/rbac/watch-any-role.yaml`](../../config/rbac/watch-any-role.yaml) (the Helm equivalent is +`rbac.watchTypes.mode: any`). ```yaml - apiGroups: ['*'] @@ -288,8 +291,9 @@ The existing ClusterRole at `config/rbac/role.yaml` already grants wildcard read verbs: [get, list, watch] ``` -This covers `wardle.example.com` flunders and fischers. No additional RBAC overlay is needed for -the gitops-reverser controller. +This covers `wardle.example.com` flunders and fischers, so no additional RBAC overlay is needed. An +install that narrowed `watchTypes` to `selected` would have to name them explicitly; the e2e setup +does not. ### Integration into `prepare-e2e` diff --git a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md index 216836721..6b38bc4b0 100644 --- a/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md +++ b/docs/design/manifest/gitpathaccepted-projection-race-and-external-drift.md @@ -33,7 +33,7 @@ is and is not the cause, then the design direction. > 5-minute `RequeueSteadyInterval`, so the happy-path GitTarget reconcile requeues after > **5 minutes, not 10** (§1.5, §2). The projection-race analysis and its GitPath-channel fix > are otherwise unaffected. See -> [`../../future/secret-value-retention-plan.md`](../../future/secret-value-retention-plan.md). +> [`../../security-model.md`](../../security-model.md). ### 1.1 The failure diff --git a/docs/design/reconcile-triggering.md b/docs/design/reconcile-triggering.md index 1441c39bf..b426b5ee6 100644 --- a/docs/design/reconcile-triggering.md +++ b/docs/design/reconcile-triggering.md @@ -36,7 +36,7 @@ Facts first, then the fix, then the gaps, then the toolbox. > Secrets, so out-of-band credential/age-key changes are picked up on this unified steady > cadence instead of via a Secret informer. Where the narrative below says "up to 10 > minutes," read 5 minutes. See -> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). +> [`security-model.md`](../security-model.md). ### 1.2 Per-controller triggers @@ -162,7 +162,7 @@ until the 5-minute steady requeue, so `Ready=False (ConnectionFailed)` lingers t > reads credential and age-key Secrets directly by name and relies on the 5-minute > `RequeueSteadyInterval` to pick up rotations; the GitTarget's former encryption-Secret watch > was also removed. Adding a Secret watch would reintroduce cluster-wide Secret retention. See -> [`future/secret-value-retention-plan.md`](../future/secret-value-retention-plan.md). The +> [`security-model.md`](../security-model.md). The > §5 recommendation to "watch the auth Secret" below is retained only as historical context. --- diff --git a/docs/future/least-privilege-remaining-work.md b/docs/future/least-privilege-remaining-work.md new file mode 100644 index 000000000..595084b50 --- /dev/null +++ b/docs/future/least-privilege-remaining-work.md @@ -0,0 +1,68 @@ +# Least privilege: what is left + +> Status: three open items. Everything else from the old Secret-handling/RBAC plans has shipped +> and now lives in [`../rbac.md`](../rbac.md) and [`../security-model.md`](../security-model.md). +> Origin: issue [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). + +## Already shipped (do not re-plan) + +The controller holds no Secret values (cache `DisableFor`), runs no control-plane Secret watch, +refreshes on a 5-minute steady reconcile, and passes only public age recipients to SOPS. The +wildcard cluster read moved out of the manager ClusterRole into `rbac.watchTypes`, so an install +can grant read on named types only, and the manager role's Secret verbs are down to +`get,create,update`. + +**The one constraint that shapes everything below:** Kubernetes RBAC is additive. There is no deny +rule and no "everything except Secrets". While `mode: any` is bound, no namespaced Secret `Role` +reduces anything. + +## 1. Never mirror Secrets + +An install should be able to say: *git credentials are controller input; cluster Secrets are not +mirrored output*. Today a `WatchRule` naming `secrets` is honoured. + +Shape: an exclusion policy (`--exclude-resources=secrets`, or a structured counterpart to +`SensitiveResourcePolicy`) that makes the followability funnel refuse the type, explains the refusal +on `WatchRule`/`ClusterWatchRule` status, and never opens the watch. Generated RBAC can then omit +the grant entirely. + +## 2. Permission-aware followability + +The funnel checks the verbs **discovery advertises**, not the verbs this ServiceAccount **holds** +([`internal/typeset/funnel.go`](../../internal/typeset/funnel.go)). Under `mode: selected` a type +can look followable and then `403` when the watch opens. + +Shape: collect effective permissions (`SelfSubjectRulesReview` in bulk, `SelfSubjectAccessReview` +for ambiguous wildcard/aggregated cases), add permitted verbs to the type observation, and fail the +verbs requirement with a `ReasonNotPermitted` surfaced on rule status. + +This is what makes a narrowed install diagnosable instead of mysterious. The API-surface trigger +informers already do the honest thing on `403` — they stop and log once, and re-arm when the +permission is granted — but watched types still churn. + +## 3. Scope the Secret grant, and an RBAC generator + +`secrets: get,create,update` is cluster-scoped because a `GitProvider` may reference a Secret in any +namespace. A `selected` install therefore cannot *enumerate* Secrets but can still read one it can +name. Closing that needs a namespaced `Role` per referenced namespace, which the chart cannot render +without knowing the manifests. + +Which is the same input an **RBAC generator** needs: read `GitProvider`, `GitTarget`, `WatchRule` +and `ClusterWatchRule` manifests and emit the narrow role set — fixed controller grants, Secret +`get` scoped to referenced namespaces, Secret write grants only where `generateWhenMissing` or +signing-key generation applies, and one `get,list,watch` per selected GVR. Manifest input first: it +is reviewable in CI and needs no cluster access. + +Until it exists, the escape hatch is `rbac.create: false` with a hand-written role set. + +## What not to do + +- Do not claim a metadata-only Secret watch lowers RBAC. Kubernetes has no metadata-only Secret + permission; it still needs `get,list,watch`. +- Do not reintroduce a namespace-scoped Secret **value** cache to satisfy a namespace flag. +- Do not add a Secret watch by default. Direct reads plus the 5-minute reconcile are the baseline; + add `--secret-watch-namespaces` only if measured rotation latency demands it, and never let it + become a value cache. +- Do not use `resourceNames` as the model for Secret watches: `list`/`watch` and rotation make it + brittle. +- Do not remove the `any` default. It is the convenient path, and it is honest about its cost. diff --git a/docs/future/publish-age-recipients-as-metadata.md b/docs/future/publish-age-recipients-as-metadata.md new file mode 100644 index 000000000..71d03d160 --- /dev/null +++ b/docs/future/publish-age-recipients-as-metadata.md @@ -0,0 +1,36 @@ +# Publish age recipients as metadata + +> Status: idea, not started. +> Context: the SOPS write path is already public-recipient only (see +> [`../security-model.md`](../security-model.md)). This removes the last reason to read the +> age-key Secret's `data` on a hot path. + +## The idea + +For a **generated** age key the controller already writes `configbutler.ai/age-recipient` on the +Secret. For a **BYO** age-key Secret it re-reads `data` to derive the recipient each time it needs +it, even though only the public half is ever used. + +Derive the recipient once, publish it as a controller-owned annotation +(`configbutler.ai/age-recipients`), and let the write path read recipients from the `GitTarget` +spec plus that annotation. The private key `data` is then read only at bootstrap and rotation. + +## The security rule that makes or breaks it + +**Never trust a user-written recipient annotation.** If an attacker can set it to their own public +key, every Secret mirrored under that `GitTarget` is encrypted to a key they hold — a silent, +complete compromise of the encrypted-at-rest guarantee, with no error anywhere. + +So exactly one of: + +- the controller derives the annotation from the real key material and **overwrites** it on every + reconcile, treating any other value as untrusted; or +- the `GitTarget` spec carries the public recipient directly, and the annotation is a cache keyed + by the Secret's `resourceVersion`. + +Anything that reads a recipient it did not itself derive is wrong. + +## Why it is not urgent + +Reading `data` to take a public half is not a leak — the value never leaves the process and never +reaches `sops`. This is a hot-path and blast-radius improvement, not a fix. diff --git a/docs/future/scoped-rbac-least-privilege-plan.md b/docs/future/scoped-rbac-least-privilege-plan.md deleted file mode 100644 index 8adcfda02..000000000 --- a/docs/future/scoped-rbac-least-privilege-plan.md +++ /dev/null @@ -1,253 +0,0 @@ -# Scoped RBAC and least-privilege install - -> Status: design plan — **not started** (this is Track B). Its runtime prerequisites -> (stop retaining Secret values, drop control-plane Secret watches, 5-minute reconcile -> fallback) shipped in PR -> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208), so the runtime no longer -> *needs* Secret `list/watch` for controller-owned inputs. What remains is the packaging/RBAC -> work below: the default install still *grants* the broad Secret permission and the -> `*/* get,list,watch` wildcard. -> Date: 2026-07-07 (updated 2026-07-08). -> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). -> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). - -## Summary - -This plan is about what the operator's ServiceAccount is allowed to read. It is separate -from the Secret cache cleanup in -[secret-value-retention-plan.md](secret-value-retention-plan.md), which reduces what the -process keeps in memory. - -The key constraint is Kubernetes RBAC: permissions are additive. There is no deny rule and -no "all resources except Secrets" rule. The default install currently grants: - -- core `secrets get,list,watch,create,update,patch`; -- core `namespaces get,list,watch`; -- `*/* get,list,watch` for dynamic watching. - -While the wildcard exists, a namespaced Secret Role does not reduce the ServiceAccount's -cluster-wide Secret read permission. Real least privilege means running without the -wildcard and granting only the resources the install needs. - -## What the reporter wants - -The reported use case is narrower than the default product model: - -- use one Git credential Secret in one namespace; -- do not mirror Kubernetes Secrets into Git at all; -- avoid cluster-wide Secret reads; -- keep SOPS encryption one-way and avoid private material on the write path. - -The right answer is not only `--secret-namespaces`. That flag name also implies a scoped -value cache, which is not the target after the retention cleanup. The better plan is: - -1. ✅ stop retaining Secret values in the controller process (PR #208); -2. ✅ drop control-plane Secret watches and use a 5-minute reconcile fallback (PR #208); -3. ⬜ add a mode that refuses to mirror Secrets; -4. ⬜ support BYO-RBAC without the wildcard; -5. ⬜ generate the narrow RBAC needed for a specific install. - -## Current RBAC sources - -The generated ClusterRole is in -[config/rbac/role.yaml](../../config/rbac/role.yaml) and mirrored into the chart at -[charts/gitops-reverser/config/role.yaml](../../charts/gitops-reverser/config/role.yaml). - -Important grants: - -```yaml -- apiGroups: [""] - resources: ["namespaces"] - verbs: ["get", "list", "watch"] -- apiGroups: [""] - resources: ["secrets"] - verbs: ["create", "get", "list", "patch", "update", "watch"] -- apiGroups: ["*"] - resources: ["*"] - verbs: ["get", "list", "watch"] -``` - -Why they exist: - -- The wildcard comes from the dynamic watch manager: - [internal/watch/manager.go](../../internal/watch/manager.go#L33). It lets - `WatchRule` and `ClusterWatchRule` select many current and future resource types without - an RBAC redeploy. -- Secret reads support Git credentials, age encryption configuration, and signing keys. - Secret list/watch is only needed for the current Secret reactivity watch and should go - away in the retention plan. -- Secret writes support generated signing and age-key Secrets. -- namespace reads are needed for namespaced rule resolution. - -## Supported install modes - -### Default mode - -Keep the generated ClusterRole and wildcard. - -Good for convenience and broad discovery. Not least privilege. - -### Hardened static mode - -Operators set: - -```yaml -rbac: - create: false -serviceAccount: - create: false - name: existing-service-account -``` - -They bind that ServiceAccount to a hand-written or generated role set. This already works -at the packaging layer. What is missing is documentation and better runtime behavior when -a rule selects something not granted. - -### Future dynamic mode - -The operator reconciles its own Roles and RoleBindings from active rules. This is not a -near-term target. It requires RBAC write permission, privilege-escalation safeguards, and -an explicit opt-in. - -## Plan - -### 1. Document BYO-RBAC - -Write an operator-facing recipe for hardened static installs: - -- disable chart-created RBAC and ServiceAccount; -- create a ServiceAccount owned by the platform team; -- grant fixed controller permissions; -- grant variable watch permissions based on the install's rules; -- document the trade-off: no wildcard means no automatic access to new resource types. - -Fixed permissions include: - -- GitOps Reverser CRDs, `/status`, and `/finalizers`; -- leader-election leases only if leader election is enabled. It is not enabled today, and - the generated role does not currently grant `coordination.k8s.io/leases`; -- events if emitted; -- namespace reads for namespaced rule resolution; -- scoped Secret `get` for referenced Git credentials and age-key inputs; -- scoped Secret writes for generated signing and age-key material; -- Secret `list/watch` only if optional fast-rotation metadata watches are enabled. - -Variable permissions include: - -- one `get,list,watch` rule per selected GVR; -- Role when every selection is namespace-scoped; -- ClusterRole when the selected resource or selection scope is cluster-wide. - -### 2. Add "never mirror Secrets" - -Add an exclusion policy, for example: - -```text ---exclude-resources=secrets -``` - -or a structured counterpart to `SensitiveResourcePolicy`. - -Behavior: - -- Secrets are refused by the followability funnel; -- `WatchRule` or `ClusterWatchRule` status explains the refusal; -- the dynamic target watch path never opens a Secret watch; -- generated RBAC can omit mirrored-Secret read grants. - -This serves the reporter's strongest requirement: Git credentials remain controller input, -but cluster Secrets are not mirrored output. - -### 3. Make followability permission-aware - -The current funnel checks discovery-advertised verbs, not the ServiceAccount's actual -authorization: -[internal/typeset/funnel.go](../../internal/typeset/funnel.go#L16). - -With narrow RBAC, a type can look followable in discovery and then fail with `403 -Forbidden` when the dynamic watch opens. That should become a clean refusal. - -Implementation shape: - -- collect effective permissions for relevant namespaces and cluster scope; -- prefer `SelfSubjectRulesReview` for bulk rules; -- use `SelfSubjectAccessReview` as a fallback for ambiguous wildcard or aggregated cases; -- add permitted verbs to the type observation; -- fail the verbs requirement with a new reason such as `ReasonNotPermitted`; -- surface the reason on `WatchRule` and `ClusterWatchRule` status. - -This is not required to remove Secret values from the cache, but it is required to make -least-privilege installs pleasant and diagnosable. - -### 4. Add an RBAC generator - -Add a command or task that reads manifests or live cluster state and emits the narrow role -set for: - -- `GitProvider`; -- `GitTarget`; -- `WatchRule`; -- `ClusterWatchRule`. - -Output: - -- fixed controller grants; -- Secret input grants for referenced Git credentials and age-key Secrets; -- generated Secret write grants where `generateWhenMissing` or signing-key generation can - create/update Secrets; -- one watch grant per selected GVR; -- no `*/*` wildcard when `--no-wildcard` is selected. - -Start with manifest input. It is reviewable in CI and does not require cluster access. -Live-cluster input can come later. - -### 5. Use a 5-minute control-plane reconcile fallback - -The hardened default should not need Secret watches for controller-owned inputs. Instead, -control-plane controllers should use a common 5-minute periodic reconcile: - -- credential or age-key rotation is picked up within 5 minutes if no related work happens - sooner; -- branch workers still read credentials directly when work is happening; -- generated-key recovery and recipient annotation refresh can wait for the next GitTarget - reconcile; -- RBAC for controller-owned Secret inputs can drop `list/watch` and keep scoped `get`. - -This does not affect data-plane mirroring watches. Those still need `get,list,watch` for -the resources the user chooses to mirror. - -### 6. Optional: namespace-scope Secret metadata watches - -After [secret-value-retention-plan.md](secret-value-retention-plan.md) switches to direct -reads and a 5-minute reconcile fallback, metadata watches can be added back only for -installs that need faster Secret rotation. Avoid adding a public flag first if a generated -static namespace set is enough. - -```text ---secret-watch-namespaces=ns1,ns2 -``` - -If a flag is needed, prefer `--secret-watch-namespaces` over the issue's -`--secret-namespaces`: it says exactly what is scoped. This should not create a full -Secret value cache. It only scopes rotation reactivity. The matching RBAC still needs -normal Secret `get,list,watch` permissions in those namespaces. - -## What not to do - -- Do not claim metadata-only watch lowers RBAC. It does not. -- Do not reintroduce a namespace-scoped full Secret cache as the primary fix. -- Do not require a Secret metadata watch by default when a 5-minute reconcile is enough. -- Do not claim a namespaced Secret Role helps while `*/* get,list,watch` is still bound. -- Do not replace the default wildcard install for everyone. -- Do not use `resourceNames` as the main model for Secret watches; list/watch and rotation - behavior make that brittle. - -## Done - -This plan is complete when: - -- hardened static install docs exist; -- users can opt out of mirroring Secrets; -- missing watch permissions surface as rule status, not unexplained watch churn; -- an RBAC generator can produce a no-wildcard role set for a concrete install; -- public docs clearly distinguish RBAC blast radius from in-process Secret exposure. diff --git a/docs/future/secret-handling-roadmap.md b/docs/future/secret-handling-roadmap.md deleted file mode 100644 index b27c3afb3..000000000 --- a/docs/future/secret-handling-roadmap.md +++ /dev/null @@ -1,120 +0,0 @@ -# Secret handling roadmap - -> Status: roadmap. **Track A (process/crypto exposure) is implemented; Track B (RBAC blast -> radius) is not started.** -> Date: 2026-07-07 (updated 2026-07-08). -> Related issue: [#205](https://github.com/ConfigButler/gitops-reverser/issues/205). -> Detailed plans: -> [Secret value retention](secret-value-retention-plan.md) and -> [Scoped RBAC and least-privilege install](scoped-rbac-least-privilege-plan.md). - -> **Progress.** PR [#208](https://github.com/ConfigButler/gitops-reverser/pull/208) -> delivered Track A: the controller no longer retains Secret values in memory, no longer -> watches Secrets on the control plane, reconciles on a unified 5-minute fallback, and keeps -> the SOPS write path public-recipient-only. This closes the **process-exposure** and -> **rotation-reaction** axes of the table below. The **RBAC blast radius** axis — the literal -> "limit secrets permissions" ask in #205 — is Track B and still open: the default install -> still grants `secrets get;list;watch` plus the dynamic-watch wildcard. - -## Why this exists - -The issue asks for `--secret-namespaces=ns1,ns2` so the operator stops watching every -Secret in the cluster. That request is directionally right, but it mixes three different -problems: - -| Axis | Question | Main fix | -|---|---|---| -| Process exposure | What Secret values does the controller hold in memory? | Direct Secret reads only | -| Rotation reaction | How fast do changed credentials or age keys take effect? | 5-minute reconcile fallback | -| RBAC blast radius | What is the ServiceAccount allowed to read? | Drop the wildcard and generate narrow RBAC | - -Those fixes should not be shipped as one large feature. The first two are small and give -the biggest immediate security win. RBAC narrowing is a separate install mode with a real -trade-off: no wildcard means no automatic access to newly selected resource types. - -The important update: control-plane Secret watches are not required. Mirroring still needs -data-plane watches for selected resources, and controllers should still watch our own CRDs. -But Git credentials and age-key inputs can be read directly and refreshed by a 5-minute -periodic reconcile instead of a Secret informer. - -## Current state - -The operator has three Secret paths today: - -- `GitTarget` registers a full-object Secret watch to react to generated age Secret - changes. With the default controller-runtime cache, that means every Secret value in - every namespace can be held in memory. -- `GitProvider`, branch workers, encryption setup, and signing-key helpers read specific - Secrets through the cached client. -- Mirrored Secrets selected by a `WatchRule` or `ClusterWatchRule` use the dynamic watch - path. That path is about user-selected resources, not the controller's own credential - inputs. - -The default chart also grants `*/* get,list,watch` for dynamic watching. Because -Kubernetes RBAC is additive, a namespaced Secret Role does not reduce the ServiceAccount's -Secret read permission while that wildcard remains. - -## Decision - -Treat this as two tracks. - -**Track A: stop retaining Secret values.** This is the first implementation track. It -keeps the default broad install behavior, but changes the process footprint: - -- delete control-plane Secret watches instead of converting them to metadata; -- bypass the cache for typed Secret reads; -- read referenced Secret values directly by name; -- standardize control-plane periodic reconciles at 5 minutes; -- remove private age identities from the SOPS encrypt path. - -This solves the immediate "the process caches every Secret value forever" problem and -removes the need for Secret list/watch on controller-owned inputs in the default mode. - -**Track B: support narrow installs.** This is the install/RBAC track: - -- document bring-your-own RBAC; -- add a "never mirror Secrets" policy; -- make followability permission-aware so missing grants become status refusals rather than - runtime watch churn; -- add a generator for minimal Roles and ClusterRoles. - -This is what reduces what the ServiceAccount is allowed to read. - -## Recommended order - -Steps 1–3 are **done** (PR -[#208](https://github.com/ConfigButler/gitops-reverser/pull/208)); steps 4–6 are Track B and -**not started**; step 7 is optional and not needed while the 5-minute fallback suffices. - -1. ✅ **Stop value caching and drop Secret dependency watches.** - Implemented the small controller/client changes in - [secret-value-retention-plan.md](secret-value-retention-plan.md). This makes Secret - `list/watch` unnecessary for controller-owned inputs; packaging cleanup follows in the - RBAC track. -2. ✅ **Remove private age identity handling from the encrypt path.** - SOPS encryption is now public-recipient only. Small deletion, strong security payoff. -3. ✅ **Set control-plane reconcile fallback to 5 minutes.** - `RequeueSteadyInterval` is now the common periodic reconcile cadence for control-plane - controllers after removing Secret watches. -4. ⬜ **Add `--exclude-resources=secrets` or equivalent.** - This lets an install say: Git credentials are controller input; Kubernetes Secrets are - not mirrored output. -5. ⬜ **Document BYO-RBAC and add permission-aware followability.** - This makes hand-narrowed RBAC predictable. -6. ⬜ **Add an RBAC generator.** - Generate the narrow role set from `GitProvider`, `GitTarget`, `WatchRule`, and - `ClusterWatchRule` manifests. -7. ⬜ **Optional: add Secret metadata watches for faster rotation.** - Do this only if the 5-minute fallback is not responsive enough. Prefer a namespace set - derived from manifests or generated Helm values before adding a runtime flag. - -## Non-goals - -- Do not replace the default wildcard install for everyone. The default remains convenient. -- Do not reintroduce a full Secret value informer just to support a namespace flag. -- Do not add a Secret metadata watch by default; direct reads plus a 5-minute reconcile are - the simpler baseline. -- Do not add a namespace flag before proving generated or inferred configuration is not - enough. -- Do not add a long-lived in-process Secret value TTL cache unless an install measures API - pressure and explicitly opts in. diff --git a/docs/future/secret-value-retention-plan.md b/docs/future/secret-value-retention-plan.md deleted file mode 100644 index f5db818e3..000000000 --- a/docs/future/secret-value-retention-plan.md +++ /dev/null @@ -1,265 +0,0 @@ -# Stop retaining Secret values - -> Status: **implemented** (steps 1–6) in PR -> [#208](https://github.com/ConfigButler/gitops-reverser/pull/208). Step 7 (publish -> recipients as metadata) remains future work. -> Date: 2026-07-07 (implemented 2026-07-08). -> Roadmap: [Secret handling roadmap](secret-handling-roadmap.md). -> Scope: remove full Secret values from the controller-runtime cache, keep direct reads -> fresh, drop control-plane Secret watches, and remove private age identities from the SOPS -> encrypt path. - -> **What shipped.** The manager sets `Client.Cache.DisableFor: corev1.Secret`, so typed -> Secret reads bypass the cache and no informer retains Secret values. The GitTarget -> control-plane Secret watch was removed. The short/medium/long requeue intervals collapsed -> into one 5-minute `RequeueSteadyInterval`. The SOPS write path carries public age -> recipients only — no `SOPS_AGE_KEY_FILE`, no identity temp file, no blanket Secret-data -> env. And (step 5) the branch worker resolves Git credentials once per push cycle instead -> of per commit. RBAC narrowing is deliberately **not** part of this — it stays in -> [scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). - -## Summary - -This plan is the first implementation slice of the Secret handling work. It does not try -to solve RBAC. It solves the process-level problem: the controller should not keep every -Secret value in the cluster in memory. - -The target state: - -- no controller-runtime informer stores full `corev1.Secret` objects; -- no controller-runtime Secret informer is started for control-plane dependencies; -- typed Secret reads go directly to the API server; -- external Secret changes are noticed by direct reads and a 5-minute reconcile fallback; -- SOPS encryption uses public age recipients only; -- no private age identity is written to disk or passed to the `sops` process. - -TTL is a fallback load optimization, not the design. - -## Current problem - -The manager is created without cache options: -[cmd/main.go](../../cmd/main.go#L841). Any informer started by the shared cache is -cluster-wide unless configured otherwise. - -`GitTarget.SetupWithManager` registers a full Secret watch: -[gittarget_controller.go](../../internal/controller/gittarget_controller.go#L965). -The map function only needs the Secret name and namespace, but the current watch stores -full Secret objects, including `data`. - -Several code paths also read specific Secrets through the cached client: - -- GitProvider credentials: - [gitprovider_controller.go](../../internal/controller/gitprovider_controller.go#L262) -- branch-worker credentials: - [branch_worker.go](../../internal/git/branch_worker.go#L504) -- age encryption Secret: - [encryption.go](../../internal/git/encryption.go#L173) -- signing keys: - [helpers.go](../../internal/git/helpers.go#L37) - -Result: one reactivity watch can make the process retain cleartext Secret values for the -whole cluster, even though the operator only needs a small number of named Secrets. - -## Target model - -Use one default access mode for controller-owned Secret inputs: - -- **Read values directly** only when code actually needs a specific Secret. -- **Reconcile every 5 minutes** so out-of-band Secret changes are picked up without a - Secret watch. - -```mermaid -flowchart TB - Timer["5-minute reconcile fallback"] - Reconcile["GitProvider / GitTarget / branch worker"] - DirectRead["direct Secret GET
specific name + namespace"] - API["kube-apiserver"] - - Timer --> Reconcile - Reconcile --> DirectRead - DirectRead --> API -``` - -This gives up instant Secret-rotation reaction in exchange for a cleaner default: no -Secret list/watch, no metadata informer, no namespace flag, and no Secret values retained -by the controller cache. Mirroring watches are separate and remain required for selected -resources. - -## Implementation steps - -### 1. Delete the GitTarget Secret watch - -Remove the control-plane Secret watch from `GitTarget.SetupWithManager`: - -```go -// internal/controller/gittarget_controller.go -- Watches(&corev1.Secret{}, handler.EnqueueRequestsFromMapFunc(r.encryptionSecretToGitTargets)). -``` - -Do not replace it with `WatchesMetadata` by default. Generated age Secret recovery and -out-of-band age-key updates can wait for the 5-minute GitTarget reconcile. That is a -better least-privilege baseline than keeping a cluster-wide Secret metadata watch. - -### 2. Disable cached reads for typed Secrets - -Add `Client.Cache.DisableFor` in `newManager`: - -```go -mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ - Scheme: scheme, - Metrics: metricsOptions, - HealthProbeBindAddress: probeAddr, - WebhookServer: webhookServer, - Client: client.Options{ - Cache: &client.CacheOptions{ - DisableFor: []client.Object{&corev1.Secret{}}, - }, - }, -}) -``` - -This keeps typed Secret reads live and prevents a typed Secret `Get` from starting a -full-object Secret cache. - -### 3. Keep GitProvider credential rotation pull-based - -Do not add a GitProvider Secret watch by default. `GitProvider` should revalidate on its -own spec changes and on the common 5-minute periodic reconcile. - -Branch workers also read credentials directly when they need them, so writes can pick up -new credentials on the next operation. GitProvider status may lag until the next 5-minute -validation pass, which is acceptable for the default least-privilege mode. - -### 4. Standardize control-plane reconcile fallback at 5 minutes - -After removing Secret watches, use 5 minutes as the common control-plane periodic -reconcile cadence. This replaces the old split where fast paths used 2 minutes, some -Secret/config failures used 5 minutes, and steady validation could wait up to 10 minutes. -In code terms, collapse the control-plane uses of the short, medium, and long requeue -intervals to a 5-minute steady fallback for GitProvider, GitTarget, WatchRule, and -ClusterWatchRule reconcilers. - -The intent is simple to explain: - -- no Secret watch by default; -- external Secret changes become visible within 5 minutes through reconcile; -- direct reads still use the freshest Secret value when work is already happening. - -### 5. Remove redundant credential reads where practical - -The branch worker reads the same credentials Secret several times during a push cycle. -Prefer a structural fix: - -- resolve auth once at the start of a flush or push operation; -- pass the resolved auth through clone, fetch, push, force-push, and retry code; -- keep direct reads as the default. - -Only add a short TTL memo if an install measures API pressure. If added, keep it opt-in -and off by default. A long TTL recreates stale-credential behavior. - -Direct reads are acceptable because the operator reads a small set of named Secrets, not -every object during every reconcile. A direct `GET` is real API/etcd work, so avoid tight -loops; the structural read-once change above is the preferred load fix. - -### 6. Stop passing private age identities to SOPS - -The write path only encrypts. It does not decrypt. - -Evidence in the current code: - -- `Encryptor` only exposes `Encrypt`: - [encryption.go](../../internal/git/encryption.go#L35) -- `SOPSEncryptor.Encrypt` runs `sops --encrypt`: - [sops_encryptor.go](../../internal/git/sops_encryptor.go#L43) -- encrypted-content reuse compares plaintext digest plus Kubernetes identity markers and - never decrypts Git content: - [content_writer.go](../../internal/git/content_writer.go#L87) - -age encryption needs public recipients. The private age identity is for decryption, and -this operator does not decrypt. So remove it from the hot path: - -- drop `AgeIdentities` from `ResolvedEncryptionConfig`; -- stop returning `secretIdentities` from `ResolveTargetEncryption`; -- remove `writeAgeIdentityFile`, `SOPS_AGE_KEY_FILE`, and the age identity temp-file path; -- remove blanket `toSOPSEnvironment(secret.Data)` for the age path. - -After this step, the operator may still read a BYO private age key to derive a recipient, -but it does not write the private key to disk or pass it to `sops`. - -### 7. Later: publish recipients as metadata - -This is useful but larger than the first slice. - -For generated age keys, the controller already writes -`configbutler.ai/age-recipient`. For BYO age-key Secrets, the controller can derive the -recipient once and publish a controller-owned annotation such as -`configbutler.ai/age-recipients`. - -Then the write path can read recipients from spec plus metadata and avoid reading private -age-key `data` except during bootstrap or rotation. - -Security rule: never blindly trust a user-written recipient annotation. If an attacker can -set the annotation to their public key, mirrored Secrets can be encrypted to a key they -control. The controller must derive and overwrite the annotation from the actual key -material, or the GitTarget spec must carry the public recipient directly. - -## RBAC impact - -The default plan no longer needs `secrets list/watch` for controller-owned credential and -age-key inputs. It still needs scoped `secrets get` for referenced input Secrets, plus -scoped write verbs where the operator generates signing or age-key Secrets. - -The runtime change can land before the generated default RBAC is narrowed. The important -contract is that `list/watch` becomes unnecessary for controller-owned Secret inputs, so -the least-privilege packaging can remove those verbs. - -If optional Secret metadata watches are added later, they use the normal Secret endpoint -and require `secrets get,list,watch` in their scope. Kubernetes RBAC does not provide a -metadata-only Secret permission. - -RBAC narrowing is covered by -[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). - -## Optional fast-rotation mode - -Add Secret metadata watches only if 5-minute rotation reaction is not good enough: - -- first preference: no Secret watch; -- second preference: have the RBAC generator or Helm values derive the static namespace set - from `GitProvider` and `GitTarget` manifests; -- last resort: add an explicit `--secret-watch-namespaces=ns1,ns2` flag for metadata - reactivity. - -If the flag exists, use the more precise name above rather than `--secret-namespaces`. -The original issue name sounds like it scopes a value cache. This plan scopes metadata -reactivity only and must not reintroduce a full Secret value cache. - -## Tests - -- Unit: Secret-to-GitProvider and Secret-to-GitTarget map functions enqueue only matching - objects only if optional metadata watch mode is implemented. -- Unit: SOPS environment no longer includes private age identity material. -- Envtest: typed Secret reads see fresh data after a direct Secret update, proving they do - not come from a stale cache. -- Unit or controller test: control-plane steady reconciles use a 5-minute interval. -- E2E: rotating a Git credential Secret is picked up by direct reads or by GitProvider - status within 5 minutes. -- E2E: rotating an age key Secret is picked up by direct reads or by GitTarget reconcile - within 5 minutes. - -## Done - -All of the following are met as of PR -[#208](https://github.com/ConfigButler/gitops-reverser/pull/208): - -- [x] no full-object Secret informer is created by the manager; -- [x] no Secret metadata informer is created by default; -- [x] typed Secret reads bypass the cache; -- [x] control-plane controllers use a 5-minute fallback reconcile for external changes; -- [x] SOPS encryption no longer receives private age identities; -- [x] docs clearly state the default mode needs Secret `get` for inputs, not Secret - `list/watch` (see [../security-model.md](../security-model.md)). - -Note this is a **runtime** milestone: the default install still *grants* `secrets -get;list;watch` — narrowing that grant is tracked separately in -[scoped-rbac-least-privilege-plan.md](scoped-rbac-least-privilege-plan.md). diff --git a/docs/rbac.md b/docs/rbac.md index 2b8976095..e3902f3d6 100644 --- a/docs/rbac.md +++ b/docs/rbac.md @@ -79,12 +79,27 @@ for you — there is no hand-written role to maintain, and no way to drift from ServiceAccount the operator actually runs as. Result: `GitProvider` Ready, `ClusterWatchRule` Ready with its streams, commits flowing, and -**zero cluster-wide Secret access**. +the reverser **cannot enumerate Secrets** — no `list`, no `watch`, on any namespace. -If the reverser needs a Git credentials Secret in its own namespace, that is a namespaced -`Role`, not a `ClusterRole` — the manager role's `get` on `secrets` is cluster-scoped only -because a `GitProvider` may point at any namespace. A namespaced binding is enough when it -does not. +### What `selected` does not remove + +Be precise about what is left, because RBAC is additive and the manager role is still bound: + +| Verb on `secrets` | Still granted? | Why | +|---|---|---| +| `list`, `watch` | **No** | The operator never enumerates Secrets. Removed from the manager role, and `selected` adds no wildcard to put them back. | +| `get` | Yes, cluster-scoped | It reads the git-creds and age-key Secrets a `GitProvider`/`GitTarget` names — and those may live in any namespace. | +| `create`, `update` | Yes, cluster-scoped | It generates the signing key and, with `generateWhenMissing`, the age key. | + +So a `selected` install can still **read a Secret whose name and namespace it already +knows**. It cannot discover one. That is a real reduction — enumeration is what turns a +mirroring operator into a credential-harvesting one — but it is not zero Secret access, and +this page will not pretend otherwise. + +Scoping `get`/`create`/`update` down to the namespaces a `GitProvider` actually references +needs a namespaced `Role` the chart does not yet render. Until then the escape hatch is +`rbac.create: false` plus your own role set. See +[`future/least-privilege-remaining-work.md`](future/least-privilege-remaining-work.md). ## What happens if a read is denied diff --git a/docs/security-model.md b/docs/security-model.md index 8e9c0b4d9..290280615 100644 --- a/docs/security-model.md +++ b/docs/security-model.md @@ -2,30 +2,78 @@ What GitOps Reverser can access, why, and which pieces are sensitive. Read this before installing. -## Why the controller has broad access - -GitOps Reverser writes the live state of watched resource types into Git. To do that it must: - -- **Read watched resources cluster-wide.** WatchRule and ClusterWatchRule decide which types are - followed, but the controller needs read (get/list/watch) access to those types to materialize - them. Broad WatchRules imply broad read access. -- **Read referenced Secrets.** It reads the Git credentials Secret (and, when encryption is - configured, the SOPS/age key Secret) named by a GitProvider/GitTarget. At **runtime** the - control-plane code paths read these controller-owned input Secrets **directly by name** (`get`): - they no longer `list` or `watch` Secrets and never cache Secret values in memory. Out-of-band - credential or age-key rotations are picked up on the direct read the next time work happens, and - at the latest by the 5-minute periodic reconcile. **This is a runtime-behavior change, not (yet) - an RBAC change:** the default install still *grants* `secrets get;list;watch` (and the - dynamic-watch wildcard), so the ServiceAccount's effective Secret permission is unchanged until - the separate RBAC-narrowing track lands. Mirrored Secrets selected by a WatchRule are a separate - concern and still use the watched-resource read path above. See - [`future/secret-value-retention-plan.md`](future/secret-value-retention-plan.md) and - [`future/scoped-rbac-least-privilege-plan.md`](future/scoped-rbac-least-privilege-plan.md). -- **Receive audit events.** The kube-apiserver audit webhook posts events to the controller's - audit ingress. Those events carry object metadata and, for some resources, request/response - bodies. - -The controller does not need write access to watched resources. Its only write target is Git. +## What the controller is allowed to read + +The ServiceAccount is bound to **two** ClusterRoles, and keeping them apart is the whole point: +Kubernetes RBAC is additive, so a wildcard read folded in beside a narrow Secret rule silently +widens it. + +```mermaid +flowchart LR + SA["ServiceAccount
gitops-reverser"] + + subgraph M["manager-role (generated from the code)"] + M1["its own CRDs and status"] + M2["namespaces, CRDs, apiservices
get, list, watch"] + M3["secrets
get, create, update
no list, no watch"] + end + + subgraph W["watch-any or watch-selected (you choose)"] + W1["mode any
every resource: get, list, watch"] + W2["mode selected
only the types you name"] + end + + SA --> M + SA --> W +``` + +To mirror live state into Git the controller must: + +- **Read the watched types.** `WatchRule`/`ClusterWatchRule` pick them; the controller needs + `get,list,watch` on each. `rbac.watchTypes.mode: any` grants that for every type at once — + convenient, and it means the reverser can enumerate **every Secret in the cluster**. `mode: + selected` grants only the types you name. See [`rbac.md`](rbac.md). +- **Read referenced Secrets.** The git-creds Secret, and the SOPS/age key Secret when encryption + is configured, both named by a `GitProvider`/`GitTarget`. This is a `get` by name — never a + `list` or a `watch`. +- **Receive audit events.** The kube-apiserver audit webhook posts events to the audit ingress. + Those carry object metadata and, for some resources, request/response bodies. + +The controller never writes to a watched type. Its only write target is Git, plus the Secrets it +generates itself (the signing key, and the age key under `generateWhenMissing`). + +## The controller does not hold Secret values + +There is **no Secret informer**. The manager is built with `Client.Cache.DisableFor: +corev1.Secret` ([`cmd/main.go`](../cmd/main.go)), so a typed Secret read bypasses the cache and +goes straight to the API server. No control-plane Secret watch exists; the `GitTarget` one was +removed. + +```mermaid +flowchart TB + Timer["5-minute steady reconcile
RequeueSteadyInterval"] + Work["work happens:
a push, a flush, a reconcile"] + Read["direct GET
one Secret, by name and namespace"] + API["kube-apiserver"] + Cache["controller cache
never holds Secret values"] + + Timer --> Work + Work --> Read + Read --> API + Read -.->|bypasses| Cache +``` + +The trade is deliberate: no instant reaction to a rotated credential, in exchange for a process +that cannot leak Secret values it was never asked to hold. A rotated git credential or age key is +picked up by the next direct read when work happens, and at the latest by the 5-minute reconcile. + +This is why the manager role asks for `secrets: get, create, update` and nothing more — the +runtime genuinely does not need `list` or `watch`. What it does **not** do is scope those verbs to +a namespace: a `GitProvider` may reference a Secret anywhere, so the grant is cluster-scoped. A +`selected` install therefore cannot *discover* a Secret, but can still read one whose name and +namespace it already knows. Narrowing that further is +[remaining work](future/least-privilege-remaining-work.md). + ## Sensitive trust boundaries @@ -41,11 +89,17 @@ The controller does not need write access to watched resources. Its only write t Without encryption, a watched `Secret` is committed as-is (its data is plain in the repository). With SOPS + age (`GitTarget.spec.encryption`), Secret values are encrypted before commit using the age -recipients, and the private key never leaves the cluster. Because the write path only encrypts, it -uses **public age recipients only**: the private age identity is never written to disk or passed to -the `sops` process, even when the recipient is derived from a cluster age-key Secret. So: only watch -`Secret` types you intend to commit, and prefer encryption. Secret-shaped custom resource types can opt into the same -encryption path at controller startup. See [`sops-age-guide.md`](sops-age-guide.md). +recipients, and the private key never leaves the cluster. + +The write path **only encrypts** — nothing in the operator decrypts Git content — so it needs +**public age recipients only**. Concretely: no private identity is written to disk, `SOPS_AGE_KEY_FILE` +is never set, and Secret `data` is never handed to the `sops` process as environment. That holds even +when the recipient is derived from a cluster age-key Secret, which the controller reads once to take +the public half. + +So: only watch `Secret` types you intend to commit, and prefer encryption. Secret-shaped custom +resource types can opt into the same encryption path at controller startup. See +[`sops-age-guide.md`](sops-age-guide.md). ## Git credentials Secret shape