From de86bdd33c03c7761c6cd890aeed8124253d97f6 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:12:00 -0700 Subject: [PATCH 01/54] Match SSO rules against the clean path only pathMatches accepted a prefix match on either the raw or the cleaned request path. Since exceptions use the same function, a request like /public/../admin matched an exception for /public/ while the backend received /admin, bypassing SSO. Only match the cleaned path. Also add SECURITY-TODO.md to track the security review findings. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 223 ++++++++++++++++++++++++++++++++++++++ proxy/backend-sso.go | 4 +- proxy/backend-sso_test.go | 27 +++++ 3 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 SECURITY-TODO.md diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md new file mode 100644 index 00000000..16a03530 --- /dev/null +++ b/SECURITY-TODO.md @@ -0,0 +1,223 @@ +# Security review TODO + +Findings from the security review on 2026-10-06. Ordered by severity. +Each item is checked off when fixed (or explicitly declined). This file is +deleted when every item is resolved. + +## Critical / High + +- [x] **1. SSO rule bypass with dot-segments** (Critical) + *Fixed: rules match only the cleaned path. `sanitizePath: false` still + forwards the raw path unchanged (by decision).* + `proxy/backend-sso.go` `pathMatches`/`findSSORule` match `Paths` and + `Exceptions` against the raw path *or* the cleaned path, but the request is + forwarded with the cleaned path (`sanitizePath` default). With + `exceptions: ["/public/"]`, `GET /public/../admin` (or `/public/%2e%2e/admin`) + skips SSO and the backend receives `/admin`. With ordered rules, + `/public/../private` picks the permissive `/public/` rule (ACL bypass). + Fix: match only on the cleaned path; make sure the path that is checked is + the path that is forwarded, also when `sanitizePath: false`. + +- [ ] **2. Bearer tokens with local `iss` accepted when signed by any trusted issuer's key** (High) + `proxy/internal/tokenmanager/tokenmanager.go` `ValidateToken`/`getKey` fall + back to the global remote key set (union of every provider's + `trustedIssuers`) and only check `iss == local issuer`. `exp` is not + required. A token with no `scope` claim passes every scope check. Reached via + `cookiemanager.ValidateAuthorizationHeader`. + Fix: bind `kid` to its issuer; local keys only for the local issuer; require + `exp`; require `https` for `jwksUri` in `validateTrustedIssuers`. + +- [ ] **3. SSH CA issues certificates that never expire** (High) + `proxy/internal/sshca/sshca.go` `ServeCertificate`: `ttl` form value is not + checked for `<= 0` or overflow. `min(max, ttl)` doesn't cap negatives, and the + negative `ValidBefore` wraps to a huge `uint64`. OpenSSH sshd accepts it. + Fix: reject `tt <= 0`, clamp before multiplying, check `ValidBefore > now`. + +## Medium + +- [ ] **4. HTTP/3 backend request crashes the process** (Medium) + `proxy/internal/netw/quic.go`: `QUICConn.HandshakeComplete` and + `NextConnection` call themselves (stack overflow, unrecoverable). Triggered + by any request forwarded over h3 (`backendProto: h3`, or empty + `backendProto` with an h3 client). + Fix: delegate to `c.qc`; add a test with `BackendProto: "h3"`. + +- [ ] **5. Host header not checked against `ServerNames` before authentication** (Medium) + `proxy/backend-http.go`: `localHandler()` never checks Host; in + `reverseProxy()` the 421 check runs after `authenticateUser` and + `handleLocalEndpointsAndAuthorize`. The Bearer token audience + (`cookiemanager.audienceFromReq`), the ForceReAuth `hhash`, and the URL + token URL all come from the client-controlled Host. A malicious backend can + replay visitors' ID tokens (`TLSPROXYIDTOKEN`) against a sibling backend with + `Host: `. + Fix: reject Host not in `be.ServerNames` at the top of both handlers. + +- [ ] **6. OCSP checks bypassable with a forged stapled response** (Medium) + `proxy/internal/ocspcache/ocsp.go`: `ocsp.ParseResponseForCert` accepts an + embedded responder cert without the `OCSPSigning` EKU, so any cert from the + same CA (even the revoked one) can sign "Good". Stapled responses (including + client-cert staples) are cached and persisted until the attacker-chosen + `NextUpdate`. The fetched path uses `ocsp.ParseResponse` without checking + serial or freshness. Affects external CAs (`clientAuth.rootCAs`, + `forwardRootCAs`, system roots). + Fix: require responder == issuer or OCSPSigning EKU; use + `ParseResponseForCert` on fetch; check `ThisUpdate`/`NextUpdate`; don't + trust or cache client-supplied staples. + +- [ ] **7. PKI users can get serverAuth certs for any DNS name** (Medium) + `proxy/internal/pki/http.go` copies CSR `DNSNames` unchecked; `pki.go` adds + `ServerAuth` EKU. Any PKI user can get a cert for `*.example.com` or a + backend's name, which matters if the CA is in `forwardRootCAs`. + Fix: restrict DNS SANs (allowlist/suffix per user or admins only), reject + wildcards, consider CA name constraints. + +- [ ] **8. Port 80 (ACME) HTTP server has no timeouts or connection limit** (Medium) + `proxy/proxy.go` `Start()`: `http.Server{Handler: certManager.HTTPHandler}` + has no Read/ReadHeader/Write/Idle timeouts and isn't counted in `MaxOpen`. + Idle connections are held forever and exhaust file descriptors. + Fix: set timeouts and cap concurrent connections. + +- [ ] **9. Unauthenticated DoS of the local OIDC provider via device authorization** (Medium) + `proxy/internal/oidc/deviceauth.go`, `server.go` `vacuum()`: each POST to + `/device/authorization` with a (public) `client_id` adds map entries for 10 + minutes; every OIDC request runs `vacuum()` which scans all maps under + `s.mu` (quadratic). Local handlers aren't rate limited. + Fix: cap map sizes, expire entries off the request path, optionally require + the client secret. + +## Low–Medium + +- [ ] **10. Open redirect after login via Host header in URL token** (Low–Medium) + `proxy/backend-sso.go`: the URL token URL is built from `req.Host` before any + Host check; `serveLogin`/`serveLogout` don't validate the token URL's host. + Needs the victim's `__tlsproxySid` (same on every host, not HttpOnly, + forwarded to backends). URL tokens never expire. + Fix: #5, plus require the token URL host to be in `be.ServerNames`; add an + expiry to URL tokens. + +- [ ] **11. Open redirect with `//evil.com` paths** (Low–Medium) + `proxy/backend-sso.go` (ID token cookie reissue: + `http.Redirect(w, req, req.URL.String(), 302)`), and the same pattern in + `proxy/internal/passkeys/manager.go` (2 places). `GET //evil.com/x` redirects + to `//evil.com/x`. + Fix: build the redirect from the cleaned path + query. + +- [ ] **12. SAML login CSRF / session swapping** (Low–Medium) + `proxy/internal/saml/saml.go`: `InResponseTo` is checked against server state + only, not bound to the browser (OIDC uses the `TLSPROXYNONCE` cookie). An + attacker can submit their own signed response in the victim's browser. + Fix: short-lived host-only `SameSite=None; Secure; HttpOnly` cookie bound to + the request ID, checked in `HandleCallback`. + +- [ ] **13. Unbounded `events` map growth from attacker-chosen SNI** (Low–Medium) + `proxy/proxy.go` `handleConnection`: ECH accepted/rejected events include the + outer SNI and are recorded before the backend lookup. ECH needn't be + configured. + Fix: record after a successful backend lookup, or drop the SNI from the + event name. + +## Low + +- [ ] **14. Spoofable forwarding/identity headers** (Low) + `proxy/backend-sso.go`, `proxy/backend-http.go` `reverseProxyDirector`: + `X_tlsproxy_user_id` (underscore variant) and underscore variants of + `forwardHttpHeaders` keys aren't stripped (CGI-style backends map `_`/`-` to + the same variable). Client-supplied `X-Forwarded-Host`, `X-Forwarded-Proto` + and `Forwarded` reach the backend. + Fix: drop inbound headers that normalize to a proxy-owned header name; + strip/set `X-Forwarded-Host`, `X-Forwarded-Proto`, `Forwarded`. + +- [ ] **15. Local handler scopes skipped when no SSO rule matches** (Low) + `proxy/backend-sso.go` `enforceSSOPolicy` returns true when `rule == nil` + before checking `overrideScopes`. PKI/SSH/OIDC endpoints then accept tokens + without the `pki`/`ssh` scope when rules don't cover their paths. + Fix: still check `overrideScopes` when no rule matches. + +- [ ] **16. Unbounded login-state maps in OIDC RP, SAML and passkeys** (Low) + `oidc/client.go`, `saml/saml.go`, `passkeys/manager.go`: state (including an + attacker-sized `OriginalURL`) is only expired inside `HandleCallback`. + Fix: periodic expiry and a size cap. + +- [ ] **17. Passkey assertion doesn't check `clientData.origin`** (Low) + `proxy/internal/passkeys/manager.go` `processAssertion` (registration does + check it). A subdomain page can obtain an assertion for the RP ID. + Fix: require the expected origin. + +- [ ] **18. OIDC consent approval not bound to the user who started the request** (Low) + `proxy/internal/oidc/server.go`: `AuthorizeClient` and scope filtering run on + the GET; the token is minted for whoever POSTs the `request_id`. + Fix: store the user in `codeData`, require a match on POST, re-run + `AuthorizeClient` and scope filtering. + +- [ ] **19. Device flow phishing and clickjacking hardening** (Low) + `oidc/verify-template.html`, `authorize-template.html`, `deviceauth.go`: the + verify page doesn't show the client or scopes; no `X-Frame-Options` / + `frame-ancestors` on consent and device pages; `verification_uri_complete` + pre-fills the code. + Fix: show client and scopes, add framing protection, consider requiring the + code to be typed. + +- [ ] **20. Passkey account enumeration** (Low) + `proxy/internal/passkeys/manager.go`: `AssertionOptions` returns real + credential IDs for known emails and a fake 1-byte ID otherwise. + Fix: deterministic realistic fake IDs (HMAC of email), or an empty allow + list. + +- [ ] **21. `/.sso/` on a backend without SSO panics** (Low) + `proxy/backend-sso.go` `serveSSOStatus` dereferences `be.SSO` when nil. The + panic is recovered but logs a full stack trace per request. + Fix: check `be.SSO == nil`. + +- [ ] **22. Built-in PKI CRL staleness and data race** (Low) + `proxy/internal/pki/pki.go`: the cached CRL is reused up to 30m past + `NextUpdate`; same-second revocations can be missed by the + `ThisUpdate.Before(lastRevocation)` check. `maybeRotateDelegateCert` reads + `m.db` without `m.mu` (called from unauthenticated OCSP/CRL handlers). + Fix: regenerate earlier / on every revocation; take the lock. + +## Hardening / Info + +- [ ] **23.** CSRF check is skipped whenever any `Authorization` header is + present, even if it isn't a valid Bearer token and auth falls back to + cookies (`proxy/internal/csrf/csrf.go`). +- [ ] **24.** OIDC client secret compared with `==` instead of a constant-time + compare (`proxy/internal/oidc/server.go`). +- [ ] **25.** OIDC token endpoint doesn't compare `redirect_uri` with the one + used in the authorization request (RFC 6749 §4.1.3) and ignores PKCE. +- [ ] **26.** OIDC RP accepts a missing `email_verified` + (`oidc/client.go`); the local OP then asserts `email_verified: true`. +- [ ] **27.** WebAuthn signature counter is ignored. +- [ ] **28.** Revoking a cert doesn't close connections already open with it + (`reAuthorize` only re-checks ACLs, only on reconfigure). +- [ ] **29.** SSH CA accepts DSA keys (`sshca.go`). +- [ ] **30.** SSH CA holds `ca.mu` while reading the request body; a slow + upload blocks all issuance. +- [ ] **31.** `tlsclient -ocsp` uses `PeerCertificates[1]` as the issuer + instead of `VerifiedChains[0][1]` (`tlsclient/main.go`). +- [ ] **32.** `--passphrase` flag is visible in `/proc/*/cmdline` + (`main.go`); `TLSPROXY_PASSPHRASE` is the safer route. +- [ ] **33.** QUIC `GetConfigForClient` logs SNI and ALPN with `%s` (log + injection); use `%q` (`proxy/quic.go`). +- [ ] **34.** Connection-level DoS: no per-IP limit, 2-minute handshake + timeout, per-backend `connLimit.Wait` with no deadline before + authentication, non-h3 QUIC streams skip the first-request rate limit. +- [ ] **35.** `runtime.MemProfileRate = 1` in `metrics.go` `init()` profiles + every allocation process-wide. +- [ ] **36.** Data race: `handleConnection` reads `p.echKeys` without `p.mu`. +- [ ] **37.** `oidc/client.go` `HandleCallback` shadows `req` with the token + endpoint request, so the `th` session-chain claim is never carried over + (functional bug). +- [ ] **38.** Metrics page config dump doesn't redact `forwardHttpHeaders` + values or webhook URLs. +- [ ] **39.** `proxy.mjs` global `fetch` wrapper sends the sid as + `x-csrf-token` to cross-origin URLs. +- [ ] **40.** `emailMatches` treats an ACL entry equal to the email string as a + match, so a group name could match an IdP-asserted "email" of the same + value. +- [ ] **41.** OCSP delegate cert has `IsCA: true` (`pki.go`); not needed. +- [ ] **42.** PKI `?owner=all` lists every user's certs to any user with the + `pki` scope (may be intended). +- [ ] **43.** `certmanager` (test-only) generates and caches an RSA key per SNI + name without bound. +- [ ] **44.** OIDC RP nonce cookie is scoped to the parent domain, so an + attacker-controlled subdomain can toss it (login CSRF). diff --git a/proxy/backend-sso.go b/proxy/backend-sso.go index 738b520c..1f00c9dc 100644 --- a/proxy/backend-sso.go +++ b/proxy/backend-sso.go @@ -447,9 +447,11 @@ func pathMatches(prefixes []string, path string) bool { if len(prefixes) == 0 { return true } + // Only match the clean path. Matching the raw path too would let + // requests like /public/../admin match an exception for /public/. cleanPath := pathClean(path) for _, p := range prefixes { - if strings.HasPrefix(path, p) || strings.HasPrefix(cleanPath, p) { + if strings.HasPrefix(cleanPath, p) { return true } } diff --git a/proxy/backend-sso_test.go b/proxy/backend-sso_test.go index 0d2bad69..2d70450b 100644 --- a/proxy/backend-sso_test.go +++ b/proxy/backend-sso_test.go @@ -293,6 +293,33 @@ func TestEnforceSSOPolicy(t *testing.T) { t.Fatalf("response code = %d, want %d", got, want) } + // Dot-segments can't be used to match an exception or a more + // permissive rule. + proxy.cfg.Backends[0].SSO.Rules = []*SSORule{ + { + Paths: Strings{"/public/"}, + }, + { + Exceptions: Strings{"/favicon.ico"}, + ACL: &Strings{"alice@example.org"}, + }, + } + for _, p := range []string{ + "/public/../foo", + "/public/%2e%2e/foo", + "/favicon.ico/../foo", + "/favicon.ico/%2E%2E/foo", + } { + dotReq := httptest.NewRequest("GET", "https://example.com"+p, nil).WithContext(req.Context()) + w = httptest.NewRecorder() + if got, want := proxy.cfg.Backends[0].enforceSSOPolicy(w, dotReq, nil), false; got != want { + t.Errorf("encorceSSOPolicy(%q) = %v, want %v", p, got, want) + } + if got, want := w.Code, 403; got != want { + t.Errorf("response code for %q = %d, want %d", p, got, want) + } + } + // ForceReAuth fail proxy.cfg.Backends[0].SSO.Rules = []*SSORule{{ ForceReAuth: 5 * time.Minute, From 52261d8b5e9b6aff6cf27ba8390a31cb3264e8c4 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:14:42 -0700 Subject: [PATCH 02/54] Bind trusted issuer keys to their issuer ValidateToken accepted a key from any trusted issuer and only checked that the iss claim matched the local issuer. A trusted issuer, or anyone able to tamper with its JWKS, could therefore sign Bearer tokens that were accepted as locally issued, for any user. ValidateToken now only uses local keys. Tokens from trusted issuers are validated with the new ValidateRemoteToken, which requires the signing key to belong to the issuer in the iss claim. Bearer tokens and ID token cookies now require an exp claim, and trusted issuer JWKS URIs must use https. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- SECURITY-TODO.md | 5 ++- proxy/config.go | 6 ++- proxy/config_test.go | 16 ++++++++ proxy/internal/cookiemanager/cookiemanager.go | 26 ++++++------ .../cookiemanager/trusted_issuers_test.go | 41 +++++++++++++++++++ proxy/internal/tokenmanager/tokenmanager.go | 26 ++++++++++-- 7 files changed, 103 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 467b11cb..8adb20fe 100644 --- a/README.md +++ b/README.md @@ -343,7 +343,7 @@ In this example: The `TrustedIssuer` object defines an external identity provider whose tokens are accepted by `tlsproxy`. This is useful for distributed authentication where multiple proxies trust each other's user identity tokens. * `issuer`: (Required) String. The expected "iss" claim value (e.g., "https://auth.example.com/"). -* `jwksUri`: (Required) String. The URL to fetch the JSON Web Key Set (JWKS). +* `jwksUri`: (Required) String. The `https` URL to fetch the JSON Web Key Set (JWKS). **Example:** diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 16a03530..56210c68 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -18,7 +18,10 @@ deleted when every item is resolved. Fix: match only on the cleaned path; make sure the path that is checked is the path that is forwarded, also when `sanitizePath: false`. -- [ ] **2. Bearer tokens with local `iss` accepted when signed by any trusted issuer's key** (High) +- [x] **2. Bearer tokens with local `iss` accepted when signed by any trusted issuer's key** (High) + *Fixed: `ValidateToken` only uses local keys; trusted-issuer tokens go + through `ValidateRemoteToken`, which requires the key to belong to `iss`. + Bearer and ID token cookies require `exp`. `jwksUri` must be https.* `proxy/internal/tokenmanager/tokenmanager.go` `ValidateToken`/`getKey` fall back to the global remote key set (union of every provider's `trustedIssuers`) and only check `iss == local issuer`. `exp` is not diff --git a/proxy/config.go b/proxy/config.go index f2c97d75..8e947b3c 100644 --- a/proxy/config.go +++ b/proxy/config.go @@ -985,9 +985,13 @@ func validateTrustedIssuers(issuers []*TrustedIssuer) error { if ti.JWKSURI == "" { return fmt.Errorf("[%d].JWKSURI must be set", i) } - if _, err := url.Parse(ti.JWKSURI); err != nil { + u, err := url.Parse(ti.JWKSURI) + if err != nil { return fmt.Errorf("[%d].JWKSURI: %v", i, err) } + if u.Scheme != "https" { + return fmt.Errorf("[%d].JWKSURI must be an https URL", i) + } } return nil } diff --git a/proxy/config_test.go b/proxy/config_test.go index a101197b..5a1c1a66 100644 --- a/proxy/config_test.go +++ b/proxy/config_test.go @@ -461,3 +461,19 @@ func TestReadSplitConfig(t *testing.T) { } } } + +func TestValidateTrustedIssuers(t *testing.T) { + for _, tc := range []struct { + uri string + wantErr bool + }{ + {"https://example.com/.sso/jwks", false}, + {"http://example.com/.sso/jwks", true}, + {"example.com/.sso/jwks", true}, + } { + err := validateTrustedIssuers([]*TrustedIssuer{{Issuer: "https://example.com", JWKSURI: tc.uri}}) + if gotErr := err != nil; gotErr != tc.wantErr { + t.Errorf("validateTrustedIssuers(%q) = %v, wantErr %v", tc.uri, err, tc.wantErr) + } + } +} diff --git a/proxy/internal/cookiemanager/cookiemanager.go b/proxy/internal/cookiemanager/cookiemanager.go index db4661e4..8cb9d6cd 100644 --- a/proxy/internal/cookiemanager/cookiemanager.go +++ b/proxy/internal/cookiemanager/cookiemanager.go @@ -271,24 +271,26 @@ func (cm *CookieManager) validateAuthToken(req *http.Request, leeway time.Durati return nil, "", errors.New("unknown key") } - var expectedIssuer string + var tok *jwt.Token if issuer == "" { // Local - expectedIssuer = cm.issuer + tok, err = cm.tm.ValidateToken(cookie.Value, + jwt.WithIssuer(cm.issuer), + jwt.WithAudience(cm.issuer), + jwt.WithExpirationRequired(), + jwt.WithLeeway(leeway), + ) } else { // Trusted if !slices.Contains(cm.trustedIssuers, issuer) { return nil, "", fmt.Errorf("issuer %q is not trusted", issuer) } - expectedIssuer = issuer + tok, err = cm.tm.ValidateRemoteToken(cookie.Value, issuer, + jwt.WithAudience(issuer), + jwt.WithExpirationRequired(), + jwt.WithLeeway(leeway), + ) } - - tok, err := cm.tm.ValidateToken(cookie.Value, - jwt.WithIssuer(expectedIssuer), - jwt.WithAudience(expectedIssuer), - jwt.WithExpirationRequired(), - jwt.WithLeeway(leeway), - ) if err != nil { return nil, "", err } @@ -347,7 +349,7 @@ func (cm *CookieManager) ValidateIDTokenCookie(req *http.Request, authToken *jwt if err != nil { return err } - tok, err := cm.tm.ValidateToken(cookie.Value, jwt.WithIssuer(cm.issuer), jwt.WithAudience(audience)) + tok, err := cm.tm.ValidateToken(cookie.Value, jwt.WithIssuer(cm.issuer), jwt.WithAudience(audience), jwt.WithExpirationRequired()) if err != nil { return err } @@ -363,7 +365,7 @@ func (cm *CookieManager) ValidateAuthorizationHeader(req *http.Request) (*jwt.To if len(h) < 7 || strings.ToUpper(h[:7]) != "BEARER " { return nil, errors.New("invalid authorization header") } - tok, err := cm.tm.ValidateToken(h[7:], jwt.WithIssuer(cm.issuer), jwt.WithAudience(audienceFromReq(req))) + tok, err := cm.tm.ValidateToken(h[7:], jwt.WithIssuer(cm.issuer), jwt.WithAudience(audienceFromReq(req)), jwt.WithExpirationRequired()) if err != nil { return nil, err } diff --git a/proxy/internal/cookiemanager/trusted_issuers_test.go b/proxy/internal/cookiemanager/trusted_issuers_test.go index a1d7740c..85508af1 100644 --- a/proxy/internal/cookiemanager/trusted_issuers_test.go +++ b/proxy/internal/cookiemanager/trusted_issuers_test.go @@ -154,4 +154,45 @@ func TestTrustedIssuers(t *testing.T) { if err == nil { t.Error("ValidateAuthTokenCookie(wrong proxyauth): expected error, got nil") } + + // Case E: Trusted Issuer's key, local issuer claim, as bearer token + tokenLocalIss := jwt.NewWithClaims(jwt.SigningMethodES256, jwt.MapClaims{ + "iss": "https://idp.example.com", + "aud": "https://example.com/", + "sub": "user123", + "email": "admin@example.com", + "exp": time.Now().Add(time.Hour).Unix(), + }) + tokenLocalIss.Header["kid"] = "test-kid-1" + signedTokenLocalIss, _ := tokenLocalIss.SignedString(privKey) + + reqBearer, _ := http.NewRequest("GET", "https://example.com", nil) + reqBearer.Header.Set("Authorization", "Bearer "+signedTokenLocalIss) + if _, err := cm.ValidateAuthorizationHeader(reqBearer); err == nil { + t.Error("ValidateAuthorizationHeader(trusted key, local iss): expected error, got nil") + } + + // Case F: Local key, bearer token with and without exp + localClaims := jwt.MapClaims{ + "iss": "https://idp.example.com", + "aud": "https://example.com/", + "sub": "user123", + } + signedNoExp, err := tm.CreateToken(localClaims, "") + if err != nil { + t.Fatalf("CreateToken: %v", err) + } + reqBearer.Header.Set("Authorization", "Bearer "+signedNoExp) + if _, err := cm.ValidateAuthorizationHeader(reqBearer); err == nil { + t.Error("ValidateAuthorizationHeader(no exp): expected error, got nil") + } + localClaims["exp"] = time.Now().Add(time.Hour).Unix() + signedWithExp, err := tm.CreateToken(localClaims, "") + if err != nil { + t.Fatalf("CreateToken: %v", err) + } + reqBearer.Header.Set("Authorization", "Bearer "+signedWithExp) + if _, err := cm.ValidateAuthorizationHeader(reqBearer); err != nil { + t.Errorf("ValidateAuthorizationHeader(local key, exp): %v", err) + } } diff --git a/proxy/internal/tokenmanager/tokenmanager.go b/proxy/internal/tokenmanager/tokenmanager.go index dd9232b1..ee578077 100644 --- a/proxy/internal/tokenmanager/tokenmanager.go +++ b/proxy/internal/tokenmanager/tokenmanager.go @@ -453,12 +453,22 @@ func (tm *TokenManager) getKey(tok *jwt.Token) (interface{}, error) { return tk.privKey.Public(), nil } } - if pk, err := tm.remote.GetKey(kid); err == nil { - return pk, nil - } return nil, errors.New("not found") } +func (tm *TokenManager) getRemoteKey(issuer string) jwt.Keyfunc { + return func(tok *jwt.Token) (interface{}, error) { + kid, ok := tok.Header["kid"].(string) + if !ok { + return nil, errors.New("kid header missing") + } + if iss, ok := tm.remote.IssuerForKey(kid); !ok || iss != issuer { + return nil, errors.New("not found") + } + return tm.remote.GetKey(kid) + } +} + func (tm *TokenManager) IssuerForKey(kid string) (string, bool) { tm.mu.Lock() defer tm.mu.Unlock() @@ -471,12 +481,20 @@ func (tm *TokenManager) IssuerForKey(kid string) (string, bool) { return tm.remote.IssuerForKey(kid) } -// ValidateToken validates a JSON Web Token (JWT). +// ValidateToken validates a JSON Web Token (JWT) signed by one of the local +// keys. func (tm *TokenManager) ValidateToken(t string, opts ...jwt.ParserOption) (*jwt.Token, error) { opts = append(opts, jwt.WithValidMethods([]string{"ES256", "RS256", "EdDSA"})) return jwt.ParseWithClaims(t, jwt.MapClaims{}, tm.getKey, opts...) } +// ValidateRemoteToken validates a JSON Web Token (JWT) signed by one of the +// keys of a trusted issuer. The token's iss claim must be that issuer. +func (tm *TokenManager) ValidateRemoteToken(t, issuer string, opts ...jwt.ParserOption) (*jwt.Token, error) { + opts = append(opts, jwt.WithIssuer(issuer), jwt.WithValidMethods([]string{"ES256", "RS256", "EdDSA"})) + return jwt.ParseWithClaims(t, jwt.MapClaims{}, tm.getRemoteKey(issuer), opts...) +} + // ServeJWKS returns the current public keys as a JSON Web Key Set (JWKS). func (tm *TokenManager) ServeJWKS(w http.ResponseWriter, req *http.Request) { tm.mu.Lock() From 2bd4b3212a0daf28357dea9d461eb4fb847ae458 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:15:17 -0700 Subject: [PATCH 03/54] Reject invalid SSH certificate TTLs The ttl form value was not checked for negative values or overflow. A negative ttl passed the min() cap and produced a negative ValidBefore that wrapped to a huge uint64, i.e. a certificate that never expires, which OpenSSH accepts. A very large ttl overflowed with the same result. Reject ttl <= 0 and cap ttl at the maximum lifetime before converting it to a time.Duration. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 4 ++- proxy/internal/sshca/sshca.go | 21 ++++++++-------- proxy/internal/sshca/sshca_test.go | 39 ++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 12 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 56210c68..d54a5f73 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -30,7 +30,9 @@ deleted when every item is resolved. Fix: bind `kid` to its issuer; local keys only for the local issuer; require `exp`; require `https` for `jwksUri` in `validateTrustedIssuers`. -- [ ] **3. SSH CA issues certificates that never expire** (High) +- [x] **3. SSH CA issues certificates that never expire** (High) + *Fixed: `ttl <= 0` or non-numeric returns 400; `ttl` is capped at the + maximum lifetime before converting to a duration.* `proxy/internal/sshca/sshca.go` `ServeCertificate`: `ttl` form value is not checked for `<= 0` or overflow. `min(max, ttl)` doesn't cap negatives, and the negative `ValidBefore` wraps to a huge `uint64`. OpenSSH sshd accepts it. diff --git a/proxy/internal/sshca/sshca.go b/proxy/internal/sshca/sshca.go index a482eb41..1dd71259 100644 --- a/proxy/internal/sshca/sshca.go +++ b/proxy/internal/sshca/sshca.go @@ -307,7 +307,11 @@ func (ca *SSHCA) ServeCertificate(w http.ResponseWriter, req *http.Request) { return } - ttl := defaultCertsLifetime + maxTTL := defaultMaxCertsLifetime + if ca.opts.MaximumCertificateLifetime != 0 { + maxTTL = ca.opts.MaximumCertificateLifetime + } + ttl := min(defaultCertsLifetime, maxTTL) var key []byte switch ct := req.Header.Get("content-type"); ct { @@ -325,13 +329,14 @@ func (ca *SSHCA) ServeCertificate(w http.ResponseWriter, req *http.Request) { req.ParseForm() key = []byte(req.PostForm.Get("key")) if t := req.PostForm.Get("ttl"); t != "" { - tt, err := strconv.Atoi(t) - if err != nil { - ca.opts.Logger.Errorf("ERR ttl: %v", err) + tt, err := strconv.ParseInt(t, 10, 64) + if err != nil || tt <= 0 { + ca.opts.Logger.Errorf("ERR ttl: %q %v", t, err) http.Error(w, "invalid request", http.StatusBadRequest) return } - ttl = time.Second * time.Duration(tt) + // Cap before multiplying to avoid overflow. + ttl = time.Second * time.Duration(min(tt, int64(maxTTL/time.Second))) } default: @@ -340,12 +345,6 @@ func (ca *SSHCA) ServeCertificate(w http.ResponseWriter, req *http.Request) { return } - if ca.opts.MaximumCertificateLifetime != 0 { - ttl = min(ca.opts.MaximumCertificateLifetime, ttl) - } else { - ttl = min(defaultMaxCertsLifetime, ttl) - } - pub, _, _, _, err := ssh.ParseAuthorizedKey(key) if err != nil { ca.opts.Logger.Errorf("ERR key: %v", err) diff --git a/proxy/internal/sshca/sshca_test.go b/proxy/internal/sshca/sshca_test.go index 0113abd4..3685906a 100644 --- a/proxy/internal/sshca/sshca_test.go +++ b/proxy/internal/sshca/sshca_test.go @@ -209,6 +209,45 @@ func TestCertificate(t *testing.T) { t.Fatalf("cert validity = %d, want %d", got, want) } }) + t.Run("invalid ttl", func(t *testing.T) { + for _, ttl := range []string{"0", "-1", "-2000000000", "foo"} { + values := url.Values{} + values.Set("key", string(ssh.MarshalAuthorizedKey(sshPub))) + values.Set("ttl", ttl) + resp, err := http.PostForm(server.URL+"/cert", values) + if err != nil { + t.Fatalf("Post(/cert): %v", err) + } + resp.Body.Close() + if got, want := resp.StatusCode, http.StatusBadRequest; got != want { + t.Errorf("ttl=%s: status = %d, want %d", ttl, got, want) + } + } + }) + t.Run("large ttl", func(t *testing.T) { + for _, ttl := range []string{"864000", "9223372037", "9223372036854775807"} { + values := url.Values{} + values.Set("key", string(ssh.MarshalAuthorizedKey(sshPub))) + values.Set("ttl", ttl) + resp, err := http.PostForm(server.URL+"/cert", values) + if err != nil { + t.Fatalf("Post(/cert): %v", err) + } + certBytes, err := io.ReadAll(resp.Body) + resp.Body.Close() + if err != nil { + t.Fatalf("cert body: %v", err) + } + c, _, _, _, err := ssh.ParseAuthorizedKey(certBytes) + if err != nil { + t.Fatalf("ttl=%s: ssh.ParseAuthorizedKey: %v", ttl, err) + } + cert := c.(*ssh.Certificate) + if got, want := cert.ValidBefore-cert.ValidAfter, uint64(300+7*86400); got != want { + t.Errorf("ttl=%s: cert validity = %d, want %d", ttl, got, want) + } + } + }) }) } } From e79e68d73717399ff4738c7fec302105d5ddc6ca Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:17:03 -0700 Subject: [PATCH 04/54] Fix infinite recursion in QUICConn QUICConn.HandshakeComplete and NextConnection called themselves instead of the underlying connection. The http3 client calls HandshakeComplete on every request, so any request forwarded to a backend over HTTP/3 crashed the whole process with a stack overflow. Add a test with an HTTP/3 backend. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 3 +- proxy/internal/netw/quic.go | 4 +-- proxy/quic_test.go | 65 +++++++++++++++++++++++++++++++++++++ 3 files changed, 69 insertions(+), 3 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index d54a5f73..b646a64b 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -40,7 +40,8 @@ deleted when every item is resolved. ## Medium -- [ ] **4. HTTP/3 backend request crashes the process** (Medium) +- [x] **4. HTTP/3 backend request crashes the process** (Medium) + *Fixed: delegate to `c.qc`; added `TestH3Backend`.* `proxy/internal/netw/quic.go`: `QUICConn.HandshakeComplete` and `NextConnection` call themselves (stack overflow, unrecoverable). Triggered by any request forwarded over h3 (`backendProto: h3`, or empty diff --git a/proxy/internal/netw/quic.go b/proxy/internal/netw/quic.go index 8666415d..0b4e5e5a 100644 --- a/proxy/internal/netw/quic.go +++ b/proxy/internal/netw/quic.go @@ -231,11 +231,11 @@ func (c *QUICConn) ByteRateReceived() float64 { } func (c *QUICConn) HandshakeComplete() <-chan struct{} { - return c.HandshakeComplete() + return c.qc.HandshakeComplete() } func (c *QUICConn) NextConnection(ctx context.Context) (quicapi.Conn, error) { - return c.NextConnection(ctx) + return c.qc.NextConnection(ctx) } func (c *QUICConn) TLSConnectionState() tls.ConnectionState { diff --git a/proxy/quic_test.go b/proxy/quic_test.go index 800d90b0..326081bf 100644 --- a/proxy/quic_test.go +++ b/proxy/quic_test.go @@ -832,3 +832,68 @@ func (n *quicNode) run(conn quicapi.Conn) { n.t.Logf("[%s] received: %s", n.name, e) } } + +func TestH3Backend(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + extCA, err := certmanager.New("root-ca.example.com", t.Logf) + if err != nil { + t.Fatalf("certmanager.New: %v", err) + } + intCA, err := certmanager.New("internal-ca.example.com", t.Logf) + if err != nil { + t.Fatalf("certmanager.New: %v", err) + } + + tc := intCA.TLSConfig() + tc.NextProtos = []string{"h3"} + ln, err := quicapi.ListenAddr("localhost:0", tc, &quic.Config{}) + if err != nil { + t.Fatalf("ListenAddr: %v", err) + } + h3srv := &http3.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + fmt.Fprintf(w, "[h3 backend] %s %s\n", r.Proto, r.RequestURI) + }), + } + go h3srv.ServeListener(ln) + defer h3srv.Close() + + h3Value := "h3" + proxy := newTestProxy( + &Config{ + HTTPAddr: newPtr("localhost:0"), + TLSAddr: newPtr("localhost:0"), + CacheDir: newPtr(t.TempDir()), + MaxOpen: newPtr(100), + Backends: []*Backend{ + { + ServerNames: Strings{ + "https.example.com", + }, + Addresses: Strings{ + ln.Addr().String(), + }, + Mode: "HTTPS", + BackendProto: &h3Value, + ForwardRootCAs: Strings{intCA.RootCAPEM()}, + ForwardServerName: "h3-internal.example.com", + }, + }, + }, + extCA, + ) + if err := proxy.Start(ctx); err != nil { + t.Fatalf("proxy.Start: %v", err) + } + defer proxy.Stop() + + got, _, err := httpOp("https.example.com", proxy.listener.Addr().String(), "/foo", "GET", nil, extCA, nil) + if err != nil { + t.Fatalf("httpOp: %v", err) + } + if want := "HTTP/2.0 200 OK\n[h3 backend] HTTP/3.0 /foo\n"; got != want { + t.Errorf("Got %q, want %q", got, want) + } +} From c670893042b8caacdf463a0c0b0f7b10c7b017f3 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:19:41 -0700 Subject: [PATCH 05/54] Check the Host header before authenticating requests The reverse proxy only verified that the Host header matched one of the backend's server names after authentication and local endpoints were handled, and LOCAL/CONSOLE backends never checked it. The Bearer token audience, the ForceReAuth host hash, and URL token URLs are all derived from the Host header, so a client could e.g. replay an ID token issued for one backend against another backend by sending that backend's name in the Host header. Check the Host header first in both handlers and return 421 Misdirected Request when it doesn't match. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 4 ++- proxy/backend-http.go | 44 +++++++++++++++----------- proxy/backend-http_test.go | 64 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 93 insertions(+), 19 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index b646a64b..5912c9db 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -48,7 +48,9 @@ deleted when every item is resolved. `backendProto` with an h3 client). Fix: delegate to `c.qc`; add a test with `BackendProto: "h3"`. -- [ ] **5. Host header not checked against `ServerNames` before authentication** (Medium) +- [x] **5. Host header not checked against `ServerNames` before authentication** (Medium) + *Fixed: `checkRequestHost` returns 421 before authentication in both + `localHandler` and `reverseProxy`.* `proxy/backend-http.go`: `localHandler()` never checks Host; in `reverseProxy()` the 421 check runs after `authenticateUser` and `handleLocalEndpointsAndAuthorize`. The Bearer token audience diff --git a/proxy/backend-http.go b/proxy/backend-http.go index 05fd3871..c24ac821 100644 --- a/proxy/backend-http.go +++ b/proxy/backend-http.go @@ -86,6 +86,9 @@ func (be *Backend) localHandler() http.Handler { be.logPanic(req, r) } }() + if !be.checkRequestHost(w, req) { + return + } if !be.authenticateUser(w, &req) { return } @@ -96,6 +99,24 @@ func (be *Backend) localHandler() http.Handler { }) } +// checkRequestHost verifies that the HTTP request is directed at a server name +// that's configured for this backend. This prevents clients from using one +// server name in the TLS handshake, and then a different server name in the +// request. It must be called before anything uses req.Host. +func (be *Backend) checkRequestHost(w http.ResponseWriter, req *http.Request) bool { + if req.Host == "" { + req.Host = connServerName(req.Context().Value(connCtxKey).(anyConn)) + } + if !slices.Contains(be.ServerNames, hostFromReq(req)) { + if req.Body != nil { + req.Body.Close() + } + http.Error(w, "Misdirected Request", http.StatusMisdirectedRequest) + return false + } + return true +} + func (be *Backend) redirectPermanently(w http.ResponseWriter, req *http.Request, path string) { code := http.StatusMovedPermanently if req.Method != http.MethodGet && req.Method != http.MethodHead { @@ -208,6 +229,9 @@ func (be *Backend) reverseProxy() http.Handler { be.logPanic(req, r) } }() + if !be.checkRequestHost(w, req) { + return + } if !be.authenticateUser(w, &req) { return } @@ -215,31 +239,15 @@ func (be *Backend) reverseProxy() http.Handler { return } - // Verify that the HTTP request is directed at a server name - // that's configured for this backend. This prevents clients - // from using one server name in the TLS handshake, and then - // a different server name in the request. ctx := req.Context() serverName := connServerName(ctx.Value(connCtxKey).(anyConn)) - host := req.Host - if host == "" { - host = serverName - } - req.URL.Host = host - req.Header.Set(hostHeader, host) + req.URL.Host = req.Host + req.Header.Set(hostHeader, req.Host) req.URL.Scheme = "https" if be.Mode == ModeHTTP { req.URL.Scheme = "http" } - - if !slices.Contains(be.ServerNames, req.URL.Hostname()) { - if req.Body != nil { - req.Body.Close() - } - http.Error(w, "Misdirected Request", http.StatusMisdirectedRequest) - return - } ctx = context.WithValue(ctx, ctxURLKey, req.URL.String()) // Apply the forward rate limit. The first request was already diff --git a/proxy/backend-http_test.go b/proxy/backend-http_test.go index 4992dcb5..16a171b9 100644 --- a/proxy/backend-http_test.go +++ b/proxy/backend-http_test.go @@ -27,10 +27,12 @@ import ( "context" "net" "net/http" + "strings" "testing" jwt "github.com/golang-jwt/jwt/v5" + "github.com/c2FmZQ/tlsproxy/certmanager" "github.com/c2FmZQ/tlsproxy/proxy/internal/fromctx" ) @@ -122,3 +124,65 @@ func (mockConn) ByteRateSent() float64 { func (mockConn) ByteRateReceived() float64 { return 0 } + +func TestMisdirectedRequest(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + extCA, err := certmanager.New("root-ca.example.com", t.Logf) + if err != nil { + t.Fatalf("certmanager.New: %v", err) + } + be := newHTTPServer(t, ctx, "https", nil) + + proxy := newTestProxy( + &Config{ + HTTPAddr: newPtr("localhost:0"), + TLSAddr: newPtr("localhost:0"), + CacheDir: newPtr(t.TempDir()), + MaxOpen: newPtr(100), + Backends: []*Backend{ + { + ServerNames: Strings{"local.example.com"}, + Mode: "LOCAL", + DocumentRoot: ".", + }, + { + ServerNames: Strings{"http.example.com"}, + Mode: "HTTP", + Addresses: Strings{be.String()}, + }, + { + ServerNames: Strings{"other.example.com"}, + Mode: "LOCAL", + DocumentRoot: ".", + }, + }, + }, + extCA, + ) + if err := proxy.Start(ctx); err != nil { + t.Fatalf("proxy.Start: %v", err) + } + defer proxy.Stop() + + for _, tc := range []struct { + sni, host, want string + }{ + {"local.example.com", "local.example.com", "HTTP/1.1 200 OK"}, + {"local.example.com", "local.example.com:443", "HTTP/1.1 200 OK"}, + {"local.example.com", "other.example.com", "HTTP/1.1 421 Misdirected Request"}, + {"http.example.com", "http.example.com", "HTTP/1.1 200 OK"}, + {"http.example.com", "other.example.com", "HTTP/1.1 421 Misdirected Request"}, + } { + msg := "GET /proxy.go HTTP/1.1\r\nHost: " + tc.host + "\r\nConnection: close\r\n\r\n" + got, _, err := tlsGet(tc.sni, proxy.listener.Addr().String(), msg, extCA, nil, []string{"http/1.1"}) + if err != nil { + t.Fatalf("tlsGet(%q, %q): %v", tc.sni, tc.host, err) + } + if !strings.HasPrefix(got, tc.want) { + first, _, _ := strings.Cut(got, "\r\n") + t.Errorf("SNI %q Host %q: got %q, want %q", tc.sni, tc.host, first, tc.want) + } + } +} From dabecfe030c524fca646dbb73dc7e4b77c30c3de Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:22:08 -0700 Subject: [PATCH 06/54] Verify OCSP responder authorization and freshness ocsp.ParseResponseForCert accepts a response signed by any embedded certificate that was issued by the CA, without checking that it has the OCSPSigning extended key usage. Any certificate from the same CA, including the revoked certificate itself, could sign a "Good" stapled response, which was then cached. Fetched responses were parsed with ocsp.ParseResponse, which doesn't check the serial number, and neither path checked ThisUpdate/NextUpdate. Validate both stapled and fetched responses with a common function that checks the serial number, the responder's authorization, and the response's validity period. A stapled response only replaces a cached response if it is newer. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 6 +- proxy/internal/ocspcache/ocsp.go | 41 ++++- proxy/internal/ocspcache/ocsp_test.go | 241 ++++++++++++++++++++++++++ 3 files changed, 282 insertions(+), 6 deletions(-) create mode 100644 proxy/internal/ocspcache/ocsp_test.go diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 5912c9db..f79eb0d0 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -60,7 +60,11 @@ deleted when every item is resolved. `Host: `. Fix: reject Host not in `be.ServerNames` at the top of both handlers. -- [ ] **6. OCSP checks bypassable with a forged stapled response** (Medium) +- [x] **6. OCSP checks bypassable with a forged stapled response** (Medium) + *Fixed: `parseResponse` requires the responder to be the issuer or have + the OCSPSigning EKU, binds to the cert serial, and checks + `ThisUpdate`/`NextUpdate` for both stapled and fetched responses. A + staple only replaces a cached response if it is newer.* `proxy/internal/ocspcache/ocsp.go`: `ocsp.ParseResponseForCert` accepts an embedded responder cert without the `OCSPSigning` EKU, so any cert from the same CA (even the revoked one) can sign "Good". Stapled responses (including diff --git a/proxy/internal/ocspcache/ocsp.go b/proxy/internal/ocspcache/ocsp.go index b349136a..dc9a6c0c 100644 --- a/proxy/internal/ocspcache/ocsp.go +++ b/proxy/internal/ocspcache/ocsp.go @@ -30,8 +30,10 @@ import ( "crypto/x509" "encoding/hex" "errors" + "fmt" "io" "net/http" + "slices" "time" "github.com/c2FmZQ/storage" @@ -43,6 +45,7 @@ import ( const ( ocspCacheSize = 200 ocspFile = "ocsp-cache" + ocspMaxSkew = 5 * time.Minute ) var ( @@ -134,13 +137,17 @@ func (c *OCSPCache) flush() error { func (c *OCSPCache) VerifyChains(ctx context.Context, chains [][]*x509.Certificate, stapled []byte) error { if stapled != nil && len(chains) > 0 && len(chains[0]) > 1 { cert, issuer := chains[0][0], chains[0][1] - if resp, err := ocsp.ParseResponseForCert(stapled, cert, issuer); err == nil && time.Now().Before(resp.NextUpdate) && resp.Status == ocsp.Good { + if resp, err := parseResponse(stapled, cert, issuer); err == nil && !resp.NextUpdate.IsZero() && resp.Status == ocsp.Good { hash := certHash(cert.Raw) - if resp, ok := c.cache.Get(hash); ok && resp.Status == ocsp.Revoked { + cached, ok := c.cache.Get(hash) + if ok && cached.Status == ocsp.Revoked { // Someone is playing tricks on us. return errOCSPRevoked } - c.cache.Add(hash, resp) + // Only replace the cached response with a newer one. + if !ok || resp.ThisUpdate.After(cached.ThisUpdate) { + c.cache.Add(hash, resp) + } } } var lastError error @@ -248,10 +255,34 @@ func (c *OCSPCache) fetchOneOCSP(ctx context.Context, cert, issuer *x509.Certifi c.logger.Errorf("ERR body: %v", err) return nil, errOCSPProtocol } - ocspResp, err := ocsp.ParseResponse(body, issuer) + ocspResp, err := parseResponse(body, cert, issuer) if err != nil { - c.logger.Errorf("ERR ocsp.ParseResponse for %s from %s: %v", cert.Subject, server, err) + c.logger.Errorf("ERR OCSP response for %s from %s: %v", cert.Subject, server, err) return nil, errOCSPProtocol } return ocspResp, nil } + +// parseResponse parses an OCSP response for cert, and verifies that it is +// signed by issuer, or by a delegated responder that is authorized to sign +// OCSP responses on behalf of issuer, and that it is current. +func parseResponse(raw []byte, cert, issuer *x509.Certificate) (*ocsp.Response, error) { + resp, err := ocsp.ParseResponseForCert(raw, cert, issuer) + if err != nil { + return nil, err + } + // ParseResponseForCert verifies that the responder certificate is + // signed by issuer, but not that it is authorized to sign OCSP + // responses (RFC 6960 section 4.2.2.2). + if rc := resp.Certificate; rc != nil && !rc.Equal(issuer) && !slices.Contains(rc.ExtKeyUsage, x509.ExtKeyUsageOCSPSigning) { + return nil, errors.New("responder certificate is not authorized to sign OCSP responses") + } + now := time.Now() + if resp.ThisUpdate.After(now.Add(ocspMaxSkew)) { + return nil, fmt.Errorf("response is not yet valid: %v", resp.ThisUpdate) + } + if !resp.NextUpdate.IsZero() && !now.Before(resp.NextUpdate) { + return nil, fmt.Errorf("response is expired: %v", resp.NextUpdate) + } + return resp, nil +} diff --git a/proxy/internal/ocspcache/ocsp_test.go b/proxy/internal/ocspcache/ocsp_test.go new file mode 100644 index 00000000..cf999664 --- /dev/null +++ b/proxy/internal/ocspcache/ocsp_test.go @@ -0,0 +1,241 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package ocspcache + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "io" + "math/big" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/c2FmZQ/storage" + storagecrypto "github.com/c2FmZQ/storage/crypto" + "golang.org/x/crypto/ocsp" +) + +type testLogger struct { + t *testing.T +} + +func (l testLogger) Errorf(f string, args ...any) { l.t.Logf(f, args...) } +func (l testLogger) Fatalf(f string, args ...any) { l.t.Fatalf(f, args...) } + +type testCert struct { + cert *x509.Certificate + key crypto.Signer +} + +func newTestCert(t *testing.T, tmpl *x509.Certificate, parent *testCert) *testCert { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("ecdsa.GenerateKey: %v", err) + } + sn, err := rand.Int(rand.Reader, big.NewInt(1<<62)) + if err != nil { + t.Fatalf("rand.Int: %v", err) + } + tmpl.SerialNumber = sn + tmpl.NotBefore = time.Now().Add(-time.Hour) + tmpl.NotAfter = time.Now().Add(time.Hour) + parentCert, parentKey := tmpl, crypto.Signer(key) + if parent != nil { + parentCert, parentKey = parent.cert, parent.key + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, parentCert, key.Public(), parentKey) + if err != nil { + t.Fatalf("x509.CreateCertificate: %v", err) + } + cert, err := x509.ParseCertificate(der) + if err != nil { + t.Fatalf("x509.ParseCertificate: %v", err) + } + return &testCert{cert: cert, key: key} +} + +func newOCSPResponse(t *testing.T, cert, issuer, responder *testCert, status int, thisUpdate, nextUpdate time.Time) []byte { + t.Helper() + tmpl := ocsp.Response{ + Status: status, + SerialNumber: cert.cert.SerialNumber, + ThisUpdate: thisUpdate, + NextUpdate: nextUpdate, + } + if status == ocsp.Revoked { + tmpl.RevokedAt = thisUpdate + } + if responder != issuer { + tmpl.Certificate = responder.cert + } + raw, err := ocsp.CreateResponse(issuer.cert, responder.cert, tmpl, responder.key) + if err != nil { + t.Fatalf("ocsp.CreateResponse: %v", err) + } + return raw +} + +func TestVerifyChains(t *testing.T) { + now := time.Now() + + var serverResponse []byte + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + io.Copy(io.Discard, req.Body) + w.Header().Set("content-type", "application/ocsp-response") + w.Write(serverResponse) + })) + defer server.Close() + + ca := newTestCert(t, &x509.Certificate{ + Subject: pkix.Name{CommonName: "Test CA"}, + IsCA: true, + BasicConstraintsValid: true, + KeyUsage: x509.KeyUsageCertSign, + }, nil) + newLeaf := func() *testCert { + return newTestCert(t, &x509.Certificate{ + Subject: pkix.Name{CommonName: "leaf"}, + OCSPServer: []string{server.URL}, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + }, ca) + } + other := newLeaf() + responder := newTestCert(t, &x509.Certificate{ + Subject: pkix.Name{CommonName: "OCSP responder"}, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageOCSPSigning}, + }, ca) + + for _, tc := range []struct { + name string + // The stapled response, if any. + staple func(leaf *testCert) []byte + // The response returned by the OCSP server. + fetched func(leaf *testCert) []byte + wantErr error + }{ + { + name: "Fetched good", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Good, now, now.Add(time.Hour)) + }, + }, + { + name: "Fetched revoked", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Revoked, now, now.Add(time.Hour)) + }, + wantErr: errOCSPRevoked, + }, + { + name: "Fetched good from delegated responder", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, responder, ocsp.Good, now, now.Add(time.Hour)) + }, + }, + { + name: "Fetched good signed by leaf", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, leaf, ocsp.Good, now, now.Add(time.Hour)) + }, + wantErr: errOCSPProtocol, + }, + { + name: "Fetched good for other cert", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, other, ca, ca, ocsp.Good, now, now.Add(time.Hour)) + }, + wantErr: errOCSPProtocol, + }, + { + name: "Fetched expired good", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Good, now.Add(-2*time.Hour), now.Add(-time.Hour)) + }, + wantErr: errOCSPProtocol, + }, + { + name: "Fetched future good", + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Good, now.Add(time.Hour), now.Add(2*time.Hour)) + }, + wantErr: errOCSPProtocol, + }, + { + name: "Stapled good, fetched revoked", + staple: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Good, now, now.Add(time.Hour)) + }, + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Revoked, now, now.Add(time.Hour)) + }, + }, + { + name: "Stapled good signed by leaf, fetched revoked", + staple: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, leaf, ocsp.Good, now, now.Add(365*24*time.Hour)) + }, + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Revoked, now, now.Add(time.Hour)) + }, + wantErr: errOCSPRevoked, + }, + { + name: "Stapled good for other cert, fetched revoked", + staple: func(leaf *testCert) []byte { + return newOCSPResponse(t, other, ca, ca, ocsp.Good, now, now.Add(time.Hour)) + }, + fetched: func(leaf *testCert) []byte { + return newOCSPResponse(t, leaf, ca, ca, ocsp.Revoked, now, now.Add(time.Hour)) + }, + wantErr: errOCSPRevoked, + }, + } { + t.Run(tc.name, func(t *testing.T) { + mk, err := storagecrypto.CreateAESMasterKeyForTest() + if err != nil { + t.Fatalf("CreateAESMasterKeyForTest: %v", err) + } + c := New(storage.New(t.TempDir(), mk), testLogger{t}) + + leaf := newLeaf() + var staple []byte + if tc.staple != nil { + staple = tc.staple(leaf) + } + serverResponse = tc.fetched(leaf) + chains := [][]*x509.Certificate{{leaf.cert, ca.cert}} + if err := c.VerifyChains(context.Background(), chains, staple); err != tc.wantErr { + t.Errorf("VerifyChains() = %v, want %v", err, tc.wantErr) + } + }) + } +} From 7fd766ce315f0d9c2ed1183da14b75cbf703b8d0 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:27:24 -0700 Subject: [PATCH 07/54] Restrict DNS names in PKI server certificates Any user with access to the PKI endpoint could request a server certificate for any DNS name, including wildcards and the names of other backends. That is a problem wherever the CA is trusted for server authentication, e.g. in forwardRootCAs. Add a serverCertificates option to the PKI config. Each entry lists the DNS names (or one-label wildcards) that can be requested, and optionally which users or groups can request them. Without this option, only client certificates can be requested. This is a breaking change for configs that rely on users requesting server certificates. Co-Authored-By: Claude Opus 5.5 --- README.md | 10 ++ SECURITY-TODO.md | 6 +- proxy/config.go | 46 +++++++++ proxy/config_test.go | 26 ++++++ proxy/internal/pki/certs.html | 5 +- proxy/internal/pki/http.go | 14 ++- proxy/internal/pki/http_test.go | 161 ++++++++++++++++++++++++++++++++ proxy/internal/pki/pki.go | 61 ++++++++++++ proxy/proxy.go | 10 ++ 9 files changed, 336 insertions(+), 3 deletions(-) create mode 100644 proxy/internal/pki/http_test.go diff --git a/README.md b/README.md index 8adb20fe..485d8bf5 100644 --- a/README.md +++ b/README.md @@ -489,6 +489,9 @@ Defines a local Certificate Authority: * `ocspServers`: List of strings. URLs for OCSP. * `endpoint`: String. URL for certificate management. * `admins`: List of strings. Users allowed to perform administrative tasks. +* `serverCertificates`: (Optional) List of objects. Defines which DNS names users can request in server certificates. Without it, only client certificates can be requested. + * `dnsNames`: List of strings. DNS names (e.g. `foo.example.com`) or wildcards (e.g. `*.example.com`). A wildcard matches exactly one label. + * `acl`: (Optional) List of strings. Users or groups who can request these names. If not set, all users with access to the `endpoint` can. ### SSH Certificate Authority Configuration (`ConfigSSHCertificateAuthority`) @@ -708,6 +711,12 @@ pki: # Optional: Admins can revoke anybody's certificates. admins: - bob@example.com + # Optional: Allow server certificates for some DNS names. + serverCertificates: + - dnsNames: + - "*.internal.example.com" + acl: + - bob@example.com ``` This example configures a local Certificate Authority (CA) named "EXAMPLE CA". This CA can be used to issue X.509 certificates for client and backend authentication within your environment. @@ -718,6 +727,7 @@ Here is a breakdown of the configuration: * `ocspServers`: Provides the endpoint for the Online Certificate Status Protocol (OCSP), offering a real-time method for checking certificate validity. * `endpoint`: Sets up a web interface at `https://pki-internal.example.com/certs` where authenticated users can request and manage their own certificates. * `admins`: Grants administrative privileges to `bob@example.com`, allowing this user to perform actions like revoking any user's certificate. +* `serverCertificates`: Allows `bob@example.com` to request server certificates for names like `foo.internal.example.com`. Without this, users can only request client certificates. You can then use this CA to enforce client certificate-based authorization for a backend. In the following example, only clients presenting a valid certificate issued by "EXAMPLE CA" for the identity `user@example.com` are allowed access. diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index f79eb0d0..f85f0d75 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -76,7 +76,11 @@ deleted when every item is resolved. `ParseResponseForCert` on fetch; check `ThisUpdate`/`NextUpdate`; don't trust or cache client-supplied staples. -- [ ] **7. PKI users can get serverAuth certs for any DNS name** (Medium) +- [x] **7. PKI users can get serverAuth certs for any DNS name** (Medium) + *Fixed (by decision): new `pki[].serverCertificates` allowlist + (`dnsNames` patterns + optional `acl`). Without it, server certificates + are denied. **Breaking change: needs a release note.** CA name + constraints were not added (they'd require re-issuing existing CAs).* `proxy/internal/pki/http.go` copies CSR `DNSNames` unchecked; `pki.go` adds `ServerAuth` EKU. Any PKI user can get a cert for `*.example.com` or a backend's name, which matters if the CA is in `forwardRootCAs`. diff --git a/proxy/config.go b/proxy/config.go index 8e947b3c..895a34a6 100644 --- a/proxy/config.go +++ b/proxy/config.go @@ -699,6 +699,22 @@ type ConfigPKI struct { // Admins is a list of users who are allowed to perform administrative // tasks on the CA, e.g. revoke any certificate. Admins Strings `yaml:"admins"` + // ServerCertificates defines which DNS names users can request in + // server certificates. When empty, server certificates can't be + // requested. + ServerCertificates []*PKIServerCertificates `yaml:"serverCertificates,omitempty"` +} + +// PKIServerCertificates defines which DNS names can be requested in server +// certificates, and by whom. +type PKIServerCertificates struct { + // DNSNames is a list of DNS names or wildcards, e.g. foo.example.com + // or *.example.com. A wildcard matches exactly one label. + DNSNames Strings `yaml:"dnsNames"` + // ACL is a list of users and groups who are allowed to request these + // DNS names. When ACL is nil, all users with access to the PKI + // endpoint can request them. + ACL *Strings `yaml:"acl,omitempty"` } // ConfigSSHCertificateAuthority defines a certificate authority. @@ -1243,6 +1259,16 @@ func (cfg *Config) Check() error { return fmt.Errorf("pki[%d].Endpoint %q: backend must have mode %s or %s, found %s", i, p.Endpoint, ModeLocal, ModeConsole, mode) } } + for j, sc := range p.ServerCertificates { + if len(sc.DNSNames) == 0 { + return fmt.Errorf("pki[%d].ServerCertificates[%d].DNSNames: must be set", i, j) + } + for _, n := range sc.DNSNames { + if !validDNSNamePattern(n) { + return fmt.Errorf("pki[%d].ServerCertificates[%d].DNSNames: invalid name %q", i, j, n) + } + } + } } sshCAs := make(map[string]bool) @@ -1593,3 +1619,23 @@ func reflectMerge(v1, v2 reflect.Value) error { return nil } } + +// validDNSNamePattern returns true if p is a DNS name, or a wildcard that +// matches one label, e.g. *.example.com. +func validDNSNamePattern(p string) bool { + name, _ := strings.CutPrefix(p, "*.") + if name == "" || len(name) > 253 { + return false + } + for _, label := range strings.Split(name, ".") { + if label == "" || len(label) > 63 || strings.HasPrefix(label, "-") || strings.HasSuffix(label, "-") { + return false + } + for _, c := range label { + if !(c >= 'a' && c <= 'z' || c >= 'A' && c <= 'Z' || c >= '0' && c <= '9' || c == '-') { + return false + } + } + } + return true +} diff --git a/proxy/config_test.go b/proxy/config_test.go index 5a1c1a66..99506cd3 100644 --- a/proxy/config_test.go +++ b/proxy/config_test.go @@ -477,3 +477,29 @@ func TestValidateTrustedIssuers(t *testing.T) { } } } + +func TestValidDNSNamePattern(t *testing.T) { + for _, tc := range []struct { + pattern string + want bool + }{ + {"example.com", true}, + {"foo.example.com", true}, + {"*.example.com", true}, + {"xn--bcher-kva.example", true}, + {"", false}, + {"*", false}, + {"*.", false}, + {"**.example.com", false}, + {"foo.*.example.com", false}, + {"*foo.example.com", false}, + {"foo..example.com", false}, + {"-foo.example.com", false}, + {"foo_bar.example.com", false}, + {"foo.example.com.", false}, + } { + if got := validDNSNamePattern(tc.pattern); got != tc.want { + t.Errorf("validDNSNamePattern(%q) = %v, want %v", tc.pattern, got, tc.want) + } + } +} diff --git a/proxy/internal/pki/certs.html b/proxy/internal/pki/certs.html index 771ab6f6..dbfe8132 100644 --- a/proxy/internal/pki/certs.html +++ b/proxy/internal/pki/certs.html @@ -106,7 +106,10 @@

