-
Notifications
You must be signed in to change notification settings - Fork 49
feat: use go-spiffe SDK directly instead of spiffe-helper sidecar #522
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
base: main
Are you sure you want to change the base?
Changes from all commits
76d4f7c
3db0229
d989096
42302de
952a53d
afce861
c0b7d1d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,16 @@ package controller | |
|
|
||
| import ( | ||
| "context" | ||
| "encoding/json" | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "net/http" | ||
| "sort" | ||
| "strings" | ||
| "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,9 +81,9 @@ 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. | ||
|
|
@@ -275,36 +278,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) | ||
|
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. suggestion: |
||
| 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) | ||
|
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. suggestion: |
||
| 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 +628,67 @@ 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. | ||
| func (r *ClientRegistrationReconciler) fetchJWTSVID(ctx context.Context, audience string) (string, error) { | ||
| client, err := workloadapi.New(ctx, workloadapi.WithAddr(r.SpiffeSocket)) | ||
| if err != nil { | ||
| return "", fmt.Errorf("failed to create SPIFFE Workload API client: %w", err) | ||
| } | ||
| defer func() { | ||
| if closeErr := client.Close(); closeErr != nil { | ||
| // Log close error but don't override the function's return error | ||
| ctrl.Log.WithName("fetchJWTSVID").Error(closeErr, "failed to close SPIFFE Workload API client") | ||
| } | ||
| }() | ||
|
|
||
| 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. | ||
| func (r *ClientRegistrationReconciler) getKeycloakIssuer(ctx context.Context, keycloakURL, realm string) (string, error) { | ||
| 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") | ||
| } | ||
|
|
||
| return config.Issuer, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) |
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.
nit: This
{{- if and .Values.spiffe ... }}{{- end }}block is empty after removing thejwt-svidvolume mount from inside it — it can be deleted.