-
Notifications
You must be signed in to change notification settings - Fork 43
feat(nvsnap): shared-token authentication for the agent API #555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
744b617
7a59a4d
c49d23c
2dfd2d7
28ea9be
0f368ff
8185411
6c34a38
8827b43
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| {{- if .Values.agent.auth.enabled }} | ||
| # SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Shared bearer token for the agent HTTP API (GH #486). | ||
| # | ||
| # The agent API is the control surface of a privileged process and the | ||
| # DaemonSet binds it to every node's IP, so it needs authentication in the | ||
| # request path. This Secret holds the token both the agent (to verify) and its | ||
| # callers (to present) read. | ||
| # | ||
| # Generated once and preserved across upgrades: `helm upgrade` re-renders every | ||
| # template, so a freshly random token on each upgrade would rotate the | ||
| # credential out from under running callers and cause a self-inflicted outage | ||
| # mid-rollout. The lookup below reuses the existing value when the Secret is | ||
| # already present. Set agent.auth.token explicitly to manage it yourself (or to | ||
| # rotate deliberately). | ||
| {{- $ns := .Release.Namespace }} | ||
| {{- $name := "nvsnap-agent-token" }} | ||
| {{- $existing := lookup "v1" "Secret" $ns $name }} | ||
| {{- $token := "" }} | ||
| {{- if .Values.agent.auth.token }} | ||
| {{- $token = .Values.agent.auth.token | b64enc }} | ||
| {{- else if and $existing $existing.data $existing.data.token }} | ||
| {{- $token = $existing.data.token }} | ||
| {{- else }} | ||
| {{- $token = randAlphaNum 48 | b64enc }} | ||
|
Comment on lines
+20
to
+27
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Do not generate a token when Client-side Require an explicit managed token for offline rendering, or use an external Secret workflow that preserves the value outside this template. 🧰 Tools🪛 YAMLlint (1.37.1)[warning] 25-25: too many spaces after hyphen (hyphens) [warning] 27-27: too many spaces after hyphen (hyphens) 🤖 Prompt for AI Agents |
||
| {{- end }} | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: {{ $name }} | ||
| namespace: {{ $ns }} | ||
| labels: | ||
| app.kubernetes.io/name: nvsnap | ||
| app.kubernetes.io/part-of: nvsnap | ||
| annotations: | ||
| # helm.sh/resource-policy keeps the Secret if the release is removed with | ||
| # --keep-history style workflows; without it a delete/reinstall cycle | ||
| # silently rotates the token. | ||
| helm.sh/resource-policy: keep | ||
| type: Opaque | ||
| data: | ||
| token: {{ $token }} | ||
|
Comment on lines
+29
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift Distribute the token to every restore namespace. Kubernetes resolves
📍 Affects 2 files
🤖 Prompt for AI Agents |
||
| {{- end }} | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -108,7 +108,7 @@ Only rendered when the init-container strategy is selected AND | |||||||||||||||||||||
| agentHostCIDR is set. The default inline strategy does the mount inside | ||||||||||||||||||||||
| the webhook and needs no pod->agent egress. | ||||||||||||||||||||||
| */ -}} | ||||||||||||||||||||||
| {{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") .Values.webhook.agentHostCIDR .Values.agent.l2.restoreNamespaces }} | ||||||||||||||||||||||
| {{- if and .Values.webhook.enabled (eq (.Values.webhook.restorePrepStrategy | default "inline") "init-container") (or (not .Values.agent.hostNetwork) .Values.webhook.agentHostCIDR) .Values.agent.l2.restoreNamespaces }} | ||||||||||||||||||||||
| {{- range $ns := .Values.agent.l2.restoreNamespaces }} | ||||||||||||||||||||||
|
Comment on lines
+111
to
112
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Update the render-condition comment for pod networking. Line 111 renders this policy when Proposed comment update-Only rendered when the init-container strategy is selected AND
-agentHostCIDR is set. The default inline strategy does the mount inside
-the webhook and needs no pod->agent egress.
+Rendered when the init-container strategy is selected.
+Host-networked agents require agentHostCIDR and use an ipBlock.
+Pod-networked agents use namespace and pod selectors and do not require
+agentHostCIDR. The default inline strategy needs no pod->agent egress.📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| --- | ||||||||||||||||||||||
| apiVersion: networking.k8s.io/v1 | ||||||||||||||||||||||
|
|
@@ -130,8 +130,25 @@ spec: | |||||||||||||||||||||
| - Egress | ||||||||||||||||||||||
| egress: | ||||||||||||||||||||||
| - to: | ||||||||||||||||||||||
| {{- if $.Values.agent.hostNetwork }} | ||||||||||||||||||||||
| # hostNetwork: the agent carries NODE identity, so a podSelector never | ||||||||||||||||||||||
| # matches it (verified on GKE Dataplane V2 / Cilium) and the rule has | ||||||||||||||||||||||
| # to name the whole node CIDR -- every node, on this port, for every | ||||||||||||||||||||||
| # pod in the namespace. | ||||||||||||||||||||||
| - ipBlock: | ||||||||||||||||||||||
| cidr: {{ $.Values.webhook.agentHostCIDR }} | ||||||||||||||||||||||
| {{- else }} | ||||||||||||||||||||||
| # Pod networking: the agent has a pod identity again, so the rule can | ||||||||||||||||||||||
| # name exactly the agent pods and nothing else. This is the concrete | ||||||||||||||||||||||
| # payoff of GH #490 -- no operator-supplied CIDR, and the grant shrinks | ||||||||||||||||||||||
| # from "the node network" to "these pods". | ||||||||||||||||||||||
| - namespaceSelector: | ||||||||||||||||||||||
| matchLabels: | ||||||||||||||||||||||
| kubernetes.io/metadata.name: {{ $.Release.Namespace }} | ||||||||||||||||||||||
| podSelector: | ||||||||||||||||||||||
| matchLabels: | ||||||||||||||||||||||
| {{- include "nvsnap.agent.selectorLabels" $ | nindent 14 }} | ||||||||||||||||||||||
| {{- end }} | ||||||||||||||||||||||
| ports: | ||||||||||||||||||||||
| - protocol: TCP | ||||||||||||||||||||||
| port: {{ $.Values.webhook.agentHostPort | default 8081 }} | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
Repository: NVIDIA/nvcf
Length of output: 27143
🏁 Script executed:
Repository: NVIDIA/nvcf
Length of output: 34614
Sensitive Data Exposure (CWE-319): Cleartext Transmission of Sensitive Information
Reachability: Internal
Protect
NVSNAP_AGENT_TOKENwith authenticated transport.When
NVSNAP_AGENT_TOKENis set, fail closed forhttp://agent URLs. Use HTTPS with certificate validation, preferably mTLS, forsetAgentAuth, the Helm Service URL, and the default URL inmount_prep_init.go.📍 Affects 3 files
src/compute-plane-services/nvsnap/cmd/nvsnap-mount-prep/main.go#L252-L263(this comment)src/compute-plane-services/nvsnap/deploy/helm/nvsnap/templates/agent-daemonset.yaml#L91-L96src/compute-plane-services/nvsnap/internal/webhook/mount_prep_init.go#L93-L132🤖 Prompt for AI Agents