diff --git a/cmd/credential-provider/kubernetes-secrets/kubeprovider.go b/cmd/credential-provider/kubernetes-secrets/kubeprovider.go index d6e84057bc..3f47e00c14 100644 --- a/cmd/credential-provider/kubernetes-secrets/kubeprovider.go +++ b/cmd/credential-provider/kubernetes-secrets/kubeprovider.go @@ -133,6 +133,10 @@ func NewServer(client kubernetes.Interface, nsAuth *NamespaceAuthorizer) *Server // when authorization is disabled. func (s *Server) Grants() map[string][]string { return s.nsAuth.Grants() } +// PolicyError reports why the policy file failed to load while an older +// policy stays in force, for /statusz. +func (s *Server) PolicyError() error { return s.nsAuth.LoadError() } + // FetchSecret resolves one ate-secret:// URI to its Secret value. func (s *Server) FetchSecret(ctx context.Context, req *credproviderpb.FetchSecretRequest) (*credproviderpb.FetchSecretResponse, error) { ref, err := ParseURI(req.GetUri()) diff --git a/cmd/credential-provider/kubernetes-secrets/kubeprovider_test.go b/cmd/credential-provider/kubernetes-secrets/kubeprovider_test.go index 57a67f8d09..35a87037d1 100644 --- a/cmd/credential-provider/kubernetes-secrets/kubeprovider_test.go +++ b/cmd/credential-provider/kubernetes-secrets/kubeprovider_test.go @@ -16,7 +16,9 @@ package main import ( "context" + "fmt" "os" + "path/filepath" "strings" "testing" @@ -456,6 +458,105 @@ func TestLoadNamespaceAuthorizerRejectsUnknownFields(t *testing.T) { } } +// The provider rereads its policy when the file changes, and keeps the last +// good policy while the file fails to load. +func TestLoadNamespaceAuthorizerReloads(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "policy.yaml") + dataLink := filepath.Join(dir, "..data") + // path resolves through the ..data symlink, and each step swaps that + // symlink to a new directory, the way the kubelet updates a ConfigMap + // volume. + if err := os.Symlink(filepath.Join("..data", "policy.yaml"), path); err != nil { + t.Fatalf("link policy: %v", err) + } + version := 0 + replace := func(t *testing.T, content string) { + t.Helper() + version++ + target := fmt.Sprintf("..v%d", version) + if err := os.Mkdir(filepath.Join(dir, target), 0o700); err != nil { + t.Fatalf("create policy dir: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, target, "policy.yaml"), []byte(content), 0o600); err != nil { + t.Fatalf("write policy: %v", err) + } + tmp := filepath.Join(dir, "..data_tmp") + if err := os.Symlink(target, tmp); err != nil { + t.Fatalf("link policy dir: %v", err) + } + if err := os.Rename(tmp, dataLink); err != nil { + t.Fatalf("swap policy dir: %v", err) + } + } + replace(t, "policies:\n- atespace: team-a\n allowedNamespaces: [ns1]\n") + authz, err := LoadNamespaceAuthorizer(path) + if err != nil { + t.Fatalf("LoadNamespaceAuthorizer: %v", err) + } + + steps := []struct { + name string + // content replaces the file; empty removes it. + content string + wantAllowed []string + wantRefused []string + wantErr bool + }{ + { + name: "grant moves to another namespace", + content: "policies:\n- atespace: team-a\n allowedNamespaces: [ns2]\n", + wantAllowed: []string{"ns2"}, + wantRefused: []string{"ns1"}, + }, + { + name: "malformed file keeps the previous policy", + content: "policies:\n- atespace: team-a\n allowedNamespace: [ns3]\n", + wantAllowed: []string{"ns2"}, + wantRefused: []string{"ns3"}, + wantErr: true, + }, + { + name: "fixed file loads", + content: "policies:\n- atespace: team-a\n allowedNamespaces: [ns3]\n", + wantAllowed: []string{"ns3"}, + wantRefused: []string{"ns2"}, + }, + { + name: "missing file keeps the previous policy", + wantAllowed: []string{"ns3"}, + wantErr: true, + }, + { + name: "file comes back", + content: "policies: []\n", + wantRefused: []string{"ns3"}, + }, + } + for _, step := range steps { + if step.content == "" { + if err := os.Remove(dataLink); err != nil { + t.Fatalf("%s: remove policy: %v", step.name, err) + } + } else { + replace(t, step.content) + } + for _, ns := range step.wantAllowed { + if !authz.Allowed("team-a", ns) || !authz.AllowedSecret("team-a", ns, nil) { + t.Errorf("%s: team-a/%s refused, want allowed", step.name, ns) + } + } + for _, ns := range step.wantRefused { + if authz.Allowed("team-a", ns) || authz.AllowedSecret("team-a", ns, nil) { + t.Errorf("%s: team-a/%s allowed, want refused", step.name, ns) + } + } + if err := authz.LoadError(); (err != nil) != step.wantErr { + t.Errorf("%s: LoadError() = %v, want error %v", step.name, err, step.wantErr) + } + } +} + // Each policy is its own grant and they are OR-ed. Merging them into one would // let a narrow policy take away what a broad one granted, and would AND the // selectors of separate policies together. diff --git a/cmd/credential-provider/kubernetes-secrets/main.go b/cmd/credential-provider/kubernetes-secrets/main.go index 544fff4ceb..967d3bc140 100644 --- a/cmd/credential-provider/kubernetes-secrets/main.go +++ b/cmd/credential-provider/kubernetes-secrets/main.go @@ -110,7 +110,6 @@ func run(ctx context.Context) error { if err != nil { return fmt.Errorf("namespace policy: %w", err) } - slog.InfoContext(ctx, "loaded namespace authorization policy", slog.String("file", *nsPolicyFile)) creds, err := buildServerCreds(ctx) if err != nil { diff --git a/cmd/credential-provider/kubernetes-secrets/nsauthz.go b/cmd/credential-provider/kubernetes-secrets/nsauthz.go index 2b7cee3115..cbbc51d4a1 100644 --- a/cmd/credential-provider/kubernetes-secrets/nsauthz.go +++ b/cmd/credential-provider/kubernetes-secrets/nsauthz.go @@ -16,8 +16,10 @@ package main import ( "fmt" + "log/slog" "os" "sort" + "sync" "k8s.io/apimachinery/pkg/util/validation" @@ -69,8 +71,20 @@ type grant struct { // NamespaceAuthorizer decides whether an atespace may resolve secrets in a given // Kubernetes namespace. It is default-deny: an atespace absent from the mapping -// can resolve nothing. +// can resolve nothing. One loaded from a file rereads the file when it changes. type NamespaceAuthorizer struct { + // path is the policy file; empty when the authorizer was built from an + // already parsed policy and never reloads. + path string + + mu sync.Mutex + // fi is the stat of path taken before its last parse, successful or not, + // so a file that fails to parse is read and logged once rather than per + // request. Nil when path could not be stat'ed, so the next call retries. + fi os.FileInfo + // loadErr is why the file as it is now failed to load; allowed then holds + // the last policy that loaded. + loadErr error // allowed maps atespace -> namespace -> the grants that apply there. Each // policy contributes its own grant and they are OR-ed: a Secret is admitted // when any one of them admits it. Within a grant the criteria are AND-ed, @@ -79,8 +93,19 @@ type NamespaceAuthorizer struct { } // LoadNamespaceAuthorizer reads the YAML policy file at path and builds an -// authorizer, so a malformed file fails startup rather than the first request. +// authorizer that rereads the file whenever it changes. A malformed file fails +// here, at startup; once running, a file that fails to load leaves the last +// good policy in force. func LoadNamespaceAuthorizer(path string) (*NamespaceAuthorizer, error) { + a := &NamespaceAuthorizer{path: path} + if _, err := a.policy(); err != nil { + return nil, err + } + return a, nil +} + +// readNamespacePolicy parses the YAML policy file at path. +func readNamespacePolicy(path string) (*NamespaceAuthorizer, error) { data, err := os.ReadFile(path) if err != nil { return nil, fmt.Errorf("reading namespace policy file %q: %w", path, err) @@ -133,13 +158,63 @@ func newNamespaceAuthorizer(file namespacePolicyFile) (*NamespaceAuthorizer, err return &NamespaceAuthorizer{allowed: allowed}, nil } +// policy returns the grants in force, first rereading the policy file if its +// stat no longer matches the last one parsed. The kubelet updates a ConfigMap +// volume by swapping in a new file, so the stat changes even when the size +// does not. The error is why the file as it is now failed to load. +func (a *NamespaceAuthorizer) policy() (map[string]map[string][]grant, error) { + a.mu.Lock() + defer a.mu.Unlock() + if a.path == "" { + return a.allowed, nil + } + fi, err := os.Stat(a.path) + if err != nil { + a.fi = nil + return a.allowed, a.loadFailed(fmt.Errorf("reading namespace policy file %q: %w", a.path, err)) + } + if a.fi != nil && os.SameFile(a.fi, fi) && fi.ModTime().Equal(a.fi.ModTime()) && fi.Size() == a.fi.Size() { + return a.allowed, a.loadErr + } + a.fi = fi + next, err := readNamespacePolicy(a.path) + if err != nil { + return a.allowed, a.loadFailed(err) + } + a.allowed, a.loadErr = next.allowed, nil + slog.Info("loaded namespace authorization policy", slog.String("file", a.path)) + return a.allowed, nil +} + +// loadFailed records err as the load error and returns it. It logs err unless +// it repeats the last one or no policy has loaded yet, when +// LoadNamespaceAuthorizer returns it instead. The caller holds a.mu. +func (a *NamespaceAuthorizer) loadFailed(err error) error { + if a.allowed != nil && (a.loadErr == nil || a.loadErr.Error() != err.Error()) { + slog.Error("namespace policy file failed to load, keeping the last policy that loaded", slog.String("file", a.path), slog.Any("err", err)) + } + a.loadErr = err + return err +} + +// LoadError reports why the policy file failed to load, or nil when the +// policy in force is the file's current content. +func (a *NamespaceAuthorizer) LoadError() error { + if a == nil { + return nil + } + _, err := a.policy() + return err +} + // Grants returns the loaded policy as atespace → sorted namespaces func (a *NamespaceAuthorizer) Grants() map[string][]string { if a == nil { return nil } - out := make(map[string][]string, len(a.allowed)) - for atespace, namespaces := range a.allowed { + allowed, _ := a.policy() + out := make(map[string][]string, len(allowed)) + for atespace, namespaces := range allowed { list := make([]string, 0, len(namespaces)) for ns := range namespaces { list = append(list, ns) @@ -154,7 +229,8 @@ func (a *NamespaceAuthorizer) Grants() map[string][]string { // deny: an atespace absent from the mapping, or a namespace not in its list, is // refused. func (a *NamespaceAuthorizer) Allowed(atespace, namespace string) bool { - set, ok := a.allowed[atespace] + allowed, _ := a.policy() + set, ok := allowed[atespace] if !ok { return false } @@ -164,9 +240,11 @@ func (a *NamespaceAuthorizer) Allowed(atespace, namespace string) bool { // AllowedSecret reports whether atespace may resolve a Secret with these // labels in namespace. A grant without a label selector admits every Secret, // including one that does not exist: pass nil labels to ask that question -// before reading anything from Kubernetes. +// before reading anything from Kubernetes. The namespace grant is checked again +// here, so a request that straddles a reload is settled by the newer policy. func (a *NamespaceAuthorizer) AllowedSecret(atespace, namespace string, labels map[string]string) bool { - for _, g := range a.allowed[atespace][namespace] { + allowed, _ := a.policy() + for _, g := range allowed[atespace][namespace] { if len(g.labels) == 0 { return true } diff --git a/cmd/credential-provider/kubernetes-secrets/statusz.go b/cmd/credential-provider/kubernetes-secrets/statusz.go index df586ff021..61c39d5ed5 100644 --- a/cmd/credential-provider/kubernetes-secrets/statusz.go +++ b/cmd/credential-provider/kubernetes-secrets/statusz.go @@ -15,9 +15,9 @@ // The provider's /statusz page, mirroring the atenet router's: one handler // serving an HTML dashboard by default and JSON under ?format=json (or // Accept: application/json). It shows the provider's configuration as the -// running process holds it — notably the authorization policy loaded at -// startup, which the ConfigMap on disk may have since drifted from. Names -// and configuration only, never secret values. +// running process holds it — notably the authorization policy in force, which +// stays at the last version that loaded while the file on disk fails to parse. +// Names and configuration only, never secret values. package main import ( @@ -41,6 +41,7 @@ type statusContext struct { Flags map[string]string `json:"flags"` InjectorSAN string `json:"injector_san"` PolicyGrants map[string][]string `json:"policy_grants"` + PolicyError string `json:"policy_error,omitempty"` } // newStatuszHandler serves /statusz over the provider's state. @@ -57,6 +58,9 @@ func newStatuszHandler(srv *Server) http.HandlerFunc { InjectorSAN: *injectorIdentity, PolicyGrants: srv.Grants(), } + if err := srv.PolicyError(); err != nil { + data.PolicyError = err.Error() + } if strings.Contains(req.Header.Get("Accept"), "application/json") || req.URL.Query().Get("format") == "json" { w.Header().Set("Content-Type", "application/json") @@ -88,7 +92,8 @@ provider: {{.ProviderName}}
required caller SAN: {{.InjectorSAN}}
args: {{.Args}}

