fix(nri): address pre-ship review findings (CRIT-1, CRIT-2, IMP-1, IMP-2, IMP-5)

CRIT-1 — Pod identity is now verified by UID, not just (namespace, name).
Closes a name-reuse race where an attacker could force-delete a pod
between admission and CreateContainer, recreate one with the same name+SA,
and have the plugin fetch credentials for the recreated pod whose UID
doesn't match the NRI sandbox UID. fetchAndBuildMapping now refuses if
NRI sandbox UID != kube-apiserver pod.UID.

CRIT-2 — Removed dead wrap/unwrap path. The pull-not-push refactor
(76c2074) replaced the wrap_token-as-bearer-credential pattern, but left
behind:
  - pkg/vault/vault.go: WrapValues / UnwrapValues (deleted)
  - pkg/vault/wrap_test.go (deleted)
  - pkg/k8smutator/k8smutator_test.go: stubWrapper helper (deleted)
  - pkg/config/config.go: NRIConfig.WrapTokenTTL field (deleted)
  - helm/values.yml, configmaps.yaml, deployment-injector.yaml: nri.wrapTokenTTL
    knob and INJECTOR_NRI_WRAP_TOKEN_TTL env var (deleted)
Operators no longer see a knob that does nothing. Tests no longer
exercise dead code paths.

IMP-1 — Rewrote docs/how-it-works/nri-mode.md to describe the schema v2
pull-not-push design. Old text described the wrap/unwrap flow which no
longer exists. New version: architecture diagram for v2, the three-layer
identity attestation defense (UID + namespace + SA), schema upgrade
path, hardening checklist, accurate trust posture.

IMP-2 — Kyverno policy now blocks hostPath mounts of /run/vault-db-injector
in addition to /var/run/nri and /opt/nri. The credential cache lives there
and was previously not covered by the policy (PSA baseline already covered
it, but defense-in-depth).

IMP-5 — Deleted leaked pkg/nri/.tmp file (saveCache test artifact).

174 unit tests pass. K3D edge suite: 9/9 (A_substitution, B_uri_mode,
C_multi_container, D_init, E_empty_placeholder, F_malformed_json,
G_cache_persistence, H_v1_rejected, I_identity_forge_blocked).

Author: SoulKyu

docs/how-it-works/nri-mode.md

@@ -5,28 +5,59 @@ substitutes them at container creation time via a node-local DaemonSet
 plugin. Credentials never appear in any persisted Kubernetes resource
 (PodSpec, etcd, audit logs, GitOps captures).
 
+## Architecture (schema v2 — pull-not-push)
+
+```
+kube-apiserver
+     ↓ pod admitted with annotation
+     ↓ db-creds-injector.numberly.io/nri-mapping = {
+     ↓   schema:2, db_path, db_role, placeholders, request_id,
+     ↓   pod_namespace, pod_service_account
+     ↓ }   (NO Vault token, NO bearer credential)
+kubelet
+     ↓
+containerd (NRI enabled)
+     ↓ /var/run/nri/nri.sock
+[vault-db-injector NRI plugin DaemonSet]
+     ↓ on CreateContainer:
+     ↓   1. read pod-sandbox annotation, parse NRIMapping
+     ↓   2. GET pod from kube-apiserver — verify UID, namespace, SA
+     ↓      match annotation (defense vs annotation forgery)
+     ↓   3. authenticate to Vault as the plugin's OWN SA token
+     ↓      (k8s auth method)
+     ↓   4. CanIGetRoles for the actual pod identity → confirms the
+     ↓      Vault auth role binds this (namespace, SA)
+     ↓   5. GetDbCredentials — dynamic credential issued, lease tagged
+     ↓      with pod UID for renewer/revoker correlation
+     ↓   6. emit ContainerAdjustment{env: substituted}
+runc
+     ↓ execve with real envp
+app
+```
+
 ## Components
 
