diff --git a/charts/operator/templates/manager/configmap-spiffe-helper.yaml b/charts/operator/templates/manager/configmap-spiffe-helper.yaml deleted file mode 100644 index 075b3b16..00000000 --- a/charts/operator/templates/manager/configmap-spiffe-helper.yaml +++ /dev/null @@ -1,36 +0,0 @@ -{{- if and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled }} ---- -apiVersion: v1 -kind: ConfigMap -metadata: - name: operator-spiffe-helper-config - namespace: {{ .Release.Namespace }} - labels: - {{- include "chart.labels" . | nindent 4 }} -data: - config.hcl: | - agent_address = "/spiffe-workload-api/spire-agent.sock" - # cmd/cmd_args fire on X.509 SVID renewals only, not JWT SVIDs. - # File permissions are handled by running spiffe-helper as the same UID - # as the manager container (65532) so it can read the 600-mode file. - renew_signal = "" - cert_dir = "" - svid_file_name = "" - svid_bundle_file_name = "" - {{- /* - jwt_audience must equal the Keycloak realm's issuer URL — the string Keycloak - advertises in its /.well-known/openid-configuration. It is always derived from - keycloak.publicUrl (the external URL Keycloak was configured with). The internal - k8s service address must NOT be used because Keycloak's FederatedJWTClientValidator - checks the aud claim via string equality against its own issuer, which is always - the external URL. - */ -}} - {{- $audience := printf "%s/realms/%s" .Values.keycloak.publicUrl .Values.keycloak.realm -}} - {{- if not (hasPrefix "http" $audience) -}} - {{- fail "keycloak.publicUrl must be set to the external Keycloak URL (e.g. http://keycloak.example.com) when spiffe.operatorAuth.enabled=true" -}} - {{- end -}} - jwt_svids = [{ - jwt_audience = "{{ $audience }}" - jwt_svid_file_name = "/opt/jwt_svid.token" - }] -{{- end }} diff --git a/charts/operator/templates/manager/manager.yaml b/charts/operator/templates/manager/manager.yaml index ae832633..e4641a0f 100644 --- a/charts/operator/templates/manager/manager.yaml +++ b/charts/operator/templates/manager/manager.yaml @@ -118,7 +118,6 @@ spec: {{- end }} {{- if and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled }} - "--use-spiffe-auth=true" - - "--jwt-svid-path={{ .Values.spiffe.operatorAuth.jwtSVIDPath | default "/opt/jwt_svid.token" }}" - "--operator-client-id=spiffe://{{ .Values.signatureVerification.spireTrustDomain | default "localtest.me" }}/ns/{{ .Release.Namespace }}/sa/{{ .Values.controllerManager.serviceAccountName }}" {{- end }} command: @@ -181,47 +180,11 @@ spec: mountPath: /tmp/k8s-metrics-server/metrics-certs readOnly: true {{- end }} - {{- if .Values.verifiedFetch.enabled }} + {{- if or .Values.verifiedFetch.enabled (and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled) }} - name: spiffe-workload-api mountPath: /spiffe-workload-api readOnly: true {{- end }} - {{- if and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled }} - - name: jwt-svid - mountPath: /opt - readOnly: true - {{- end }} - {{- if and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled }} - - name: spiffe-helper - image: ghcr.io/spiffe/spiffe-helper:0.11.0 - imagePullPolicy: IfNotPresent - args: - - "-config" - - "/etc/spiffe-helper/config.hcl" - volumeMounts: - - name: spiffe-workload-api - mountPath: /spiffe-workload-api - readOnly: true - - name: spiffe-helper-config - mountPath: /etc/spiffe-helper - readOnly: true - - name: jwt-svid - mountPath: /opt - securityContext: - allowPrivilegeEscalation: false - runAsNonRoot: true - runAsUser: 65532 - capabilities: - drop: - - ALL - resources: - requests: - cpu: 10m - memory: 32Mi - limits: - cpu: 100m - memory: 64Mi - {{- end }} securityContext: {{- toYaml .Values.controllerManager.securityContext | nindent 8 }} serviceAccountName: {{ .Values.controllerManager.serviceAccountName }} @@ -251,11 +214,3 @@ spec: driver: "csi.spiffe.io" readOnly: true {{- end }} - {{- if and .Values.spiffe .Values.spiffe.enabled .Values.spiffe.operatorAuth .Values.spiffe.operatorAuth.enabled }} - - name: spiffe-helper-config - configMap: - name: operator-spiffe-helper-config - - name: jwt-svid - emptyDir: - medium: Memory - {{- end }} diff --git a/charts/operator/values.yaml b/charts/operator/values.yaml index f1337614..5e884799 100644 --- a/charts/operator/values.yaml +++ b/charts/operator/values.yaml @@ -205,8 +205,6 @@ spiffe: enabled: false operatorAuth: enabled: false - # Path to JWT-SVID file written by spiffe-helper sidecar - jwtSVIDPath: "/opt/jwt_svid.token" # Feature gates — highest-priority layer in the injection precedence chain. # Set globalEnabled to false to disable ALL sidecar injection (kill switch). diff --git a/operator/cmd/main.go b/operator/cmd/main.go index ffa8629f..518f1087 100644 --- a/operator/cmd/main.go +++ b/operator/cmd/main.go @@ -135,7 +135,6 @@ func main() { var credentialWaitTimeout string var enableAuthbridgeConfig bool var useSpiffeAuth bool - var jwtSVIDPath string var operatorClientID string flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ @@ -212,8 +211,6 @@ func main() { "Reconcile authbridge-config ConfigMap in namespaces labeled rossoctl-enabled=true") flag.BoolVar(&useSpiffeAuth, "use-spiffe-auth", false, "Use JWT-SVID authentication for Keycloak client registration instead of admin credentials") - flag.StringVar(&jwtSVIDPath, "jwt-svid-path", "/opt/jwt_svid.token", - "Path to JWT-SVID file written by spiffe-helper sidecar (used when --use-spiffe-auth=true)") flag.StringVar(&operatorClientID, "operator-client-id", "", "Operator SPIFFE ID (e.g. spiffe:///ns//sa/), used when --use-spiffe-auth=true") @@ -711,7 +708,7 @@ func main() { SpireTrustDomain: spireTrustDomain, KeycloakAdminTokenCache: &keycloak.CachedAdminTokenProvider{}, UseSpiffeAuth: useSpiffeAuth, - JWTSVIDPath: jwtSVIDPath, + SpiffeSocket: verifiedFetchSpiffeSocket, OperatorClientID: operatorClientID, Recorder: mgr.GetEventRecorderFor("clientregistration"), //nolint:staticcheck }).SetupWithManager(mgr); err != nil { diff --git a/operator/internal/controller/clientregistration_controller.go b/operator/internal/controller/clientregistration_controller.go index e0dcec9a..be47b8f8 100644 --- a/operator/internal/controller/clientregistration_controller.go +++ b/operator/internal/controller/clientregistration_controller.go @@ -9,13 +9,17 @@ package controller import ( "context" + "encoding/json" "fmt" - "os" - "path/filepath" + "net/http" "sort" "strings" + "sync" "time" + "github.com/spiffe/go-spiffe/v2/svid/jwtsvid" + "github.com/spiffe/go-spiffe/v2/workloadapi" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -78,14 +82,24 @@ type ClientRegistrationReconciler struct { // the Admin API with manage-clients role. When false, uses admin credentials. UseSpiffeAuth bool - // JWTSVIDPath is the file path to read the operator's JWT-SVID from. - // Only used when UseSpiffeAuth is true. Default: /opt/jwt_svid.token - JWTSVIDPath string + // SpiffeSocket is the path to the SPIFFE Workload API socket (e.g., unix:///run/spire/sockets/agent.sock). + // Only used when UseSpiffeAuth is true. Used to fetch JWT-SVIDs via go-spiffe SDK. + SpiffeSocket string // OperatorClientID is the operator's SPIFFE ID (e.g., spiffe://localtest.me/ns/rossoctl-operator-system/sa/...). // Only used when UseSpiffeAuth is true. OperatorClientID string + // keycloakIssuerCache caches Keycloak issuer URLs by (keycloakURL, realm) to avoid + // OIDC discovery HTTP requests on every reconcile. The issuer is stable per realm. + keycloakIssuerCache sync.Map // map[string]string + + // workloadAPIClientMu protects workloadAPIClient initialization + workloadAPIClientMu sync.Mutex + // workloadAPIClient is a reused connection to the SPIRE Workload API to avoid + // opening a new gRPC connection on every JWT-SVID fetch. Lazily initialized. + workloadAPIClient *workloadapi.Client + // Recorder emits Kubernetes Events to surface configuration issues visible in kubectl describe. Recorder record.EventRecorder } @@ -275,36 +289,43 @@ func (r *ClientRegistrationReconciler) reconcileOne( return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - jwtSVIDPath := r.JWTSVIDPath - if jwtSVIDPath == "" { - jwtSVIDPath = "/opt/jwt_svid.token" + if r.SpiffeSocket == "" { + err := fmt.Errorf("SpiffeSocket is required when UseSpiffeAuth=true") + logger.Error(err, "missing SPIFFE socket path") + if r.Recorder != nil { + r.Recorder.Event(owner, corev1.EventTypeWarning, "SpiffeSocketMissing", + "UseSpiffeAuth=true but SpiffeSocket is empty. Check operator configuration.") + } + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - // Path traversal protection: only allow reading from designated directories - cleanPath := filepath.Clean(jwtSVIDPath) - if !strings.HasPrefix(cleanPath, "/opt/") && !strings.HasPrefix(cleanPath, "/var/run/secrets/") { - err := fmt.Errorf("JWT-SVID path %q outside allowed directories (/opt/, /var/run/secrets/)", jwtSVIDPath) - logger.Error(err, "invalid JWT-SVID path") + // Fetch JWT-SVID from SPIRE via Workload API + // Per RFC 7523 and Keycloak SPIFFE authentication: the JWT audience must match + // Keycloak's realm issuer URL exactly. Query the OIDC discovery endpoint to get + // the authoritative issuer value, since it may differ from the in-cluster service URL. + realmIssuer, err := r.getKeycloakIssuer(ctx, ab.KeycloakURL, ab.KeycloakRealm) + if err != nil { + logger.Error(err, "Failed to get Keycloak issuer URL") if r.Recorder != nil { - r.Recorder.Eventf(owner, corev1.EventTypeWarning, "InvalidJWTSVIDPath", - "JWT-SVID path %q rejected: must be under /opt/ or /var/run/secrets/", jwtSVIDPath) + r.Recorder.Eventf(owner, corev1.EventTypeWarning, "IssuerLookupFailed", + "Failed to query Keycloak OIDC discovery: %v", err) } - return ctrl.Result{}, err // fail permanently on config error + return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } - jwtSVID, err := os.ReadFile(cleanPath) + jwtSVID, err := r.fetchJWTSVID(ctx, realmIssuer) if err != nil { - logger.Error(err, "read JWT-SVID failed", "path", cleanPath) + logger.Error(err, "JWT-SVID fetch failed") if r.Recorder != nil { - r.Recorder.Eventf(owner, corev1.EventTypeWarning, "JWTSVIDReadFailed", - "Failed to read JWT-SVID from %s: %v. Check spiffe-helper sidecar configuration.", cleanPath, err) + r.Recorder.Eventf(owner, corev1.EventTypeWarning, "JWTSVIDFetchFailed", + "Failed to fetch JWT-SVID from SPIRE: %v", err) } return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } // WARNING: JWT-SVID is a bearer token - must never appear in logs or error messages // to prevent token exposure. All code paths must handle jwtSVID as sensitive data. - token, err = kc.JWTSVIDGrantToken(ctx, ab.KeycloakRealm, r.OperatorClientID, string(jwtSVID)) + token, err = kc.JWTSVIDGrantToken(ctx, ab.KeycloakRealm, r.OperatorClientID, jwtSVID) if err != nil { logger.Error(err, "Keycloak JWT-SVID authentication failed") if r.Recorder != nil { @@ -618,3 +639,80 @@ func (r *ClientRegistrationReconciler) SetupWithManager(mgr ctrl.Manager) error return b.Complete(r) } + +// fetchJWTSVID fetches a JWT-SVID from the SPIRE Workload API for the given audience. +// Returns the JWT token as a string or an error if fetching fails. +// Reuses a long-lived workloadapi.Client to avoid gRPC connection churn on frequent reconciles. +func (r *ClientRegistrationReconciler) fetchJWTSVID(ctx context.Context, audience string) (string, error) { + // Lazily initialize the workloadapi.Client + r.workloadAPIClientMu.Lock() + if r.workloadAPIClient == nil { + client, err := workloadapi.New(ctx, workloadapi.WithAddr(r.SpiffeSocket)) + if err != nil { + r.workloadAPIClientMu.Unlock() + return "", fmt.Errorf("failed to create SPIFFE Workload API client: %w", err) + } + r.workloadAPIClient = client + } + client := r.workloadAPIClient + r.workloadAPIClientMu.Unlock() + + svid, err := client.FetchJWTSVID(ctx, jwtsvid.Params{ + Audience: audience, + }) + if err != nil { + return "", fmt.Errorf("failed to fetch JWT-SVID: %w", err) + } + + return svid.Marshal(), nil +} + +// getKeycloakIssuer queries the Keycloak OIDC discovery endpoint to get the authoritative +// issuer URL. This is necessary because the issuer may be a public URL (e.g., keycloak.localtest.me) +// while the KeycloakURL in authbridge-config is the in-cluster service address. +// Results are cached as the issuer is stable per realm. +func (r *ClientRegistrationReconciler) getKeycloakIssuer(ctx context.Context, keycloakURL, realm string) (string, error) { + // Check cache first + cacheKey := keycloakURL + ":" + realm + if cached, ok := r.keycloakIssuerCache.Load(cacheKey); ok { + return cached.(string), nil + } + + discoveryURL := strings.TrimSuffix(keycloakURL, "/") + "/realms/" + realm + "/.well-known/openid-configuration" + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, discoveryURL, nil) + if err != nil { + return "", fmt.Errorf("failed to create OIDC discovery request: %w", err) + } + + client := &http.Client{Timeout: 10 * time.Second} + resp, err := client.Do(req) + if err != nil { + return "", fmt.Errorf("failed to query OIDC discovery endpoint: %w", err) + } + defer func() { + if closeErr := resp.Body.Close(); closeErr != nil { + ctrl.Log.WithName("getKeycloakIssuer").Error(closeErr, "failed to close response body") + } + }() + + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("OIDC discovery returned status %d", resp.StatusCode) + } + + var config struct { + Issuer string `json:"issuer"` + } + if err := json.NewDecoder(resp.Body).Decode(&config); err != nil { + return "", fmt.Errorf("failed to decode OIDC discovery response: %w", err) + } + + if config.Issuer == "" { + return "", fmt.Errorf("OIDC discovery response missing issuer field") + } + + // Cache the issuer for future reconciles + r.keycloakIssuerCache.Store(cacheKey, config.Issuer) + + return config.Issuer, nil +} diff --git a/operator/internal/controller/clientregistration_controller_fetchjwtsvid_test.go b/operator/internal/controller/clientregistration_controller_fetchjwtsvid_test.go new file mode 100644 index 00000000..5b2a22d7 --- /dev/null +++ b/operator/internal/controller/clientregistration_controller_fetchjwtsvid_test.go @@ -0,0 +1,55 @@ +package controller + +import ( + "context" + "testing" + "time" +) + +func TestFetchJWTSVID_InvalidSocketPath(t *testing.T) { + // Test that fetchJWTSVID returns an error when the socket path is invalid + r := &ClientRegistrationReconciler{ + SpiffeSocket: "unix:///nonexistent/socket.sock", + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + _, err := r.fetchJWTSVID(ctx, "test-audience") + if err == nil { + t.Fatal("expected error when connecting to nonexistent socket, got nil") + } + + // Error should mention client creation failure + errMsg := err.Error() + if errMsg == "" { + t.Fatal("expected non-empty error message") + } +} + +func TestFetchJWTSVID_EmptySocketPath(t *testing.T) { + // Test that fetchJWTSVID handles empty socket path gracefully + r := &ClientRegistrationReconciler{ + SpiffeSocket: "", + } + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + _, err := r.fetchJWTSVID(ctx, "test-audience") + if err == nil { + t.Fatal("expected error when socket path is empty, got nil") + } +} + +// NOTE: Full integration tests with a real SPIRE agent require: +// 1. Running SPIRE server and agent +// 2. Properly configured workload attestation +// 3. Valid SPIFFE trust domain +// +// These are better suited for E2E tests (e.g., operator/test/e2e/) rather than +// unit tests. The tests above verify error handling for the common failure cases. +// +// For E2E token exchange verification, see: +// - rossoctl/tests/e2e/ (main repo E2E tests) +// - .github/scripts/operator/ (deployment scripts that test token exchange)