-

Namespace policy (as loaded at startup)

+

Namespace policy

+{{if .PolicyError}}

policy file failed to load, previous policy in force: {{.PolicyError}}

{{end}} {{if .PolicyGrants}} diff --git a/docs/egress-credential-injection.md b/docs/egress-credential-injection.md index d402b017a7..6a7602ac37 100644 --- a/docs/egress-credential-injection.md +++ b/docs/egress-credential-injection.md @@ -244,12 +244,10 @@ kubectl -n ate-system edit configmap k8s-credential-provider-namespace-policy - ns1 ``` -The provider loads the policy **once at startup** and does not yet reload it, -so after editing, restart the provider: - -```bash -kubectl -n ate-system rollout restart deployment/k8s-credential-provider -``` +The provider rereads the policy when the kubelet updates the mounted ConfigMap, +which can take a minute or so; no restart is needed. If the edited policy fails +to parse, the provider logs the error and keeps enforcing the previous one; +`/statusz` on the provider shows the error and the grants in force. For a first smoke test, grant your actor's atespace `ns1` as above and apply a sample secret that the URI in the sample policy resolves to: diff --git a/internal/e2e/credprovider.go b/internal/e2e/credprovider.go index bd18c9d819..c575ecf60b 100644 --- a/internal/e2e/credprovider.go +++ b/internal/e2e/credprovider.go @@ -49,8 +49,9 @@ const ( // ConfigureCredentialProvider points the install's k8s-credential-provider at // the credinject fixture: the Secret behind CredentialInjectionURI and the // authorization policy that lets the probe atespaces resolve it. The provider -// Deployment is restarted after the policy ConfigMap is applied because it -// reads the policy once at startup. When the test passes the fixture is +// Deployment is restarted after the policy ConfigMap is applied, which is +// quicker than waiting for the kubelet to update the mounted policy the +// provider rereads. When the test passes the fixture is // removed, the shipped policy put back, and the provider restarted again so // it drops the fixture's grants. // diff --git a/manifests/egress-credential-injection/namespace-policy.yaml b/manifests/egress-credential-injection/namespace-policy.yaml index e90b9c6c32..94a55c6474 100644 --- a/manifests/egress-credential-injection/namespace-policy.yaml +++ b/manifests/egress-credential-injection/namespace-policy.yaml @@ -16,10 +16,7 @@ # atespace may only resolve Secrets whose URI namespace is listed for it here. # Enforcement is default-deny — an atespace absent from this file resolves # nothing. The install creates this ConfigMap only when it is absent, granting -# nothing; the provider needs it to start. -# -# TODO: k8s-credential-provider loads this once at startup, so editing this ConfigMap -# requires restarting the k8s-credential-provider Deployment. Make it reload dynamically. +# nothing; the provider needs it to start, and rereads it when it changes. apiVersion: v1 kind: ConfigMap metadata:
atespaceallowed namespaces