Option 1

Label:
Usage:
+{{- if .ServerCerts }} + +{{- end }} +
DNS Name:
Password:
diff --git a/proxy/internal/pki/http.go b/proxy/internal/pki/http.go index 3db1e407..32fbe777 100644 --- a/proxy/internal/pki/http.go +++ b/proxy/internal/pki/http.go @@ -334,6 +334,7 @@ func (m *PKIManager) ServeCertificateManagement(w http.ResponseWriter, req *http CASN string CASubjectKeyId string Certs []cert + ServerCerts bool }{ Status: statusFilter, Owner: ownerFilter, @@ -342,6 +343,7 @@ func (m *PKIManager) ServeCertificateManagement(w http.ResponseWriter, req *http CASN: bytesToHex(caCert.SerialNumber.Bytes()), CASubjectKeyId: bytesToHex(caCert.SubjectKeyId), Certs: certs, + ServerCerts: m.canRequestServerCerts(email), } w.Header().Set("X-Frame-Options", "DENY") w.Header().Set("Content-Security-Policy", "default-src 'self'; style-src 'unsafe-inline' 'self'; script-src 'unsafe-eval' 'unsafe-inline' 'self'; frame-ancestors 'none'") @@ -385,6 +387,16 @@ func (m *PKIManager) handleRequestCert(w http.ResponseWriter, req *http.Request) http.Error(w, "invalid request", http.StatusBadRequest) return } + var dnsNames []string + for _, name := range in.DNSNames { + name = strings.ToLower(name) + if !m.canRequestDNSName(email, name) { + m.opts.Logger.Errorf("ERR %s not allowed to request DNS name %q", email, name) + http.Error(w, "DNS name not allowed", http.StatusForbidden) + return + } + dnsNames = append(dnsNames, name) + } cr := &x509.CertificateRequest{ PublicKeyAlgorithm: in.PublicKeyAlgorithm, PublicKey: in.PublicKey, @@ -392,7 +404,7 @@ func (m *PKIManager) handleRequestCert(w http.ResponseWriter, req *http.Request) EmailAddresses: []string{ email, }, - DNSNames: in.DNSNames, + DNSNames: dnsNames, } if in.Subject.CommonName != "" { cr.Subject.CommonName += "::" + in.Subject.CommonName diff --git a/proxy/internal/pki/http_test.go b/proxy/internal/pki/http_test.go new file mode 100644 index 00000000..13e2baa0 --- /dev/null +++ b/proxy/internal/pki/http_test.go @@ -0,0 +1,161 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package pki + +import ( + "bytes" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/x509" + "crypto/x509/pkix" + "encoding/json" + "encoding/pem" + "net/http" + "net/http/httptest" + "slices" + "strings" + "testing" + + jwt "github.com/golang-jwt/jwt/v5" + + "github.com/c2FmZQ/tlsproxy/proxy/internal/fromctx" +) + +func TestDNSNameMatches(t *testing.T) { + for _, tc := range []struct { + pattern, name string + want bool + }{ + {"foo.example.com", "foo.example.com", true}, + {"foo.example.com", "FOO.example.com", true}, + {"foo.example.com", "bar.example.com", false}, + {"foo.example.com", "", false}, + {"*.example.com", "foo.example.com", true}, + {"*.example.com", "*.example.com", true}, + {"*.example.com", "example.com", false}, + {"*.example.com", ".example.com", false}, + {"*.example.com", "foo.bar.example.com", false}, + {"*.example.com", "*.bar.example.com", false}, + {"*.example.com", "foo.example.com.evil.com", false}, + {"*.example.com", "fooexample.com", false}, + {"*.bar.example.com", "*.example.com", false}, + } { + if got := DNSNameMatches(tc.pattern, tc.name); got != tc.want { + t.Errorf("DNSNameMatches(%q, %q) = %v, want %v", tc.pattern, tc.name, got, tc.want) + } + } +} + +func TestRequestServerCert(t *testing.T) { + m := newPKI(t, nil) + m.opts.AdminMatcher = func(acl []string, email string) bool { + return slices.Contains(acl, email) + } + + requestCert := func(email string, dnsNames ...string) (int, *x509.Certificate) { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatalf("ecdsa.GenerateKey: %v", err) + } + csr, err := x509.CreateCertificateRequest(rand.Reader, &x509.CertificateRequest{ + Subject: pkix.Name{CommonName: "test"}, + DNSNames: dnsNames, + }, key) + if err != nil { + t.Fatalf("x509.CreateCertificateRequest: %v", err) + } + body := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE REQUEST", Bytes: csr}) + req := httptest.NewRequest(http.MethodPost, "https://pki.example.com/?get=requestCert", bytes.NewReader(body)) + req.Header.Set("content-type", "application/x-pem-file") + req = req.WithContext(fromctx.WithClaims(req.Context(), jwt.MapClaims{"email": email})) + w := httptest.NewRecorder() + m.ServeCertificateManagement(w, req) + if w.Code != http.StatusOK { + return w.Code, nil + } + var resp struct { + Cert string `json:"cert"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("json.Decode: %v", err) + } + block, _ := pem.Decode([]byte(resp.Cert)) + if block == nil { + t.Fatalf("pem.Decode failed: %q", resp.Cert) + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + t.Fatalf("x509.ParseCertificate: %v", err) + } + return w.Code, cert + } + + // No policy: only client certs. + if code, cert := requestCert("alice@example.com"); code != http.StatusOK || len(cert.DNSNames) != 0 { + t.Errorf("client cert: code %d", code) + } + if code, _ := requestCert("alice@example.com", "foo.example.com"); code != http.StatusForbidden { + t.Errorf("server cert without policy: code %d, want %d", code, http.StatusForbidden) + } + + m.opts.ServerCertificates = []ServerCertificatePolicy{ + {DNSNames: []string{"*.internal.example.com"}}, + {DNSNames: []string{"*.example.com", "example.com"}, ACL: &[]string{"bob@example.com"}}, + } + for _, tc := range []struct { + email string + dnsNames []string + want int + }{ + {"alice@example.com", []string{"foo.internal.example.com"}, http.StatusOK}, + {"alice@example.com", []string{"FOO.Internal.example.com"}, http.StatusOK}, + {"alice@example.com", []string{"*.internal.example.com"}, http.StatusOK}, + {"alice@example.com", []string{"foo.internal.example.com", "www.example.com"}, http.StatusForbidden}, + {"alice@example.com", []string{"www.example.com"}, http.StatusForbidden}, + {"alice@example.com", []string{"foo.bar.internal.example.com"}, http.StatusForbidden}, + {"bob@example.com", []string{"www.example.com", "example.com", "x.internal.example.com"}, http.StatusOK}, + {"bob@example.com", []string{"www.google.com"}, http.StatusForbidden}, + } { + code, cert := requestCert(tc.email, tc.dnsNames...) + if code != tc.want { + t.Errorf("%s %v: code %d, want %d", tc.email, tc.dnsNames, code, tc.want) + continue + } + if cert == nil { + continue + } + var want []string + for _, n := range tc.dnsNames { + want = append(want, strings.ToLower(n)) + } + if !slices.Equal(cert.DNSNames, want) { + t.Errorf("%s %v: DNSNames = %v, want %v", tc.email, tc.dnsNames, cert.DNSNames, want) + } + if !slices.Equal(cert.ExtKeyUsage, []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}) { + t.Errorf("%s %v: ExtKeyUsage = %v", tc.email, tc.dnsNames, cert.ExtKeyUsage) + } + } +} diff --git a/proxy/internal/pki/pki.go b/proxy/internal/pki/pki.go index cdeead5b..cfd6ab35 100644 --- a/proxy/internal/pki/pki.go +++ b/proxy/internal/pki/pki.go @@ -119,6 +119,22 @@ type Options struct { } // AdminMatcher returns true if group contains email. AdminMatcher func(acl []string, email string) bool + // ServerCertificates defines which DNS names users can request in + // server certificates. When empty, server certificates can't be + // requested. + ServerCertificates []ServerCertificatePolicy +} + +// ServerCertificatePolicy defines which DNS names can be requested in server +// certificates, and by whom. +type ServerCertificatePolicy struct { + // DNSNames is a list of DNS name patterns. A pattern is either a + // DNS name, e.g. foo.example.com, or a wildcard, e.g. *.example.com. + // A wildcard matches exactly one label, like in TLS certificates. + DNSNames []string + // ACL is a list of users and groups who can request these names, + // using AdminMatcher. When nil, all users can request them. + ACL *[]string } // New returns a new initialized PKI manager. The Certificate Authority's key @@ -842,3 +858,48 @@ func wipe(b []byte) { b[i] = 0 } } + +// canRequestServerCerts returns true if email is allowed to request server +// certificates for at least some DNS names. +func (m *PKIManager) canRequestServerCerts(email string) bool { + for _, p := range m.opts.ServerCertificates { + if p.ACL == nil || m.opts.AdminMatcher(*p.ACL, email) { + return true + } + } + return false +} + +// canRequestDNSName returns true if email is allowed to request a server +// certificate for name. +func (m *PKIManager) canRequestDNSName(email, name string) bool { + for _, p := range m.opts.ServerCertificates { + if !slices.ContainsFunc(p.DNSNames, func(pattern string) bool { return DNSNameMatches(pattern, name) }) { + continue + } + if p.ACL == nil || m.opts.AdminMatcher(*p.ACL, email) { + return true + } + } + return false +} + +// DNSNameMatches returns true if name matches pattern. The pattern is either a +// DNS name, or a wildcard that matches exactly one label, e.g. *.example.com +// matches foo.example.com, but not foo.bar.example.com. A wildcard name only +// matches the same wildcard pattern. +func DNSNameMatches(pattern, name string) bool { + pattern, name = strings.ToLower(pattern), strings.ToLower(name) + if name == "" { + return false + } + if pattern == name { + return true + } + suffix, ok := strings.CutPrefix(pattern, "*") + if !ok || !strings.HasPrefix(suffix, ".") { + return false + } + label, ok := strings.CutSuffix(name, suffix) + return ok && label != "" && !strings.ContainsAny(label, ".*") +} diff --git a/proxy/proxy.go b/proxy/proxy.go index 973abd47..58c9d2e6 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -431,6 +431,15 @@ func (p *Proxy) Reconfigure(cfg *Config) error { pkis := make(map[string]*pki.PKIManager) for _, pp := range cfg.PKI { + var serverCerts []pki.ServerCertificatePolicy + for _, sc := range pp.ServerCertificates { + policy := pki.ServerCertificatePolicy{DNSNames: sc.DNSNames} + if sc.ACL != nil { + acl := []string(*sc.ACL) + policy.ACL = &acl + } + serverCerts = append(serverCerts, policy) + } opts := pki.Options{ Name: pp.Name, KeyType: pp.KeyType, @@ -443,6 +452,7 @@ func (p *Proxy) Reconfigure(cfg *Config) error { Store: p.store, EventRecorder: er, AdminMatcher: aclMatcher.emailMatches, + ServerCertificates: serverCerts, } m, err := pki.New(opts) if err != nil { From 9a42de0190f0cef58979d666b7c0c7a1f5a992d1 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:29:00 -0700 Subject: [PATCH 08/54] Add timeouts and a connection limit to the plain HTTP server The HTTP server that serves ACME challenges and HTTPS redirects had no timeouts and no connection limit. Idle connections were held forever and could exhaust the process's file descriptors, taking down the whole proxy. Use 10s read and write timeouts, and limit the number of concurrent connections. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 4 ++- proxy/http.go | 25 +++++++++++++++ proxy/http_test.go | 80 ++++++++++++++++++++++++++++++++++++++++++++++ proxy/proxy.go | 6 +--- 4 files changed, 109 insertions(+), 6 deletions(-) create mode 100644 proxy/http_test.go diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index f85f0d75..db315cc7 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -87,7 +87,9 @@ deleted when every item is resolved. Fix: restrict DNS SANs (allowlist/suffix per user or admins only), reject wildcards, consider CA name constraints. -- [ ] **8. Port 80 (ACME) HTTP server has no timeouts or connection limit** (Medium) +- [x] **8. Port 80 (ACME) HTTP server has no timeouts or connection limit** (Medium) + *Fixed: 10s read/write timeouts and at most min(MaxOpen, 1000) + concurrent connections.* `proxy/proxy.go` `Start()`: `http.Server{Handler: certManager.HTTPHandler}` has no Read/ReadHeader/Write/Idle timeouts and isn't counted in `MaxOpen`. Idle connections are held forever and exhaust file descriptors. diff --git a/proxy/http.go b/proxy/http.go index f511d554..0a357930 100644 --- a/proxy/http.go +++ b/proxy/http.go @@ -30,12 +30,37 @@ import ( "net/http" "sync" "time" + + "golang.org/x/net/netutil" ) type ctxKey int var connCtxKey ctxKey = 1 +var ( + // plainHTTPTimeout is the read and write timeout of the plain HTTP + // server, i.e. the one serving ACME challenges and redirects. + plainHTTPTimeout = 10 * time.Second + // plainHTTPMaxConns is the maximum number of concurrent connections + // to the plain HTTP server. + plainHTTPMaxConns = 1000 +) + +// startPlainHTTPServer starts the HTTP server that serves ACME challenges and +// redirects to HTTPS. +func startPlainHTTPServer(handler http.Handler, l net.Listener, maxConns int) *http.Server { + s := &http.Server{ + Handler: handler, + ReadHeaderTimeout: plainHTTPTimeout, + ReadTimeout: plainHTTPTimeout, + WriteTimeout: plainHTTPTimeout, + } + s.SetKeepAlivesEnabled(false) + go serveHTTP(s, netutil.LimitListener(l, min(maxConns, plainHTTPMaxConns))) + return s +} + func startInternalHTTPServer(handler http.Handler, conns <-chan net.Conn) *http.Server { l := &proxyListener{ ch: conns, diff --git a/proxy/http_test.go b/proxy/http_test.go new file mode 100644 index 00000000..6fb3f0bc --- /dev/null +++ b/proxy/http_test.go @@ -0,0 +1,80 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package proxy + +import ( + "bufio" + "io" + "net" + "net/http" + "testing" + "time" +) + +func TestPlainHTTPServer(t *testing.T) { + defer func(d time.Duration) { plainHTTPTimeout = d }(plainHTTPTimeout) + plainHTTPTimeout = 500 * time.Millisecond + + l, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("net.Listen: %v", err) + } + s := startPlainHTTPServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + w.Write([]byte("ok")) + }), l, 2) + defer s.Close() + + // Idle connections are closed by the server. + idle := make([]net.Conn, 2) + for i := range idle { + if idle[i], err = net.Dial("tcp", l.Addr().String()); err != nil { + t.Fatalf("net.Dial: %v", err) + } + defer idle[i].Close() + } + // The third connection is accepted after the idle ones time out. + c, err := net.Dial("tcp", l.Addr().String()) + if err != nil { + t.Fatalf("net.Dial: %v", err) + } + defer c.Close() + if _, err := c.Write([]byte("GET / HTTP/1.1\r\nHost: example.com\r\n\r\n")); err != nil { + t.Fatalf("Write: %v", err) + } + c.SetReadDeadline(time.Now().Add(5 * time.Second)) + resp, err := http.ReadResponse(bufio.NewReader(c), nil) + if err != nil { + t.Fatalf("ReadResponse: %v", err) + } + if resp.StatusCode != http.StatusOK { + t.Errorf("StatusCode = %d, want 200", resp.StatusCode) + } + + for i, ic := range idle { + ic.SetReadDeadline(time.Now().Add(5 * time.Second)) + if _, err := io.ReadAll(ic); err != nil { + t.Errorf("idle conn %d: ReadAll: %v", i, err) + } + } +} diff --git a/proxy/proxy.go b/proxy/proxy.go index 58c9d2e6..c51fcf40 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -1055,15 +1055,11 @@ func (p *Proxy) Start(ctx context.Context) error { p.connClosed = sync.NewCond(&p.mu) var httpServer *http.Server if p.cfg.HTTPAddr != nil && *p.cfg.HTTPAddr != "" { - httpServer = &http.Server{ - Handler: p.certManager.HTTPHandler(nil), - } httpListener, err := net.Listen("tcp", *p.cfg.HTTPAddr) if err != nil { return err } - httpServer.SetKeepAlivesEnabled(false) - go serveHTTP(httpServer, httpListener) + httpServer = startPlainHTTPServer(p.certManager.HTTPHandler(nil), httpListener, *p.cfg.MaxOpen) } p.ctx, p.cancel = context.WithCancel(ctx) From 1284c6ebd53a6e74c0e8740ca964e24ea01b01b7 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:31:01 -0700 Subject: [PATCH 09/54] Limit pending OIDC requests and vacuum less often Every request to the local OIDC provider scanned all pending requests under the server's lock, and unauthenticated device authorization requests could add entries without limit, making the cost quadratic. Scan for expired requests at most every 30 seconds, check expiration when looking up a request, and cap the number of pending authorization and device authorization requests. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 6 +- proxy/internal/oidc/deviceauth.go | 10 ++- proxy/internal/oidc/deviceauth_test.go | 102 +++++++++++++++++++++++++ proxy/internal/oidc/server.go | 35 +++++++-- 4 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 proxy/internal/oidc/deviceauth_test.go diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index db315cc7..466138ef 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -95,7 +95,11 @@ deleted when every item is resolved. Idle connections are held forever and exhaust file descriptors. Fix: set timeouts and cap concurrent connections. -- [ ] **9. Unauthenticated DoS of the local OIDC provider via device authorization** (Medium) +- [x] **9. Unauthenticated DoS of the local OIDC provider via device authorization** (Medium) + *Fixed: `vacuum()` scans at most every 30s, lookups check expiry + themselves, and pending authorization/device requests are capped at + 10000 (503 beyond). Client secret is not required for device + authorization (unchanged).* `proxy/internal/oidc/deviceauth.go`, `server.go` `vacuum()`: each POST to `/device/authorization` with a (public) `client_id` adds map entries for 10 minutes; every OIDC request runs `vacuum()` which scans all maps under diff --git a/proxy/internal/oidc/deviceauth.go b/proxy/internal/oidc/deviceauth.go index 8426e14d..d8a79783 100644 --- a/proxy/internal/oidc/deviceauth.go +++ b/proxy/internal/oidc/deviceauth.go @@ -99,6 +99,12 @@ func (s *ProviderServer) ServeDeviceAuthorization(w http.ResponseWriter, req *ht now := time.Now().UTC() s.mu.Lock() + if len(s.deviceTokens) >= maxPendingRequests || len(s.deviceCodes) >= maxPendingRequests { + s.mu.Unlock() + s.opts.Logger.Errorf("ERR ServeDeviceAuthorization: too many pending requests") + http.Error(w, "too many pending requests", http.StatusServiceUnavailable) + return + } s.deviceCodes[userCode] = &deviceCodeData{ created: now, clientID: clientID, @@ -189,8 +195,10 @@ func (s *ProviderServer) ServeDeviceVerification(w http.ResponseWriter, req *htt data, ok := s.deviceCodes[userCode] delete(s.deviceCodes, userCode) var devToken *deviceToken - if ok { + if ok && !expired(data.created) { devToken, ok = s.deviceTokens[data.deviceCode] + } else { + ok = false } if !ok { http.Error(w, "request expired", http.StatusBadRequest) diff --git a/proxy/internal/oidc/deviceauth_test.go b/proxy/internal/oidc/deviceauth_test.go new file mode 100644 index 00000000..20c81d05 --- /dev/null +++ b/proxy/internal/oidc/deviceauth_test.go @@ -0,0 +1,102 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package oidc + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + "time" + + jwt "github.com/golang-jwt/jwt/v5" + + "github.com/c2FmZQ/tlsproxy/proxy/internal/fromctx" +) + +type nopRecorder struct{} + +func (nopRecorder) Record(string) {} + +func TestDeviceAuthorizationLimits(t *testing.T) { + s := NewServer(ServerOptions{ + Clients: []Client{{ID: "client"}}, + ACLMatcher: func([]string, string) bool { return true }, + EventRecorder: nopRecorder{}, + Logger: nopLogger{}, + }) + + deviceAuth := func() (int, string) { + req := httptest.NewRequest(http.MethodPost, "https://idp.example.com/device/authorization", strings.NewReader("client_id=client")) + req.Header.Set("content-type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.ServeDeviceAuthorization(w, req) + var resp struct { + UserCode string `json:"user_code"` + } + json.NewDecoder(w.Body).Decode(&resp) + return w.Code, resp.UserCode + } + + // An expired request can't be used, even before it is vacuumed. + code, userCode := deviceAuth() + if code != http.StatusOK { + t.Fatalf("deviceAuth: code %d", code) + } + s.mu.Lock() + s.deviceCodes[userCode].created = time.Now().Add(-2 * codeExpiration) + s.mu.Unlock() + + form := url.Values{"user_code": {userCode}, "approve": {"true"}} + req := httptest.NewRequest(http.MethodPost, "https://idp.example.com/device/verify", strings.NewReader(form.Encode())) + req.Header.Set("content-type", "application/x-www-form-urlencoded") + req = req.WithContext(fromctx.WithClaims(req.Context(), jwt.MapClaims{"email": "bob@example.com"})) + w := httptest.NewRecorder() + s.ServeDeviceVerification(w, req) + if got, want := w.Code, http.StatusBadRequest; got != want { + t.Errorf("ServeDeviceVerification(expired) = %d, want %d", got, want) + } + + // The number of pending requests is limited. + for range maxPendingRequests { + if code, _ := deviceAuth(); code != http.StatusOK { + s.mu.Lock() + n := len(s.deviceTokens) + s.mu.Unlock() + if code != http.StatusServiceUnavailable || n < maxPendingRequests { + t.Fatalf("deviceAuth: code %d with %d pending", code, n) + } + break + } + } + if code, _ := deviceAuth(); code != http.StatusServiceUnavailable { + t.Errorf("deviceAuth: code %d, want %d", code, http.StatusServiceUnavailable) + } +} + +type nopLogger struct{} + +func (nopLogger) Errorf(string, ...any) {} diff --git a/proxy/internal/oidc/server.go b/proxy/internal/oidc/server.go index 927472d7..4a380199 100644 --- a/proxy/internal/oidc/server.go +++ b/proxy/internal/oidc/server.go @@ -62,6 +62,10 @@ const ( defaultTokenLifetime = time.Hour codeExpiration = 10 * time.Minute pollInterval = 5 * time.Second + vacuumInterval = 30 * time.Second + // maxPendingRequests is the maximum number of pending authorization + // requests and device authorization requests. + maxPendingRequests = 10000 ) var ( @@ -154,6 +158,7 @@ type ProviderServer struct { opts ServerOptions mu sync.Mutex + lastVacuum time.Time codes map[string]*codeData deviceCodes map[string]*deviceCodeData deviceTokens map[string]*deviceToken @@ -166,22 +171,32 @@ type Client struct { ACL *[]string } +func expired(created time.Time) bool { + return created.Add(codeExpiration).Before(time.Now().UTC()) +} + +// vacuum removes expired requests. It scans all the pending requests at most +// once every vacuumInterval. Lookups must still check expiration. func (s *ProviderServer) vacuum() { s.mu.Lock() defer s.mu.Unlock() now := time.Now().UTC() + if now.Sub(s.lastVacuum) < vacuumInterval { + return + } + s.lastVacuum = now for k, v := range s.codes { - if v.created.Add(codeExpiration).Before(now) { + if expired(v.created) { delete(s.codes, k) } } for k, v := range s.deviceCodes { - if v.created.Add(codeExpiration).Before(now) { + if expired(v.created) { delete(s.deviceCodes, k) } } for k, v := range s.deviceTokens { - if v.created.Add(codeExpiration).Before(now) { + if expired(v.created) { delete(s.deviceTokens, k) } } @@ -268,7 +283,7 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req var code string var data *codeData for k, v := range s.codes { - if v.requestID == requestID { + if v.requestID == requestID && !expired(v.created) { code = k data = v break @@ -424,6 +439,12 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req } s.mu.Lock() + if len(s.codes) >= maxPendingRequests { + s.mu.Unlock() + s.opts.Logger.Errorf("ERR ServeAuthorization: too many pending requests") + http.Error(w, "too many pending requests", http.StatusServiceUnavailable) + return + } s.codes[code] = &codeData{ created: time.Now().UTC(), clientID: clientID, @@ -488,7 +509,7 @@ func (s *ProviderServer) ServeToken(w http.ResponseWriter, req *http.Request) { delete(s.codes, code) s.mu.Unlock() - if !ok || data.clientID != clientID { + if !ok || expired(data.created) || data.clientID != clientID { http.Error(w, "invalid request", http.StatusBadRequest) return } @@ -534,6 +555,10 @@ func (s *ProviderServer) ServeToken(w http.ResponseWriter, req *http.Request) { } s.mu.Lock() data, ok = s.deviceTokens[deviceCode] + if ok && expired(data.created) { + delete(s.deviceTokens, deviceCode) + ok = false + } defer s.mu.Unlock() var resp struct { From 3ca71b6dd962ccc045e461ce9e2804d41ad9ee2a Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:33:10 -0700 Subject: [PATCH 10/54] Check URL token hosts on login and logout, add expiration serveLogin and serveLogout redirected to the URL in a URL token without checking its host. URL tokens were built from the Host header, so a client could create a token for an arbitrary host and get a victim to use it, resulting in an open redirect after login. URL tokens also never expired. Require the URL token's host to be one of the backend's server names on login and logout, and make URL tokens expire after 24 hours. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 5 ++- proxy/backend-sso.go | 4 +- proxy/backend-sso_test.go | 43 ++++++++++++++++++++ proxy/internal/tokenmanager/urltoken.go | 7 +++- proxy/internal/tokenmanager/urltoken_test.go | 13 ++++++ 5 files changed, 68 insertions(+), 4 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 466138ef..852bbda5 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -109,7 +109,10 @@ deleted when every item is resolved. ## Low–Medium -- [ ] **10. Open redirect after login via Host header in URL token** (Low–Medium) +- [x] **10. Open redirect after login via Host header in URL token** (Low–Medium) + *Fixed: Host is validated first (#5); `serveLogin`/`serveLogout` require + the token URL host to be one of the backend's server names; URL tokens + expire after 24h.* `proxy/backend-sso.go`: the URL token URL is built from `req.Host` before any Host check; `serveLogin`/`serveLogout` don't validate the token URL's host. Needs the victim's `__tlsproxySid` (same on every host, not HttpOnly, diff --git a/proxy/backend-sso.go b/proxy/backend-sso.go index 1f00c9dc..ab3a03ce 100644 --- a/proxy/backend-sso.go +++ b/proxy/backend-sso.go @@ -266,7 +266,7 @@ func (be *Backend) serveLogin(w http.ResponseWriter, req *http.Request) { return } url, claims, err := be.tm.ValidateURLToken(req, tok) - if err != nil { + if err != nil || !slices.Contains(be.ServerNames, url.Hostname()) { http.Error(w, "invalid request", http.StatusBadRequest) return } @@ -292,7 +292,7 @@ func (be *Backend) serveLogout(w http.ResponseWriter, req *http.Request) { req.ParseForm() if tokenStr := req.Form.Get("u"); tokenStr != "" { url, _, err := be.tm.ValidateURLToken(req, tokenStr) - if err != nil { + if err != nil || !slices.Contains(be.ServerNames, url.Hostname()) { http.Error(w, "invalid request", http.StatusBadRequest) return } diff --git a/proxy/backend-sso_test.go b/proxy/backend-sso_test.go index 2d70450b..414230bf 100644 --- a/proxy/backend-sso_test.go +++ b/proxy/backend-sso_test.go @@ -31,6 +31,8 @@ import ( "net" "net/http" "net/http/httptest" + "net/url" + "strings" "testing" "time" @@ -349,6 +351,47 @@ func TestEnforceSSOPolicy(t *testing.T) { } } +func TestServeLoginRedirectHost(t *testing.T) { + proxy := newBackendSSOTestProxy(t) + be := proxy.cfg.Backends[0] + + for _, tc := range []struct { + url string + want int + }{ + {"https://example.com/foo", http.StatusFound}, + {"https://evil.example.org/foo", http.StatusBadRequest}, + } { + req := httptest.NewRequest("GET", "https://example.com/", nil) + w := httptest.NewRecorder() + sid.SetSessionID(w, req, "") + req.Header.Set("Cookie", w.Header().Get("Set-Cookie")) + + u, err := url.Parse(tc.url) + if err != nil { + t.Fatalf("url.Parse: %v", err) + } + token, _, err := be.tm.URLToken(httptest.NewRecorder(), req, u, nil) + if err != nil { + t.Fatalf("URLToken: %v", err) + } + + for _, path := range []string{"/.sso/login?redirect=", "/.sso/logout?u="} { + req := httptest.NewRequest("GET", "https://example.com"+path+url.QueryEscape(token), nil) + req.Header.Set("Cookie", w.Header().Get("Set-Cookie")) + rec := httptest.NewRecorder() + if strings.HasPrefix(path, "/.sso/login") { + be.serveLogin(rec, req) + } else { + be.serveLogout(rec, req) + } + if got := rec.Code; got != tc.want { + t.Errorf("%s%s: code %d, want %d", path, tc.url, got, tc.want) + } + } + } +} + func newBackendSSOTestProxy(t *testing.T) *Proxy { return newTestProxy( &Config{ diff --git a/proxy/internal/tokenmanager/urltoken.go b/proxy/internal/tokenmanager/urltoken.go index b77979cf..797da82a 100644 --- a/proxy/internal/tokenmanager/urltoken.go +++ b/proxy/internal/tokenmanager/urltoken.go @@ -30,6 +30,7 @@ import ( "net/http" "net/url" "slices" + "time" jwt "github.com/golang-jwt/jwt/v5" "golang.org/x/net/idna" @@ -38,6 +39,9 @@ import ( "github.com/c2FmZQ/tlsproxy/proxy/internal/sid" ) +// urlTokenLifetime is how long URL tokens are valid. +const urlTokenLifetime = 24 * time.Hour + // URLToken returns a signed token for URL u in the context of request req. func (tm *TokenManager) URLToken(w http.ResponseWriter, req *http.Request, u *url.URL, extra map[string]any) (string, string, error) { realHost := u.Host @@ -56,6 +60,7 @@ func (tm *TokenManager) URLToken(w http.ResponseWriter, req *http.Request, u *ur } claims["url"] = u.String() claims["hsid"] = base64.StdEncoding.EncodeToString(tm.HMAC([]byte(sid))) + claims["exp"] = time.Now().Add(urlTokenLifetime).Unix() token, err := tm.CreateToken(claims, "") return token, displayURL, err } @@ -63,7 +68,7 @@ func (tm *TokenManager) URLToken(w http.ResponseWriter, req *http.Request, u *ur // ValidateURLToken validates a signed token and returns the URL. The request // must on the same host as the one where the token was created. func (tm *TokenManager) ValidateURLToken(req *http.Request, token string) (*url.URL, jwt.MapClaims, error) { - tok, err := tm.ValidateToken(token) + tok, err := tm.ValidateToken(token, jwt.WithExpirationRequired()) if err != nil { return nil, nil, err } diff --git a/proxy/internal/tokenmanager/urltoken_test.go b/proxy/internal/tokenmanager/urltoken_test.go index 9eb8d090..f664823a 100644 --- a/proxy/internal/tokenmanager/urltoken_test.go +++ b/proxy/internal/tokenmanager/urltoken_test.go @@ -24,6 +24,7 @@ package tokenmanager import ( + "encoding/base64" "net/http/httptest" "testing" @@ -70,6 +71,18 @@ func TestURLToken(t *testing.T) { t.Errorf("url = %q, want %q", got, want) } + // Token without expiration + noExp, err := tm.CreateToken(jwt.MapClaims{ + "url": "https://example.com/foo/bar", + "hsid": base64.StdEncoding.EncodeToString(tm.HMAC([]byte(sessionID))), + }, "") + if err != nil { + t.Fatalf("CreateToken: %v", err) + } + if _, _, err := tm.ValidateURLToken(req, noExp); err == nil { + t.Error("ValidateURLToken(no exp) should fail") + } + // Wrong session id req.Header.Del("cookie") if _, _, err := tm.ValidateURLToken(req, tok); err == nil { From 30839d64cf0cfde0d7961394ebf1749d2bbcceaf Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:35:17 -0700 Subject: [PATCH 11/54] Use absolute URLs when redirecting to the current request After setting the ID token cookie, the proxy redirected to req.URL.String(), which is a relative URL. For a request like GET //evil.com/x, that produced Location: //evil.com/x, an open redirect. Redirect to an absolute URL with the request's (validated) host. Do the same in the passkeys manager for consistency. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 5 ++++- proxy/backend-sso.go | 7 ++++++- proxy/backend-sso_test.go | 22 ++++++++++++++++++++++ proxy/internal/passkeys/manager.go | 2 ++ 4 files changed, 34 insertions(+), 2 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 852bbda5..fd8641c3 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -120,7 +120,10 @@ deleted when every item is resolved. Fix: #5, plus require the token URL host to be in `be.ServerNames`; add an expiry to URL tokens. -- [ ] **11. Open redirect with `//evil.com` paths** (Low–Medium) +- [x] **11. Open redirect with `//evil.com` paths** (Low–Medium) + *Fixed: redirects use absolute `https://` URLs. (The + passkeys sites weren't exploitable, since local handlers run after the + clean-path redirect, but now use the same pattern.)* `proxy/backend-sso.go` (ID token cookie reissue: `http.Redirect(w, req, req.URL.String(), 302)`), and the same pattern in `proxy/internal/passkeys/manager.go` (2 places). `GET //evil.com/x` redirects diff --git a/proxy/backend-sso.go b/proxy/backend-sso.go index ab3a03ce..874299c0 100644 --- a/proxy/backend-sso.go +++ b/proxy/backend-sso.go @@ -156,7 +156,12 @@ func (be *Backend) checkCookies(w http.ResponseWriter, req *http.Request) (jwt.M http.Error(w, "internal error", http.StatusInternalServerError) return nil, "", false } - http.Redirect(w, req, req.URL.String(), http.StatusFound) + // Use an absolute URL. A relative URL like //evil.com/ would redirect + // to another host. + u := *req.URL + u.Scheme = "https" + u.Host = req.Host + http.Redirect(w, req, u.String(), http.StatusFound) return nil, "", false } diff --git a/proxy/backend-sso_test.go b/proxy/backend-sso_test.go index 414230bf..fb7f022d 100644 --- a/proxy/backend-sso_test.go +++ b/proxy/backend-sso_test.go @@ -114,6 +114,28 @@ func TestAuthenticateUser(t *testing.T) { } } +func TestIDTokenRedirect(t *testing.T) { + proxy := newBackendSSOTestProxy(t) + + req := httptest.NewRequest("GET", "https://example.com//evil.example.org/foo?a=b", nil) + // Server requests don't have a scheme and host in the URL. + req.URL.Scheme = "" + req.URL.Host = "" + if err := setAuthCookie(req, "bob@", "example.com", "https://example.com/", proxy.tokenManager); err != nil { + t.Fatalf("setAuthCookie: %v", err) + } + w := httptest.NewRecorder() + if got, want := proxy.cfg.Backends[0].authenticateUser(w, &req), false; got != want { + t.Fatalf("authenticateUser() = %v, want %v", got, want) + } + if got, want := w.Code, 302; got != want { + t.Errorf("response code = %d, want %d", got, want) + } + if got, want := w.Header().Get("Location"), "https://example.com//evil.example.org/foo?a=b"; got != want { + t.Errorf("Location = %q, want %q", got, want) + } +} + func TestAuthenticateDevice(t *testing.T) { proxy := newBackendSSOTestProxy(t) diff --git a/proxy/internal/passkeys/manager.go b/proxy/internal/passkeys/manager.go index 25086ab1..0248a5d7 100644 --- a/proxy/internal/passkeys/manager.go +++ b/proxy/internal/passkeys/manager.go @@ -301,6 +301,8 @@ func (m *Manager) HandleCallback(w http.ResponseWriter, req *http.Request) { args.Del("nonce") args.Set("redirect", token) req.URL.RawQuery = args.Encode() + req.URL.Scheme = "https" + req.URL.Host = req.Host http.Redirect(w, req, req.URL.String(), http.StatusFound) return } From 5560b7dfcca1208a3dc2ade993f62fa106363d42 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:37:12 -0700 Subject: [PATCH 12/54] Bind SAML requests to the browser The SAML assertion consumer service only checked InResponseTo against server-side state. An attacker could start a login, sign in as themselves, and get a victim's browser to post the resulting SAML response, logging the victim into the attacker's account. Set a short-lived cookie with the request ID when sending the AuthnRequest, and require it to match InResponseTo in the callback. The cookie is SameSite=None because the SAML response is a cross-site POST. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 6 ++- proxy/internal/cookiemanager/cookiemanager.go | 34 +++++++++++++++ proxy/internal/saml/saml.go | 7 +++- proxy/sso-saml_test.go | 42 ++++++++++++------- 4 files changed, 72 insertions(+), 17 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index fd8641c3..e8efee9d 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -130,7 +130,11 @@ deleted when every item is resolved. to `//evil.com/x`. Fix: build the redirect from the cleaned path + query. -- [ ] **12. SAML login CSRF / session swapping** (Low–Medium) +- [x] **12. SAML login CSRF / session swapping** (Low–Medium) + *Fixed: `RequestLogin` sets a `TLSPROXYSAMLNONCE` cookie (SameSite=None, + Secure, HttpOnly, 10 min) with the request ID; `HandleCallback` requires + it to match `InResponseTo`. Scoped to the SSO domain like the OIDC nonce + cookie (see #44), since the ACS can be on a different host.* `proxy/internal/saml/saml.go`: `InResponseTo` is checked against server state only, not bound to the browser (OIDC uses the `TLSPROXYNONCE` cookie). An attacker can submit their own signed response in the victim's browser. diff --git a/proxy/internal/cookiemanager/cookiemanager.go b/proxy/internal/cookiemanager/cookiemanager.go index 8cb9d6cd..ad2faf19 100644 --- a/proxy/internal/cookiemanager/cookiemanager.go +++ b/proxy/internal/cookiemanager/cookiemanager.go @@ -45,6 +45,7 @@ const ( tlsProxyAuthCookie = "TLSPROXYAUTH" tlsProxyIDTokenCookie = "TLSPROXYIDTOKEN" tlsProxyNonce = "TLSPROXYNONCE" + tlsProxySAMLNonce = "TLSPROXYSAMLNONCE" expiredAuthTokenLeeway = 7 * 24 * time.Hour defaultTokenLifetime = 20 * time.Hour @@ -230,6 +231,39 @@ func (cm *CookieManager) Nonce(w http.ResponseWriter, req *http.Request) string return "" } +// SetSAMLNonce sets a cookie that binds a SAML request to the browser. The SAML +// response is a cross-site POST, so the cookie must be SameSite=None. +func (cm *CookieManager) SetSAMLNonce(w http.ResponseWriter, nonce string) { + http.SetCookie(w, &http.Cookie{ + Name: tlsProxySAMLNonce, + Value: nonce, + Domain: cm.domain, + Path: "/", + MaxAge: 600, + SameSite: http.SameSiteNoneMode, + Secure: true, + HttpOnly: true, + }) +} + +// SAMLNonce returns the value of the cookie set by SetSAMLNonce, and deletes +// the cookie. +func (cm *CookieManager) SAMLNonce(w http.ResponseWriter, req *http.Request) string { + http.SetCookie(w, &http.Cookie{ + Name: tlsProxySAMLNonce, + Domain: cm.domain, + Path: "/", + MaxAge: -1, + SameSite: http.SameSiteNoneMode, + Secure: true, + HttpOnly: true, + }) + if c, err := req.Cookie(tlsProxySAMLNonce); err == nil { + return c.Value + } + return "" +} + func (cm *CookieManager) ClearCookies(w http.ResponseWriter) error { cookie := &http.Cookie{ Name: tlsProxyAuthCookie, diff --git a/proxy/internal/saml/saml.go b/proxy/internal/saml/saml.go index 22ca4d7e..02f4fb27 100644 --- a/proxy/internal/saml/saml.go +++ b/proxy/internal/saml/saml.go @@ -27,6 +27,7 @@ import ( "bytes" "compress/flate" "crypto/rand" + "crypto/subtle" "crypto/x509" "encoding/base64" "encoding/hex" @@ -52,6 +53,8 @@ import ( type CookieManager interface { SetAuthTokenCookie(w http.ResponseWriter, req *http.Request, userID, email, sessionID, host string, extraClaims map[string]any) error ClearCookies(w http.ResponseWriter) error + SetSAMLNonce(w http.ResponseWriter, nonce string) + SAMLNonce(w http.ResponseWriter, req *http.Request) string } type EventRecorder interface { @@ -144,6 +147,8 @@ func (p *Provider) RequestLogin(w http.ResponseWriter, req *http.Request, origUR http.Error(w, err.Error(), http.StatusInternalServerError) return } + // Bind the request to this browser. + p.cm.SetSAMLNonce(w, idStr) http.Redirect(w, req, url, http.StatusFound) p.er.Record("saml auth request") } @@ -199,7 +204,7 @@ func (p *Provider) HandleCallback(w http.ResponseWriter, req *http.Request) { } p.mu.Unlock() - if !ok { + if nonce := p.cm.SAMLNonce(w, req); !ok || nonce == "" || subtle.ConstantTimeCompare([]byte(nonce), []byte(id)) != 1 { p.er.Record("invalid state") http.Error(w, "timeout", http.StatusForbidden) return diff --git a/proxy/sso-saml_test.go b/proxy/sso-saml_test.go index 5bf18f01..9a704fe9 100644 --- a/proxy/sso-saml_test.go +++ b/proxy/sso-saml_test.go @@ -192,24 +192,36 @@ func TestSSOEnforceSAML(t *testing.T) { return resp.StatusCode, string(body), host } - code, body, _ := get("https://https.example.com/blah", http.Header{"x-skip-login-confirmation": []string{"yes"}}, nil) - if got, want := code, 200; got != want { - t.Errorf("Code = %v, want %v", got, want) - } - m := regexp.MustCompile(`
Date: Tue, 6 Oct 2026 23:38:43 -0700 Subject: [PATCH 13/54] Record ECH events only for known server names The ECH accepted/rejected events included the server name from the ClientHello and were recorded before the backend lookup. Any client could send an ECH extension with an arbitrary outer SNI, and each distinct name added a permanent entry to the events map. Record these events only after the server name matched a backend. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 3 ++- proxy/ech_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++++++ proxy/proxy.go | 10 +++++--- 3 files changed, 71 insertions(+), 4 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index e8efee9d..ee33383d 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -141,7 +141,8 @@ deleted when every item is resolved. Fix: short-lived host-only `SameSite=None; Secure; HttpOnly` cookie bound to the request ID, checked in `HandleCallback`. -- [ ] **13. Unbounded `events` map growth from attacker-chosen SNI** (Low–Medium) +- [x] **13. Unbounded `events` map growth from attacker-chosen SNI** (Low–Medium) + *Fixed: ECH events are recorded after a successful backend lookup.* `proxy/proxy.go` `handleConnection`: ECH accepted/rejected events include the outer SNI and are recorded before the backend lookup. ECH needn't be configured. diff --git a/proxy/ech_test.go b/proxy/ech_test.go index 6e0b3d34..12361661 100644 --- a/proxy/ech_test.go +++ b/proxy/ech_test.go @@ -30,7 +30,9 @@ import ( "crypto/tls" "encoding/base64" "errors" + "fmt" "io" + "strings" "testing" "github.com/c2FmZQ/ech" @@ -237,3 +239,63 @@ func echGetQUIC(name, addr, msg string, rootCA *certmanager.CertManager, configL } return string(b), nil } + +func TestECHEventsUnknownName(t *testing.T) { + ctx := t.Context() + + extCA, err := certmanager.New("root-ca.example.com", t.Logf) + if err != nil { + t.Fatalf("certmanager.New: %v", err) + } + proxy := newTestProxy( + &Config{ + HTTPAddr: newPtr("localhost:0"), + TLSAddr: newPtr("localhost:0"), + CacheDir: newPtr(t.TempDir()), + MaxOpen: newPtr(100), + Backends: []*Backend{ + { + ServerNames: Strings{"local.example.com"}, + Mode: "LOCAL", + DocumentRoot: ".", + }, + }, + }, + extCA, + ) + if err := proxy.Start(ctx); err != nil { + t.Fatalf("proxy.Start: %v", err) + } + defer proxy.Stop() + + // ECH configs with public names that the proxy doesn't know. The proxy + // can't decrypt them, so it sees the public names in the outer + // ClientHello. + for i := range 5 { + _, conf, err := ech.NewConfig(uint8(i), []byte(fmt.Sprintf("unknown-%d.example.net", i))) + if err != nil { + t.Fatalf("ech.NewConfig: %v", err) + } + configList, err := ech.ConfigList([]ech.Config{conf}) + if err != nil { + t.Fatalf("ech.ConfigList: %v", err) + } + conn, err := tls.Dial("tcp", proxy.listener.Addr().String(), &tls.Config{ + ServerName: "local.example.com", + RootCAs: extCA.RootCACertPool(), + EncryptedClientHelloConfigList: configList, + }) + if err == nil { + conn.Close() + t.Fatalf("tls.Dial: unexpected success") + } + } + + proxy.eventsmu.Lock() + defer proxy.eventsmu.Unlock() + for k := range proxy.events { + if strings.Contains(k, "unknown-") { + t.Errorf("unexpected event %q", k) + } + } +} diff --git a/proxy/proxy.go b/proxy/proxy.go index c51fcf40..d7473f82 100644 --- a/proxy/proxy.go +++ b/proxy/proxy.go @@ -1333,10 +1333,7 @@ func (p *Proxy) handleConnection(conn *netw.Conn) { } conn.Conn = echConn if echConn.ECHAccepted() { - p.recordEvent("encrypted client hello accepted " + idnaToUnicode(echConn.ServerName())) conn.SetAnnotation(echAcceptedKey, true) - } else if echConn.ECHPresented() { - p.recordEvent("encrypted client hello rejected " + idnaToUnicode(echConn.ServerName())) } serverName := echConn.ServerName() if serverName == "" { @@ -1352,6 +1349,13 @@ func (p *Proxy) handleConnection(conn *netw.Conn) { sendUnrecognizedName(conn) return } + // Record ECH events only after the server name is known to be valid. + // Otherwise, clients could create any number of distinct events. + if echConn.ECHAccepted() { + p.recordEvent("encrypted client hello accepted " + idnaToUnicode(serverName)) + } else if echConn.ECHPresented() { + p.recordEvent("encrypted client hello rejected " + idnaToUnicode(serverName)) + } conn.SetAnnotation(backendKey, be) be.incInFlight(1) p.setCounters(conn, serverName) From 281a3aa5a101070f69fb655f57b4625cde380358 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:41:00 -0700 Subject: [PATCH 14/54] Strip spoofable forwarding and identity headers Only the exact names of proxy-owned headers were removed from client requests. Backends that map header names to CGI-style variables treat X_tlsproxy_user_id the same as X-tlsproxy-user-id, so a client could spoof the user identity, or any header set with forwardHttpHeaders. Client-supplied X-Forwarded-Host, X-Forwarded-Proto and Forwarded headers were also forwarded as is. Remove all case and underscore variants of the identity header, the forwardHttpHeaders keys, X-Forwarded-For and X-Forwarded-Client-Cert. Remove client-supplied Forwarded, X-Forwarded-Host and X-Forwarded-Proto, and set X-Forwarded-Host and X-Forwarded-Proto. They can still be overridden with forwardHttpHeaders. Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- SECURITY-TODO.md | 7 ++- proxy/backend-http.go | 19 ++++++-- proxy/backend-http_test.go | 96 ++++++++++++++++++++++++++++++++++++++ proxy/backend-sso.go | 2 +- proxy/util.go | 18 +++++++ 6 files changed, 136 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 485d8bf5..e373f904 100644 --- a/README.md +++ b/README.md @@ -236,7 +236,7 @@ Each `Backend` object defines a service and its behavior: * `forwardServerName`: (Optional) String. ServerName to send in TLS handshake with backend. * `forwardRootCAs`: (Optional) List of strings. CA names or PEM-encoded certificates for backend verification. * `forwardTimeout`: (Optional) Duration. Connection timeout to backend servers. -* `forwardHttpHeaders`: (Optional) Map of strings. HTTP headers to add to forwarded requests. +* `forwardHttpHeaders`: (Optional) Map of strings. HTTP headers to add to forwarded requests. Headers with the same names sent by clients are removed, including variants with `_` instead of `-`. `X-Forwarded-For`, `X-Forwarded-Host`, and `X-Forwarded-Proto` are always set by the proxy, and `Forwarded` is removed. They can be overridden here. * `forwardECH`: (Optional) Object. ECH parameters for connecting to the backend. * `pathOverrides`: (Optional) List of `PathOverride` objects. Defines different backend parameters for specific path prefixes. * `proxyProtocolVersion`: (Optional) String. Enables PROXY protocol on this backend (`v1` or `v2`). diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index ee33383d..1b2e0b4c 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -151,7 +151,12 @@ deleted when every item is resolved. ## Low -- [ ] **14. Spoofable forwarding/identity headers** (Low) +- [x] **14. Spoofable forwarding/identity headers** (Low) + *Fixed: `delHeaderVariants` strips `_`/`-` and case variants of the + identity header, `forwardHttpHeaders` keys, XFF and XFCC. Client + `Forwarded`/`X-Forwarded-Host`/`X-Forwarded-Proto` are removed; the proxy + now sets `X-Forwarded-Host` and `X-Forwarded-Proto: https` (overridable + via `forwardHttpHeaders`).* `proxy/backend-sso.go`, `proxy/backend-http.go` `reverseProxyDirector`: `X_tlsproxy_user_id` (underscore variant) and underscore variants of `forwardHttpHeaders` keys aren't stripped (CGI-style backends map `_`/`-` to diff --git a/proxy/backend-http.go b/proxy/backend-http.go index c24ac821..6603584c 100644 --- a/proxy/backend-http.go +++ b/proxy/backend-http.go @@ -55,9 +55,12 @@ const ( hstsHeader = "Strict-Transport-Security" hstsValue = "max-age=2592000" // 30 days - viaHeader = "Via" - hostHeader = "Host" - xForwardedForHeader = "X-Forwarded-For" + viaHeader = "Via" + hostHeader = "Host" + xForwardedForHeader = "X-Forwarded-For" + xForwardedHostHeader = "X-Forwarded-Host" + xForwardedProtoHeader = "X-Forwarded-Proto" + forwardedHeader = "Forwarded" ) type ctxURLKeyType int @@ -331,7 +334,13 @@ func (be *Backend) reverseProxy() http.Handler { if sanitizePath { req.URL.Path = cleanPath } + delHeaderVariants(req.Header, forwardedHeader) + delHeaderVariants(req.Header, xForwardedHostHeader) + delHeaderVariants(req.Header, xForwardedProtoHeader) + req.Header.Set(xForwardedHostHeader, req.Host) + req.Header.Set(xForwardedProtoHeader, "https") for k, v := range httpHeaders { + delHeaderVariants(req.Header, k) v = expandVars(v, req) if v != "" { req.Header.Set(k, v) @@ -509,8 +518,8 @@ func (be *Backend) handleLocalEndpointsAndAuthorize(w http.ResponseWriter, req * } func (be *Backend) reverseProxyDirector(req *http.Request) { - req.Header.Del(xForwardedForHeader) - req.Header.Del(xFCCHeader) + delHeaderVariants(req.Header, xForwardedForHeader) + delHeaderVariants(req.Header, xFCCHeader) if req.TLS != nil && len(req.TLS.PeerCertificates) > 0 && be.ClientAuth != nil && len(be.ClientAuth.AddClientCertHeader) > 0 { addXFCCHeader(req, be.ClientAuth.AddClientCertHeader) } diff --git a/proxy/backend-http_test.go b/proxy/backend-http_test.go index 16a171b9..e94c3b4e 100644 --- a/proxy/backend-http_test.go +++ b/proxy/backend-http_test.go @@ -25,8 +25,10 @@ package proxy import ( "context" + "fmt" "net" "net/http" + "slices" "strings" "testing" @@ -186,3 +188,97 @@ func TestMisdirectedRequest(t *testing.T) { } } } + +func TestForwardedHeaders(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + extCA, err := certmanager.New("root-ca.example.com", t.Logf) + if err != nil { + t.Fatalf("certmanager.New: %v", err) + } + + // A backend that returns all the request headers. + l, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("net.Listen: %v", err) + } + beServer := &http.Server{ + Handler: http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + var lines []string + for k, v := range req.Header { + lines = append(lines, fmt.Sprintf("%s: %s", k, strings.Join(v, ","))) + } + slices.Sort(lines) + fmt.Fprintln(w, strings.Join(lines, "\n")) + }), + } + go beServer.Serve(l) + defer beServer.Close() + + proxy := newTestProxy( + &Config{ + HTTPAddr: newPtr("localhost:0"), + TLSAddr: newPtr("localhost:0"), + CacheDir: newPtr(t.TempDir()), + MaxOpen: newPtr(100), + Backends: []*Backend{ + { + ServerNames: Strings{"http.example.com"}, + Mode: "HTTP", + Addresses: Strings{l.Addr().String()}, + ForwardHTTPHeaders: map[string]string{ + "X-Server-Name": "${SERVER_NAME}", + }, + }, + }, + }, + extCA, + ) + if err := proxy.Start(ctx); err != nil { + t.Fatalf("proxy.Start: %v", err) + } + defer proxy.Stop() + + msg := "GET / HTTP/1.1\r\n" + + "Host: http.example.com\r\n" + + "X_tlsproxy_user_id: admin@example.com\r\n" + + "x-tlsproxy-user-id: admin@example.com\r\n" + + "X_Server_Name: evil.example.com\r\n" + + "X_Forwarded_For: 1.2.3.4\r\n" + + "X-Forwarded-Host: evil.example.com\r\n" + + "X-Forwarded-Proto: http\r\n" + + "Forwarded: for=1.2.3.4\r\n" + + "X-Other: foo\r\n" + + "Connection: close\r\n\r\n" + got, _, err := tlsGet("http.example.com", proxy.listener.Addr().String(), msg, extCA, nil, []string{"http/1.1"}) + if err != nil { + t.Fatalf("tlsGet: %v", err) + } + _, body, _ := strings.Cut(got, "\r\n\r\n") + headers := make(map[string]string) + for _, line := range strings.Split(strings.TrimSpace(body), "\n") { + if k, v, ok := strings.Cut(line, ": "); ok { + headers[k] = v + } + } + for k, want := range map[string]string{ + "X-Server-Name": "http.example.com", + "X-Forwarded-Host": "http.example.com", + "X-Forwarded-Proto": "https", + "X-Other": "foo", + } { + if got := headers[k]; got != want { + t.Errorf("%s = %q, want %q", k, got, want) + } + } + for _, k := range []string{"X_tlsproxy_user_id", "X-Tlsproxy-User-Id", "X_server_name", "X_forwarded_for", "Forwarded"} { + if v, ok := headers[k]; ok { + t.Errorf("unexpected header %s: %q", k, v) + } + } + if got := headers["X-Forwarded-For"]; got == "" || strings.Contains(got, "1.2.3.4") { + t.Errorf("X-Forwarded-For = %q", got) + } + t.Logf("Headers:\n%s", body) +} diff --git a/proxy/backend-sso.go b/proxy/backend-sso.go index 874299c0..dba356f4 100644 --- a/proxy/backend-sso.go +++ b/proxy/backend-sso.go @@ -80,7 +80,7 @@ func init() { // available. It modifies the request headers and context. // It returns true if processing of the request should continue. func (be *Backend) authenticateUser(w http.ResponseWriter, req **http.Request) bool { - (*req).Header.Del(xTLSProxyUserIDHeader) + delHeaderVariants((*req).Header, xTLSProxyUserIDHeader) if be.SSO != nil { claims, tokenHash, cont := be.checkCookies(w, *req) if !cont { diff --git a/proxy/util.go b/proxy/util.go index 72217887..d67b12c7 100644 --- a/proxy/util.go +++ b/proxy/util.go @@ -29,6 +29,8 @@ import ( "fmt" "io" "net" + "net/http" + "strings" "github.com/pires/go-proxyproto" "golang.org/x/net/idna" @@ -207,3 +209,19 @@ func quicStream(c anyConn) (*netw.QUICStream, bool) { return nil, false } } + +// delHeaderVariants deletes all the headers whose name matches name, ignoring +// case and treating '_' and '-' as the same character. Some backends, e.g. +// CGI, map both X-Foo-Bar and X_Foo_Bar to the same variable. +func delHeaderVariants(h http.Header, name string) { + want := normalizeHeaderName(name) + for k := range h { + if normalizeHeaderName(k) == want { + delete(h, k) + } + } +} + +func normalizeHeaderName(name string) string { + return strings.ToLower(strings.ReplaceAll(name, "_", "-")) +} From e29af4d67fd0da27d6906465eb254f28e1008658 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:42:14 -0700 Subject: [PATCH 15/54] Check local handler scopes when no SSO rule matches enforceSSOPolicy returned early when no SSO rule matched the request path, without checking the local handler's scopes. A token with limited scope, e.g. an OIDC access token with only the openid scope, could then use PKI or SSH CA endpoints when the backend's SSO rules didn't cover their paths. When no rule matches, still check the handler's scopes for authenticated requests. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 4 +++- proxy/backend-sso.go | 5 +++++ proxy/backend-sso_test.go | 23 +++++++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 1b2e0b4c..33fd94b1 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -165,7 +165,9 @@ deleted when every item is resolved. Fix: drop inbound headers that normalize to a proxy-owned header name; strip/set `X-Forwarded-Host`, `X-Forwarded-Proto`, `Forwarded`. -- [ ] **15. Local handler scopes skipped when no SSO rule matches** (Low) +- [x] **15. Local handler scopes skipped when no SSO rule matches** (Low) + *Fixed: when no rule matches, a handler's scopes are still checked for + authenticated requests (unauthenticated behavior unchanged).* `proxy/backend-sso.go` `enforceSSOPolicy` returns true when `rule == nil` before checking `overrideScopes`. PKI/SSH/OIDC endpoints then accept tokens without the `pki`/`ssh` scope when rules don't cover their paths. diff --git a/proxy/backend-sso.go b/proxy/backend-sso.go index dba356f4..a0b2dab6 100644 --- a/proxy/backend-sso.go +++ b/proxy/backend-sso.go @@ -364,6 +364,11 @@ func (be *Backend) enforceSSOPolicy(w http.ResponseWriter, req *http.Request, ov } rule := be.findSSORule(req) if rule == nil { + // No rule applies, but the scopes of local handlers still + // apply to authenticated users. + if overrideScopes != nil && fromctx.Claims(req.Context()) != nil { + return be.checkScopes(overrideScopes, w, req) + } return true } claims := fromctx.Claims(req.Context()) diff --git a/proxy/backend-sso_test.go b/proxy/backend-sso_test.go index fb7f022d..81f35c53 100644 --- a/proxy/backend-sso_test.go +++ b/proxy/backend-sso_test.go @@ -317,6 +317,29 @@ func TestEnforceSSOPolicy(t *testing.T) { t.Fatalf("response code = %d, want %d", got, want) } + // No rule matches, but the local handler's scopes still apply. + proxy.cfg.Backends[0].SSO.Rules = []*SSORule{{ + Paths: Strings{"/bar"}, + }} + for _, tc := range []struct { + scope any + want bool + }{ + {nil, true}, + {[]any{"pki"}, true}, + {[]any{"openid"}, false}, + } { + claims := jwt.MapClaims{"email": "bob@example.org"} + if tc.scope != nil { + claims["scope"] = tc.scope + } + scopeReq := req.WithContext(fromctx.WithClaims(req.Context(), claims)) + w = httptest.NewRecorder() + if got := proxy.cfg.Backends[0].enforceSSOPolicy(w, scopeReq, Strings{"pki"}); got != tc.want { + t.Errorf("enforceSSOPolicy(scope=%v) = %v, want %v", tc.scope, got, tc.want) + } + } + // Dot-segments can't be used to match an exception or a more // permissive rule. proxy.cfg.Backends[0].SSO.Rules = []*SSORule{ From 02e2ced9d1a6100a87d8a1b8f1aa828d301a55f2 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:45:01 -0700 Subject: [PATCH 16/54] Bound pending login state in identity providers The OIDC client, SAML provider and passkeys manager kept pending login requests in maps that were only pruned when a callback was handled, and each callback scanned the whole map. Unauthenticated clients could add entries, including an arbitrarily long return URL, without limit. Add idp.PendingLogins, which checks expiration on lookup, prunes expired entries periodically when new ones are added, and limits the number of entries. Use it for the OIDC client and SAML states, and for passkey nonces and challenges. Also reject return URLs longer than 4096 bytes. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 6 ++- proxy/internal/idp/idp.go | 70 ++++++++++++++++++++++++++++ proxy/internal/idp/idp_test.go | 75 ++++++++++++++++++++++++++++++ proxy/internal/oidc/client.go | 27 ++++++----- proxy/internal/passkeys/manager.go | 61 +++++++++++------------- proxy/internal/saml/saml.go | 27 +++++------ 6 files changed, 205 insertions(+), 61 deletions(-) create mode 100644 proxy/internal/idp/idp_test.go diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 33fd94b1..83974201 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -173,7 +173,11 @@ deleted when every item is resolved. without the `pki`/`ssh` scope when rules don't cover their paths. Fix: still check `overrideScopes` when no rule matches. -- [ ] **16. Unbounded login-state maps in OIDC RP, SAML and passkeys** (Low) +- [x] **16. Unbounded login-state maps in OIDC RP, SAML and passkeys** (Low) + *Fixed: new `idp.PendingLogins` (expiry checked on lookup, pruned on + insert every 30s or when full, max 50000 entries) used by the OIDC RP, + SAML, and passkeys (nonces and challenges). Return URLs over 4096 bytes + are rejected.* `oidc/client.go`, `saml/saml.go`, `passkeys/manager.go`: state (including an attacker-sized `OriginalURL`) is only expired inside `HandleCallback`. Fix: periodic expiry and a size cap. diff --git a/proxy/internal/idp/idp.go b/proxy/internal/idp/idp.go index 6a0ba14d..4cf69786 100644 --- a/proxy/internal/idp/idp.go +++ b/proxy/internal/idp/idp.go @@ -23,6 +23,8 @@ package idp +import "time" + type LoginOptions struct { loginHint string selectAccount bool @@ -68,3 +70,71 @@ func ApplyOptions(opts []Option) LoginOptions { } return lo } + +const ( + // MaxPendingLogins is the maximum number of pending login requests + // that an identity provider keeps track of. + MaxPendingLogins = 50000 + // MaxOriginalURLLength is the maximum length of the URL that users + // are sent back to after logging in. + MaxOriginalURLLength = 4096 + // pruneInterval is how often pending login requests are pruned. + pruneInterval = 30 * time.Second +) + +// PendingLogins keeps track of pending login requests, with a limited lifetime +// and size. It is not safe for concurrent use. +type PendingLogins[V any] struct { + ttl time.Duration + created func(V) time.Time + m map[string]V + lastPrune time.Time +} + +// NewPendingLogins returns a new PendingLogins. Entries expire ttl after the +// time returned by created. +func NewPendingLogins[V any](ttl time.Duration, created func(V) time.Time) *PendingLogins[V] { + return &PendingLogins[V]{ + ttl: ttl, + created: created, + m: make(map[string]V), + } +} + +func (p *PendingLogins[V]) expired(v V) bool { + return time.Since(p.created(v)) > p.ttl +} + +// Add adds a new entry. It returns false if there are too many pending +// requests. +func (p *PendingLogins[V]) Add(key string, v V) bool { + if time.Since(p.lastPrune) > pruneInterval || len(p.m) >= MaxPendingLogins { + p.lastPrune = time.Now() + for k, v := range p.m { + if p.expired(v) { + delete(p.m, k) + } + } + } + if len(p.m) >= MaxPendingLogins { + return false + } + p.m[key] = v + return true +} + +// Get returns the entry for key, if it exists and isn't expired. +func (p *PendingLogins[V]) Get(key string) (V, bool) { + v, ok := p.m[key] + if ok && p.expired(v) { + delete(p.m, key) + var zero V + return zero, false + } + return v, ok +} + +// Delete deletes the entry for key. +func (p *PendingLogins[V]) Delete(key string) { + delete(p.m, key) +} diff --git a/proxy/internal/idp/idp_test.go b/proxy/internal/idp/idp_test.go new file mode 100644 index 00000000..2428345d --- /dev/null +++ b/proxy/internal/idp/idp_test.go @@ -0,0 +1,75 @@ +// MIT License +// +// Copyright (c) 2026 TTBT Enterprises LLC +// Copyright (c) 2026 Robin Thellend +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in all +// copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +// SOFTWARE. + +package idp + +import ( + "fmt" + "testing" + "time" +) + +func TestPendingLogins(t *testing.T) { + p := NewPendingLogins(5*time.Minute, func(t time.Time) time.Time { return t }) + + now := time.Now() + if !p.Add("fresh", now) { + t.Fatal("Add(fresh) = false") + } + if !p.Add("old", now.Add(-10*time.Minute)) { + t.Fatal("Add(old) = false") + } + if _, ok := p.Get("fresh"); !ok { + t.Error("Get(fresh) = false") + } + if _, ok := p.Get("old"); ok { + t.Error("Get(old) = true") + } + p.Delete("fresh") + if _, ok := p.Get("fresh"); ok { + t.Error("Get(fresh) after Delete = true") + } + + // Expired entries are pruned when the limit is reached. + for i := range MaxPendingLogins { + if !p.Add(fmt.Sprintf("old-%d", i), now.Add(-10*time.Minute)) { + t.Fatalf("Add(old-%d) = false", i) + } + } + if !p.Add("new", now) { + t.Fatal("Add(new) = false") + } + if got := len(p.m); got != 1 { + t.Errorf("len = %d, want 1", got) + } + + // Fresh entries are not. + for i := 1; i < MaxPendingLogins; i++ { + if !p.Add(fmt.Sprintf("new-%d", i), now) { + t.Fatalf("Add(new-%d) = false", i) + } + } + if p.Add("one-too-many", now) { + t.Error("Add(one-too-many) = true") + } +} diff --git a/proxy/internal/oidc/client.go b/proxy/internal/oidc/client.go index efe32e4a..22398bdb 100644 --- a/proxy/internal/oidc/client.go +++ b/proxy/internal/oidc/client.go @@ -94,7 +94,7 @@ type ProviderClient struct { er EventRecorder mu sync.Mutex - states map[string]*oauthState + states *idp.PendingLogins[*oauthState] } type oauthState struct { @@ -112,7 +112,7 @@ func New(cfg Config, er EventRecorder, cm CookieManager) (*ProviderClient, error cfg: cfg, cm: cm, er: er, - states: make(map[string]*oauthState), + states: idp.NewPendingLogins(5*time.Minute, func(s *oauthState) time.Time { return s.Created }), } if p.cfg.DiscoveryURL != "" { resp, err := http.Get(p.cfg.DiscoveryURL) @@ -149,6 +149,10 @@ func New(cfg Config, er EventRecorder, cm CookieManager) (*ProviderClient, error func (p *ProviderClient) RequestLogin(w http.ResponseWriter, req *http.Request, originalURL string, opts ...idp.Option) { loginOptions := idp.ApplyOptions(opts) + if len(originalURL) > idp.MaxOriginalURLLength { + http.Error(w, "URL too long", http.StatusRequestURITooLong) + return + } ou, err := url.Parse(originalURL) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) @@ -168,14 +172,18 @@ func (p *ProviderClient) RequestLogin(w http.ResponseWriter, req *http.Request, codeVerifierStr := base64.RawURLEncoding.EncodeToString(codeVerifier[:]) cvh := sha256.Sum256([]byte(codeVerifierStr)) p.mu.Lock() - p.states[nonceStr] = &oauthState{ + added := p.states.Add(nonceStr, &oauthState{ Created: time.Now(), OriginalURL: originalURL, Host: ou.Host, CodeVerifier: codeVerifierStr, Depth: loginOptions.Depth(), - } + }) p.mu.Unlock() + if !added { + http.Error(w, "too many pending login requests", http.StatusServiceUnavailable) + return + } scopes := p.cfg.Scopes if len(scopes) == 0 { scopes = []string{"openid", "email"} @@ -208,13 +216,8 @@ func (p *ProviderClient) HandleCallback(w http.ResponseWriter, req *http.Request req.ParseForm() p.mu.Lock() - for k, v := range p.states { - if time.Since(v.Created) > 5*time.Minute { - delete(p.states, k) - } - } nonce := req.Form.Get("state") - state, ok := p.states[nonce] + state, ok := p.states.Get(nonce) invalid := !ok || state.Seen || nonce != p.cm.Nonce(w, req) if ok { state.Seen = true @@ -306,8 +309,8 @@ func (p *ProviderClient) HandleCallback(w http.ResponseWriter, req *http.Request claims.Nonce = nonce } p.mu.Lock() - state, ok = p.states[claims.Nonce] - delete(p.states, claims.Nonce) + state, ok = p.states.Get(claims.Nonce) + p.states.Delete(claims.Nonce) p.mu.Unlock() if !ok { p.er.Record("invalid nonce") diff --git a/proxy/internal/passkeys/manager.go b/proxy/internal/passkeys/manager.go index 0248a5d7..ed544fc1 100644 --- a/proxy/internal/passkeys/manager.go +++ b/proxy/internal/passkeys/manager.go @@ -122,8 +122,8 @@ func NewManager(cfg Config) (*Manager, error) { } m := &Manager{ cfg: cfg, - challenges: make(map[string]*challenge), - nonces: make(map[string]*nonceData), + challenges: idp.NewPendingLogins(5*time.Minute, func(c *challenge) time.Time { return c.created }), + nonces: idp.NewPendingLogins(5*time.Minute, func(n *nonceData) time.Time { return n.created }), } m.db.Handles = make(map[string]*user) m.db.Subjects = make(map[string]string) @@ -143,10 +143,10 @@ type Manager struct { acl *[]string mu sync.Mutex - challenges map[string]*challenge + challenges *idp.PendingLogins[*challenge] noncesMu sync.Mutex - nonces map[string]*nonceData + nonces *idp.PendingLogins[*nonceData] } type challenge struct { @@ -180,17 +180,6 @@ func (m *Manager) SetACL(acl *[]string) { } } -func (m *Manager) vacuum() { - m.mu.Lock() - defer m.mu.Unlock() - now := time.Now().UTC() - for k, v := range m.challenges { - if v.created.Add(5 * time.Minute).Before(now) { - delete(m.challenges, k) - } - } -} - // ServeWellKnown serves a list of passkey endpoints. // https://github.com/ms-id-standards/MSIdentityStandardsExplainers/blob/main/PasskeyEndpointsWellKnownUrl/explainer.md#proposed-solution func (m *Manager) ServeWellKnown(w http.ResponseWriter, req *http.Request) { @@ -222,6 +211,10 @@ func (m *Manager) ServeWellKnown(w http.ResponseWriter, req *http.Request) { func (m *Manager) RequestLogin(w http.ResponseWriter, req *http.Request, origURL string, opts ...idp.Option) { m.cfg.EventRecorder.Record("passkey auth request") + if len(origURL) > idp.MaxOriginalURLLength { + http.Error(w, "URL too long", http.StatusRequestURITooLong) + return + } n := make([]byte, 16) if _, err := io.ReadFull(rand.Reader, n); err != nil { @@ -237,12 +230,16 @@ func (m *Manager) RequestLogin(w http.ResponseWriter, req *http.Request, origURL } m.noncesMu.Lock() - m.nonces[nonce] = &nonceData{ + added := m.nonces.Add(nonce, &nonceData{ created: time.Now().UTC(), origURL: ou, opts: idp.ApplyOptions(opts), - } + }) m.noncesMu.Unlock() + if !added { + http.Error(w, "too many pending login requests", http.StatusServiceUnavailable) + return + } u, err := url.Parse(m.cfg.Endpoint) if err != nil { @@ -273,14 +270,8 @@ func (m *Manager) HandleCallback(w http.ResponseWriter, req *http.Request) { nonce := req.Form.Get("nonce") m.noncesMu.Lock() - now := time.Now().UTC() - for k, v := range m.nonces { - if v.created.Add(5 * time.Minute).Before(now) { - delete(m.nonces, k) - } - } - nData, ok := m.nonces[nonce] - delete(m.nonces, nonce) + nData, ok := m.nonces.Get(nonce) + m.nonces.Delete(nonce) m.noncesMu.Unlock() if ok { @@ -762,7 +753,6 @@ func (m *Manager) setAuthToken(w http.ResponseWriter, req *http.Request, u *url. } func (m *Manager) attestationOptions(claims map[string]any) (*AttestationOptions, error) { - m.vacuum() opts, err := newAttestationOptions() if err != nil { return nil, err @@ -803,16 +793,17 @@ func (m *Manager) attestationOptions(claims map[string]any) (*AttestationOptions opts.User.ID = uid } - m.challenges[base64.RawURLEncoding.EncodeToString(opts.Challenge)] = &challenge{ + if !m.challenges.Add(base64.RawURLEncoding.EncodeToString(opts.Challenge), &challenge{ created: time.Now().UTC(), claims: claims, uid: opts.User.ID, + }) { + return nil, errors.New("too many pending requests") } return opts, nil } func (m *Manager) processAttestation(claims map[string]any, host, jsargs string, allowNewKey bool) (newClaims map[string]any, retErr error) { - m.vacuum() email, ok := claims["email"].(string) if !ok { return nil, errors.New("invalid email") @@ -839,8 +830,8 @@ func (m *Manager) processAttestation(claims map[string]any, host, jsargs string, } m.mu.Lock() defer m.mu.Unlock() - challenge, ok := m.challenges[cd.Challenge] - delete(m.challenges, cd.Challenge) + challenge, ok := m.challenges.Get(cd.Challenge) + m.challenges.Delete(cd.Challenge) if !ok || !reflect.DeepEqual(challenge.claims, claims) { return nil, errors.New("invalid challenge") @@ -907,15 +898,16 @@ func (m *Manager) processAttestation(claims map[string]any, host, jsargs string, } func (m *Manager) assertionOptions(email string) (*AssertionOptions, error) { - m.vacuum() opts, err := newAssertionOptions() if err != nil { return nil, err } m.mu.Lock() defer m.mu.Unlock() - m.challenges[base64.RawURLEncoding.EncodeToString(opts.Challenge)] = &challenge{ + if !m.challenges.Add(base64.RawURLEncoding.EncodeToString(opts.Challenge), &challenge{ created: time.Now().UTC(), + }) { + return nil, errors.New("too many pending requests") } if h, ok := m.db.Subjects[email]; ok { if u, ok := m.db.Handles[h]; ok { @@ -940,7 +932,6 @@ func (m *Manager) assertionOptions(email string) (*AssertionOptions, error) { } func (m *Manager) processAssertion(jsargs string, token *jwt.Token) (claims map[string]any, retErr error) { - m.vacuum() var args struct { ID string `json:"id"` ClientDataJSON Bytes `json:"clientDataJSON"` @@ -972,10 +963,10 @@ func (m *Manager) processAssertion(jsargs string, token *jwt.Token) (claims map[ m.mu.Lock() defer m.mu.Unlock() - if _, ok := m.challenges[cd.Challenge]; !ok { + if _, ok := m.challenges.Get(cd.Challenge); !ok { return nil, errors.New("invalid challenge") } - delete(m.challenges, cd.Challenge) + m.challenges.Delete(cd.Challenge) commit, err := m.cfg.Store.OpenForUpdate(passkeyFile, &m.db) if err != nil { diff --git a/proxy/internal/saml/saml.go b/proxy/internal/saml/saml.go index 02f4fb27..abeca8b5 100644 --- a/proxy/internal/saml/saml.go +++ b/proxy/internal/saml/saml.go @@ -75,7 +75,7 @@ type Provider struct { dsigCtx *dsig.ValidationContext mu sync.Mutex - states map[string]*samlState + states *idp.PendingLogins[*samlState] } type samlState struct { @@ -97,7 +97,7 @@ func New(cfg Config, er EventRecorder, cm CookieManager) (*Provider, error) { er: er, cm: cm, dsigCtx: dsigCtx, - states: make(map[string]*samlState), + states: idp.NewPendingLogins(5*time.Minute, func(s *samlState) time.Time { return s.Created }), } if _, err := url.Parse(cfg.SSOURL); err != nil { return nil, fmt.Errorf("SSOURL: %v", err) @@ -109,6 +109,10 @@ func New(cfg Config, er EventRecorder, cm CookieManager) (*Provider, error) { } func (p *Provider) RequestLogin(w http.ResponseWriter, req *http.Request, origURL string, opts ...idp.Option) { + if len(origURL) > idp.MaxOriginalURLLength { + http.Error(w, "URL too long", http.StatusRequestURITooLong) + return + } ou, err := url.Parse(origURL) if err != nil { http.Error(w, err.Error(), http.StatusInternalServerError) @@ -121,12 +125,16 @@ func (p *Provider) RequestLogin(w http.ResponseWriter, req *http.Request, origUR } idStr := hex.EncodeToString(id[:]) p.mu.Lock() - p.states[idStr] = &samlState{ + added := p.states.Add(idStr, &samlState{ Created: time.Now(), OriginalURL: origURL, Host: ou.Host, - } + }) p.mu.Unlock() + if !added { + http.Error(w, "too many pending login requests", http.StatusServiceUnavailable) + return + } authReq := &samlAuthnRequest{ XMLName: xml.Name{Local: "samlp:samlAuthnRequest"}, @@ -193,15 +201,8 @@ func (p *Provider) HandleCallback(w http.ResponseWriter, req *http.Request) { id := findElementAttr(v, "./Subject/SubjectConfirmation/SubjectConfirmationData", "InResponseTo") p.mu.Lock() - for k, v := range p.states { - if time.Since(v.Created) > 5*time.Minute { - delete(p.states, k) - } - } - state, ok := p.states[id] - if ok { - delete(p.states, id) - } + state, ok := p.states.Get(id) + p.states.Delete(id) p.mu.Unlock() if nonce := p.cm.SAMLNonce(w, req); !ok || nonce == "" || subtle.ConstantTimeCompare([]byte(nonce), []byte(id)) != 1 { From 6346ae0e8e60e4047021599ec38c6208d1251f81 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:47:01 -0700 Subject: [PATCH 17/54] Check clientData.origin in passkey assertions Passkey registration checked the origin in the client data, but login assertions didn't. A page on another origin that can use the same RP ID, e.g. a subdomain, could get an assertion from the user's authenticator and use it to log in as the user. Require the origin to be the passkey endpoint's origin. Also make the fake authenticator use its configured origin for assertions. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 4 +- proxy/internal/passkeys/manager.go | 8 +++- proxy/internal/passkeys/testutils.go | 2 +- proxy/sso-passkeys_test.go | 67 +++++++++++++++------------- 4 files changed, 47 insertions(+), 34 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 83974201..c8a81ff3 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -182,7 +182,9 @@ deleted when every item is resolved. attacker-sized `OriginalURL`) is only expired inside `HandleCallback`. Fix: periodic expiry and a size cap. -- [ ] **17. Passkey assertion doesn't check `clientData.origin`** (Low) +- [x] **17. Passkey assertion doesn't check `clientData.origin`** (Low) + *Fixed: `processAssertion` requires `https://`, like + registration.* `proxy/internal/passkeys/manager.go` `processAssertion` (registration does check it). A subdomain page can obtain an assertion for the RP ID. Fix: require the expected origin. diff --git a/proxy/internal/passkeys/manager.go b/proxy/internal/passkeys/manager.go index ed544fc1..e85d316d 100644 --- a/proxy/internal/passkeys/manager.go +++ b/proxy/internal/passkeys/manager.go @@ -416,7 +416,7 @@ func (m *Manager) HandleCallback(w http.ResponseWriter, req *http.Request) { http.Error(w, "invalid request", http.StatusBadRequest) return } - claims, err := m.processAssertion(req.Form.Get("args"), token) + claims, err := m.processAssertion(req.Host, req.Form.Get("args"), token) if err != nil { m.cfg.Logger.Errorf("ERR processAssertion: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) @@ -931,7 +931,7 @@ func (m *Manager) assertionOptions(email string) (*AssertionOptions, error) { return opts, nil } -func (m *Manager) processAssertion(jsargs string, token *jwt.Token) (claims map[string]any, retErr error) { +func (m *Manager) processAssertion(host, jsargs string, token *jwt.Token) (claims map[string]any, retErr error) { var args struct { ID string `json:"id"` ClientDataJSON Bytes `json:"clientDataJSON"` @@ -950,6 +950,10 @@ func (m *Manager) processAssertion(jsargs string, token *jwt.Token) (claims map[ if cd.Type != "webauthn.get" { return nil, errors.New("unexpected clientData.type") } + if origin := "https://" + host; cd.Origin != origin { + m.cfg.Logger.Errorf("ERR cd.Origin: %q != %q", cd.Origin, origin) + return nil, errors.New("unexpected clientData.origin") + } var authData authenticatorData if err := parseAuthenticatorData(args.AuthenticatorData, &authData); err != nil { return nil, err diff --git a/proxy/internal/passkeys/testutils.go b/proxy/internal/passkeys/testutils.go index 1d11b348..09480cab 100644 --- a/proxy/internal/passkeys/testutils.go +++ b/proxy/internal/passkeys/testutils.go @@ -154,7 +154,7 @@ func (a *FakeAuthenticator) Get(options *AssertionOptions) (id []byte, clientDat cd := clientData{ Type: "webauthn.get", Challenge: base64.RawURLEncoding.EncodeToString(options.Challenge), - Origin: "https://example.com/", + Origin: a.origin, } if clientDataJSON, err = json.Marshal(cd); err != nil { return diff --git a/proxy/sso-passkeys_test.go b/proxy/sso-passkeys_test.go index 5508ca51..2c972ded 100644 --- a/proxy/sso-passkeys_test.go +++ b/proxy/sso-passkeys_test.go @@ -286,40 +286,47 @@ func TestSSOEnforcePasskey(t *testing.T) { token = m[1] t.Logf("TOKEN: %s", token) - // Get the assertion options. - code, body, _ = get("https://"+host+"/passkey?get=AssertionOptions&redirect="+token, nil, []byte{}) - if got, want := code, 200; got != want { - t.Errorf("Code = %v, want %v", got, want) - } - var aso passkeys.AssertionOptions - if err := json.Unmarshal([]byte(body), &aso); err != nil { - t.Fatalf("AssertionOptions: %v", err) - } - id, clientDataJSON, authData, signature, userHandle, err := auth.Get(&aso) - if err != nil { - t.Fatalf("auth.Get: %v", err) - } - data2 := struct { - ID string `json:"id"` - ClientDataJSON passkeys.Bytes `json:"clientDataJSON"` - AuthenticatorData passkeys.Bytes `json:"authenticatorData"` - Signature passkeys.Bytes `json:"signature"` - UserHandle passkeys.Bytes `json:"userHandle"` - }{ - ID: base64.RawURLEncoding.EncodeToString(id), - ClientDataJSON: clientDataJSON, - AuthenticatorData: authData, - Signature: signature, - UserHandle: userHandle, + check := func(origin string) (int, string) { + code, body, _ := get("https://"+host+"/passkey?get=AssertionOptions&redirect="+token, nil, []byte{}) + if got, want := code, 200; got != want { + t.Errorf("Code = %v, want %v", got, want) + } + var aso passkeys.AssertionOptions + if err := json.Unmarshal([]byte(body), &aso); err != nil { + t.Fatalf("AssertionOptions: %v", err) + } + auth.SetOrigin(origin) + id, clientDataJSON, authData, signature, userHandle, err := auth.Get(&aso) + if err != nil { + t.Fatalf("auth.Get: %v", err) + } + data := struct { + ID string `json:"id"` + ClientDataJSON passkeys.Bytes `json:"clientDataJSON"` + AuthenticatorData passkeys.Bytes `json:"authenticatorData"` + Signature passkeys.Bytes `json:"signature"` + UserHandle passkeys.Bytes `json:"userHandle"` + }{ + ID: base64.RawURLEncoding.EncodeToString(id), + ClientDataJSON: clientDataJSON, + AuthenticatorData: authData, + Signature: signature, + UserHandle: userHandle, + } + dataJSON, _ := json.Marshal(data) + hdr := http.Header{} + hdr.Set("content-type", "application/x-www-form-urlencoded") + code, body, _ = get("https://"+host+"/passkey?get=Check&redirect="+token, hdr, []byte("args="+url.QueryEscape(string(dataJSON)))) + return code, body } - dataJSON, _ = json.Marshal(data2) - hdr = http.Header{} - hdr.Set("content-type", "application/x-www-form-urlencoded") - postBody = "args=" + url.QueryEscape(string(dataJSON)) + // An assertion for another origin is rejected. + if code, _ := check("https://evil.example.com"); code == 200 { + t.Errorf("Check with wrong origin: Code = %v", code) + } // Send the passkey assertion. - code, body, _ = get("https://"+host+"/passkey?get=Check&redirect="+token, hdr, []byte(postBody)) + code, body = check("https://" + host) if got, want := code, 200; got != want { t.Errorf("Code = %v, want %v", got, want) } From f2d49460f58e881da4303588f2494ce9903a7513 Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:48:43 -0700 Subject: [PATCH 18/54] Bind OIDC authorization requests to the requesting user The local OIDC server checked the client ACL and filtered scopes for the user who made the authorization request, but minted the tokens for whichever user approved the request ID. A user denied by the client's ACL could get tokens by approving another user's request. Record the requesting user with the pending request, and reject approval by any other user. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 5 ++- proxy/internal/oidc/server.go | 12 +++++ proxy/internal/oidc/server_test.go | 70 ++++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 1 deletion(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index c8a81ff3..82410bc7 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -189,7 +189,10 @@ deleted when every item is resolved. check it). A subdomain page can obtain an assertion for the RP ID. Fix: require the expected origin. -- [ ] **18. OIDC consent approval not bound to the user who started the request** (Low) +- [x] **18. OIDC consent approval not bound to the user who started the request** (Low) + *Fixed: the pending code stores the requesting user's email and sub; + approval by a different user is rejected. (ACL and scope filtering + done at GET time therefore apply to the approver.)* `proxy/internal/oidc/server.go`: `AuthorizeClient` and scope filtering run on the GET; the token is minted for whoever POSTs the `request_id`. Fix: store the user in `codeData`, require a match on POST, re-run diff --git a/proxy/internal/oidc/server.go b/proxy/internal/oidc/server.go index 4a380199..be5664d9 100644 --- a/proxy/internal/oidc/server.go +++ b/proxy/internal/oidc/server.go @@ -98,6 +98,8 @@ type codeData struct { created time.Time requestID string clientID string + email string + sub string redirectURI *url.URL nonce string state string @@ -293,6 +295,13 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req http.Error(w, "request expired", http.StatusBadRequest) return } + // The request must be approved by the same user who made it. + // The client ACL and scopes were checked for that user. + if sub, _ := userClaims["sub"].(string); data.email != email || data.sub != sub { + s.opts.EventRecorder.Record("openid auth request approved by wrong user for " + data.clientID) + http.Error(w, "request expired", http.StatusBadRequest) + return + } if req.Form.Get("approve") != "true" && !AutoApproveForTests { s.opts.EventRecorder.Record("denied openid auth request for " + data.clientID) http.Error(w, "request was denied", http.StatusForbidden) @@ -372,6 +381,7 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req } // GET + sub, _ := userClaims["sub"].(string) if rt := req.Form.Get("response_type"); rt != "code" { s.opts.Logger.Errorf("ERR ServeAuthorization: invalid response_type %q", rt) http.Error(w, "invalid response_type", http.StatusBadRequest) @@ -449,6 +459,8 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req created: time.Now().UTC(), clientID: clientID, requestID: requestID, + email: email, + sub: sub, redirectURI: ru, state: req.Form.Get("state"), nonce: req.Form.Get("nonce"), diff --git a/proxy/internal/oidc/server_test.go b/proxy/internal/oidc/server_test.go index 4d3faf41..030c47ec 100644 --- a/proxy/internal/oidc/server_test.go +++ b/proxy/internal/oidc/server_test.go @@ -24,9 +24,19 @@ package oidc import ( + "net/http" + "net/http/httptest" + "net/url" + "strings" "testing" + "github.com/c2FmZQ/storage" + "github.com/c2FmZQ/storage/crypto" jwt "github.com/golang-jwt/jwt/v5" + + "github.com/c2FmZQ/tlsproxy/proxy/internal/cookiemanager" + "github.com/c2FmZQ/tlsproxy/proxy/internal/fromctx" + "github.com/c2FmZQ/tlsproxy/proxy/internal/tokenmanager" ) func TestRewriteRules(t *testing.T) { @@ -87,3 +97,63 @@ func TestRewriteRules(t *testing.T) { t.Errorf("username2 = %q, want %q", got, want) } } + +func TestAuthorizationSameUser(t *testing.T) { + mk, err := crypto.CreateAESMasterKeyForTest() + if err != nil { + t.Fatalf("crypto.CreateAESMasterKeyForTest: %v", err) + } + tm, err := tokenmanager.New(storage.New(t.TempDir(), mk), nil, nil) + if err != nil { + t.Fatalf("tokenmanager.New: %v", err) + } + s := NewServer(ServerOptions{ + CookieManager: cookiemanager.New(tm, "idp", "example.com", "https://idp.example.com", 0, nil), + Clients: []Client{{ID: "client", RedirectURI: []string{"https://app.example.com/callback"}}}, + ACLMatcher: func([]string, string) bool { return true }, + GroupsForEmail: func(string) []string { return nil }, + EventRecorder: nopRecorder{}, + Logger: nopLogger{}, + }) + + withUser := func(req *http.Request, email string) *http.Request { + return req.WithContext(fromctx.WithClaims(req.Context(), jwt.MapClaims{"email": email, "sub": email})) + } + + // Alice makes the request. + args := url.Values{ + "response_type": {"code"}, + "client_id": {"client"}, + "redirect_uri": {"https://app.example.com/callback"}, + "scope": {"openid email"}, + } + req := withUser(httptest.NewRequest(http.MethodGet, "https://idp.example.com/authorization?"+args.Encode(), nil), "alice@example.com") + w := httptest.NewRecorder() + s.ServeAuthorization(w, req) + if w.Code != http.StatusOK { + t.Fatalf("GET: code %d", w.Code) + } + var requestID string + s.mu.Lock() + for _, v := range s.codes { + requestID = v.requestID + } + s.mu.Unlock() + + approve := func(email string) int { + form := url.Values{"request_id": {requestID}, "approve": {"true"}} + req := httptest.NewRequest(http.MethodPost, "https://idp.example.com/authorization", strings.NewReader(form.Encode())) + req.Header.Set("content-type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.ServeAuthorization(w, withUser(req, email)) + return w.Code + } + // Bob can't approve it. + if got, want := approve("bob@example.com"), http.StatusBadRequest; got != want { + t.Errorf("approve(bob) = %d, want %d", got, want) + } + // Alice can. + if got, want := approve("alice@example.com"), http.StatusOK; got != want { + t.Errorf("approve(alice) = %d, want %d", got, want) + } +} From 473a749b84ef5a47ed15699dcf92d463ba9c904f Mon Sep 17 00:00:00 2001 From: Robin Thellend Date: Tue, 6 Oct 2026 23:50:46 -0700 Subject: [PATCH 19/54] Show the client and scopes on the device verification page The device verification page only showed the user code, so users couldn't tell which client was requesting access, and to what. Neither the device verification page nor the consent page prevented framing. Make device verification two steps: after the user code is entered, the page shows the requesting client and scopes before the user can approve. Send X-Frame-Options and frame-ancestors headers on both pages. Co-Authored-By: Claude Opus 5.5 --- SECURITY-TODO.md | 7 +++- proxy/internal/oidc/deviceauth.go | 17 ++++++++ proxy/internal/oidc/deviceauth_test.go | 51 ++++++++++++++++++++++++ proxy/internal/oidc/server.go | 8 ++++ proxy/internal/oidc/verify-template.html | 15 ++++++- 5 files changed, 96 insertions(+), 2 deletions(-) diff --git a/SECURITY-TODO.md b/SECURITY-TODO.md index 82410bc7..0bd5e458 100644 --- a/SECURITY-TODO.md +++ b/SECURITY-TODO.md @@ -198,7 +198,12 @@ deleted when every item is resolved. Fix: store the user in `codeData`, require a match on POST, re-run `AuthorizeClient` and scope filtering. -- [ ] **19. Device flow phishing and clickjacking hardening** (Low) +- [x] **19. Device flow phishing and clickjacking hardening** (Low) + *Fixed: the device verification page is now two steps (enter code → + page shows client ID and scopes → approve/deny), using existing + translation keys. Consent and device pages send `X-Frame-Options: DENY` + and `frame-ancestors 'none'`. `verification_uri_complete` is kept + (RFC 8628), since the client and scopes are now shown before approval.* `oidc/verify-template.html`, `authorize-template.html`, `deviceauth.go`: the verify page doesn't show the client or scopes; no `X-Frame-Options` / `frame-ancestors` on consent and device pages; `verification_uri_complete` diff --git a/proxy/internal/oidc/deviceauth.go b/proxy/internal/oidc/deviceauth.go index d8a79783..a1489f7f 100644 --- a/proxy/internal/oidc/deviceauth.go +++ b/proxy/internal/oidc/deviceauth.go @@ -175,10 +175,27 @@ func (s *ProviderServer) ServeDeviceVerification(w http.ResponseWriter, req *htt data := struct { Email string UserCode string + Invalid bool + ClientID string + Scopes string }{ Email: email, UserCode: req.Form.Get("user_code"), } + // Show which client is requesting access, and to what, before + // the user approves the request. + if data.UserCode != "" { + s.mu.Lock() + if dc, ok := s.deviceCodes[strings.ToUpper(data.UserCode)]; ok && !expired(dc.created) { + if dt, ok := s.deviceTokens[dc.deviceCode]; ok { + data.ClientID = dc.clientID + data.Scopes = strings.Join(dt.scope, ",") + } + } + s.mu.Unlock() + data.Invalid = data.ClientID == "" + } + setNoFrameHeaders(w) w.Header().Set("content-type", "text/html; charset=utf-8") verifyTemplate.Execute(w, data) return diff --git a/proxy/internal/oidc/deviceauth_test.go b/proxy/internal/oidc/deviceauth_test.go index 20c81d05..1eee36ed 100644 --- a/proxy/internal/oidc/deviceauth_test.go +++ b/proxy/internal/oidc/deviceauth_test.go @@ -100,3 +100,54 @@ func TestDeviceAuthorizationLimits(t *testing.T) { type nopLogger struct{} func (nopLogger) Errorf(string, ...any) {} + +func TestDeviceVerificationPage(t *testing.T) { + s := NewServer(ServerOptions{ + Clients: []Client{{ID: "my-client"}}, + Scopes: []string{"openid", "email"}, + ACLMatcher: func([]string, string) bool { return true }, + EventRecorder: nopRecorder{}, + Logger: nopLogger{}, + }) + + req := httptest.NewRequest(http.MethodPost, "https://idp.example.com/device/authorization", strings.NewReader("client_id=my-client&scope=openid+email")) + req.Header.Set("content-type", "application/x-www-form-urlencoded") + w := httptest.NewRecorder() + s.ServeDeviceAuthorization(w, req) + var resp struct { + UserCode string `json:"user_code"` + } + if err := json.NewDecoder(w.Body).Decode(&resp); err != nil { + t.Fatalf("json.Decode: %v", err) + } + + verify := func(userCode string) *httptest.ResponseRecorder { + req := httptest.NewRequest(http.MethodGet, "https://idp.example.com/device/verify?user_code="+url.QueryEscape(userCode), nil) + req = req.WithContext(fromctx.WithClaims(req.Context(), jwt.MapClaims{"email": "bob@example.com"})) + w := httptest.NewRecorder() + s.ServeDeviceVerification(w, req) + return w + } + + // A valid code shows the client and scopes, and the approve button. + w = verify(strings.ToLower(resp.UserCode)) + if got, want := w.Header().Get("X-Frame-Options"), "DENY"; got != want { + t.Errorf("X-Frame-Options = %q, want %q", got, want) + } + for _, want := range []string{"
my-client
", "
(openid,email)
", "oidc-approve-button"} { + if body := w.Body.String(); !strings.Contains(body, want) { + t.Errorf("Body doesn't contain %q", want) + } + } + + // An invalid code doesn't. + w = verify("0000-0000-0000") + for _, notWant := range []string{"my-client", "oidc-approve-button"} { + if body := w.Body.String(); strings.Contains(body, notWant) { + t.Errorf("Body contains %q", notWant) + } + } + if body := w.Body.String(); !strings.Contains(body, `class="invalid"`) { + t.Error("Body doesn't contain invalid class") + } +} diff --git a/proxy/internal/oidc/server.go b/proxy/internal/oidc/server.go index be5664d9..642dcc8c 100644 --- a/proxy/internal/oidc/server.go +++ b/proxy/internal/oidc/server.go @@ -173,6 +173,13 @@ type Client struct { ACL *[]string } +// setNoFrameHeaders prevents the page from being framed by other pages, e.g. for +// clickjacking. +func setNoFrameHeaders(w http.ResponseWriter) { + w.Header().Set("X-Frame-Options", "DENY") + w.Header().Set("Content-Security-Policy", "frame-ancestors 'none'") +} + func expired(created time.Time) bool { return created.Add(codeExpiration).Before(time.Now().UTC()) } @@ -485,6 +492,7 @@ func (s *ProviderServer) ServeAuthorization(w http.ResponseWriter, req *http.Req RequestID: requestID, Scopes: strings.Join(scopes, ","), } + setNoFrameHeaders(w) w.Header().Set("content-type", "text/html; charset=utf-8") authorizeTemplate.Execute(w, data) return diff --git a/proxy/internal/oidc/verify-template.html b/proxy/internal/oidc/verify-template.html index 84ae844d..a7ab4798 100644 --- a/proxy/internal/oidc/verify-template.html +++ b/proxy/internal/oidc/verify-template.html @@ -32,6 +32,9 @@ #usercode { text-align: center; } +#usercode.invalid { + border: 2px solid red; +}