fix(scheduler): require caller authentication on /refit (#2882)

* fix(scheduler): authenticate /refit endpoint callers via TokenReview

Any in-cluster caller that could reach the scheduler Service could
previously call /refit and move another pod's device allocation onto a
different device, since the endpoint had no caller authentication
(#2878).

The device plugin now sends its bound ServiceAccount token as a bearer
credential on refit calls. The scheduler verifies it with the
TokenReview API, requires the token to belong to the configured
device-plugin ServiceAccount, and confirms the token's bound pod runs
on the node the request claims -- all before touching any allocation
state. The expected ServiceAccount identity is chart-populated so it
stays consistent with the device-plugin's actual namespace/name
instead of being hardcoded.

As defense in depth, the scheduler Service now defaults to ClusterIP
instead of NodePort, and a new NetworkPolicy (enabled by default)
restricts ingress on the scheduler's HTTP port to the device-plugin's
pods and the kube-system namespace.

/filter and /bind are unaffected: kube-scheduler is their caller by
convention and they follow the standard extender protocol, which is
out of scope for this issue.

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>

* fix(chart): default scheduler.networkPolicy.enabled to false

/filter, /bind, /refit, and the admission /webhook all share one port
on the scheduler extender -- NetworkPolicy has no per-path awareness,
so restricting that port also gates /webhook, which kube-apiserver
calls to run HAMi's mutation.

kube-apiserver commonly runs hostNetwork (kubeadm/most on-prem
clusters), and Kubernetes documents NetworkPolicy behavior for
hostNetwork pods as undefined -- Calico (projectcalico/calico#1987)
and Cilium (host-network traffic needs its separate Host Firewall
feature) are both known not to match such traffic against
namespaceSelector. With admissionWebhook.failurePolicy: Ignore
(the chart default), a blocked /webhook doesn't fail closed: pods
admit without HAMi's mutation, so a pod requesting only gpucores/
gpumem never gets the nvidia.com/gpu count resource injected, never
enters HAMi's Filter/Bind, and runs with zero GPU enforcement and no
error surfaced.

/refit's own caller authentication (TokenReview) is an application-layer
check and is unaffected by this either way.

Default the policy off (opt-in) and document the requirement in
values.yaml, the template's own header comment, and NOTES.txt so an
operator who does enable it is warned to verify their CNI matches
apiserver traffic against namespaceSelector first.

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>

* fix(refit): address PR #2882 review comments

- Thread r.Context() into RefitNumaAllocation/authenticateRefitCaller
  instead of context.Background(), so TokenReview respects request
  cancellation and deadline (issue #2878).

- Validate --device-plugin-namespace and --device-plugin-service-account
  at scheduler startup; refuse to start with empty values that would
  cause every /refit TokenReview to fail silently.

- Remove dead req.Header.Del in CheckRedirect: the redirect is never
  executed once the error is returned, so no header manipulation is needed.

- Fail closed when the projected SA token file cannot be read instead of
  silently degrading to unauthenticated; surface a clear error rather than
  letting the server reject an unauthenticated request after a full round
  trip.

- Fix all tests that relied on plain-HTTP test servers: convert them to
  use HTTPS plus a synthetic token via numaRefitTestServerTLS. Add
  TestRequestNumaRefitMissingToken to cover the new fail-closed path.

- Fix deployment.yaml to always bind the scheduler on
  scheduler.service.httpTargetPort (default 443) regardless of whether
  admissionWebhook is enabled, eliminating the silent port mismatch that
  left /refit unreachable when the webhook was disabled.

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>

---------

Signed-off-by: AyushSrivastava1818 <ayush.sri0705@gmail.com>

Author: AyushSrivastava1818

charts/hami/README.md

@@ -145,7 +145,7 @@ This document provides detailed descriptions of all configurable values paramete
 
 | Parameter | Description | Default Value |
 |-----------|-------------|---------------|
-| `scheduler.service.type` | Service type | `NodePort` |
+| `scheduler.service.type` | Service type | `ClusterIP` |
 | `scheduler.service.httpPort` | HTTP port | `443` |
 | `scheduler.service.schedulerPort` | Scheduler NodePort | `31998` |
 | `scheduler.service.monitorPort` | Monitor port | `31993` |

charts/hami/templates/NOTES.txt

@@ -36,3 +36,15 @@ The device plugin relabels shared vGPU host directories under
 directory permissions. Uninstalling this Helm release does not restore the host
 SELinux labels or directory permissions; restore them manually if required.
 {{- end }}
+{{- if .Values.scheduler.networkPolicy.enabled }}
+
+WARNING: scheduler.networkPolicy.enabled is true. This restricts the
+scheduler's HTTP port, which also serves the admission /webhook, to the
+device-plugin's pods and the kube-system namespace. If kube-apiserver runs
+hostNetwork (kubeadm/most on-prem clusters) and your CNI doesn't match
+hostNetwork traffic against namespaceSelector, this can silently block the
+webhook -- with admissionWebhook.failurePolicy: Ignore, pods then admit
+without HAMi's mutation (no GPU resource injected, no error). Verify your
+CNI matches kube-apiserver traffic against this policy before relying on
+it in production.
+{{- end }}

charts/hami/templates/scheduler/clusterrole.yaml

@@ -21,6 +21,9 @@ rules:
   - apiGroups: [""]
     resources: ["resourcequotas"]
     verbs: ["get", "list", "watch"]
+  - apiGroups: ["authentication.k8s.io"]
+    resources: ["tokenreviews"]
+    verbs: ["create"]
 ---
 {{- if .Values.mockDevicePlugin.enabled }}
 apiVersion: rbac.authorization.k8s.io/v1

charts/hami/templates/scheduler/deployment.yaml

@@ -114,6 +114,10 @@ spec:
             - --cert_file=/tls/tls.crt
             - --key_file=/tls/tls.key
             {{- else }}
+            {{- /* No TLS: bind on the same port the Service routes to so the
+                   /refit endpoint is reachable. Authenticated refit still
+                   requires HTTPS; operators must supply cert/key when
+                   device-plugin token authentication is required. */}}
             - --http_bind=0.0.0.0:{{ .Values.scheduler.service.httpTargetPort | default 9443 }}
             {{- end }}
             - --scheduler-name={{ .Values.schedulerName }}
@@ -125,6 +129,8 @@ spec:
             - --leader-elect={{ .Values.scheduler.leaderElect }}
             - --leader-elect-resource-name={{ .Values.schedulerName }}
             - --leader-elect-resource-namespace={{ include "hami-vgpu.namespace" . }}
+            - --device-plugin-namespace={{ include "hami-vgpu.namespace" . }}
+            - --device-plugin-service-account={{ include "hami-vgpu.device-plugin" . }}
             {{- if .Values.devices.ascend.enabled }}
             - --enable-ascend=true
             {{- end }}

charts/hami/templates/scheduler/networkpolicy.yaml

@@ -0,0 +1,49 @@
+{{- if .Values.scheduler.networkPolicy.enabled }}
+# Restricts who can reach the scheduler extender's HTTP port (serving
+# /filter, /bind, /refit and the admission /webhook -- they all share this
+# one port; NetworkPolicy has no per-path awareness to split them) to the
+# device-plugin's pods and to the kube-system namespace, where kube-scheduler
+# and kube-apiserver normally run. This is defense in depth on top of the
+# /refit caller authentication (see issue #2878); it does not replace it,
+# and /refit's own TokenReview check is unaffected if this policy is off.
+#
+# Off by default (see scheduler.networkPolicy.enabled in values.yaml): if
+# kube-apiserver runs hostNetwork (the common kubeadm/on-prem default) and
+# your CNI doesn't match hostNetwork traffic against namespaceSelector, this
+# can silently block /webhook, and admissionWebhook.failurePolicy: Ignore
+# then lets pods admit without HAMi's mutation -- no scheduler-name rewrite,
+# no GPU resource injected, no error surfaced.
+#
+# The metrics port is left open to any source, matching the chart's
+# pre-existing behavior, since it carries no privileged action.
+apiVersion: networking.k8s.io/v1
+kind: NetworkPolicy
+metadata:
+  name: {{ include "hami-vgpu.scheduler" . }}
+  namespace: {{ include "hami-vgpu.namespace" . }}
+  labels:
+    app.kubernetes.io/component: hami-scheduler
+    {{- include "hami-vgpu.labels" . | nindent 4 }}
+spec:
+  podSelector:
+    matchLabels:
+      app.kubernetes.io/component: hami-scheduler
+      {{- include "hami-vgpu.selectorLabels" . | nindent 6 }}
+  policyTypes:
+    - Ingress
+  ingress:
+    - from:
+        - podSelector:
+            matchLabels:
+              app.kubernetes.io/component: hami-device-plugin
+              {{- include "hami-vgpu.selectorLabels" . | nindent 14 }}
+        - namespaceSelector:
+            matchLabels:
+              kubernetes.io/metadata.name: kube-system
+      ports:
+        - protocol: TCP
+          port: {{ .Values.scheduler.service.httpTargetPort | default 443 }}
+    - ports:
+        - protocol: TCP
+          port: {{ last (splitList ":" .Values.scheduler.metricsBindAddress) | int }}
+{{- end }}

charts/hami/templates/scheduler/service.yaml

@@ -13,19 +13,19 @@ metadata:
   annotations: {{ toYaml .Values.scheduler.service.annotations | nindent 4 }}
   {{- end }}
 spec:
-  type: {{ .Values.scheduler.service.type | default "NodePort" }}  # Default type is NodePort
+  type: {{ .Values.scheduler.service.type | default "ClusterIP" }}  # Default type is ClusterIP
   ports:
     - name: http
       port: {{ .Values.scheduler.service.httpPort | default 443 }}  # Default HTTP port is 443
       targetPort: {{ .Values.scheduler.service.httpTargetPort | default 9443 }}
-      {{- if eq (.Values.scheduler.service.type | default "NodePort") "NodePort" }}  # If type is NodePort, set nodePort
+      {{- if eq (.Values.scheduler.service.type | default "ClusterIP") "NodePort" }}  # If type is NodePort, set nodePort
       nodePort: {{ .Values.scheduler.service.schedulerPort | default 31998 }}
       {{- end }}
       protocol: TCP
     - name: monitor
       port: {{ .Values.scheduler.service.monitorPort | default 31993 }}  # Default monitoring port is 31993
       targetPort: {{ .Values.scheduler.service.monitorTargetPort | default "metrics" }}
-      {{- if eq (.Values.scheduler.service.type | default "NodePort") "NodePort" }}  # If type is NodePort, set nodePort
+      {{- if eq (.Values.scheduler.service.type | default "ClusterIP") "NodePort" }}  # If type is NodePort, set nodePort
       nodePort: {{ .Values.scheduler.service.monitorPort | default 31993 }}
       {{- end }}
       protocol: TCP

charts/hami/values.yaml

@@ -295,14 +295,34 @@ scheduler:
     # OpenShift omits this value at render time so its SCC can allocate an allowed UID.
     runAsUser: 2000
   service:
-    type: NodePort  # Default type is NodePort, can be changed to ClusterIP
+    type: ClusterIP  # Default type is ClusterIP, can be changed to NodePort
     httpPort: 443   # HTTP port
     schedulerPort: 31998  # NodePort for HTTP
     monitorPort: 31993    # Monitoring port
     monitorTargetPort: metrics  # Name of the container port, so it follows scheduler.metricsBindAddress
     httpTargetPort: 9443
     labels: {}
     annotations: {}
+  networkPolicy:
+    # Restrict ingress on the scheduler's HTTP port (filter/bind/refit/webhook
+    # all share this one port -- NetworkPolicy can't separate them by path)
+    # to the device-plugin's pods and to the kube-system namespace, where
+    # kube-scheduler and kube-apiserver normally run. See issue #2878.
+    #
+    # Off by default: /refit's own caller authentication (TokenReview) does
+    # not depend on this and is unaffected either way -- this is only an
+    # extra layer on top. Enable it only after confirming your CNI actually
+    # matches kube-apiserver traffic against namespaceSelector/podSelector.
+    # When kube-apiserver runs as a hostNetwork pod (the kubeadm/most on-prem
+    # default), Kubernetes documents NetworkPolicy behavior for hostNetwork
+    # pods as undefined, and it is known not to match on at least Calico
+    # (projectcalico/calico#1987) and Cilium (which requires its separate
+    # Host Firewall feature for host-network traffic). If this policy blocks
+    # kube-apiserver's calls to /webhook, admissionWebhook.failurePolicy:
+    # Ignore lets pod admission continue with HAMi's mutation silently
+    # skipped -- pods can then be scheduled by the default scheduler with no
+    # GPU device allocated and no error surfaced.
+    enabled: false
 
 devicePlugin:
   enabled: true

cmd/scheduler/main.go

@@ -79,6 +79,8 @@ func init() {
 	rootCmd.Flags().DurationVar(&config.NodeLockTimeout, "node-lock-timeout", time.Minute*5, "timeout for node locks")
 	rootCmd.Flags().DurationVar(&config.NodeLockRetryTimeout, "node-lock-retry-timeout", 28*time.Second, "timeout for retrying LockNode when contended by another PodGroup member (0 disables retry). Align the Extender's httpTimeout in KubeSchedulerConfiguration with this value.")
 	rootCmd.Flags().BoolVar(&config.ForceOverwriteDefaultScheduler, "force-overwrite-default-scheduler", true, "Overwrite schedulerName in Pod Spec when set to the const DefaultSchedulerName in https://k8s.io/api/core/v1 package")
+	rootCmd.Flags().StringVar(&config.DevicePluginNamespace, "device-plugin-namespace", "", "namespace of the device-plugin ServiceAccount allowed to call the /refit endpoint")
+	rootCmd.Flags().StringVar(&config.DevicePluginServiceAccount, "device-plugin-service-account", "", "name of the device-plugin ServiceAccount allowed to call the /refit endpoint")
 
 	rootCmd.Flags().BoolVar(&config.LeaderElect, "leader-elect", false, "The pod of hami-scheduler enable leader select")
 	rootCmd.Flags().StringVar(&config.LeaderElectResourceName, "leader-elect-resource-name", "", "The name of resource object that is used for leader election")
@@ -130,6 +132,18 @@ func start() error {
 		return fmt.Errorf("empty hostname returned")
 	}
 
+	// Refuse to start with an unusable /refit identity: empty namespace or
+	// service-account means every TokenReview will fail, but silently – the
+	// scheduler would appear healthy while all refit calls are rejected.
+	if config.DevicePluginNamespace == "" || config.DevicePluginServiceAccount == "" {
+		return fmt.Errorf(
+			"--device-plugin-namespace and --device-plugin-service-account must both be set; "+
+				"they identify the ServiceAccount whose token is accepted on the /refit endpoint "+
+				"(see issue #2878). Got namespace=%q service-account=%q",
+			config.DevicePluginNamespace, config.DevicePluginServiceAccount,
+		)
+	}
+
 	sher = scheduler.NewScheduler()
 	go sher.RegisterFromNodeAnnotations()
 	err = sher.Start()

pkg/device-plugin/nvidiadevice/nvinternal/plugin/numa_refit_client.go

@@ -26,6 +26,7 @@ import (
 	"fmt"
 	"io"
 	"net/http"
+	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -64,10 +65,19 @@ const (
 	numaRefitTimeout = 2 * time.Second
 )
 
-// numaRefitTLSConfig verifies the scheduler certificate by default, against
-// SchedulerCAFileEnvName when provided; SchedulerTLSInsecureEnvName is an
-// explicit operator opt-out for the self-signed webhook certificate.
-func numaRefitTLSConfig() (*tls.Config, error) {
+var (
+	// serviceAccountTokenFile is the default projected/bound ServiceAccount
+	// token every pod gets mounted, used to authenticate the refit call
+	// against the scheduler's TokenReview check. See issue #2878.
+	serviceAccountTokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token"
+)
+
+// numaRefitTLSConfig returns a tls.Config for reaching the scheduler. When
+// authenticated is true (a bearer token is being transmitted), TLS certificate
+// verification is strictly enforced and HAMI_SCHEDULER_TLS_INSECURE cannot
+// disable it. When unauthenticated, HAMI_SCHEDULER_TLS_INSECURE may skip
+// verification for clusters using the self-signed webhook certificate.
+func numaRefitTLSConfig(authenticated bool) (*tls.Config, error) {
 	config := &tls.Config{MinVersion: tls.VersionTLS12}
 	if caFile := os.Getenv(SchedulerCAFileEnvName); caFile != "" {
 		pem, err := os.ReadFile(caFile)
@@ -80,23 +90,36 @@ func numaRefitTLSConfig() (*tls.Config, error) {
 		}
 		config.RootCAs = pool
 	}
-	if insecure, err := strconv.ParseBool(os.Getenv(SchedulerTLSInsecureEnvName)); err == nil {
-		config.InsecureSkipVerify = insecure
+	if insecure, err := strconv.ParseBool(os.Getenv(SchedulerTLSInsecureEnvName)); err == nil && insecure {
+		if authenticated {
+			return nil, errors.New("insecure TLS verification is not permitted for authenticated refit requests")
+		}
+		config.InsecureSkipVerify = true
 	}
 	return config, nil
 }
 
 // numaRefitHTTPClient reaches the scheduler service. It is built per refit so
 // a rotated CA bundle is picked up without restarting the device plugin; the
-// refit is a rare path, taken only when an allocation mismatches.
-func numaRefitHTTPClient() (*http.Client, error) {
-	tlsConfig, err := numaRefitTLSConfig()
+// refit is a rare path, taken only when an allocation mismatches. It rejects
+// redirects to prevent token leakage across endpoints.
+func numaRefitHTTPClient(authenticated bool) (*http.Client, error) {
+	tlsConfig, err := numaRefitTLSConfig(authenticated)
 	if err != nil {
 		return nil, err
 	}
 	return &http.Client{
-		Timeout:   numaRefitTimeout,
-		Transport: &http.Transport{TLSClientConfig: tlsConfig},
+		Timeout: numaRefitTimeout,
+		Transport: &http.Transport{
+			TLSClientConfig: tlsConfig,
+		},
+		// Never follow redirects: the bearer token must not be forwarded to
+		// an unvalidated redirect destination. Returning a non-nil error here
+		// is sufficient; no header manipulation is needed because the redirect
+		// request is never sent.
+		CheckRedirect: func(_ *http.Request, _ []*http.Request) error {
+			return errors.New("redirects are not permitted for refit requests")
+		},
 	}, nil
 }
 
@@ -146,6 +169,30 @@ func (plugin *NvidiaDevicePlugin) tryNumaRefit(ctx context.Context, pod *corev1.
 
 // requestNumaRefit performs one refit round trip against the scheduler.
 func (plugin *NvidiaDevicePlugin) requestNumaRefit(ctx context.Context, pod *corev1.Pod, containerIndex int, allowedUUIDs []string) (device.ContainerDevices, error) {
+	rawEndpoint := os.Getenv(SchedulerEndpointEnvName)
+	if rawEndpoint == "" {
+		return nil, errors.New("scheduler endpoint is not configured")
+	}
+	rawURL := strings.TrimSuffix(rawEndpoint, "/") + numaRefitPath
+	parsedURL, err := url.Parse(rawURL)
+	if err != nil {
+		return nil, fmt.Errorf("invalid scheduler endpoint URL: %w", err)
+	}
+
+	// Always authenticate: the scheduler requires a valid device-plugin token
+	// for /refit (see issue #2878). Fail immediately if the token is missing so
+	// the caller gets a clear error rather than a server-side authentication
+	// rejection after a full round trip.
+	tokenBytes, tokenErr := os.ReadFile(serviceAccountTokenFile)
+	if tokenErr != nil {
+		return nil, fmt.Errorf("cannot read service account token for refit authentication: %w", tokenErr)
+	}
+	token := strings.TrimSpace(string(tokenBytes))
+
+	if parsedURL.Scheme != "https" {
+		return nil, fmt.Errorf("authenticated refit requires HTTPS, endpoint scheme is %q", parsedURL.Scheme)
+	}
+
 	payload, err := json.Marshal(device.NumaRefitRequest{
 		PodUID:             string(pod.UID),
 		PodNamespace:       pod.Namespace,
@@ -162,14 +209,14 @@ func (plugin *NvidiaDevicePlugin) requestNumaRefit(ctx context.Context, pod *cor
 
 	ctx, cancel := context.WithTimeout(ctx, numaRefitTimeout)
 	defer cancel()
-	url := strings.TrimSuffix(os.Getenv(SchedulerEndpointEnvName), "/") + numaRefitPath
-	httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(payload))
+	httpReq, err := http.NewRequestWithContext(ctx, http.MethodPost, rawURL, bytes.NewReader(payload))
 	if err != nil {
 		return nil, err
 	}
 	httpReq.Header.Set("Content-Type", "application/json")
+	httpReq.Header.Set("Authorization", "Bearer "+token)
 
-	httpClient, err := numaRefitHTTPClient()
+	httpClient, err := numaRefitHTTPClient(true)
 	if err != nil {
 		return nil, err
 	}