feat: Kubernetes ServiceAccount auth for gRPC#424
Conversation
wseaton
changed the title
wseaton
force-pushed
the
weaton/k8s-sa-auth
branch
from
5adb15b to
65c47cd
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
wseaton
force-pushed
the
weaton/k8s-sa-auth
branch
2 times, most recently
from
4f1f2ac to
f7b00f5
Compare
AuthN verifies the caller's projected ServiceAccount token via the Kubernetes TokenReview API in-process (no sidecar/mesh). AuthZ is an exact-match namespace/serviceaccount allowlist. Modes are off (default) and enforce; enforce fails config validation without a token audience and a non-empty allowlist. The health service stays ungated so kubelet probes need no token. The Rust and Python clients attach the token when a projected token file is present and send nothing otherwise, so the same client works off-cluster against an off server. The Helm chart binds the server SA to system:auth-delegator when enabled. Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
wseaton
force-pushed
the
weaton/k8s-sa-auth
branch
from
f7b00f5 to
dedb4f8
Compare
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
The env var pointing at the token file is process-global, so the three scenarios share one test fn and one token file instead of racing. Signed-off-by: Will Eaton <weaton@redhat.com>
WalkthroughAdds optional Kubernetes ServiceAccount authentication for gRPC services, with TokenReview verification, allowlist authorization, caching, Helm configuration, and Rust/Python client bearer-token propagation. Tests cover token handling, middleware behavior, configuration, and end-to-end RPC outcomes. ChangesServiceAccount authentication
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
modelexpress_client/src/lib.rs (1)
187-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEach
Client::newcreates its ownTokenProvider, unlike the Python client's process-wideshared_provider().Every call to
Client::newconstructs a freshTokenProvider::from_env()with an empty cache, so callers that create multiple short-livedClients (e.g.request_model_with_smart_fallback) each pay a cold-start file read instead of reusing a cached token, unlikemodelexpress_client/python/modelexpress/auth.py'sshared_provider(). Not a correctness issue since the cost per read is a cheapstat/read, but worth aligning with the Python design for consistency if multipleClients are commonly constructed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_client/src/lib.rs` around lines 187 - 191, Update Client::new and the Rust authentication setup around AuthInterceptor and TokenProvider to reuse a process-wide shared TokenProvider, matching the Python auth.py shared_provider() behavior. Ensure multiple Client instances share the same cached provider instead of calling TokenProvider::from_env() for each construction, while preserving interceptor cloning and existing client initialization.helm/values.yaml (1)
30-44: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winEnabling security with defaults guarantees a crash-looping deployment.
Default
mode: enforcecombined with emptytokenAudiences/allowedServiceAccountsmeans simply flippingsecurity.enabled: true(without also filling in the required lists) causes the server's ownvalidate_resolved(inmodelexpress_server/src/config.rs) to reject the config at startup, crash-looping the pod. Consider having Helm fail fast with a clear message (e.g. viarequired/failtemplate checks whensecurity.enabledis true but the lists are empty) instead of deferring to an opaque runtime crash.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@helm/values.yaml` around lines 30 - 44, Helm currently permits security.enabled=true with enforce mode and empty required lists, causing a runtime crash-loop. Add template validation for security.enabled in the chart’s relevant template helpers, using required/fail to reject empty tokenAudiences or allowedServiceAccounts with clear configuration messages before rendering.modelexpress_server/src/auth/token.rs (1)
134-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
expect()overunwrap()in tests.This test module relies on
.unwrap()(e.g. Lines 147-160, 207, 246). Coding guidelines requireexpect()in tests, which also yields clearer failure messages.As per coding guidelines: "Do not use
unwrap()except in benchmarks. Useexpect()only in tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_server/src/auth/token.rs` around lines 134 - 161, Replace every .unwrap() call in the tests module, including those in extracts_bearer_case_insensitively and the other referenced tests, with .expect() using clear failure messages while preserving the existing assertions and behavior.Source: Coding guidelines
modelexpress_server/src/auth.rs (1)
111-138: 🩺 Stability & Availability | 🔵 TrivialConsider load-shedding for unauthenticated TokenReview traffic.
An unauthenticated network caller can send an unbounded stream of distinct random bearer tokens. Each distinct token misses both caches and issues a fresh
TokenReviewcreate against the Kubernetes API server before any caching applies, so this path can be used to amplify load onto (or degrade) the cluster API server. The 10k-entry caches don't help against high-cardinality random input.Consider a per-source rate limit / concurrency cap on cache-miss
review_tokencalls, and/or a short-lived negative cache for backend failures to bound outbound API-server pressure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modelexpress_server/src/auth.rs` around lines 111 - 138, Bound cache-miss TokenReview traffic in verify by adding per-source rate limiting or a concurrency cap around review_token calls, using the request source information available from headers or the surrounding auth context; also consider briefly caching backend failures separately from token rejections. Ensure rejected requests fail with the appropriate denial and that successful and token-rejection caching behavior remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/templates/deployment.yaml`:
- Around line 79-82: Update the conditional around
MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS in the deployment template to distinguish
an unset value from an explicit zero, using a presence check such as hasKey on
.Values.security before rendering the quoted cacheTtlSecs value.
In `@modelexpress_client/python/modelexpress/auth.py`:
- Around line 24-34: Handle invalid MX_AUTH_TOKEN_TTL_SECONDS values gracefully
in TokenProvider.__init__: wrap the float conversion in safe parsing and fall
back to DEFAULT_TTL_SECONDS when parsing fails, matching the Rust
implementation. Preserve explicit valid TTL values and ensure construction never
raises ValueError for malformed environment input.
---
Nitpick comments:
In `@helm/values.yaml`:
- Around line 30-44: Helm currently permits security.enabled=true with enforce
mode and empty required lists, causing a runtime crash-loop. Add template
validation for security.enabled in the chart’s relevant template helpers, using
required/fail to reject empty tokenAudiences or allowedServiceAccounts with
clear configuration messages before rendering.
In `@modelexpress_client/src/lib.rs`:
- Around line 187-191: Update Client::new and the Rust authentication setup
around AuthInterceptor and TokenProvider to reuse a process-wide shared
TokenProvider, matching the Python auth.py shared_provider() behavior. Ensure
multiple Client instances share the same cached provider instead of calling
TokenProvider::from_env() for each construction, while preserving interceptor
cloning and existing client initialization.
In `@modelexpress_server/src/auth.rs`:
- Around line 111-138: Bound cache-miss TokenReview traffic in verify by adding
per-source rate limiting or a concurrency cap around review_token calls, using
the request source information available from headers or the surrounding auth
context; also consider briefly caching backend failures separately from token
rejections. Ensure rejected requests fail with the appropriate denial and that
successful and token-rejection caching behavior remains unchanged.
In `@modelexpress_server/src/auth/token.rs`:
- Around line 134-161: Replace every .unwrap() call in the tests module,
including those in extracts_bearer_case_insensitively and the other referenced
tests, with .expect() using clear failure messages while preserving the existing
assertions and behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b1b6401f-b338-4e19-b94c-80dbacbde2dd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
.github/copy-pr-bot.yamlCargo.tomldocs/DEPLOYMENT.mdhelm/templates/deployment.yamlhelm/templates/rbac.yamlhelm/values.yamlmodelexpress_client/python/modelexpress/auth.pymodelexpress_client/python/modelexpress/client.pymodelexpress_client/python/modelexpress/envs.pymodelexpress_client/python/tests/test_auth.pymodelexpress_client/src/auth.rsmodelexpress_client/src/lib.rsmodelexpress_common/src/envs.rsmodelexpress_server/Cargo.tomlmodelexpress_server/src/auth.rsmodelexpress_server/src/auth/layer.rsmodelexpress_server/src/auth/token.rsmodelexpress_server/src/config.rsmodelexpress_server/src/lib.rsmodelexpress_server/src/server.rsmodelexpress_server/tests/auth_e2e.rs
| {{- if .Values.security.cacheTtlSecs }} | ||
| - name: MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS | ||
| value: {{ .Values.security.cacheTtlSecs | quote }} | ||
| {{- end }} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
cacheTtlSecs: 0 is silently dropped.
{{- if .Values.security.cacheTtlSecs }} treats 0 as falsy in Helm/Go templates, so an operator who explicitly sets cacheTtlSecs: 0 (e.g., to disable caching) will have that value silently ignored, falling back to the server's default of 60s instead.
🐛 Proposed fix to distinguish "unset" from "zero"
- {{- if .Values.security.cacheTtlSecs }}
+ {{- if hasKey .Values.security "cacheTtlSecs" }}
- name: MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS
value: {{ .Values.security.cacheTtlSecs | quote }}
{{- end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{- if .Values.security.cacheTtlSecs }} | |
| - name: MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS | |
| value: {{ .Values.security.cacheTtlSecs | quote }} | |
| {{- end }} | |
| {{- if hasKey .Values.security "cacheTtlSecs" }} | |
| - name: MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS | |
| value: {{ .Values.security.cacheTtlSecs | quote }} | |
| {{- end }} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@helm/templates/deployment.yaml` around lines 79 - 82, Update the conditional
around MODEL_EXPRESS_SECURITY_CACHE_TTL_SECS in the deployment template to
distinguish an unset value from an explicit zero, using a presence check such as
hasKey on .Values.security before rendering the quoted cacheTtlSecs value.
| def __init__(self, path: str | None = None, ttl_seconds: float | None = None): | ||
| self._path = path or envs.MX_AUTH_TOKEN_PATH or DEFAULT_TOKEN_PATH | ||
| if ttl_seconds is None: | ||
| raw_ttl = envs.MX_AUTH_TOKEN_TTL_SECONDS | ||
| ttl_seconds = float(raw_ttl) if raw_ttl else DEFAULT_TTL_SECONDS | ||
| self._ttl = ttl_seconds | ||
| self._lock = threading.Lock() | ||
| self._cached: str | None = None | ||
| self._cached_at: float = 0.0 | ||
| self._cached_mtime: float = 0.0 | ||
| self._warned_missing = False |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unhandled ValueError on malformed MX_AUTH_TOKEN_TTL_SECONDS.
float(raw_ttl) will raise ValueError if the env var is set to a non-numeric string, crashing TokenProvider() construction and thus the whole client. The Rust counterpart (modelexpress_client/src/auth.rs, .and_then(|raw| raw.parse::<u64>().ok())) silently falls back to the default on parse failure. Mirror that graceful fallback here.
🛡️ Proposed fix
if ttl_seconds is None:
raw_ttl = envs.MX_AUTH_TOKEN_TTL_SECONDS
- ttl_seconds = float(raw_ttl) if raw_ttl else DEFAULT_TTL_SECONDS
+ try:
+ ttl_seconds = float(raw_ttl) if raw_ttl else DEFAULT_TTL_SECONDS
+ except ValueError:
+ logger.warning(
+ "Invalid MX_AUTH_TOKEN_TTL_SECONDS=%r; using default %.0fs",
+ raw_ttl,
+ DEFAULT_TTL_SECONDS,
+ )
+ ttl_seconds = DEFAULT_TTL_SECONDS📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, path: str | None = None, ttl_seconds: float | None = None): | |
| self._path = path or envs.MX_AUTH_TOKEN_PATH or DEFAULT_TOKEN_PATH | |
| if ttl_seconds is None: | |
| raw_ttl = envs.MX_AUTH_TOKEN_TTL_SECONDS | |
| ttl_seconds = float(raw_ttl) if raw_ttl else DEFAULT_TTL_SECONDS | |
| self._ttl = ttl_seconds | |
| self._lock = threading.Lock() | |
| self._cached: str | None = None | |
| self._cached_at: float = 0.0 | |
| self._cached_mtime: float = 0.0 | |
| self._warned_missing = False | |
| def __init__(self, path: str | None = None, ttl_seconds: float | None = None): | |
| self._path = path or envs.MX_AUTH_TOKEN_PATH or DEFAULT_TOKEN_PATH | |
| if ttl_seconds is None: | |
| raw_ttl = envs.MX_AUTH_TOKEN_TTL_SECONDS | |
| try: | |
| ttl_seconds = float(raw_ttl) if raw_ttl else DEFAULT_TTL_SECONDS | |
| except ValueError: | |
| logger.warning( | |
| "Invalid MX_AUTH_TOKEN_TTL_SECONDS=%r; using default %.0fs", | |
| raw_ttl, | |
| DEFAULT_TTL_SECONDS, | |
| ) | |
| ttl_seconds = DEFAULT_TTL_SECONDS | |
| self._ttl = ttl_seconds | |
| self._lock = threading.Lock() | |
| self._cached: str | None = None | |
| self._cached_at: float = 0.0 | |
| self._cached_mtime: float = 0.0 | |
| self._warned_missing = False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modelexpress_client/python/modelexpress/auth.py` around lines 24 - 34, Handle
invalid MX_AUTH_TOKEN_TTL_SECONDS values gracefully in TokenProvider.__init__:
wrap the float conversion in safe parsing and fall back to DEFAULT_TTL_SECONDS
when parsing fails, matching the Rust implementation. Preserve explicit valid
TTL values and ensure construction never raises ValueError for malformed
environment input.
Signed-off-by: Will Eaton <weaton@redhat.com>
…lowlist Signed-off-by: Will Eaton <weaton@redhat.com>
Signed-off-by: Will Eaton <weaton@redhat.com>
|
Smoke-tested live on a Kubernetes cluster (real apiserver TokenReview,
Client without a token warns and degrades as designed: Server-side denials: Health stays reachable without a token, so kubelet probes are unaffected. |
nicolasnoble
left a comment
nicolasnoble
left a comment
There was a problem hiding this comment.
Looks solid, a few non-blocking notes inline.
One cross-cutting thing worth handling before merge: with auth on but no TLS, the projected token rides on the wire in cleartext. The Python client attaches it to an insecure channel, so it's capturable and replayable until the token expires. Can we get a clear "enable auth only over TLS" warning in the deployment docs, and maybe a client-side warn when a token is attached to an insecure channel? Fine to merge without the client guard, but I'd like the docs warning in.
| let state = self.state.clone(); | ||
|
|
||
| Box::pin(async move { | ||
| match state.verify(req.headers()).await { |
There was a problem hiding this comment.
Auth runs once when the stream opens, so a long-lived server-streaming RPC keeps flowing even after the token expires or the SA is revoked. Fine for a first cut. Might be worth a doc line noting auth is enforced at RPC start, not per message.
| let token = extract_bearer(headers).ok_or(Denial::Unauthenticated)?; | ||
| let key = TokenCacheKey::new(token.expose_secret()); | ||
|
|
||
| if self.negative_cache.get(&key).await.is_some() { |
There was a problem hiding this comment.
The negative cache is keyed on the raw token, so a caller cycling unique bad tokens skips it and every one lands a fresh TokenReview on the apiserver. Fine for an opt-in feature. If you ever want to harden it, a global or per-source TokenReview rate limit would cap the amplification.
3 participants
Opt-in ServiceAccount auth for the gRPC server. Off by default; existing deployments are unaffected.
namespace:serviceaccountallowlist.off(default) andenforce.enforcefails config validation without a token audience and a non-empty allowlist; the Helm chart fails rendering on the same misconfiguration.UNAVAILABLE, notUNAUTHENTICATED, and are never cached.security.enabledinjects the server env and, withserviceAccount.create, binds the SA tosystem:auth-delegatorfor TokenReview access.Config via
MODEL_EXPRESS_SECURITY_*env vars orsecurity.*in the config file / Helm values. Covered by unit tests and an end-to-end test driving the real client against a real tonic server.Summary by CodeRabbit
New Features
Documentation
Tests