feat(kms): add Vault Kubernetes and AppRole authentication#1
Conversation
The Vault KMS previously accepted only a static token. Route the X-Vault-Token through a token source so it can authenticate via AppRole: it logs in against the configured AppRole mount and re-authenticates on demand when the cached token nears expiry or Vault rejects a call with 403. A static token stays the default when no AppRole credentials are set. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Add KKP_VAULT_ROLE_ID / KKP_VAULT_SECRET_ID / KKP_VAULT_APPROLE_MOUNT and validate that Vault auth is exactly one of a static token or an AppRole (both a role id and a secret id). Pass the settings through to the KMS. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Describe the AppRole option in the README and operator guide, and correct the guide's stale reference to a Kubernetes-projected ServiceAccount token path that the proxy does not implement. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds Vault Transit AppRole authentication alongside static token auth, with config validation, proxy wiring, retryable Vault login handling, and updated operator-facing documentation and tests. ChangesVault AppRole Authentication Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Proxy
participant VaultKMS
participant vaultAuth
participant Vault
Proxy->>VaultKMS: encrypt/decrypt request
VaultKMS->>vaultAuth: currentToken()
alt token cached and valid
vaultAuth-->>VaultKMS: cached token
else login needed
vaultAuth->>Vault: AppRole login
Vault-->>vaultAuth: client_token, lease_duration
vaultAuth-->>VaultKMS: new token
end
VaultKMS->>Vault: transit POST
alt 403 Forbidden
VaultKMS->>vaultAuth: reauth()
vaultAuth->>Vault: AppRole login
Vault-->>vaultAuth: new client_token
VaultKMS->>Vault: retry POST
Vault-->>VaultKMS: 200 OK
else 200 OK
Vault-->>VaultKMS: response
end
VaultKMS-->>Proxy: result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for Vault AppRole authentication as an alternative to static tokens for the Vault Transit KMS, updating configuration loading, validation, and documentation accordingly. The review feedback highlights a performance bottleneck in the token retrieval logic, where holding a single mutex during network I/O blocks concurrent requests. The reviewer suggests introducing a separate mutex and implementing a non-blocking token renewal strategy to optimize concurrent performance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| type vaultAuth struct { | ||
| client *http.Client | ||
| static string // set for static-token auth; empty for AppRole. | ||
| loginURL string // set for AppRole auth. | ||
| roleID string | ||
| secretID string | ||
|
|
||
| mu sync.Mutex | ||
| token string | ||
| expiry time.Time // zero once logged in => the token does not expire. | ||
| } |
There was a problem hiding this comment.
Holding the main mutex mu during the HTTP request in login blocks all other concurrent requests from retrieving the token, even if the cached token is still valid (e.g., during the renewSkew window). To prevent this performance bottleneck, we should introduce a separate loginMu mutex to serialize login attempts while allowing concurrent reads of the cached token.
| type vaultAuth struct { | |
| client *http.Client | |
| static string // set for static-token auth; empty for AppRole. | |
| loginURL string // set for AppRole auth. | |
| roleID string | |
| secretID string | |
| mu sync.Mutex | |
| token string | |
| expiry time.Time // zero once logged in => the token does not expire. | |
| } | |
| type vaultAuth struct { | |
| client *http.Client | |
| static string // set for static-token auth; empty for AppRole. | |
| loginURL string // set for AppRole auth. | |
| roleID string | |
| secretID string | |
| mu sync.Mutex | |
| token string | |
| expiry time.Time // zero once logged in => the token does not expire. | |
| loginMu sync.Mutex | |
| } |
| func (a *vaultAuth) currentToken(ctx context.Context) (string, error) { | ||
| if a.loginURL == "" { | ||
| return a.static, nil | ||
| } | ||
| a.mu.Lock() | ||
| defer a.mu.Unlock() | ||
| if a.token != "" && (a.expiry.IsZero() || time.Until(a.expiry) > renewSkew) { | ||
| return a.token, nil | ||
| } | ||
| if err := a.login(ctx); err != nil { | ||
| return "", err | ||
| } | ||
| return a.token, nil | ||
| } | ||
|
|
||
| // reauth drops the cached AppRole token so the next token call logs in again. | ||
| // It reports whether re-authentication is possible (false for static tokens). | ||
| func (a *vaultAuth) reauth() bool { | ||
| if a.loginURL == "" { | ||
| return false | ||
| } | ||
| a.mu.Lock() | ||
| a.token = "" | ||
| a.mu.Unlock() | ||
| return true | ||
| } | ||
|
|
||
| // login performs an AppRole login and caches the issued token and its TTL. | ||
| // The caller must hold a.mu. | ||
| func (a *vaultAuth) login(ctx context.Context) error { | ||
| reqBody, err := json.Marshal(map[string]string{"role_id": a.roleID, "secret_id": a.secretID}) | ||
| if err != nil { | ||
| return fmt.Errorf("kms: marshal approle login: %w", err) | ||
| } | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, a.loginURL, bytes.NewReader(reqBody)) | ||
| if err != nil { | ||
| return fmt.Errorf("kms: build approle login: %w", err) | ||
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
|
|
||
| resp, err := a.client.Do(req) | ||
| if err != nil { | ||
| return fmt.Errorf("kms: approle login request: %w", err) | ||
| } | ||
| defer func() { _ = resp.Body.Close() }() | ||
|
|
||
| body, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return fmt.Errorf("kms: read vault response: %w", err) | ||
| return fmt.Errorf("kms: read approle login response: %w", err) | ||
| } | ||
| if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { | ||
| return fmt.Errorf("kms: vault returned status %d: %s", resp.StatusCode, bytes.TrimSpace(body)) | ||
| return fmt.Errorf("kms: approle login returned status %d: %s", resp.StatusCode, bytes.TrimSpace(body)) | ||
| } | ||
|
|
||
| var lr struct { | ||
| Auth struct { | ||
| ClientToken string `json:"client_token"` | ||
| LeaseDuration int `json:"lease_duration"` | ||
| } `json:"auth"` | ||
| } | ||
| if err := json.Unmarshal(body, respOut); err != nil { | ||
| return fmt.Errorf("kms: decode vault response: %w", err) | ||
| if err := json.Unmarshal(body, &lr); err != nil { | ||
| return fmt.Errorf("kms: decode approle login response: %w", err) | ||
| } | ||
| if lr.Auth.ClientToken == "" { | ||
| return errors.New("kms: approle login returned an empty client token") | ||
| } | ||
|
|
||
| a.token = lr.Auth.ClientToken | ||
| if lr.Auth.LeaseDuration > 0 { | ||
| a.expiry = time.Now().Add(time.Duration(lr.Auth.LeaseDuration) * time.Second) | ||
| } else { | ||
| a.expiry = time.Time{} | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Implement a non-blocking token renewal strategy using loginMu.TryLock(). If a login is already in progress by another goroutine, other concurrent requests can continue using the existing token if it is not fully expired, completely avoiding blocking in the hot path. Additionally, we refactor login to be a pure, thread-safe function that does not modify the struct state directly, allowing it to run safely without holding the main mutex mu during network I/O.
func (a *vaultAuth) currentToken(ctx context.Context) (string, error) {
if a.loginURL == "" {
return a.static, nil
}
a.mu.Lock()
token := a.token
expiry := a.expiry
a.mu.Unlock()
if token != "" && (expiry.IsZero() || time.Until(expiry) > renewSkew) {
return token, nil
}
if !a.loginMu.TryLock() {
if token != "" && (expiry.IsZero() || time.Now().Before(expiry)) {
return token, nil
}
a.loginMu.Lock()
}
defer a.loginMu.Unlock()
a.mu.Lock()
token = a.token
expiry = a.expiry
a.mu.Unlock()
if token != "" && (expiry.IsZero() || time.Until(expiry) > renewSkew) {
return token, nil
}
newToken, newExpiry, err := a.login(ctx)
if err != nil {
if token != "" && (expiry.IsZero() || time.Now().Before(expiry)) {
return token, nil
}
return "", err
}
a.mu.Lock()
a.token = newToken
a.expiry = newExpiry
a.mu.Unlock()
return newToken, nil
}
// reauth drops the cached AppRole token so the next token call logs in again.
// It reports whether re-authentication is possible (false for static tokens).
func (a *vaultAuth) reauth() bool {
if a.loginURL == "" {
return false
}
a.mu.Lock()
a.token = ""
a.expiry = time.Time{}
a.mu.Unlock()
return true
}
// login performs an AppRole login and returns the issued token and its expiry.
func (a *vaultAuth) login(ctx context.Context) (string, time.Time, error) {
reqBody, err := json.Marshal(map[string]string{"role_id": a.roleID, "secret_id": a.secretID})
if err != nil {
return "", time.Time{}, fmt.Errorf("kms: marshal approle login: %w", err)
}
req, err := http.NewRequestWithContext(ctx, http.MethodPost, a.loginURL, bytes.NewReader(reqBody))
if err != nil {
return "", time.Time{}, fmt.Errorf("kms: build approle login: %w", err)
}
req.Header.Set("Content-Type", "application/json")
resp, err := a.client.Do(req)
if err != nil {
return "", time.Time{}, fmt.Errorf("kms: approle login request: %w", err)
}
defer func() { _ = resp.Body.Close() }()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", time.Time{}, fmt.Errorf("kms: read approle login response: %w", err)
}
if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices {
return "", time.Time{}, fmt.Errorf("kms: approle login returned status %d: %s", resp.StatusCode, bytes.TrimSpace(body))
}
var lr struct {
Auth struct {
ClientToken string "json:\"client_token\""
LeaseDuration int "json:\"lease_duration\""
} "json:\"auth\""
}
if err := json.Unmarshal(body, &lr); err != nil {
return "", time.Time{}, fmt.Errorf("kms: decode approle login response: %w", err)
}
if lr.Auth.ClientToken == "" {
return "", time.Time{}, errors.New("kms: approle login returned an empty client token")
}
var expiry time.Time
if lr.Auth.LeaseDuration > 0 {
expiry = time.Now().Add(time.Duration(lr.Auth.LeaseDuration) * time.Second)
}
return lr.Auth.ClientToken, expiry, nil
}Address review: when the cached AppRole token is still valid but within renewSkew of expiry, a transient login failure now falls back to the still-usable token instead of failing the request; a hard 403 (which clears the token first) still surfaces the error. Document that the AppRole secret_id must stay reusable for the on-demand re-login. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Address review: serialize logins with a dedicated loginMu and make login a pure function returning the token and expiry, so mu is never held during the network call and a caller holding a valid token is not blocked by an in-flight login. A concurrent login uses TryLock and falls back to the still-usable token. Add a concurrent-Wrap race test. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
|
Thanks! Addressed in b555a95. The token mutex is no longer held across the AppRole login: logins are serialized by a dedicated |
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
Nice work overall — the security mechanics are solid: fail-closed on hard 403 (the token is cleared before re-login, so a failed login surfaces the error instead of serving a stale token), no credentials leak into logs or error messages, locking is correct (mu is never held across network I/O, logins are serialized via loginMu with a TryLock fast path), and the tests cover exactly the scenarios that matter, including the concurrent path under -race.
Question (design): the design doc (data-encryption-design.md) prescribes Kubernetes Auth (projected ServiceAccount token) for this proxy, while this PR implements AppRole — and the operator guide edit removes the SA-token mention entirely. Since the secret_id must stay reusable with a long TTL, a long-lived secret still sits in a Kubernetes Secret, so the gain over a static token is moderate. Is AppRole a deliberate replacement for Kubernetes Auth (then let's update the design doc) or an intermediate step (then let's keep the SA-token path in the operator guide as the target)?
Must fix: a failed proactive re-login in currentToken falls back to the still-valid cached token silently. If re-login is permanently broken (expired secret_id, removed policy), the operator learns about it only when the cached token dies and the next DEK operation fails — at rotation time, the worst moment. Please log a warning on this fallback path.
Optional nits:
reauth()clears the cached token unconditionally: a goroutine handling a stale 403 can wipe a fresh token another goroutine just obtained, causing an extra login. Cheap fix: clear only if the cache still holds the token that got the 403.TestVaultKMSAppRoleConcurrentassertslogins >= 1, but the design guarantees exactly one login for concurrent cold-start Wraps —== 1would catch a serialization regression.- Docs: worth mentioning a sensible minimum
token_ttl(> 30s, therenewSkew), and that the Vault address must be HTTPS since thesecret_idnow travels on every re-login.
Address review: - Log a warning when a re-login fails and the still-valid cached token is served, so a permanently broken login (expired secret_id, revoked policy) is visible before it bites at DEK-rotation time. - reauth now clears the cache only when it still holds the rejected token, so a concurrent goroutine's freshly obtained token is not wiped. - Tighten the concurrent test to assert exactly one login (serialization). - Docs: recommend a token_ttl above the renew skew and require an HTTPS Vault address, since the secret_id travels on every re-login. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Kubernetes auth is the cleaner target for this proxy, so support it as a first-class mode. The auth strategy is generalized into a tokenLogin, so the proxy can log in with its projected ServiceAccount token (KKP_VAULT_KUBERNETES_ROLE / _MOUNT / _JWT_FILE), re-reading the token from disk on each login so the kubelet's rotation is picked up without a restart. AppRole and the static token remain; the config enforces exactly one auth mode. Docs mark Kubernetes as the recommended method. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Kirill Ilin (sircthulhu)
changed the title
Lock in the advertised kubelet-rotation behavior: rotate the on-disk JWT and revoke the cached token between two Wraps, then assert the forced re-login sends the rotated token rather than a cached one. Assisted-By: Claude AI Signed-off-by: Kirill Ilin <stitch14@yandex.ru>
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
All review points are addressed in 7829546 — the warning on the cached-token fallback, the scoped re-auth invalidation, the exact single-login test assertion, and the doc notes on token_ttl and HTTPS. Per the team discussion, AppRole stays for now and Kubernetes Auth lands as a follow-up together with a design-doc update. LGTM.
|
Good call — I agree Kubernetes auth is the cleaner target. Added it as a first-class, recommended mode ( The earlier round is in too ( |
2 participants
What this PR does
The Vault Transit KMS previously accepted only a static token (
KKP_VAULT_TOKEN). This adds two on-demand login methods so no long-lived token has to sit in a Secret:KKP_VAULT_KUBERNETES_ROLE/_MOUNT/_JWT_FILE) — recommended. The proxy logs in with its projected ServiceAccount token, re-read from disk on each login so the kubelet's rotation is picked up. No stored credential.KKP_VAULT_ROLE_ID/KKP_VAULT_SECRET_ID/KKP_VAULT_APPROLE_MOUNT).The static token remains supported; config enforces exactly one auth mode. The login strategy is generalized behind a
tokenLoginfunc:muis never held across the network call (logins serialized vialoginMuwith aTryLockfast path), and a failed proactive re-login falls back to the still-valid cached token with a warning. The proxy touches Vault only at startup and on DEK rotation, so a short-lived token is sufficient.Docs (README, operator guide) updated — the operator guide's earlier mention of a Kubernetes-projected ServiceAccount token path is now the real, implemented Kubernetes-auth method.
Tests cover Kubernetes and AppRole round-trips, transparent re-auth on 403, the cached-token fallback, the concurrent path under
-race, and config validation for all three modes.