diff --git a/docs/reconcilers/authentication.md b/docs/reconcilers/authentication.md index 6e92fd6..d5c3ce9 100644 --- a/docs/reconcilers/authentication.md +++ b/docs/reconcilers/authentication.md @@ -193,6 +193,33 @@ spec: - `redirectURL`: Defaults to `https://{hostname}/oauth2/callback` - `scopes`: Uses `spec.auth.scopes` or defaults to `["openid", "profile", "email"]` +### Issuer Semantics (Keycloak) + +For Keycloak, `provider.issuer` is always the **in-cluster** realm URL, even when +`KEYCLOAK_EXTERNAL_URL` is set and Keycloak (with a public `frontendUrl`) stamps the +**public** URL into every token's `iss` claim. This mismatch is deliberate and harmless +([investigation in #112](https://github.com/nebari-dev/nebari-operator/issues/112)): + +- Envoy Gateway uses the issuer **only at control-plane time** to fetch + `/.well-known/openid-configuration`, and only when `authorizationEndpoint` or + `tokenEndpoint` is not explicitly set. When `KEYCLOAK_EXTERNAL_URL` is configured the + operator sets both, so the issuer URL is never contacted and never reaches the data + plane. +- The Envoy `oauth2` filter has no issuer field in its configuration and never inspects + the token's `iss` claim (its only JWT decode is an unverified `exp` read for cookie + lifetimes). +- Envoy Gateway does not enforce the OIDC Discovery issuer-match rule: it reads only the + endpoint fields from the discovery document and ignores its `issuer` field. +- Consumers that **do** strictly validate `iss` — e.g. a SecurityPolicy `spec.jwt` + provider configured by a downstream pack — must use the public issuer. They read it + from the client Secret's `issuer-url` key, which is populated from + `GetExternalIssuerURL()` (the public URL). That key is empty when + `KEYCLOAK_EXTERNAL_URL` is unset, so strict-`iss` consumers require it to be set. + +Keeping the issuer in-cluster means the discovery fallback (used when +`KEYCLOAK_EXTERNAL_URL` is unset) runs from the envoy-gateway controller pod over +cluster DNS, avoiding public TLS-chain trust and hairpin-routing requirements. + **On Failure:** - Event: `Warning` with reason `SecurityPolicyFailed` - Condition: `AuthReady=False` with reason `SecurityPolicyFailed` diff --git a/internal/controller/reconcilers/auth/providers/keycloak.go b/internal/controller/reconcilers/auth/providers/keycloak.go index 71fba4e..d9adb2e 100644 --- a/internal/controller/reconcilers/auth/providers/keycloak.go +++ b/internal/controller/reconcilers/auth/providers/keycloak.go @@ -71,7 +71,29 @@ func (p *KeycloakProvider) externalRealmURL() string { } // GetIssuerURL returns the internal cluster URL for the Keycloak realm. -// Envoy uses this to fetch OIDC configuration from within the cluster. +// +// This deliberately stays on the in-cluster URL even when Keycloak issues +// tokens with a public `iss` claim (frontendUrl / KC_HOSTNAME configured). +// Investigated in https://github.com/nebari-dev/nebari-operator/issues/112: +// +// - Envoy Gateway uses SecurityPolicy.spec.oidc.provider.issuer only at +// control-plane time, to fetch /.well-known/openid-configuration — and +// only when authorizationEndpoint or tokenEndpoint is not explicitly set +// (gateway v1.6.3 internal/gatewayapi/securitypolicy.go). The issuer is +// never passed to the Envoy oauth2 filter; the filter's config proto has +// no issuer field and the filter never inspects the token's `iss` claim. +// - Envoy Gateway does not enforce the OIDC Discovery issuer-match rule +// either: it unmarshals only the endpoint fields from the discovery +// document, so a public `issuer` value in the document is ignored. +// - Consumers that DO strictly validate `iss` (e.g. SecurityPolicy +// spec.jwt providers configured by downstream packs) must use the public +// issuer, which they read from the client Secret's issuer-url key — +// populated from GetExternalIssuerURL, not from this method. +// +// Keeping the issuer in-cluster means the discovery fallback (when endpoint +// overrides are incomplete) is fetched from the envoy-gateway controller pod +// over cluster DNS, avoiding public TLS-chain trust and hairpin-routing +// requirements. func (p *KeycloakProvider) GetIssuerURL(ctx context.Context, nebariApp *appsv1.NebariApp) (string, error) { return p.internalRealmURL(), nil } @@ -88,6 +110,11 @@ func (p *KeycloakProvider) GetIssuerURL(ctx context.Context, nebariApp *appsv1.N // the in-cluster URL causes the browser to fail DNS resolution and dead-end // the OAuth2 flow. // +// When KEYCLOAK_EXTERNAL_URL is set, both Authorization and Token endpoints +// are explicitly provided, so Envoy Gateway skips OIDC discovery entirely and +// the issuer URL never leaves the control plane (see GetIssuerURL). Tests +// assert this invariant; do not remove one override without the other. +// // Fallback when KEYCLOAK_EXTERNAL_URL is unset: Authorization and EndSession // are returned as nil overrides. Envoy Gateway then triggers OIDC discovery // against the (in-cluster) issuer URL and uses whatever Authorization / diff --git a/internal/controller/reconcilers/auth/providers/keycloak_test.go b/internal/controller/reconcilers/auth/providers/keycloak_test.go index 51f6bdd..7f44ba8 100644 --- a/internal/controller/reconcilers/auth/providers/keycloak_test.go +++ b/internal/controller/reconcilers/auth/providers/keycloak_test.go @@ -91,6 +91,23 @@ func TestKeycloakProvider_GetIssuerURL(t *testing.T) { }, expectedURL: "http://custom-keycloak.auth.svc.cluster.local:9090/realms/custom-realm", }, + { + // Issue #112: the issuer deliberately stays in-cluster even when + // KEYCLOAK_EXTERNAL_URL is set. The external URL only affects + // browser-facing endpoint overrides and the client Secret's + // issuer-url key, never SecurityPolicy.spec.oidc.provider.issuer. + name: "ExternalURL set: issuer remains in-cluster", + kcConfig: config.KeycloakConfig{ + URL: "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080", + Realm: "nebari", + IssuerServiceName: "keycloak-keycloakx-http", + IssuerServiceNamespace: "keycloak", + IssuerServicePort: 8080, + IssuerContextPath: "", + ExternalURL: "https://keycloak.example.com", + }, + expectedURL: "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080/realms/nebari", + }, } for _, tt := range tests { diff --git a/internal/controller/reconcilers/auth/reconciler_test.go b/internal/controller/reconcilers/auth/reconciler_test.go index b874ce7..13c78f4 100644 --- a/internal/controller/reconcilers/auth/reconciler_test.go +++ b/internal/controller/reconcilers/auth/reconciler_test.go @@ -25,6 +25,7 @@ import ( egv1alpha1 "github.com/envoyproxy/gateway/api/v1alpha1" appsv1 "github.com/nebari-dev/nebari-operator/api/v1" + "github.com/nebari-dev/nebari-operator/internal/config" "github.com/nebari-dev/nebari-operator/internal/controller/reconcilers/auth/providers" "github.com/nebari-dev/nebari-operator/internal/controller/utils/constants" "github.com/nebari-dev/nebari-operator/internal/controller/utils/naming" @@ -601,6 +602,105 @@ func TestBuildSecurityPolicySpec(t *testing.T) { } } +// TestBuildSecurityPolicySpec_KeycloakIssuerAndEndpointSplit exercises the +// real KeycloakProvider end-to-end through buildSecurityPolicySpec (issues +// #112 and #113): the issuer stays on the in-cluster URL, the token endpoint +// stays on the in-cluster host (Envoy back-channel), and the browser-facing +// authorization/end-session endpoints move to KEYCLOAK_EXTERNAL_URL. +// +// The "ExternalURL set" case also locks in the invariant that makes the +// in-cluster issuer safe even when Keycloak emits a public `iss` claim +// (frontendUrl configured): Envoy Gateway performs OIDC discovery against the +// issuer only when authorizationEndpoint or tokenEndpoint is missing, so with +// both explicitly set the issuer never leaves the control plane and is never +// compared against the token's `iss` claim (the Envoy oauth2 filter has no +// issuer field at all). See #112 for the full investigation. +func TestBuildSecurityPolicySpec_KeycloakIssuerAndEndpointSplit(t *testing.T) { + scheme := runtime.NewScheme() + _ = appsv1.AddToScheme(scheme) + _ = corev1.AddToScheme(scheme) + _ = egv1alpha1.AddToScheme(scheme) + + const internalRealm = "http://keycloak-keycloakx-http.keycloak.svc.cluster.local:8080/realms/nebari" + + tests := []struct { + name string + externalURL string + expectedAuthorization *string + expectedEndSession *string + }{ + { + name: "ExternalURL set: browser endpoints public, token and issuer in-cluster", + externalURL: "https://keycloak.example.com", + expectedAuthorization: ptr.To("https://keycloak.example.com/realms/nebari/protocol/openid-connect/auth"), + expectedEndSession: ptr.To("https://keycloak.example.com/realms/nebari/protocol/openid-connect/logout"), + }, + { + // Without an external URL the browser-facing endpoints are left + // unset so Envoy Gateway falls back to OIDC discovery against the + // in-cluster issuer; the token endpoint stays pinned in-cluster. + name: "ExternalURL unset: only token override is set", + externalURL: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + nebariApp := &appsv1.NebariApp{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-app", + Namespace: "default", + }, + Spec: appsv1.NebariAppSpec{ + Hostname: "test.example.com", + Auth: &appsv1.AuthConfig{ + Enabled: true, + Provider: constants.ProviderKeycloak, + }, + }, + } + + client := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(nebariApp). + Build() + + reconciler := &AuthReconciler{ + Client: client, + Scheme: scheme, + Recorder: record.NewFakeRecorder(10), + } + + provider := &providers.KeycloakProvider{ + Config: config.KeycloakConfig{ + Realm: "nebari", + IssuerServiceName: "keycloak-keycloakx-http", + IssuerServiceNamespace: "keycloak", + IssuerServicePort: 8080, + ExternalURL: tt.externalURL, + }, + } + + spec, err := reconciler.buildSecurityPolicySpec(context.Background(), nebariApp, provider) + if err != nil { + t.Fatalf("buildSecurityPolicySpec returned error: %v", err) + } + if spec.OIDC == nil { + t.Fatal("OIDC config is nil") + } + + p := spec.OIDC.Provider + if p.Issuer != internalRealm { + t.Errorf("expected in-cluster issuer %q, got %q", internalRealm, p.Issuer) + } + expectedToken := ptr.To(internalRealm + "/protocol/openid-connect/token") + verifyOptionalEndpoint(t, "token", p.TokenEndpoint, expectedToken) + verifyOptionalEndpoint(t, "authorization", p.AuthorizationEndpoint, tt.expectedAuthorization) + verifyOptionalEndpoint(t, "endSession", p.EndSessionEndpoint, tt.expectedEndSession) + }) + } +} + // TestBuildSecurityPolicySpec_ForwardAccessToken covers the forwardAccessToken // passthrough in isolation. Kept separate from TestBuildSecurityPolicySpec // to keep that table-driven test below the gocyclo complexity threshold.