-- **Webhook** — generates placeholders, wraps the credential payload as a
-  single-use Vault wrap token (5min TTL by default), attaches both as a
-  pod annotation `db-creds-injector.numberly.io/nri-mapping`.
-- **DaemonSet** — node-local NRI plugin. On `CreateContainer`: reads the
-  annotation, unwraps the token, substitutes placeholders in env vars,
-  returns a `ContainerAdjustment` to containerd. Runs **before runc**, so
-  the new process is born with the real envp.
+- **Webhook** — generates placeholders, stamps the
+  `db-creds-injector.numberly.io/nri-mapping` annotation. Calls Vault
+  CanIGetRoles to fail-fast at admission if the pod's SA isn't bound to
+  the requested role. **Does not** call Vault sys/wrapping/wrap; no
+  credential or token is placed in the PodSpec.
+- **DaemonSet (NRI plugin)** — node-local. Authenticates to Vault using
+  its own ServiceAccount token. Verifies pod identity against the K8s
+  API. Creates the dynamic credential. Substitutes placeholders in the
+  container env at `CreateContainer` (before runc).
 - **Cache** — per-node tmpfs at `/run/vault-db-injector/nri/cache.json`
   persists unwrapped credentials so they survive plugin pod restart but
-  not node reboot. Without persistence, a CrashLoop pod whose plugin DS
-  restarts in the meantime would see the literal placeholder string in
-  env (the wrap token is single-use).
+  not node reboot. A pod whose plugin DS restarts mid-CrashLoop continues
+  to receive the substituted env on retry instead of the placeholder.
 
 ## Failure modes and detection
 
 The plugin emits Prometheus metrics:
 
 - `vdbi_nri_substitutions_total` — successful adjustments emitted
 - `vdbi_nri_unwrap_failures_total{reason}` — labels: `malformed_annotation`,
-  `unwrap_error`
+  `fetch_error` (covers identity mismatch, Vault errors, missing pod, etc.)
 
 ### What can still go wrong
 
