Automattic / Automattic/wordpress-rs

Kotlin: consolidate the two cert-exception clients into one host-aware trust manager

Open
#1,556 0 comments 0 reactions 1 assignee Claimed by @jkmassel View on GitHub
Kotlin
Dominant language
Rust
Stars
36
Forks
5
Avg merge
17h 30m
Merged PRs (30d)
43

Description

Extracted from the review of #1527. **Not a vulnerability** — the current behavior is fail-safe; this is a cross-platform parity and simplification issue.

## Summary

- Kotlin's `disableCertificateValidation` routes opted-out hosts to a separate all-trusting `OkHttpClient` and guards it with a redirect interceptor that **refuses every cross-host redirect** — including redirects to hosts with a valid public certificate.
- Swift makes the trust decision per server-trust challenge, so it follows such redirects and validates the new host normally. Same input, opposite outcome by platform.
- Replace the two-client split with a single client whose `X509ExtendedTrustManager` decides trust per host. This matches Swift, and retires two other review findings (the `GenericError` flattening and the connection-pool drift) at the same time.

## Current behavior

`WpHttpClient.DefaultHttpClient` keeps a strict `client` and a lazily-built all-trusting `insecureClient`, and routes the *initial* request by host (`WpHttpClient.kt:141`). Because OkHttp follows redirects internally on whichever client took the call, a network interceptor on the insecure client throws when a redirect leaves the opt-out set (`WpHttpClient.kt:129`):

```
disableCertificateValidation("dev.example.test")
GET https://dev.example.test/ → insecure client, trust-all handshake OK
← 301 Location: https://www.example.test/ (valid, publicly-trusted cert)
interceptor: "www.example.test" not in opt-out set → throw IOException
→ RequestExecutionErrorReason.GenericError (WpRequestExecutor.kt:260)
```

The identical flow **succeeds on iOS**: the redirect target gets a fresh server-trust challenge and passes default validation. The canonical apex→www redirect (opt out the self-signed dev apex; it 301s to a `www` that has a real cert) therefore works on iOS and dead-ends on Android as an opaque error.

## Root cause

OkHttp configures trust **per client**, so the design encodes "which trust policy" in *which client took the request* and then applies that one policy to every redirect hop. The guard is a patch over that: the all-trusting client can't tell "valid cert, accept normally" from "invalid cert, bypassing," so it refuses all cross-host hops.

## Proposed change

One client. Move the opt-out decision into the trust layer, keyed on the connecting host:

- **`X509ExtendedTrustManager`**: read the host from the handshake (`(socket as SSLSocket).handshakeSession.peerHost`, and the `SSLEngine` overload). Opted-out host → return (trust all). Otherwise → delegate to the platform default `X509ExtendedTrustManager`. **Fail closed**: if the host can't be determined, use strict validation.
- **`HostnameVerifier`**: opted-out host → `true`; otherwise the existing allow-list + `OkHostnameVerifier` path (unchanged).

Delete `insecureClient`, the redirect network-interceptor, and `getClient(host)`'s routing.

### Behavior after (per hop, across redirects — matches Swift)

| Redirect target | Result |
|---|---|
| Opted-out host | trust-all (as configured) |
| Non-opted host, valid cert | validated normally → works |
| Non-opted host, invalid cert | rejected as `InvalidSslError` (not `GenericError`) |

The trust-all bypass is now scoped to the exact host in the set and re-checked at every handshake, so it structurally cannot extend to a non-opted host — the same invariant the guard enforced by refusing, now enforced by correctly validating.

## Also retires

- The redirect refusal throwing a bare `IOException` that flattens into `GenericError` (`WpRequestExecutor.kt:260`): there is no thrown refusal anymore.
- `insecureClient` is built once from `client` v0 (`WpHttpClient.kt:94`) and stops sharing the pool/dispatcher after a later `addAllowedAlternativeNamesForHostname` rebuild: there is only one client.

## Risks / must validate before merging

- **`SSLSession.getPeerHost()` reliability.** The whole decision hinges on it returning OkHttp's target host. It is reliable in OkHttp's usage (sockets are created with the hostname for SNI) but can be null in some JSSE paths — hence fail-closed. Add a test asserting `peerHost` matches the target across direct, redirected, and HTTP/2-coalesced connections.
- **HTTP/2 coalescing under a single client.** With one `SSLSocketFactory`/`HostnameVerifier` instance, confirm a trust-all connection to an opted-out host cannot be coalesced for a non-opted host (the per-host verifier should gate it; prove it).

## Acceptance criteria

- [ ] A request to an opted-out host that 301s to a valid-cert host completes (parity with Swift), on the real `OkHttpClient` path.
- [ ] A redirect to a non-opted host with an invalid cert surfaces as `InvalidSslError`, not `GenericError`.
- [ ] A redirect from a non-opted host *to* an opted-out host still gets normal validation (i.e. the opt-out does not silently apply on a hop the router did not start on).
- [ ] The `peerHost`-unavailable path falls back to strict validation (fail closed).
- [ ] `insecureClient`, the redirect interceptor, and `getClient(host)` routing are gone.

Context: supersedes the redirect guard introduced in #1527.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.