-
Notifications
You must be signed in to change notification settings - Fork 36
feat(pam): gateway auth for kubernetes #185
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
Changes from all commits
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 |
|---|---|---|
|
|
@@ -11,11 +11,13 @@ import ( | |
| "net" | ||
| "net/http" | ||
| "net/url" | ||
| "os" | ||
| "strings" | ||
| "sync" | ||
| "time" | ||
|
|
||
| "github.com/Infisical/infisical-merge/packages/pam/session" | ||
| "github.com/Infisical/infisical-merge/packages/util" | ||
| "github.com/google/uuid" | ||
| "github.com/rs/zerolog/log" | ||
| ) | ||
|
|
@@ -24,6 +26,8 @@ type KubernetesProxyConfig struct { | |
| TargetApiServer string | ||
| AuthMethod string | ||
| InjectServiceAccountToken string | ||
| ImpersonateNamespace string | ||
| ImpersonateServiceAccount string | ||
| TLSConfig *tls.Config | ||
| SessionID string | ||
| SessionLogger session.SessionLogger | ||
|
|
@@ -40,6 +44,36 @@ func NewKubernetesProxy(config KubernetesProxyConfig) *KubernetesProxy { | |
| return &KubernetesProxy{config: config} | ||
| } | ||
|
|
||
| // injectAuthHeaders sets the appropriate auth headers based on the configured auth method. | ||
| // For service-account-token: injects the stored Bearer token. | ||
| // For gateway-kubernetes-auth: reads the gateway pod's own token (fresh each call) and sets | ||
| // Impersonate-User/Group headers to act as the target service account. | ||
| func (p *KubernetesProxy) injectAuthHeaders(headers http.Header) error { | ||
| switch p.config.AuthMethod { | ||
| case "service-account-token", "": | ||
| headers.Set("Authorization", fmt.Sprintf("Bearer %s", p.config.InjectServiceAccountToken)) | ||
| case "gateway-kubernetes-auth": | ||
| if p.config.ImpersonateNamespace == "" || p.config.ImpersonateServiceAccount == "" { | ||
| return fmt.Errorf("gateway-kubernetes-auth requires non-empty namespace and service account name") | ||
| } | ||
| // Read fresh on each request — K8s auto-rotates projected volume tokens | ||
| token, err := os.ReadFile(util.KUBERNETES_SERVICE_ACCOUNT_TOKEN_PATH) | ||
| if err != nil { | ||
| return fmt.Errorf("gateway not running in K8s cluster, unable to read pod service account token: %w", err) | ||
| } | ||
| headers.Set("Authorization", fmt.Sprintf("Bearer %s", strings.TrimSpace(string(token)))) | ||
|
|
||
| saUser := fmt.Sprintf("system:serviceaccount:%s:%s", | ||
| p.config.ImpersonateNamespace, p.config.ImpersonateServiceAccount) | ||
| headers.Set("Impersonate-User", saUser) | ||
| headers.Set("Impersonate-Group", "system:serviceaccounts") | ||
| headers.Add("Impersonate-Group", fmt.Sprintf("system:serviceaccounts:%s", p.config.ImpersonateNamespace)) | ||
| default: | ||
| return fmt.Errorf("unsupported Kubernetes auth method: %s", p.config.AuthMethod) | ||
| } | ||
|
Comment on lines
+66
to
+73
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. 🟡 In injectAuthHeaders, the Impersonate-User header is built as system:serviceaccount:{namespace}:{sa} with no check that either field is non-empty; a misconfigured PAM account (or a backend that omits these fields due to their omitempty JSON tags) produces a malformed header like system:serviceaccount::, causing an opaque 403 from the Kubernetes API server instead of a clear error from the gateway. Add an early validation check before constructing the impersonation headers and return a descriptive error if either field is empty. Extended reasoning...What the bug is and how it manifests In injectAuthHeaders (proxy.go lines 63-70), the gateway-kubernetes-auth branch constructs the Impersonate-User header as system:serviceaccount:{ImpersonateNamespace}:{ImpersonateServiceAccount} with no guard on empty strings. The same empty namespace is also embedded in the second Impersonate-Group header (system:serviceaccounts:{ImpersonateNamespace}). If either value is the empty string, both headers become malformed Kubernetes identities. The specific code path that triggers it PAMSessionCredentials in the API model declares both new fields with omitempty: ServiceAccountName and Namespace. If the backend omits either field from its JSON response (e.g. due to misconfiguration or a backend bug), Go's JSON decoder leaves the corresponding struct field as the zero value — the empty string. These empty values flow through credentials.go directly into KubernetesProxyConfig.ImpersonateNamespace / ImpersonateServiceAccount without any validation, reaching injectAuthHeaders unchanged. Why existing code does not prevent it There is no validation at any layer in the call chain — not in GetPAMSessionCredentials, not in the gateway-kubernetes-auth block in pam-proxy.go that assigns the fields, and not in injectAuthHeaders itself before the fmt.Sprintf call. What the impact would be When either field is empty the gateway sends headers such as Impersonate-User: system:serviceaccount:: and Impersonate-Group: system:serviceaccounts: to the Kubernetes API server. The API server rejects these as invalid impersonation identities with a 403. The operator debugging the failure sees a cryptic 403 from Kubernetes with no indication of which configuration field is missing — the gateway silently passes the malformed header rather than surfacing a clear configuration error. Step-by-step proof
How to fix it Add an early validation guard at the top of the gateway-kubernetes-auth case in injectAuthHeaders: if p.config.ImpersonateNamespace == "" || p.config.ImpersonateServiceAccount == "" { This causes injectAuthHeaders to return an error before any header is written, the existing error-handling path sends a 500 with a descriptive message, and the operator immediately knows which configuration field to fix. |
||
| return nil | ||
| } | ||
|
|
||
| func buildHttpInternalServerError(message string) string { | ||
| return fmt.Sprintf("HTTP/1.1 500 Internal Server Error\r\nContent-Type: application/json\r\n\r\n{\"message\": \"gateway: %s\"}", message) | ||
| } | ||
|
|
@@ -165,7 +199,14 @@ func (p *KubernetesProxy) HandleConnection(ctx context.Context, clientConn net.C | |
| continue // Continue to next request | ||
| } | ||
| proxyReq.Header = req.Header.Clone() | ||
| proxyReq.Header.Set("Authorization", fmt.Sprintf("Bearer %s", p.config.InjectServiceAccountToken)) | ||
| if err := p.injectAuthHeaders(proxyReq.Header); err != nil { | ||
| l.Error().Err(err).Msg("Failed to inject auth headers") | ||
| _, err = clientConn.Write([]byte(buildHttpInternalServerError("failed to configure auth headers"))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| continue | ||
|
Comment on lines
200
to
+208
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. 🔴 Two related error handling issues were introduced in the injectAuthHeaders failure paths. (1) In the HTTP path (~line 201), err.Error() is passed directly into buildHttpInternalServerError(), exposing internal OS-level details like 'open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory' to the kubectl client — inconsistent with every other error path in the file which uses static generic strings. (2) In the WebSocket upgrade path (~line 296), when injectAuthHeaders fails the function returns immediately with no HTTP response written to clientConn, causing a silent TCP connection drop; the client is still speaking HTTP at this point (it sent an Upgrade request) and expects either a 101 or an error status, so an HTTP 500 should be written before returning, as is done in the HTTP path. Extended reasoning...Issue 1 — Raw error string leaked to kubectl client (HTTP path) In HandleConnection, the new code at line ~201 calls buildHttpInternalServerError(err.Error()) and writes it to the client. For gateway-kubernetes-auth, the error returned by injectAuthHeaders would be 'gateway not running in K8s cluster, unable to read pod service account token: open /var/run/secrets/kubernetes.io/serviceaccount/token: no such file or directory'. This exposes the OS-level file path and internal diagnostic message to the kubectl client. Every other error path in the same function uses a static, sanitized string: buildHttpInternalServerError("failed to read request body") and buildHttpInternalServerError("failed to create proxy request"). The full error is already captured server-side via l.Error().Err(err).Msg("Failed to inject auth headers"), so there is no debugging cost to sanitizing the client-facing message. One verifier argued this is intentional because the client is authenticated and the K8s token path (/var/run/secrets/kubernetes.io/serviceaccount/token) is public knowledge. This is true — the impact is low. However, it still violates the existing file-wide pattern of using static generic client messages, and sending raw OS errors (which may include host-specific paths or vary across environments) is a deviation from least-privilege information disclosure. A simple fix: replace err.Error() with "failed to configure auth headers". Concrete proof for Issue 1:
Issue 2 — Silent TCP drop on auth failure in WebSocket path In forwardWebsocketConnection, the new code at line ~296 calls injectAuthHeaders and returns the error directly if it fails, with no write to clientConn. Before this PR, the auth step was a simple headers.Set call that could not fail, so no error response was needed. Now that injectAuthHeaders can return errors (missing token file, unsupported auth method), this path leaves the kubectl client in limbo. At the point of the auth failure, the TCP connection to the target K8s API server has already succeeded (lines ~278-287), but no HTTP response has been sent back to clientConn. The kubectl client sent an HTTP Upgrade request and is waiting for either 101 Switching Protocols or an HTTP error. Instead it receives a connection reset with no status code — for kubectl exec, kubectl logs -f, or kubectl port-forward, this manifests as an unexplained disconnect. One verifier noted that other failure paths in forwardWebsocketConnection (dial failures at lines ~278-287) also return errors without writing to clientConn, so this is internally consistent. However, those failures happen before any response is expected — the backend connection simply never established. The auth failure here occurs after the backend connection is open and the client is explicitly waiting for an HTTP response. The HTTP proxy path correctly handles this with buildHttpInternalServerError, and the same pattern should be applied here. Concrete proof for Issue 2:
Fix: In forwardWebsocketConnection, before returning err, write clientConn.Write([]byte(buildHttpInternalServerError("failed to configure auth headers"))) — mirroring the HTTP path. Also sanitize the HTTP path from err.Error() to a static string. |
||
| } | ||
|
|
||
| resp, err := selfServerClient.Do(proxyReq) | ||
| if err != nil { | ||
|
|
@@ -255,8 +296,11 @@ func (p *KubernetesProxy) forwardWebsocketConnection( | |
| sb.WriteString(fmt.Sprintf("%s %s HTTP/1.1\r\n", req.Method, newUrl.RequestURI())) | ||
| headers := req.Header.Clone() | ||
| headers.Set("Host", newUrl.Host) | ||
| // Inject the auth header | ||
| headers.Set("Authorization", fmt.Sprintf("Bearer %s", p.config.InjectServiceAccountToken)) | ||
| if err := p.injectAuthHeaders(headers); err != nil { | ||
| l.Error().Err(err).Msg("Failed to inject auth headers for websocket") | ||
| clientConn.Write([]byte(buildHttpInternalServerError("failed to configure auth headers"))) | ||
| return err | ||
| } | ||
| for key, values := range headers { | ||
| for _, value := range values { | ||
| sb.WriteString(fmt.Sprintf("%s: %s\r\n", key, value)) | ||
|
|
||
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.
The new switch rejects any auth method outside the two explicit strings, but session credentials still treat
authMethodas optional and can decode as empty. In that case requests now returnunsupported Kubernetes auth methodand the session fails, whereas previous behavior always usedServiceAccountToken. Treat an empty method asservice-account-token(or normalize it earlier) to avoid breaking existing token-based sessions.Useful? React with 👍 / 👎.