@@ -56,24 +87,62 @@ The plugin emits Prometheus metrics:
        summary: NRI plugin not ready on node {{ $labels.node }} — pods with credentials are starting unsubstituted
    ```
 
-2. **Wrap token expired before CreateContainer.** Slow image pull, long
-   pod scheduling delay (> 5 min) → unwrap fails. Plugin logs the error
-   and increments `vdbi_nri_unwrap_failures_total{reason="unwrap_error"}`.
-   Container starts unsubstituted, app crashes with bad credentials,
-   visible. Increase `nri.wrapTokenTTL` if this is recurring on a slow
-   cluster.
+2. **Annotation forgery** (closed by Hunter finding #H6). An attacker
+   with `pods.create` or `pods.update` RBAC can craft an annotation
+   claiming any `pod_namespace`/`pod_service_account`. The plugin
+   defends against this in three layers:
+   - The pod's actual UID (from NRI sandbox) must match
+     `pod.metadata.uid` recorded by kube-apiserver.
+   - The pod's actual namespace and `spec.serviceAccountName` (from
+     kube-apiserver) must match the annotation's claim.
+   - Vault `CanIGetRoles` is called with the K8s-attested identity, not
+     the annotation's, so a mismatched claim fails authorization.
 
 3. **Plugin DS pod and main container restart in the wrong order.** The
    on-disk cache covers this: the second CreateContainer attempt for the
-   same pod UID reuses the stored mapping.
+   same pod UID reuses the stored credential, no second Vault round-trip.
+
+4. **Force-deleted pod cache leak** — a pod deleted with
+   `--grace-period=0 --force` does not fire NRI's `RemovePodSandbox`
+   event. The plugin runs a periodic 5-minute sweep that lists pods on
+   its node via the K8s API and evicts cache entries whose UIDs no
+   longer exist.
+
+## Schema versioning
+
+The plugin only accepts annotations with `"schema":2`. Schema 1 (the
+legacy `wrap_token` design) is rejected with a clear error so an
+operator never silently runs in an inconsistent state during upgrade.
+
+**Upgrade path** — when moving from a v1 webhook + v1 plugin
+deployment to v2:
+
+1. Set `nri.enabled: false` in helm values and apply. New pods now
+   inject literal credentials in PodSpec (legacy mode, byte-identical
+   to pre-NRI behavior).
+2. Upgrade the webhook and plugin Deployment/DaemonSet images together.
+3. Set `nri.enabled: true` and apply.
+
+If you upgrade hot (without disabling NRI), pods admitted by an old
+webhook just before the upgrade will hit the new plugin and be rejected
+with `unsupported nri-mapping schema version 1`. Container starts with
+placeholder, app crashes with bad cred, kubelet restarts it. Within a
+few seconds the new webhook is admitting v2 annotations and recovery
+is automatic — but expect ~30 seconds of pod CrashLoop noise during the
+window. Cleaner to drain.
 
 ## Hardening checklist
 
 - Set resource requests on the DS so it is not OOM-killed on memory pressure
 - Use `priorityClass: system-node-critical` to make eviction less likely
-- Monitor the alert above
-- Set `nri.wrapTokenTTL` higher than the worst-case scheduling + image pull
-  time on your cluster (default 5min is fine for most clusters)
+- Monitor `NRIPluginMissingOnNode` (above) and
+  `vdbi_nri_unwrap_failures_total{reason="fetch_error"}`
+- Apply the Kyverno policy at
+  [helm/policies/kyverno-restrict-nri-socket.yaml](../../helm/policies/kyverno-restrict-nri-socket.yaml)
+  to block hostPath mounts of `/var/run/nri` and `/opt/nri` outside the
+  plugin's namespace
+- On RHEL/CoreOS leave SELinux enforcing; do not run user pods with
+  `seLinuxOptions.type: spc_t`
 
 ## Trust posture
 
@@ -89,3 +158,11 @@ A root-on-node attacker can already read `/proc/<pid>/environ` of every
 container, so the cache adds no new attack surface beyond what root already
 has. The cache is **never on persistent disk** (tmpfs) and **never in
 backups** (`/run` is excluded by every node backup tool).
+
+A pod that mounts hostPath `/run` AND runs as UID 0 (root) can read the
+cache. PSA `restricted` and `baseline` profiles forbid hostPath mounts
+entirely, which is the recommended baseline for user namespaces. The
+Kyverno policy referenced above does not currently include
+`/run/vault-db-injector` because PSA covers it; if you must keep
+`baseline` off and root-on-pod allowed, extend the Kyverno policy
+manually.

helm/policies/kyverno-restrict-nri-socket.yaml

@@ -63,9 +63,14 @@ spec:
               - /var/run/nri/nri.sock
               - /opt/nri
               - /opt/nri/plugins
+              - /run/vault-db-injector
+              - /run/vault-db-injector/nri
             - key: "{{ regex_match('^/var/run/nri/', element.hostPath.path || '') }}"
               operator: Equals
               value: true
             - key: "{{ regex_match('^/opt/nri/', element.hostPath.path || '') }}"
               operator: Equals
               value: true
+            - key: "{{ regex_match('^/run/vault-db-injector/', element.hostPath.path || '') }}"
+              operator: Equals
+              value: true

helm/templates/configmaps.yaml

@@ -71,7 +71,6 @@ data:
     sentryDsn: {{ .Values.vaultDbInjector.configuration.sentryDsn }}
     nri:
       enabled: true
-      wrapTokenTTL: {{ .Values.nri.wrapTokenTTL }}
       socketPath: /var/run/nri/nri.sock
       cachePath: /run/vault-db-injector/nri/cache.json
 {{- end }}

helm/templates/deployment-injector.yaml

@@ -29,8 +29,6 @@ spec:
         env:
         - name: INJECTOR_NRI_ENABLED
           value: "true"
-        - name: INJECTOR_NRI_WRAP_TOKEN_TTL
-          value: {{ .Values.nri.wrapTokenTTL | quote }}
         {{- end }}
         ports:
         - containerPort: 8443

helm/values.yml

@@ -82,8 +82,4 @@ nri:
       cpu: 200m
       memory: 256Mi
   tolerations: []
-  nodeSelector: {}
-  # Vault response-wrapping TTL for the wrap tokens that travel through
-  # the PodSpec. Must outlive the longest expected scheduling + image-pull
-  # delay on the cluster.
-  wrapTokenTTL: 5m
+  nodeSelector: {}

pkg/config/config.go

@@ -2,7 +2,6 @@ package config
 
 import (
 	"os"
-	"time"
 
 	"github.com/cockroachdb/errors"
 
@@ -27,9 +26,8 @@ const (
 // behavior). When true, the webhook wraps every credential and the NRI
 // DaemonSet substitutes placeholders at CreateContainer time.
 type NRIConfig struct {
-	Enabled      bool          `yaml:"enabled" envconfig:"nri_enabled"`
-	WrapTokenTTL time.Duration `yaml:"wrapTokenTTL" envconfig:"nri_wrap_token_ttl"`
-	SocketPath   string        `yaml:"socketPath" envconfig:"nri_socket_path"`
+	Enabled    bool   `yaml:"enabled" envconfig:"nri_enabled"`
+	SocketPath string `yaml:"socketPath" envconfig:"nri_socket_path"`
 	// CachePath is the on-disk JSON cache that persists unwrapped credentials
 	// across plugin DS restarts (hostPath tmpfs). Survives DS pod restart;
 	// cleared on node reboot. Must be writable by the plugin user.
@@ -79,9 +77,8 @@ func NewConfig(configFile string) (*Config, error) {
 		DefaultEngine:     "databases",
 		VaultRateLimit:    30,
 		NRI: NRIConfig{
-			WrapTokenTTL: 5 * time.Minute,
-			SocketPath:   "/var/run/nri/nri.sock",
-			CachePath:    "/run/vault-db-injector/nri/cache.json",
+			SocketPath: "/var/run/nri/nri.sock",
+			CachePath:  "/run/vault-db-injector/nri/cache.json",
 		},
 	}
 	if configFile != "" {

pkg/config/config_test.go

@@ -3,7 +3,6 @@ package config
 import (
 	"os"
 	"testing"
-	"time"
 
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
@@ -397,8 +396,8 @@ vaultSecretPrefix: prefix
 
 func TestNRIConfig_Defaults(t *testing.T) {
 	cfg := newConfigForNRITest(t)
-	assert.Equal(t, 5*time.Minute, cfg.NRI.WrapTokenTTL)
 	assert.Equal(t, "/var/run/nri/nri.sock", cfg.NRI.SocketPath)
+	assert.Equal(t, "/run/vault-db-injector/nri/cache.json", cfg.NRI.CachePath)
 }
 
 func TestNRIConfig_LoadsExplicitValues(t *testing.T) {
@@ -413,7 +412,6 @@ vaultSecretName: secret
 vaultSecretPrefix: prefix
 nri:
   enabled: true
-  wrapTokenTTL: 10m
   socketPath: /custom/nri.sock
 `
 	_, err = tmpfile.WriteString(y)
