feat: address feedback

Author: lalitadithya

distros/kubernetes/nvsentinel/templates/_helpers.tpl

@@ -371,7 +371,16 @@ platform-connector's own ConfigMap; publishers do not need it.
 */}}
 {{- define "nvsentinel.pcAuth.mode" -}}
 {{- $auth := ((.Values.global).platformConnectorAuth) | default dict -}}
-{{- $mode := $auth.mode | default "enforce" -}}
+{{- /*
+`default` treats false, 0, "" and nil as empty, so `$auth.mode | default
+"enforce"` would silently accept a typo'd non-string value by defaulting it
+away instead of rejecting it. Check presence explicitly, then validate
+whatever was actually supplied.
+*/ -}}
+{{- $mode := "enforce" -}}
+{{- if hasKey $auth "mode" -}}
+{{- $mode = index $auth "mode" -}}
+{{- end -}}
 {{- if not (kindIs "string" $mode) -}}
 {{- fail (printf "global.platformConnectorAuth.mode must be a string (\"enforce\" or \"audit\"), got %s %#v." (kindOf $mode) $mode) -}}
 {{- end -}}
@@ -390,7 +399,15 @@ ConfigMap.
 */}}
 {{- define "nvsentinel.pcAuth.failOpenOnUnavailable" -}}
 {{- $auth := ((.Values.global).platformConnectorAuth) | default dict -}}
-{{- $failOpen := $auth.failOpenOnUnavailable | default false -}}
+{{- /*
+Same reasoning as nvsentinel.pcAuth.mode: `default` would treat an explicit
+0 as absent and silently coerce it to false rather than rejecting the wrong
+type, so presence is checked explicitly first.
+*/ -}}
+{{- $failOpen := false -}}
+{{- if hasKey $auth "failOpenOnUnavailable" -}}
+{{- $failOpen = index $auth "failOpenOnUnavailable" -}}
+{{- end -}}
 {{- if not (kindIs "bool" $failOpen) -}}
 {{- fail (printf "global.platformConnectorAuth.failOpenOnUnavailable must be a boolean (true or false), got %s %#v." (kindOf $failOpen) $failOpen) -}}
 {{- end -}}

docs/METRICS.md

@@ -168,7 +168,7 @@ submitting health events naming another node. See
 |------------|------|--------|-------------|
 | `platform_connector_auth_decisions_total` | Counter | `decision` | Health event batches by the node scope granted to the caller. Values: `node_local`, `cross_node` |
 | `platform_connector_auth_node_claim_total` | Counter | `result` | Whether an authenticated caller's token carried a node claim. `verified`: it carried one and it named this node. `absent`: it carried none, so the caller was pinned to this node — the same scope a tokenless caller gets. |
-| `platform_connector_auth_violations_total` | Counter | `reason` | Health event batches that violated the node-binding rule. Under `global.platformConnectorAuth.mode: enforce` (default) these are rejected; under `mode: audit` they are recorded here but let through. Reasons: `node_mismatch` (a node-local caller named a different node), `node_claim_mismatch` (the token was issued on another node — applies to every caller, allowlisted or not), `unbound_cross_node_token` (an allowlisted caller presented a token bound to no pod, e.g. from `kubectl create token`), `cross_node_claim_absent` (pod-bound, but the pod was never scheduled), `missing_node_name` (no node name on the event and none could be stamped), `token_invalid` (TokenReview rejected the token), `malformed_credentials` (the authorization header was duplicated or did not use the Bearer scheme). Three further reasons mean **no verdict could be reached**, not that the caller was rejected: `validator_unavailable` (the API server was unreachable), `validator_timeout` (the caller gave up or its deadline passed) and `validator_error` (an unexpected failure, typically this component's own RBAC to create TokenReviews). With `failOpenOnUnavailable: true`, `validator_unavailable` and `validator_timeout` still increment this counter but fall back to node-local scope instead of rejecting the request, in enforce mode too — an outage says nothing about the caller's credential. Exclude those three when alerting on suspected credential abuse — a control-plane outage increments them for every in-flight request and would otherwise look identical to an attack: `sum(rate(platform_connector_auth_violations_total{reason=~"node_mismatch\|token_invalid\|node_claim_mismatch\|unbound_cross_node_token"}[5m]))` |
+| `platform_connector_auth_violations_total` | Counter | `reason` | Health event batches that violated the node-binding rule. Under `global.platformConnectorAuth.mode: enforce` (default) these are rejected; under `mode: audit` they are recorded here but let through. Reasons: `node_mismatch` (a node-local caller named a different node), `node_claim_mismatch` (the token was issued on another node — applies to every caller, allowlisted or not), `unbound_cross_node_token` (an allowlisted caller presented a token bound to no pod, e.g. from `kubectl create token`), `cross_node_claim_absent` (pod-bound, but the pod was never scheduled), `missing_node_name` (no node name on the event and none could be stamped), `token_invalid` (TokenReview rejected the token), `malformed_credentials` (the authorization header was duplicated or did not use the Bearer scheme). Three further reasons mean **no verdict could be reached**, not that the caller was rejected: `validator_unavailable` (the API server was unreachable), `validator_timeout` (the caller gave up or its deadline passed) and `validator_error` (an unexpected failure, typically this component's own RBAC to create TokenReviews). With `failOpenOnUnavailable: true`, `validator_unavailable` and `validator_timeout` still increment this counter but fall back to node-local scope instead of rejecting the request, in enforce mode too — an outage says nothing about the caller's credential. Exclude those three when alerting on suspected credential abuse — a control-plane outage increments them for every in-flight request and would otherwise look identical to an attack: `sum(rate(platform_connector_auth_violations_total{reason=~"node_mismatch\|token_invalid\|node_claim_mismatch\|unbound_cross_node_token\|malformed_credentials\|cross_node_claim_absent\|missing_node_name"}[5m]))` |
 
 ### Kubernetes Connector Metrics
 

docs/configuration/authentication.md

@@ -44,8 +44,11 @@ request through instead of rejecting it.
 Use `audit` to roll node-binding out against real traffic: run it for a
 period, confirm the violation counters stay at zero, then switch to `enforce`
 with evidence rather than finding out what it breaks in production. `audit`
-still requires `enabled: true` and a working validator — it changes what
-happens after a violation is detected, not whether requests are checked.
+still requires `enabled: true`, and every request is still validated: a
+validator failure (including `validator_unavailable` and
+`validator_timeout`) is still recorded, it just no longer rejects the
+request. `audit` changes what happens after a violation is detected, not
+whether requests are checked.
 
 ### `failOpenOnUnavailable`
 

platform-connectors/main.go

@@ -468,12 +468,12 @@ func initializeAuthInterceptor(
 
 	mode, err := authMode(config)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parse AuthMode: %w", err)
 	}
 
 	failOpenOnUnavailable, err := boolFromConfig(config, "AuthFailOpenOnUnavailable", false)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("parse AuthFailOpenOnUnavailable: %w", err)
 	}
 
 	interceptor, err := auth.NewNodeBindingInterceptor(auth.Config{