From 1c1272c8a54afa1f236f0259ec5434bad7b34d51 Mon Sep 17 00:00:00 2001 From: niksis02 Date: Thu, 10 Sep 2026 17:36:18 +0400 Subject: [PATCH] feat: add per-provider OIDC discovery URL override `AssumeRoleWithWebIdentity` always fetched a provider's discovery document from `/.well-known/openid-configuration`, so an identity provider that issues tokens naming a public issuer while serving its metadata and keys on a cluster-internal path could not be used: reaching it meant relaxing the endpoint checks for every registered provider. `--oidc-discovery-url` moves that one fetch to an operator-named endpoint, which is how keys can be looked up over an optimized private path while the tokens themselves stay verifiable from the public internet against the issuer alone, as the JWT spec requires. The flag takes `=` pairs, can be repeated once per provider, and is also read from `VGW_IAM_OIDC_DISCOVERY_URLS` as a comma-separated list; the Helm chart exposes the same list as `iamServer.oidc.discoveryUrls`. The discovery URL is fetched exactly as written, so it must carry the `/.well-known/openid-configuration` path when the provider serves it there. A malformed pair is rejected at startup rather than at the first assume-role call. Only the fetch moves. The provider URL is still what a token's `iss` claim is matched against, the fetched document's own `issuer` field must still equal it, and the key set still comes from the `jwks_uri` that document publishes. A configured discovery endpoint is named by the operator at startup rather than by a request, so it and the `jwks_uri` it publishes waive the private-address check for that provider's fetch chain only, without `--oidc-allow-private-endpoints` and its far broader effect on every other provider. Transport rules are unchanged: a plaintext discovery URL still requires `--oidc-allow-insecure-transport`. Thumbprint auto-fetch follows the override and pins the discovery endpoint's certificate chain, since that is the host every later fetch is verified against. --- chart/Chart.yaml | 2 +- chart/README.md | 2 +- chart/templates/iam-deployment.yaml | 4 + chart/values.yaml | 13 ++ cmd/internal/gwcli/iam.go | 5 + cmd/versitygw/iam.go | 1 + embedgw/iam.go | 12 ++ extra/example-iam.conf | 11 +- iamapi/controller.go | 8 +- iamapi/controller_test.go | 96 +++++++++++ iamapi/internal/iamutil/oidc.go | 39 ++++- iamapi/internal/iamutil/oidc_test.go | 38 +++++ iamapi/internal/iamutil/oidc_thumbprint.go | 13 +- .../internal/iamutil/oidc_thumbprint_test.go | 25 +++ iamapi/internal/iamutil/webidentity.go | 35 ++-- iamapi/internal/iamutil/webidentity_test.go | 16 +- iamapi/server.go | 82 +++++++++- iamapi/server_test.go | 149 ++++++++++++++++++ 18 files changed, 512 insertions(+), 39 deletions(-) create mode 100644 iamapi/server_test.go diff --git a/chart/Chart.yaml b/chart/Chart.yaml index 0bbc9778..0aeb0636 100644 --- a/chart/Chart.yaml +++ b/chart/Chart.yaml @@ -2,7 +2,7 @@ apiVersion: v2 name: versitygw description: A Helm chart for deploying the Versity S3 Gateway on Kubernetes type: application -version: 0.4.2 +version: 0.4.3 sources: - https://github.com/versity/versitygw icon: https://raw.githubusercontent.com/versity/versitygw/main/webui/web/assets/images/Versity-logo-blue-horizontal.png diff --git a/chart/README.md b/chart/README.md index 7a2ac9c6..504a0e76 100644 --- a/chart/README.md +++ b/chart/README.md @@ -146,7 +146,7 @@ Key points: - **Private mTLS endpoint**: gateways reach the standalone IAM service over a private endpoint (`iamServer.private.port`, default `7443`) that always requires mutual TLS on TCP. Provide certificates either via `existingSecret` (bring your own `tls.crt`/`tls.key`/`ca.crt`) or `certificate.create=true` to auto-provision via cert-manager. - **Shared CA requirement**: when using cert-manager auto-provisioning, `iamServer.private.certificate.issuerRef` and `iam.standalone.certificate.issuerRef` **must reference the same CA-type issuer** (an `Issuer`/`ClusterIssuer` of kind `CA`, or a Vault issuer) — one that populates `ca.crt` in the resulting Secret. Both sides verify their peer using their own certificate's `ca.crt`, which only works when both certificates share the same issuing CA. - **External IAM service**: to point a gateway at a standalone IAM service deployed outside this chart (or by a separate chart release), set `iam.standalone.endpoint` to its `host:port` and provide the mTLS material via `iam.standalone.certificate.existingSecret`. -- **OIDC identity providers**: `AssumeRoleWithWebIdentity` only trusts an OIDC provider reachable over verified https at a publicly routable address on the implicit `:443`. An in-cluster provider satisfies none of that, so `iamServer.oidc.allowPrivateEndpoints` permits private/loopback addresses and an explicit port (a SPIRE OIDC discovery provider on a ClusterIP `Service`), and `iamServer.oidc.allowInsecureTransport` additionally permits plaintext `http://` providers and drops TLS verification (the same provider bound to `127.0.0.1` as a sidecar). Each relaxes real protections — see the comments in `values.yaml` — so enable only the one your topology needs. +- **OIDC identity providers**: `AssumeRoleWithWebIdentity` only trusts an OIDC provider reachable over verified https at a publicly routable address on the implicit `:443`. An in-cluster provider satisfies none of that, so `iamServer.oidc.allowPrivateEndpoints` permits private/loopback addresses and an explicit port (a SPIRE OIDC discovery provider on a ClusterIP `Service`), and `iamServer.oidc.allowInsecureTransport` additionally permits plaintext `http://` providers and drops TLS verification (the same provider bound to `127.0.0.1` as a sidecar). Each relaxes real protections — see the comments in `values.yaml` — so enable only the one your topology needs. `iamServer.oidc.discoveryUrls` is the narrower alternative for a provider that is public but reachable in-cluster: it moves only one provider's discovery fetch to an address you name (private addresses included, without `allowPrivateEndpoints`), while tokens keep naming their public issuer. - **WebUI access**: to manage IAM users from the WebUI, set `webui.iamGateways` to the URL a browser can reach `iamServer` on, and `iamServer.corsAllowOrigin` to the WebUI's own origin. Every WebUI call to the IAM API is cross-origin, so without `corsAllowOrigin` the browser blocks it and the WebUI's IAM navigation silently never appears. - **Secret rotation**: the processes load mTLS material and environment-based credentials at startup. After a referenced Secret rotates, restart both Deployments or configure a Secret-reloader controller through `deploymentAnnotations` and `iamServer.deploymentAnnotations`. diff --git a/chart/templates/iam-deployment.yaml b/chart/templates/iam-deployment.yaml index b3f06540..8a439578 100644 --- a/chart/templates/iam-deployment.yaml +++ b/chart/templates/iam-deployment.yaml @@ -124,6 +124,10 @@ spec: - name: VGW_IAM_OIDC_ALLOW_INSECURE_TRANSPORT value: "true" {{- end }} + {{- if $iamServerOidc.discoveryUrls }} + - name: VGW_IAM_OIDC_DISCOVERY_URLS + value: {{ join "," $iamServerOidc.discoveryUrls | quote }} + {{- end }} {{- if .Values.iamServer.corsAllowOrigin }} - name: VGW_CORS_ALLOW_ORIGIN value: {{ .Values.iamServer.corsAllowOrigin | quote }} diff --git a/chart/values.yaml b/chart/values.yaml index 89b2f389..1a6f2587 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -435,6 +435,19 @@ iamServer: # own pod. Needs allowPrivateEndpoints as well for a loopback or # cluster-internal address. allowInsecureTransport: false + # Fetch individual providers' discovery documents from somewhere other + # than the provider URL itself, as "=" + # entries. The discovery URL is fetched exactly as written, so include the + # "/.well-known/openid-configuration" path, and it may be a private + # in-cluster address without allowPrivateEndpoints above -- it is named + # here rather than by a request. Only the fetch moves: tokens are still + # matched against the provider URL, the fetched document's own issuer must + # still equal it, and keys still come from the jwks_uri that document + # publishes. This is how an identity provider hands out tokens naming its + # public issuer while the gateway reads its keys over a cluster-internal + # Service. + discoveryUrls: [] + # - "https://oidc.example.com=https://oidc.oidc-ns/.well-known/openid-configuration" # Access-Control-Allow-Origin for the control-plane API. Required before a # browser can reach this service: the WebUI is served from another origin, so # every call it makes is cross-origin and is blocked without this. Set it to diff --git a/cmd/internal/gwcli/iam.go b/cmd/internal/gwcli/iam.go index b49bce3c..60ae6e0e 100644 --- a/cmd/internal/gwcli/iam.go +++ b/cmd/internal/gwcli/iam.go @@ -124,6 +124,11 @@ func IAMCommand() *cli.Command { Usage: "allow plaintext http OIDC provider URLs and skip TLS certificate verification (thumbprint pinning included) for https ones; only for an identity provider reached over an already-trusted path, such as a loopback-bound sidecar", EnvVars: []string{"VGW_IAM_OIDC_ALLOW_INSECURE_TRANSPORT"}, }, + &cli.StringSliceFlag{ + Name: "oidc-discovery-url", + Usage: "fetch one OIDC provider's discovery document from somewhere other than the provider URL itself, as '=' (can be specified multiple times); the discovery URL is fetched exactly as given, so include the '/.well-known/openid-configuration' path, and it may be a private in-cluster address without --oidc-allow-private-endpoints. Tokens are still matched against the provider URL, and keys are still read from the jwks_uri the fetched document publishes", + EnvVars: []string{"VGW_IAM_OIDC_DISCOVERY_URLS"}, + }, &cli.StringSliceFlag{ Name: "private-ports", Usage: "private endpoint listen address: a unix socket path, or :/: when mTLS (--private-cert/--private-cert-key/--private-client-ca) is also configured — refuses to start otherwise (can be specified multiple times)", diff --git a/cmd/versitygw/iam.go b/cmd/versitygw/iam.go index 93254f4a..610dc331 100644 --- a/cmd/versitygw/iam.go +++ b/cmd/versitygw/iam.go @@ -73,6 +73,7 @@ func runIAM(ctx *cli.Context) error { DisableOIDCThumbprintAutoFetch: ctx.Bool("disable-oidc-thumbprint-autofetch"), OIDCAllowPrivateEndpoints: ctx.Bool("oidc-allow-private-endpoints"), OIDCAllowInsecureTransport: ctx.Bool("oidc-allow-insecure-transport"), + OIDCDiscoveryURLs: ctx.StringSlice("oidc-discovery-url"), CORSAllowOrigin: corsAllowOrigin, Region: region, WebuiPorts: webuiPorts, diff --git a/embedgw/iam.go b/embedgw/iam.go index 9bbf1431..00006406 100644 --- a/embedgw/iam.go +++ b/embedgw/iam.go @@ -208,6 +208,15 @@ type IAMConfig struct { // itself trusted, such as a discovery provider bound to loopback as a // sidecar in this process's own pod. OIDCAllowInsecureTransport bool + + // OIDCDiscoveryURLs redirects individual providers' discovery-document + // fetches, as "=" pairs. The discovery URL + // is fetched exactly as given, path included. Only the fetch moves: the + // provider Url stays what a token's iss claim and the fetched document's + // own issuer field must match, so an IdP can hand out tokens naming its + // public issuer while this process reads its keys over a private, + // in-cluster path. + OIDCDiscoveryURLs []string } // privateAPIServer is the standalone IAM service's private endpoint set @@ -441,6 +450,9 @@ func RunIAMAPI(ctx context.Context, cfg *IAMConfig) error { if cfg.OIDCAllowInsecureTransport { opts = append(opts, iamapi.WithOIDCAllowInsecureTransport()) } + if len(cfg.OIDCDiscoveryURLs) > 0 { + opts = append(opts, iamapi.WithOIDCDiscoveryURLs(cfg.OIDCDiscoveryURLs)) + } corsAllowOrigin := strings.TrimSpace(cfg.CORSAllowOrigin) if len(cfg.WebuiPorts) > 0 && corsAllowOrigin == "" { // Every WebUI call to this API is cross-origin, so without an allowed diff --git a/extra/example-iam.conf b/extra/example-iam.conf index 5ae70692..d6bb7e42 100644 --- a/extra/example-iam.conf +++ b/extra/example-iam.conf @@ -153,4 +153,13 @@ ROOT_SECRET_ACCESS_KEY= # verification (thumbprint pinning included) for https ones; only for an # identity provider reached over an already-trusted path, such as a # loopback-bound sidecar. -#VGW_IAM_OIDC_ALLOW_INSECURE_TRANSPORT=false \ No newline at end of file +#VGW_IAM_OIDC_ALLOW_INSECURE_TRANSPORT=false + +# Fetch one OIDC provider's discovery document from somewhere other than the +# provider URL itself, as '='. Specify one or +# more comma-separated entries. The discovery URL is fetched exactly as +# given, so include the '/.well-known/openid-configuration' path, and it may +# be a private in-cluster address without VGW_IAM_OIDC_ALLOW_PRIVATE_ENDPOINTS. +# Tokens are still matched against the provider URL, and keys are still read +# from the jwks_uri the fetched document publishes. +#VGW_IAM_OIDC_DISCOVERY_URLS= \ No newline at end of file diff --git a/iamapi/controller.go b/iamapi/controller.go index b6bcdc66..8e3a59c6 100644 --- a/iamapi/controller.go +++ b/iamapi/controller.go @@ -18,6 +18,7 @@ import ( "errors" "fmt" "slices" + "strings" "time" "github.com/gofiber/fiber/v3" @@ -1054,13 +1055,14 @@ func (c IAMApiController) CreateOpenIDConnectProvider(ctx fiber.Ctx) (*Response, thumbprints := iamutil.ParseStringList(ctx, "ThumbprintList") if len(thumbprints) == 0 { + endpoint, _ := c.oidcPolicy.ResolveDiscovery(url) switch { - case iamutil.IsInsecureOIDCProviderURL(url): - // A plaintext http provider never presents a certificate, so + case !strings.HasPrefix(endpoint, "https://"): + // A plaintext http endpoint never presents a certificate, so // there is nothing to auto-fetch and nothing for a later JWKS // fetch to pin against: an empty ThumbprintList is the accurate // record of that, not a missing one. - debuglogger.Logf("CreateOpenIDConnectProvider: %q is a plaintext http provider; storing an empty ThumbprintList", url) + debuglogger.Logf("CreateOpenIDConnectProvider: %q is reached over plaintext http; storing an empty ThumbprintList", endpoint) case c.oidc.ThumbprintAutoFetchDisabled: debuglogger.Logf("CreateOpenIDConnectProvider: ThumbprintList omitted and auto-fetch is disabled") return nil, iamerr.MissingValue("thumbprintList") diff --git a/iamapi/controller_test.go b/iamapi/controller_test.go index a2527a72..aa990495 100644 --- a/iamapi/controller_test.go +++ b/iamapi/controller_test.go @@ -3864,6 +3864,102 @@ func TestIAMApiControllerAssumeRoleWithWebIdentityLoopbackIdP(t *testing.T) { }) } +// TestIAMApiControllerAssumeRoleWithWebIdentityDiscoveryURL exercises a +// configured discovery URL: the token's issuer is a public URL that is never +// contacted, while the discovery document and the JWKS it points to are +// served from loopback. Only the discovery override makes that address +// reachable — insecure transport is about schemes, not addresses — and the +// document's own issuer must still equal the provider Url. +func TestIAMApiControllerAssumeRoleWithWebIdentityDiscoveryURL(t *testing.T) { + key, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatalf("rsa.GenerateKey: %v", err) + } + + const goodIssuer = "https://oidc.discovery.example" + const badIssuer = "https://oidc.mismatch.example" + + var keysURI string + idp := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/good/.well-known/openid-configuration": + json.NewEncoder(w).Encode(map[string]any{"issuer": goodIssuer, "jwks_uri": keysURI}) + case "/mismatch/.well-known/openid-configuration": + // Served for badIssuer, but naming its own private location. + json.NewEncoder(w).Encode(map[string]any{"issuer": r.Host, "jwks_uri": keysURI}) + case "/keys": + json.NewEncoder(w).Encode(map[string]any{"keys": []any{map[string]any{ + "kty": "RSA", + "kid": "k1", + "n": base64.RawURLEncoding.EncodeToString(key.N.Bytes()), + "e": base64.RawURLEncoding.EncodeToString(big.NewInt(int64(key.E)).Bytes()), + }}}) + default: + w.WriteHeader(http.StatusNotFound) + } + })) + defer idp.Close() + keysURI = idp.URL + "/keys" + + server := newIAMControllerTestServerWith(t, + WithOIDCAllowInsecureTransport(), + WithOIDCDiscoveryURLs([]string{ + goodIssuer + "=" + idp.URL + "/good/.well-known/openid-configuration", + badIssuer + "=" + idp.URL + "/mismatch/.well-known/openid-configuration", + })) + + for _, issuer := range []string{goodIssuer, badIssuer} { + providerArn := createTestOIDCProviderForTrust(t, server, issuer, "versitygw") + roleName := "role-for-" + iamutil.CanonicalOIDCProviderURL(issuer) + createTestRoleForTrust(t, server, roleName, + `{"Version":"2012-10-17","Statement":[{"Effect":"Allow","Principal":{"Federated":"`+providerArn+`"},`+ + `"Action":"sts:AssumeRoleWithWebIdentity"}]}`) + } + + assume := func(t *testing.T, issuer, roleName string) *http.Response { + t.Helper() + token := jwt.NewWithClaims(jwt.SigningMethodRS256, jwt.MapClaims{ + "iss": issuer, + "aud": "versitygw", + "sub": "spiffe://example.org/ns/default/sa/versitygw", + "iat": time.Now().Unix(), + "exp": time.Now().Add(time.Hour).Unix(), + }) + token.Header["kid"] = "k1" + signed, err := token.SignedString(key) + if err != nil { + t.Fatalf("sign token: %v", err) + } + return doSTSAction(t, server, url.Values{ + "Action": {"AssumeRoleWithWebIdentity"}, + "RoleArn": {"arn:aws:iam::000000000000:role/" + roleName}, + "RoleSessionName": {"discovery-session"}, + "WebIdentityToken": {signed}, + }) + } + + t.Run("keys read over the private path", func(t *testing.T) { + resp := assume(t, goodIssuer, "role-for-oidc.discovery.example") + if resp.StatusCode != http.StatusOK { + t.Fatalf("AssumeRoleWithWebIdentity status = %d, body=%s", resp.StatusCode, readBody(t, resp)) + } + var out iamtypes.AssumeRoleWithWebIdentityResponse + unmarshalXML(t, readBody(t, resp), &out) + if out.Result.Provider != goodIssuer { + t.Errorf("Provider = %q, want the public issuer %q", out.Result.Provider, goodIssuer) + } + if out.Result.Credentials.SessionToken == "" { + t.Errorf("no session credentials returned: %+v", out.Result.Credentials) + } + }) + + t.Run("discovery document naming another issuer is rejected", func(t *testing.T) { + resp := assume(t, badIssuer, "role-for-oidc.mismatch.example") + requireSTSError(t, resp, http.StatusBadRequest, "Sender", "InvalidIdentityToken", + "Couldn't retrieve verification key from your identity provider, please reference AssumeRoleWithWebIdentity documentation for requirements") + }) +} + // accessKeyImplicitUserNameActions are the four access-key actions that // accept an omitted UserName and infer it from the calling access key. var accessKeyImplicitUserNameActions = []string{"CreateAccessKey", "UpdateAccessKey", "DeleteAccessKey", "ListAccessKeys"} diff --git a/iamapi/internal/iamutil/oidc.go b/iamapi/internal/iamutil/oidc.go index b0643bda..43f47277 100644 --- a/iamapi/internal/iamutil/oidc.go +++ b/iamapi/internal/iamutil/oidc.go @@ -79,6 +79,12 @@ type OIDCEndpointPolicy struct { // path is trustworthy on its own, such as a sidecar bound to loopback // inside the gateway's own pod. AllowInsecureTransport bool + + // DiscoveryURLs maps a stored provider Url to the exact URL its + // discovery document is fetched from, so an IdP's keys can be read over + // a private in-cluster path while the tokens it issues keep naming their + // public issuer. Only the fetch moves; see ResolveDiscovery. + DiscoveryURLs map[string]string } // IsInsecureOIDCProviderURL reports whether providerURL, a stored provider @@ -103,6 +109,32 @@ func OIDCEndpointURL(providerURL string) string { return "https://" + providerURL } +// CanonicalOIDCProviderURL reduces a full provider URL to the form providers +// are stored under, the inverse of OIDCEndpointURL. +func CanonicalOIDCProviderURL(rawURL string) string { + if IsInsecureOIDCProviderURL(rawURL) { + return rawURL + } + return strings.TrimPrefix(rawURL, "https://") +} + +// ResolveDiscovery returns where providerURL's discovery document is fetched +// from, and the policy governing that fetch and the jwks_uri the document +// publishes: the well-known path under the provider's own endpoint under an +// unchanged policy, or a configured DiscoveryURLs entry under one whose +// private-address check is waived. That entry is named by the operator at +// startup rather than by a request, so it needs no AllowPrivateEndpoints to +// be private. Transport is unaffected either way. +func (p OIDCEndpointPolicy) ResolveDiscovery(providerURL string) (string, OIDCEndpointPolicy) { + endpoint, ok := p.DiscoveryURLs[providerURL] + if !ok { + base := strings.TrimRight(OIDCEndpointURL(providerURL), "/") + return base + "/.well-known/openid-configuration", p + } + p.AllowPrivateEndpoints = true + return endpoint, p +} + // ParseStringList reads flat indexed list members ".member.1", // ".member.2", ... — the AWS Query-protocol wire form for a bare // []string (distinct from ParseTags's Key/Value-pair member form, used by @@ -256,10 +288,7 @@ func ValidateOIDCProviderURL(rawURL string, policy OIDCEndpointPolicy) (string, return "", iamerr.InvalidInput("Invalid Open ID Connect Provider URL.") } - if insecure { - return rawURL, nil - } - return strings.TrimPrefix(rawURL, "https://"), nil + return CanonicalOIDCProviderURL(rawURL), nil } func isValidOIDCHostname(host string) bool { @@ -269,7 +298,7 @@ func isValidOIDCHostname(host string) bool { if host == "" || len(host) > 253 { return false } - for _, label := range strings.Split(host, ".") { + for label := range strings.SplitSeq(host, ".") { if !oidcHostLabelPattern.MatchString(label) { return false } diff --git a/iamapi/internal/iamutil/oidc_test.go b/iamapi/internal/iamutil/oidc_test.go index bb77f7e0..150dee5a 100644 --- a/iamapi/internal/iamutil/oidc_test.go +++ b/iamapi/internal/iamutil/oidc_test.go @@ -25,6 +25,44 @@ var ( insecureOIDCPolicy = OIDCEndpointPolicy{AllowPrivateEndpoints: true, AllowInsecureTransport: true} ) +// TestResolveDiscovery covers where a provider's discovery document is +// fetched from, and the address-check waiver a configured discovery URL +// carries: the endpoint is named by the operator at startup, not by a +// request, so it needs no AllowPrivateEndpoints to be private. +func TestResolveDiscovery(t *testing.T) { + const clusterURL = "https://oidc.oidc-ns/.well-known/openid-configuration" + policy := OIDCEndpointPolicy{DiscoveryURLs: map[string]string{"oidc.example.com": clusterURL}} + + tests := []struct { + name string + providerURL string + want string + wantPrivate bool + }{ + {"override", "oidc.example.com", clusterURL, true}, + {"other provider unaffected", "other.example.com", "https://other.example.com/.well-known/openid-configuration", false}, + {"trailing slash", "other.example.com/", "https://other.example.com/.well-known/openid-configuration", false}, + {"http provider keeps its scheme", "http://127.0.0.1:8080", "http://127.0.0.1:8080/.well-known/openid-configuration", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, gotPolicy := policy.ResolveDiscovery(tt.providerURL) + if got != tt.want { + t.Errorf("ResolveDiscovery(%q) = %q, want %q", tt.providerURL, got, tt.want) + } + if gotPolicy.AllowPrivateEndpoints != tt.wantPrivate { + t.Errorf("AllowPrivateEndpoints = %v, want %v", gotPolicy.AllowPrivateEndpoints, tt.wantPrivate) + } + }) + } + + // The waiver is scoped to the returned copy: the policy the rest of the + // request is validated against keeps its address check. + if policy.AllowPrivateEndpoints { + t.Error("ResolveDiscovery mutated the receiver's AllowPrivateEndpoints") + } +} + func TestValidateOIDCProviderURL(t *testing.T) { tests := []struct { name string diff --git a/iamapi/internal/iamutil/oidc_thumbprint.go b/iamapi/internal/iamutil/oidc_thumbprint.go index 0157e2f7..3cd935d0 100644 --- a/iamapi/internal/iamutil/oidc_thumbprint.go +++ b/iamapi/internal/iamutil/oidc_thumbprint.go @@ -38,7 +38,9 @@ const oidcThumbprintFetchTimeout = 8 * time.Second // the presented chain against the system trust store and the provider's own // hostname like any normal TLS client, and returns the SHA-1 thumbprint of // the last (top-most/intermediate CA) certificate in the peer's presented -// chain. +// chain. A configured discovery URL moves the handshake to that endpoint's +// host, since that is the host every later fetch pins this thumbprint +// against. // // SSRF hardening: the hostname is resolved once via // net.DefaultResolver.LookupIP; if any resolved address is @@ -70,14 +72,15 @@ const oidcThumbprintFetchTimeout = 8 * time.Second // verification for this handshake entirely and pins whatever is presented. func FetchThumbprint(ctx context.Context, providerURL string, policy OIDCEndpointPolicy) (string, error) { displayURL := OIDCEndpointURL(providerURL) - if IsInsecureOIDCProviderURL(providerURL) { - // A plaintext http provider performs no handshake, so there is no + endpoint, policy := policy.ResolveDiscovery(providerURL) + if !strings.HasPrefix(endpoint, "https://") { + // A plaintext http endpoint performs no handshake, so there is no // certificate to observe. Callers skip auto-fetch for these // entirely; this is the guard for the ones that don't. - debuglogger.Logf("oidc thumbprint fetch: %q is a plaintext http provider and presents no certificate", displayURL) + debuglogger.Logf("oidc thumbprint fetch: %q is reached over plaintext http and presents no certificate", endpoint) return "", iamerr.OpenIdIdpCommunicationError(displayURL) } - host, port := splitOIDCHostPort(hostFromOIDCUrl(providerURL)) + host, port := splitOIDCHostPort(hostFromOIDCUrl(CanonicalOIDCProviderURL(endpoint))) ctx, cancel := context.WithTimeout(ctx, oidcThumbprintFetchTimeout) defer cancel() diff --git a/iamapi/internal/iamutil/oidc_thumbprint_test.go b/iamapi/internal/iamutil/oidc_thumbprint_test.go index 677164ec..10e1b618 100644 --- a/iamapi/internal/iamutil/oidc_thumbprint_test.go +++ b/iamapi/internal/iamutil/oidc_thumbprint_test.go @@ -180,6 +180,31 @@ func TestFetchThumbprintAllowPrivateEndpoints(t *testing.T) { } } +// TestFetchThumbprintDiscoveryURL confirms auto-fetch pins the endpoint it +// will actually talk to: with a discovery URL configured, the handshake goes +// to that endpoint's host — a loopback one here, which the default address +// check would refuse — rather than to the provider's own unreachable host. +func TestFetchThumbprintDiscoveryURL(t *testing.T) { + srv := httptest.NewTLSServer(nil) + defer srv.Close() + + policy := OIDCEndpointPolicy{ + AllowInsecureTransport: true, + DiscoveryURLs: map[string]string{ + "idp.example": "https://" + srv.Listener.Addr().String() + "/.well-known/openid-configuration", + }, + } + got, err := FetchThumbprint(context.Background(), "idp.example", policy) + if err != nil { + t.Fatalf("FetchThumbprint(discovery url): %v", err) + } + + sum := sha1.Sum(srv.Certificate().Raw) + if want := hex.EncodeToString(sum[:]); got != want { + t.Fatalf("FetchThumbprint thumbprint = %q, want the discovery endpoint's %q", got, want) + } +} + // TestFetchThumbprintRejectsPlaintextProvider covers the defensive branch // for an http provider Url: there is no handshake to observe a certificate // in, so auto-fetch must report a failure rather than dial anything. diff --git a/iamapi/internal/iamutil/webidentity.go b/iamapi/internal/iamutil/webidentity.go index 721d8e6e..7e12daa4 100644 --- a/iamapi/internal/iamutil/webidentity.go +++ b/iamapi/internal/iamutil/webidentity.go @@ -617,18 +617,22 @@ var ( // current ThumbprintList (freshly read from storage for the request being // verified), so a changed configuration always maps to a different key here; // thumbprints are sorted first since storage doesn't guarantee list order is -// stable across reads of an unchanged provider. -func jwksCacheKey(issuerURL string, thumbprints []string) string { +// stable across reads of an unchanged provider. The discovery endpoint +// policy resolves issuerURL to is bound in as well, so an in-process restart +// with a different discovery override never reuses key material fetched +// from the previous endpoint. +func jwksCacheKey(issuerURL string, thumbprints []string, policy OIDCEndpointPolicy) string { sorted := slices.Clone(thumbprints) slices.Sort(sorted) - return issuerURL + "|" + strings.Join(sorted, ",") + discoveryURL, _ := policy.ResolveDiscovery(issuerURL) + return issuerURL + "|" + discoveryURL + "|" + strings.Join(sorted, ",") } // cachedJWKS returns issuerURL's key set from cache if a fresh-enough entry // exists for the current thumbprints, otherwise fetches and caches a fresh // one. func cachedJWKS(ctx context.Context, issuerURL string, thumbprints []string, policy OIDCEndpointPolicy) (*jwkSet, error) { - key := jwksCacheKey(issuerURL, thumbprints) + key := jwksCacheKey(issuerURL, thumbprints, policy) jwksCacheMu.Lock() entry, ok := jwksCache[key] jwksCacheMu.Unlock() @@ -656,7 +660,7 @@ func cachedJWKS(ctx context.Context, issuerURL string, thumbprints []string, pol // gated the next one. Recording the attempt up front bounds retries to one // per jwksMinForcedRefreshInterval regardless of whether the fetch succeeds. func forceRefreshJWKSCache(ctx context.Context, issuerURL string, thumbprints []string, policy OIDCEndpointPolicy) (*jwkSet, error) { - key := jwksCacheKey(issuerURL, thumbprints) + key := jwksCacheKey(issuerURL, thumbprints, policy) jwksCacheMu.Lock() entry, ok := jwksCache[key] if ok && time.Since(entry.lastForcedRefresh) < jwksMinForcedRefreshInterval { @@ -679,12 +683,12 @@ func forceRefreshJWKSCache(ctx context.Context, issuerURL string, thumbprints [] } // fetchAndCacheJWKS fetches issuerURL's key set and, on success, replaces -// its cache entry, coalescing concurrent callers for the same issuerURL AND -// thumbprints via jwksFetchGroup (keyed identically to jwksCache, so a +// its cache entry, coalescing concurrent callers for the same issuerURL, +// thumbprints AND discovery endpoint via jwksFetchGroup (keyed identically to jwksCache, so a // caller mid-fetch for one thumbprint configuration never receives a result // coalesced from a differently-configured concurrent caller). func fetchAndCacheJWKS(ctx context.Context, issuerURL string, thumbprints []string, policy OIDCEndpointPolicy) (*jwkSet, error) { - key := jwksCacheKey(issuerURL, thumbprints) + key := jwksCacheKey(issuerURL, thumbprints, policy) v, err, _ := jwksFetchGroup.Do(key, func() (any, error) { keys, err := fetchJWKS(ctx, issuerURL, thumbprints, policy) if err != nil { @@ -704,17 +708,18 @@ func fetchAndCacheJWKS(ctx context.Context, issuerURL string, thumbprints []stri return v.(*jwkSet), nil } -// fetchJWKS retrieves issuerURL's OIDC discovery document, then the JWKS it -// points to. issuerURL is the provider's stored Url. thumbprints, if -// non-empty, lets the fetch's TLS connections succeed against a -// self-signed/private-CA certificate whose chain matches one of them, the -// same trust-pinning fallback real AWS documents for OIDC providers. +// fetchJWKS retrieves issuerURL's OIDC discovery document — from wherever +// ResolveDiscovery places it — then the JWKS that document points to. +// issuerURL is the provider's stored Url. thumbprints, if non-empty, lets +// the fetch's TLS connections succeed against a self-signed/private-CA +// certificate whose chain matches one of them, the same trust-pinning +// fallback real AWS documents for OIDC providers. func fetchJWKS(ctx context.Context, issuerURL string, thumbprints []string, policy OIDCEndpointPolicy) (*jwkSet, error) { + discoveryURL, policy := policy.ResolveDiscovery(issuerURL) client := ssrfSafeHTTPClient(thumbprints, policy) - base := OIDCEndpointURL(issuerURL) var doc oidcDiscoveryDoc - if err := fetchJSON(ctx, client, strings.TrimRight(base, "/")+"/.well-known/openid-configuration", &doc); err != nil { + if err := fetchJSON(ctx, client, discoveryURL, &doc); err != nil { return nil, err } if err := validateDiscoveryIssuer(doc, issuerURL); err != nil { diff --git a/iamapi/internal/iamutil/webidentity_test.go b/iamapi/internal/iamutil/webidentity_test.go index 4147222d..ffccedb1 100644 --- a/iamapi/internal/iamutil/webidentity_test.go +++ b/iamapi/internal/iamutil/webidentity_test.go @@ -572,27 +572,31 @@ func TestValidateDiscoveryIssuer(t *testing.T) { } func TestJWKSCacheKeyBindsThumbprints(t *testing.T) { - base := jwksCacheKey("example.com", []string{"aaaa"}) + base := jwksCacheKey("example.com", []string{"aaaa"}, OIDCEndpointPolicy{}) - if got := jwksCacheKey("example.com", []string{"bbbb"}); got == base { + if got := jwksCacheKey("example.com", []string{"bbbb"}, OIDCEndpointPolicy{}); got == base { t.Errorf("jwksCacheKey did not change when thumbprint changed: %q", got) } - if got := jwksCacheKey("example.com", nil); got == base { + if got := jwksCacheKey("example.com", nil, OIDCEndpointPolicy{}); got == base { t.Errorf("jwksCacheKey did not change when thumbprint was removed: %q", got) } - if got := jwksCacheKey("other.example.com", []string{"aaaa"}); got == base { + if got := jwksCacheKey("other.example.com", []string{"aaaa"}, OIDCEndpointPolicy{}); got == base { t.Errorf("jwksCacheKey did not change when issuer changed: %q", got) } + overridden := OIDCEndpointPolicy{DiscoveryURLs: map[string]string{"example.com": "https://oidc.internal/.well-known/openid-configuration"}} + if got := jwksCacheKey("example.com", []string{"aaaa"}, overridden); got == base { + t.Errorf("jwksCacheKey did not change when the discovery endpoint changed: %q", got) + } // Storage doesn't guarantee ThumbprintList order is stable across reads // of an unchanged provider, so the key must not depend on input order. - if got := jwksCacheKey("example.com", []string{"bbbb", "aaaa"}); got != jwksCacheKey("example.com", []string{"aaaa", "bbbb"}) { + if got := jwksCacheKey("example.com", []string{"bbbb", "aaaa"}, OIDCEndpointPolicy{}); got != jwksCacheKey("example.com", []string{"aaaa", "bbbb"}, OIDCEndpointPolicy{}) { t.Errorf("jwksCacheKey is sensitive to thumbprint order: %q", got) } } func TestForceRefreshJWKSCacheGatesFailedAttempts(t *testing.T) { issuer := "localhost" - key := jwksCacheKey(issuer, nil) + key := jwksCacheKey(issuer, nil, OIDCEndpointPolicy{}) jwksCacheMu.Lock() delete(jwksCache, key) jwksCacheMu.Unlock() diff --git a/iamapi/server.go b/iamapi/server.go index 023e8a81..575c923b 100644 --- a/iamapi/server.go +++ b/iamapi/server.go @@ -18,6 +18,7 @@ import ( "fmt" "net" "net/http" + "net/url" "os" "strings" "time" @@ -84,17 +85,74 @@ type OIDCConfig struct { // drops TLS verification for https ones; see // WithOIDCAllowInsecureTransport. AllowInsecureTransport bool + // DiscoveryURLs holds "=" pairs, each + // redirecting one provider's discovery-document fetch; see + // WithOIDCDiscoveryURLs. + DiscoveryURLs []string + // discovery is DiscoveryURLs parsed and keyed by stored provider Url, + // built by New. + discovery map[string]string } -// endpointPolicy projects the two endpoint relaxations into the form -// iamutil's URL-validation and fetch helpers take. +// endpointPolicy projects the endpoint relaxations into the form iamutil's +// URL-validation and fetch helpers take. func (c OIDCConfig) endpointPolicy() iamutil.OIDCEndpointPolicy { return iamutil.OIDCEndpointPolicy{ AllowPrivateEndpoints: c.AllowPrivateEndpoints, AllowInsecureTransport: c.AllowInsecureTransport, + DiscoveryURLs: c.discovery, } } +// parseDiscoveryURLs turns DiscoveryURLs' pairs into the map the endpoint +// policy takes, keyed by stored provider Url so a lookup by a provider's +// stored form hits directly. The provider Url is held to the same rules +// CreateOpenIDConnectProvider applies, so a pair naming a provider that could +// never be registered fails here; the discovery URL must be an absolute +// http or https URL, since it is fetched exactly as written. +func (c *OIDCConfig) parseDiscoveryURLs() error { + if len(c.DiscoveryURLs) == 0 { + return nil + } + c.discovery = make(map[string]string, len(c.DiscoveryURLs)) + for _, pair := range c.DiscoveryURLs { + providerURL, discoveryURL, ok := cutDiscoveryURLPair(pair) + providerURL, discoveryURL = strings.TrimSpace(providerURL), strings.TrimSpace(discoveryURL) + if !ok || providerURL == "" { + return fmt.Errorf("iamapi: oidc discovery url %q must be in = form, with an http:// or https:// discovery url", pair) + } + stored, err := iamutil.ValidateOIDCProviderURL(providerURL, c.endpointPolicy()) + if err != nil { + return fmt.Errorf("iamapi: oidc discovery url %q: invalid provider url %q: %w", pair, providerURL, err) + } + parsed, err := url.Parse(discoveryURL) + if err != nil || (parsed.Scheme != "https" && parsed.Scheme != "http") || parsed.Host == "" { + return fmt.Errorf("iamapi: oidc discovery url %q: invalid discovery url %q", pair, discoveryURL) + } + if parsed.Scheme == "http" && !c.AllowInsecureTransport { + return fmt.Errorf("iamapi: plaintext oidc discovery url %q requires insecure transport to be allowed", discoveryURL) + } + c.discovery[stored] = discoveryURL + } + return nil +} + +// cutDiscoveryURLPair splits a "=" pair at the +// "=" immediately preceding the discovery URL's scheme rather than at the +// first "=", since a provider Url's path may itself contain "=". +func cutDiscoveryURLPair(pair string) (providerURL, discoveryURL string, ok bool) { + i := -1 + for _, sep := range []string{"=https://", "=http://"} { + if j := strings.Index(pair, sep); j >= 0 && (i < 0 || j < i) { + i = j + } + } + if i < 0 { + return "", "", false + } + return pair[:i], pair[i+1:], true +} + func New(store storage.Storer, root RootCredentials, opts ...Option) (*IAMApiServer, error) { if store == nil { return nil, fmt.Errorf("iamapi: storer is required") @@ -112,6 +170,10 @@ func New(store storage.Storer, root RootCredentials, opts ...Option) (*IAMApiSer opt(server) } + if err := server.oidc.parseDiscoveryURLs(); err != nil { + return nil, err + } + app := fiber.New(fiber.Config{ AppName: "versitygw-iam", ServerHeader: "VERSITYGW", @@ -236,6 +298,22 @@ func WithOIDCAllowInsecureTransport() Option { return func(s *IAMApiServer) { s.oidc.AllowInsecureTransport = true } } +// WithOIDCDiscoveryURLs redirects the discovery-document fetch of individual +// providers, taking "=" pairs. The discovery URL +// is fetched exactly as given, so it must include the +// "/.well-known/openid-configuration" path when the IdP serves it there. +// +// Only the fetch moves: the provider Url is still what a token's iss claim +// and the fetched document's own issuer field must match, and the JWKS is +// still fetched from the jwks_uri that document publishes. That is what lets +// an IdP hand out tokens naming a public issuer while this gateway reads its +// keys over a cluster-internal path — the endpoints being private is the +// point, so a configured discovery URL and the jwks_uri it publishes are +// exempt from the private-address check without WithOIDCAllowPrivateEndpoints. +func WithOIDCDiscoveryURLs(pairs []string) Option { + return func(s *IAMApiServer) { s.oidc.DiscoveryURLs = pairs } +} + func (s *IAMApiServer) ServeMultiPort(ports []string) error { if len(ports) == 0 { return fmt.Errorf("no ports specified") diff --git a/iamapi/server_test.go b/iamapi/server_test.go new file mode 100644 index 00000000..5f286c26 --- /dev/null +++ b/iamapi/server_test.go @@ -0,0 +1,149 @@ +// Copyright 2026 Versity Software +// This file is licensed under the Apache License, Version 2.0 +// (the "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package iamapi + +import ( + "testing" + + "github.com/versity/versitygw/iamapi/storage" +) + +// TestOIDCConfigParseDiscoveryURLs covers the "=" pairs the CLI and Helm chart pass through: a malformed pair fails at +// startup rather than at the first AssumeRoleWithWebIdentity, and a valid one +// is keyed by the provider's stored Url so the lookup at fetch time hits. +func TestOIDCConfigParseDiscoveryURLs(t *testing.T) { + const discoveryURL = "https://oidc.oidc-ns/.well-known/openid-configuration" + + tests := []struct { + name string + pairs []string + insecure bool + private bool + want map[string]string + }{ + { + name: "https provider keyed scheme-stripped", + pairs: []string{"https://oidc.example.com=" + discoveryURL}, + want: map[string]string{"oidc.example.com": discoveryURL}, + }, + { + name: "provider port survives into the key", + pairs: []string{"https://oidc.example.com:8443=" + discoveryURL}, + private: true, + want: map[string]string{"oidc.example.com:8443": discoveryURL}, + }, + { + name: "provider port without private endpoints", + pairs: []string{"https://oidc.example.com:8443=" + discoveryURL}, + }, + { + name: "provider path containing =", + pairs: []string{"https://oidc.example.com/tenant=abc=" + discoveryURL}, + want: map[string]string{"oidc.example.com/tenant=abc": discoveryURL}, + }, + { + name: "discovery url query containing =", + pairs: []string{"https://oidc.example.com=" + discoveryURL + "?tenant=abc"}, + want: map[string]string{"oidc.example.com": discoveryURL + "?tenant=abc"}, + }, + { + name: "http provider keeps its scheme", + pairs: []string{"http://127.0.0.1:8080=" + discoveryURL}, + insecure: true, + private: true, + want: map[string]string{"http://127.0.0.1:8080": discoveryURL}, + }, + { + name: "plaintext discovery url with insecure transport", + pairs: []string{"https://oidc.example.com=http://127.0.0.1:8080/.well-known/openid-configuration"}, + insecure: true, + want: map[string]string{"oidc.example.com": "http://127.0.0.1:8080/.well-known/openid-configuration"}, + }, + { + name: "plaintext discovery url without insecure transport", + pairs: []string{"https://oidc.example.com=http://127.0.0.1:8080/.well-known/openid-configuration"}, + }, + { + name: "no separator", + pairs: []string{"https://oidc.example.com"}, + }, + { + name: "empty discovery url", + pairs: []string{"https://oidc.example.com="}, + }, + { + name: "provider url without a scheme", + pairs: []string{"oidc.example.com=" + discoveryURL}, + }, + { + name: "discovery url without a scheme", + pairs: []string{"https://oidc.example.com=oidc.oidc-ns"}, + }, + { + name: "provider url without a host", + pairs: []string{"https://=" + discoveryURL}, + }, + { + name: "provider url with an invalid host", + pairs: []string{"https://bad host=" + discoveryURL}, + }, + { + name: "discovery url without a host", + pairs: []string{"https://oidc.example.com=https://"}, + }, + { + name: "discovery url with an invalid host", + pairs: []string{"https://oidc.example.com=https://bad host/.well-known/openid-configuration"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := OIDCConfig{DiscoveryURLs: tt.pairs, AllowInsecureTransport: tt.insecure, AllowPrivateEndpoints: tt.private} + err := cfg.parseDiscoveryURLs() + if tt.want == nil { + if err == nil { + t.Fatalf("parseDiscoveryURLs(%q) = nil, want an error", tt.pairs) + } + return + } + if err != nil { + t.Fatalf("parseDiscoveryURLs(%q): %v", tt.pairs, err) + } + got := cfg.endpointPolicy().DiscoveryURLs + if len(got) != len(tt.want) { + t.Fatalf("DiscoveryURLs = %v, want %v", got, tt.want) + } + for provider, want := range tt.want { + if got[provider] != want { + t.Errorf("DiscoveryURLs[%q] = %q, want %q", provider, got[provider], want) + } + } + }) + } +} + +// TestNewRejectsInvalidDiscoveryURLs confirms the parse runs at construction +// time, so a bad flag value never reaches a running server. +func TestNewRejectsInvalidDiscoveryURLs(t *testing.T) { + store, err := storage.New(storage.Config{Dir: t.TempDir()}) + if err != nil { + t.Fatalf("storage.New: %v", err) + } + if _, err := New(store, testRoot, WithQuiet(), WithOIDCDiscoveryURLs([]string{"not-a-pair"})); err == nil { + t.Fatal("New with a malformed discovery url = nil error, want a failure") + } +}