@@ -423,7 +421,7 @@ nri:
 	for _, k := range []string{
 		"INJECTOR_MODE", "INJECTOR_VAULT_ADDRESS", "INJECTOR_VAULT_AUTH_PATH",
 		"INJECTOR_KUBE_ROLE", "INJECTOR_VAULT_SECRET_NAME", "INJECTOR_VAULT_SECRET_PREFIX",
-		"INJECTOR_NRI_ENABLED", "INJECTOR_NRI_WRAP_TOKEN_TTL", "INJECTOR_NRI_SOCKET_PATH",
+		"INJECTOR_NRI_ENABLED", "INJECTOR_NRI_SOCKET_PATH",
 	} {
 		t.Setenv(k, "")
 		os.Unsetenv(k)
@@ -432,6 +430,5 @@ nri:
 	cfg, err := NewConfig(tmpfile.Name())
 	require.NoError(t, err)
 	assert.True(t, cfg.NRI.Enabled)
-	assert.Equal(t, 10*time.Minute, cfg.NRI.WrapTokenTTL)
 	assert.Equal(t, "/custom/nri.sock", cfg.NRI.SocketPath)
 }

pkg/k8smutator/k8smutator_test.go

@@ -1,10 +1,8 @@
 package k8smutator
 
 import (
-	"context"
 	"strings"
 	"testing"
-	"time"
 
 	"github.com/numberly/vault-db-injector/pkg/k8s"
 	"github.com/numberly/vault-db-injector/pkg/placeholder"
@@ -293,13 +291,6 @@ func findEnvVar(envs []corev1.EnvVar, key string) string {
 	return ""
 }
 
-// stubWrapper implements vaultWrapper for tests — no real Vault needed.
-type stubWrapper struct{ wrapToken string }
-
-func (s *stubWrapper) WrapValues(_ context.Context, _ map[string]string, _ time.Duration) (string, error) {
-	return s.wrapToken, nil
-}
-
 func TestApplyEnvToContainers_NRIEnabled_Classic(t *testing.T) {
 	pod := &corev1.Pod{
 		Spec: corev1.PodSpec{

pkg/nri/vault.go

@@ -44,6 +44,17 @@ func fetchAndBuildMapping(ctx context.Context, cfg *config.Config, m k8s.NRIMapp
 	if err != nil {
 		return nil, nil, errors.Wrapf(err, "get pod %s/%s", podNamespace, podName)
 	}
+	// UID equality closes a name-reuse race: if the original pod was
+	// force-deleted between admission and CreateContainer, an attacker who
+	// can recreate a pod with the same name+namespace would otherwise
+	// hijack the credential fetch. NRI's contextID is the sandbox UID;
+	// kube-apiserver's pod.UID is the API-recorded UID. They must match.
+	if string(pod.UID) != contextID {
+		return nil, nil, errors.Newf(
+			"pod UID mismatch: NRI sandbox UID %s != API pod UID %s for %s/%s — refusing to fetch credentials",
+			contextID, string(pod.UID), podNamespace, podName,
+		)
+	}
 	actualSA := pod.Spec.ServiceAccountName
 	if actualSA == "" {
 		actualSA = "default"

pkg/vault/vault.go

@@ -244,65 +244,3 @@ func sliceToStrings(slice []any) ([]string, error) {
 	return stringSlice, nil
 }
 
-// WrapValues wraps the given key/value payload in Vault and returns the
-// resulting wrap token. The token is single-use: the next caller of
-// sys/wrapping/unwrap with this token gets the payload and the token dies.
-func (c *Connector) WrapValues(ctx context.Context, payload map[string]string, ttl time.Duration) (string, error) {
-	data := make(map[string]any, len(payload))
-	for k, v := range payload {
-		data[k] = v
-	}
-	// SetWrappingLookupFunc mutates state on the shared *api.Client. The
-	// deferred reset to nil restores the client's default wrap behaviour and
-	// runs on every exit path, including panic unwinds — Go's defer fires
-	// during stack unwinding before the panic propagates.
-	//
-	// Concurrency caveat: two WrapValues calls running in parallel on the
-	// same *Connector would race on this client-level state. The current
-	// architecture (per-request *Connector clone with its own login) makes
-	// this safe in practice. If the invariant ever changes (Connector
-	// shared across goroutines), serialize WrapValues with a mutex or
-	// switch to a Connector pool.
-	c.client.SetWrappingLookupFunc(func(operation, path string) string {
-		return ttl.String()
-	})
-	defer c.client.SetWrappingLookupFunc(nil)
-
-	secret, err := c.client.Logical().WriteWithContext(ctx, "sys/wrapping/wrap", data)
-	if err != nil {
-		return "", errors.Wrap(err, "vault wrap")
-	}
-	if secret == nil || secret.WrapInfo == nil || secret.WrapInfo.Token == "" {
-		return "", errors.New("vault wrap returned no token")
-	}
-	return secret.WrapInfo.Token, nil
-}
-
-// UnwrapValues consumes the given wrap token and returns the previously
-// wrapped payload. The token is consumed in Vault on the first call
-// regardless of whether this method returns an error: a parse failure
-// (non-string field, missing data) leaves the caller without recourse
-// because Vault has already burned the token. Callers must therefore
-// treat any UnwrapValues error as a permanent loss of those credentials
-// and trigger a fresh credential issuance flow.
-//
-// Calling unwrap twice with the same token always fails on the second
-// call.
-func (c *Connector) UnwrapValues(ctx context.Context, token string) (map[string]string, error) {
-	secret, err := c.client.Logical().UnwrapWithContext(ctx, token)
-	if err != nil {
-		return nil, errors.Wrap(err, "vault unwrap")
-	}
-	if secret == nil || secret.Data == nil {
-		return nil, errors.New("vault unwrap returned no data")
-	}
-	out := make(map[string]string, len(secret.Data))
-	for k, v := range secret.Data {
-		s, ok := v.(string)
-		if !ok {
-			return nil, errors.Newf("vault unwrap returned non-string for %q", k)
-		}
-		out[k] = s
-	}
-	return out, nil
-}