#5 - X-SSL-Client-DN trust gated only on isLoopback() — split into a dedicated certAuthTrusted() hook - uwe.admin/oatpp-authkit

uwe.admin commented

2026-04-25 13:24:10 +02:00

Owner

Location: include/oatpp-authkit/auth/AuthInterceptor.hpp:120-130 and IRuntimeConfig.hpp

Cert-DN auth is granted whenever IRuntimeConfig::isLoopback() is true. Operators binding to 127.0.0.1 but exposing the port via SSH tunnel or a non-TLS proxy that forwards X-SSL-Client-DN from untrusted clients can be impersonated. Add a dedicated certAuthTrusted() hook (or strip X-SSL-Client-DN from untrusted ingress before downstream handlers see it).

From audit #1.

**Location:** `include/oatpp-authkit/auth/AuthInterceptor.hpp:120-130` and `IRuntimeConfig.hpp` Cert-DN auth is granted whenever `IRuntimeConfig::isLoopback()` is true. Operators binding to `127.0.0.1` but exposing the port via SSH tunnel or a non-TLS proxy that forwards `X-SSL-Client-DN` from untrusted clients can be impersonated. Add a dedicated `certAuthTrusted()` hook (or strip `X-SSL-Client-DN` from untrusted ingress before downstream handlers see it). From audit #1.

uwe.admin referenced this issue

2026-04-25 13:24:26 +02:00

Code Audit Report — 2026-04-23 #1

uwe.admin referenced this issue

2026-04-25 13:24:26 +02:00

Code Audit Report — 2026-04-23 #1

uwe.admin commented

2026-04-25 13:38:02 +02:00

Author

Owner

Agent Evaluation

Feasibility: Small. The current trust path is a single if (m_runtime->isLoopback() && certDnH && !certDnH->empty()) in AuthInterceptor.hpp:120-130. The remediation is two small additions: a new policy/runtime hook and (optionally) stripping the header on untrusted ingress.

Impact: High if exploited, low probability. The audit framing is right: a misconfigured nginx that copies X-SSL-Client-DN from untrusted clients, an SSH tunnel to a loopback bind, or an operator who flips bindAddress without realising — any of those turns a single forgeable header into full impersonation of any cert user. The cost of fixing is low and the failure mode is silent (no log line, no test catches it), so the cost/benefit is good.

Effort: Small.

Recommendation: Accept.

Implementation plan

Add IRuntimeConfig::certAuthTrusted() — a new virtual returning bool. Default impl: return isLoopback(); so existing consumers keep their behaviour. Consumers that want stricter gating override it (e.g. "trust X-SSL-Client-DN only when both bound to loopback AND the consumer-controlled TRUST_CERT_DN env var is set").
Switch the trust check in AuthInterceptor::intercept from isLoopback() → certAuthTrusted().
Strip X-SSL-Client-DN from the request when certAuthTrusted() returns false, so downstream handlers cannot accidentally read it either. (oatpp's IncomingRequest exposes mutable headers via getHeaders()/putHeader — a remove API may not exist; in that case set to empty string and document.)
Update the docstring on IRuntimeConfig::isLoopback() to clarify it is the binding gate (used for trusted XFF / X-Real-IP); cert-DN trust now flows through the new hook.
Test (in the harness from #2): mock runtime with certAuthTrusted=false + a forged X-SSL-Client-DN header → expect the interceptor to ignore it and fall through to token auth.

Decision needed

Check one (edit this comment):

Option A — New IRuntimeConfig::certAuthTrusted(), defaults to isLoopback() — backwards compatible, no consumer break, opt-in stricter behaviour.
Option B — Rename isLoopback() → certAuthTrusted() — single concept; cleaner long-term but a breaking change for every downstream that overrides isLoopback() (currently fewo-webapp does). Bump major version.
Option C — Both methods, isLoopback() deprecated — A's API surface plus a [[deprecated]] warning on isLoopback() to migrate consumers over a release cycle. Most polite, slightly more code to remove later.

Default recommendation: A (cheapest, no downstream churn). Option C if the library is heading toward v1.0 soon and we want a deprecation window.

## Agent Evaluation **Feasibility:** Small. The current trust path is a single `if (m_runtime->isLoopback() && certDnH && !certDnH->empty())` in `AuthInterceptor.hpp:120-130`. The remediation is two small additions: a new policy/runtime hook and (optionally) stripping the header on untrusted ingress. **Impact:** High **if exploited**, low **probability**. The audit framing is right: a misconfigured nginx that copies `X-SSL-Client-DN` from untrusted clients, an SSH tunnel to a loopback bind, or an operator who flips `bindAddress` without realising — any of those turns a single forgeable header into full impersonation of any cert user. The cost of fixing is low and the failure mode is silent (no log line, no test catches it), so the cost/benefit is good. **Effort:** Small. **Recommendation:** Accept. ### Implementation plan 1. **Add `IRuntimeConfig::certAuthTrusted()`** — a new virtual returning `bool`. Default impl: `return isLoopback();` so existing consumers keep their behaviour. Consumers that want stricter gating override it (e.g. "trust X-SSL-Client-DN only when both bound to loopback AND the consumer-controlled `TRUST_CERT_DN` env var is set"). 2. **Switch the trust check** in `AuthInterceptor::intercept` from `isLoopback()` → `certAuthTrusted()`. 3. **Strip `X-SSL-Client-DN` from the request** when `certAuthTrusted()` returns false, so downstream handlers cannot accidentally read it either. (oatpp's `IncomingRequest` exposes mutable headers via `getHeaders()`/`putHeader` — a remove API may not exist; in that case set to empty string and document.) 4. **Update the docstring** on `IRuntimeConfig::isLoopback()` to clarify it is the *binding* gate (used for trusted XFF / X-Real-IP); cert-DN trust now flows through the new hook. 5. **Test** (in the harness from #2): mock runtime with `certAuthTrusted=false` + a forged `X-SSL-Client-DN` header → expect the interceptor to ignore it and fall through to token auth. ### Decision needed Check one (edit this comment): - [ ] **Option A — New `IRuntimeConfig::certAuthTrusted()`, defaults to `isLoopback()`** — backwards compatible, no consumer break, opt-in stricter behaviour. - [ ] **Option B — Rename `isLoopback()` → `certAuthTrusted()`** — single concept; cleaner long-term but a breaking change for every downstream that overrides `isLoopback()` (currently fewo-webapp does). Bump major version. - [ ] **Option C — Both methods, `isLoopback()` deprecated** — A's API surface plus a `[[deprecated]]` warning on `isLoopback()` to migrate consumers over a release cycle. Most polite, slightly more code to remove later. Default recommendation: **A** (cheapest, no downstream churn). Option C if the library is heading toward v1.0 soon and we want a deprecation window.

uwe.admin added the

evaluated

label 2026-04-25 13:38:02 +02:00

uwe.admin commented

2026-04-25 13:38:02 +02:00

Author

Owner

Evaluated #5 — Small, recommend accept; A/B/C decision on new hook vs rename vs deprecate.