unlock unlimited code runs for $79, limited time →
NOSSLETTER.techLIVE
back to OSSpedia
kubernetes/kubernetes
mid7/10
error-handlingsecuritytesting-strategyapi-design

Fix NodeRestriction audience authz fallback on volume lookup error

Project: kubernetes/kubernetes | Language: Go | Concepts: error-handling, security, testing-strategy, api-design
Difficulty: mid
Link: https://github.com/kubernetes/kubernetes/pull/140607

The problem

When the ServiceAccountNodeAudienceRestriction feature is enabled, Kubernetes nodes may request service account tokens for specific audiences if the pod references that audience, or if the node is explicitly authorized via RBAC. However, if resolving a pod's CSI volume sources failed (e.g., referenced CSIDriver, PVC, or PV not found), the admission plugin would return an error early and never consult the authorizer. This meant that the explicit RBAC bypass (request-serviceaccounts-token-audience grant) did not work when there was a lookup error, even though it was intended as an escape hatch.

Why it's non-trivial

A naive fix might only surface the lookup error or only consult the authorizer, but not both. The correct fix needs to:

  • Preserve deny-by-default: Do not accidentally allow requests that should be denied.
  • Ensure the authorizer is always consulted, even on error paths, so explicit grants work as intended.
  • Return the most actionable error to the user (e.g., a missing resource error over a generic authorization error).

The approach

1. Always consult the authorizer on lookup errors, but only allow if explicitly granted:

-foundAudiencesInPodSpec, err := p.podReferencesAudience(ctx, pod, requestedAudience)
-if err != nil {
-    return fmt.Errorf("error validating audience %q: %w", requestedAudience, err)
-}
-if foundAudiencesInPodSpec {
-    return nil
-}
+foundAudiencesInPodSpec, podRefErr := p.podReferencesAudience(ctx, pod, requestedAudience)
+if podRefErr == nil && foundAudiencesInPodSpec {
+    return nil
+}
+
+// The pod spec does not reference the requested audience, or we could not
+// determine whether it does because resolving the pod's volume sources failed
+// (for example a referenced PVC, PV, or CSIDriver was not found). Either way,
+// fall back to the authorizer so that an explicit grant of the
+// request-serviceaccounts-token-audience verb still acts as an escape hatch,
+// including on the error path.

This change ensures that even if there is a lookup error, the authorizer is still consulted, allowing explicit RBAC grants to take effect as an escape hatch.

2. Prefer surfacing the lookup error if the authorizer denies:

-authorized, _, err := p.authz.Authorize(ctx, attrs)
-// an authorizer like RBAC could encounter evaluation errors and still allow the request, so authorizer decision is checked before error here.
-// following the same pattern as withAuthorization (ref: https://github.com/kubernetes/kubernetes/blob/2b025e645975d6d51bf38c008f972c632cf49657/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go#L71-L91)
-if authorized == authorizer.DecisionAllow {
-    return nil
-}
-if err != nil {
-    return fmt.Errorf("error authorizing %s to request tokens for audience %q: %w", a.GetUserInfo().GetName(), requestedAudience, err)
-}
-
-return fmt.Errorf("%s is not authorized to request tokens for audience %q", a.GetUserInfo().GetName(), requestedAudience)
+authorized, _, authzErr := p.authz.Authorize(ctx, attrs)
+// an authorizer like RBAC could encounter evaluation errors and still allow the request, so authorizer decision is checked before error here.
+// following the same pattern as withAuthorization (ref: https://github.com/kubernetes/kubernetes/blob/2b025e645975d6d51bf38c008f972c632cf49657/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go#L71-L91)
+if authorized == authorizer.DecisionAllow {
+    return nil
+}
+
+// The authorizer did not allow the request. Surface the most actionable error:
+// a failure to resolve the pod's audiences points at a real misconfiguration,
+// so prefer it over the generic authorization errors.
+if podRefErr != nil {
+    return fmt.Errorf("error validating audience %q: %w", requestedAudience, podRefErr)
+}
+if authzErr != nil {
+    return fmt.Errorf("error authorizing %s to request tokens for audience %q: %w", a.GetUserInfo().GetName(), requestedAudience, authzErr)
+}
+
+return fmt.Errorf("%s is not authorized to request tokens for audience %q", a.GetUserInfo().GetName(), requestedAudience)

This ensures that if the authorizer denies the request, the user still sees the original lookup error if present, which is more actionable than a generic denial.

3. Comprehensive test coverage for both allow and deny paths:

The tests were expanded to cover cases where:

  • The referenced CSIDriver, PVC, or PV is missing and the authorizer denies (should fail with the lookup error).
  • The referenced resource is missing and the authorizer allows (should succeed due to the explicit grant).

Example test case for the allow path:

+{
+    name: "allow create of token when audience in pod --> csi --> driver --> tokenrequest with audience and ServiceAccountNodeAudienceRestriction is enabled, csidriver not found, authorizer allows",
+    podsGetter: existingPods,
+    csiDriverGetter: noexistingCSIDriverLister,
+    features: feature.DefaultFeatureGate,
+    setupFunc: func(t *testing.T) {
+        t.Helper()
+        featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.ServiceAccountNodeAudienceRestriction, true)
+    },
+    attributes: admission.NewAttributesRecord(makeTokenRequest(coremypodWithCSI.Name, v1mypodWithCSI.UID, []string{"foo"}), nil, tokenrequestKind, coremypod.Namespace, "mysa", svcacctResource, "token", admission.Create, &metav1.CreateOptions{}, false, mynode),
+    authz: saAuthorizerBuilder{
+        t:                  t,
+        serviceAccountName: "mysa",
+        namespace:          coremypod.Namespace,
+        requestAudience:    "foo",
+        decision:           authorizer.DecisionAllow,
+    }.build(),
+},

This test ensures that the explicit grant works even when the referenced CSI driver is missing.

From the review thread

liggitt:
This change lgtm, I don't feel strongly about it going into 1.37 vs 1.38. If you'd like to bring it into 1.37 post-freeze, let's make a stronger case for this being an impactful bug and tag in the release folks for visibility.

Takeaways

  • Always consult the authorizer on error paths to ensure explicit security grants are honored, even when resource lookups fail.
  • When multiple errors are possible, surface the most actionable error to the user (e.g., a missing resource over a generic denial).
  • Comprehensive tests should cover both allow and deny cases for all code paths, especially when security and error handling interact.