fix(tls): complete incomplete server cert chains via AIA fetching#24
Conversation
A self-hosted instance behind a QNAP/Synology reverse proxy often serves an incomplete TLS chain (leaf only, or leaf + an intermediate that doesn't reach a trusted root). Browsers — and Firefox with its own trust store — paper over this by fetching the missing intermediate from the certificate's AIA "CA Issuers" URL. The app, using the Android system trust store with no AIA fetching, instead failed the handshake with "Trust anchor for certification path not found", so onboarding never got past "Discover library" for a perfectly valid Let's Encrypt certificate. Reproduced in the emulator against a real user's QNAP instance: letsencrypt.org validated fine, the user's host did not. With this change the same host completes the chain (2 -> 4 certs) and /api/v1/health returns 200, discovery + login proceed. AiaCompletingTrustManager wraps the platform X509ExtendedTrustManager: on a validation failure it walks the chain, downloads the missing issuer(s) from the AIA URL, and re-validates. It extends X509ExtendedTrustManager (not the plain X509TrustManager) because a domain-specific network-security-config makes the platform trust manager reject the hostname-unaware two-arg checkServerTrusted. Security: validation is never weakened. The platform trust manager still makes the final decision, with the real peer hostname, against the system trust store. Only intermediates that cryptographically chain to an already-trusted root are added; on any failure the ORIGINAL exception is re-thrown, so genuinely untrusted certs fail identically. Guarded with runCatching so any TLS setup issue falls back to the default behaviour.
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughIntrodotta una nuova classe ChangesCompletamento catena certificati via AIA
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OkHttpClient
participant NetworkModule
participant AiaCompletingTrustManager
participant AIAServer
NetworkModule->>AiaCompletingTrustManager: sslSocketFactory()
AiaCompletingTrustManager->>NetworkModule: SSLSocketFactory, X509TrustManager
NetworkModule->>OkHttpClient: applica sslSocketFactory/trustManager
OkHttpClient->>AiaCompletingTrustManager: checkServerTrusted(chain)
AiaCompletingTrustManager->>AiaCompletingTrustManager: valida catena ricevuta
alt CertificateException
AiaCompletingTrustManager->>AIAServer: fetch certificato intermedio (AIA URL)
AIAServer->>AiaCompletingTrustManager: certificato scaricato
AiaCompletingTrustManager->>AiaCompletingTrustManager: valida catena completata
end
AiaCompletingTrustManager->>OkHttpClient: risultato validazione
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt`:
- Around line 132-137: The download flow in AiaCompletingTrustManager.download
currently trusts caIssuerUrls and follows redirects without validating the
destination. Before opening the connection, and again after each redirect if
redirects remain enabled, reject any URL whose host resolves to private,
loopback, or link-local addresses; alternatively disable automatic redirects
entirely. Keep the check close to download and any helper that consumes the URL
so only trusted AIA fetch targets are allowed.
- Around line 138-149: The AIA fetch path in AiaCompletingTrustManager is
reading an unbounded response body via conn.inputStream.use { it.readBytes() },
which can trigger OOM during handshake. Update the certificate download logic in
the helper that parses the AIA “CA Issuers” response to enforce a small maximum
byte limit while reading, stop once the limit is exceeded, and return null for
oversized responses before passing bytes to certFactory.generateCertificate.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82896306-e1c1-4f92-8d05-91ac70ef493e
📒 Files selected for processing (2)
app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.ktapp/src/main/java/com/pinakes/app/data/network/NetworkModule.kt
| private fun download(url: String): X509Certificate? { | ||
| val conn = (URL(url).openConnection() as HttpURLConnection).apply { | ||
| connectTimeout = FETCH_TIMEOUT_MS | ||
| readTimeout = FETCH_TIMEOUT_MS | ||
| instanceFollowRedirects = true | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and surrounding symbols.
git ls-files 'app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt'
wc -l app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt
ast-grep outline app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt --view expanded
# Show the relevant section with line numbers.
sed -n '1,260p' app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.ktRepository: fabiodalez-dev/Pinakes-Android
Length of output: 10556
Blocca il fetch AIA verso host non fidati
caIssuerUrls legge URL da un certificato non ancora attendibile e download esegue una GET con redirect automatici, senza allowlist né filtro per indirizzi privati/loopback/link-local. Un certificato malevolo può quindi indurre richieste verso LAN o servizi locali durante l’handshake. Rifiuta questi indirizzi prima della connessione e dopo ogni redirect, oppure disabilita i redirect.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 132-132: An outbound HTTP request is built from a non-literal URL: URL(...).openConnection(), OkHttp's Request.Builder().url(...), or a Spring RestTemplate call (getForObject/getForEntity/postForObject/postForEntity/exchange/execute). When the URL is derived from a variable, method call, concatenation, or interpolation, an attacker can point the request at internal services, cloud metadata endpoints (e.g. 169.254.169.254), or arbitrary hosts (Server-Side Request Forgery). Do not build request targets from untrusted input: validate the URL against a strict allowlist of permitted hosts/schemes, resolve and re-check the host after DNS resolution, and reject internal/link-local/loopback addresses before connecting.
Context: URL(url).openConnection()
Note: [CWE-918] Server-Side Request Forgery (SSRF).
(ssrf-outbound-request-tainted-url-kotlin)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt`
around lines 132 - 137, The download flow in AiaCompletingTrustManager.download
currently trusts caIssuerUrls and follows redirects without validating the
destination. Before opening the connection, and again after each redirect if
redirects remain enabled, reject any URL whose host resolves to private,
loopback, or link-local addresses; alternatively disable automatic redirects
entirely. Keep the check close to download and any helper that consumes the URL
so only trusted AIA fetch targets are allowed.
Source: Linters/SAST tools
| return try { | ||
| val bytes = conn.inputStream.use { it.readBytes() } | ||
| // AIA "CA Issuers" is usually a single DER certificate; CertificateFactory also parses | ||
| // PEM. A non-certificate body (e.g. an OCSP responder answering a bare GET) throws, and | ||
| // the caller simply tries the next URL. | ||
| certFactory.generateCertificate(ByteArrayInputStream(bytes)) as? X509Certificate | ||
| } catch (t: Throwable) { | ||
| null | ||
| } finally { | ||
| conn.disconnect() | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Lettura non limitata: readBytes() può causare OOM da un endpoint AIA malevolo.
conn.inputStream.use { it.readBytes() } legge l'intero corpo della risposta senza alcun limite di dimensione, su un percorso attivato da un host non fidato e durante l'handshake. Un endpoint AIA che restituisce un corpo di grandi dimensioni può esaurire la memoria e far crashare il processo. Un certificato DER/PEM è di dimensioni ridotte: imponi un tetto massimo di lettura e interrompi oltre tale soglia.
🛡️ Esempio di lettura limitata
return try {
- val bytes = conn.inputStream.use { it.readBytes() }
+ val bytes = conn.inputStream.use { input ->
+ input.readNBytes(MAX_CERT_BYTES + 1)
+ }
+ if (bytes.size > MAX_CERT_BYTES) return null
// AIA "CA Issuers" is usually a single DER certificate; CertificateFactory also parses📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return try { | |
| val bytes = conn.inputStream.use { it.readBytes() } | |
| // AIA "CA Issuers" is usually a single DER certificate; CertificateFactory also parses | |
| // PEM. A non-certificate body (e.g. an OCSP responder answering a bare GET) throws, and | |
| // the caller simply tries the next URL. | |
| certFactory.generateCertificate(ByteArrayInputStream(bytes)) as? X509Certificate | |
| } catch (t: Throwable) { | |
| null | |
| } finally { | |
| conn.disconnect() | |
| } | |
| } | |
| return try { | |
| val bytes = conn.inputStream.use { input -> | |
| input.readNBytes(MAX_CERT_BYTES + 1) | |
| } | |
| if (bytes.size > MAX_CERT_BYTES) return null | |
| // AIA "CA Issuers" is usually a single DER certificate; CertificateFactory also parses | |
| // PEM. A non-certificate body (e.g. an OCSP responder answering a bare GET) throws, and | |
| // the caller simply tries the next URL. | |
| certFactory.generateCertificate(ByteArrayInputStream(bytes)) as? X509Certificate | |
| } catch (t: Throwable) { | |
| null | |
| } finally { | |
| conn.disconnect() | |
| } | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 144-144: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/src/main/java/com/pinakes/app/data/network/AiaCompletingTrustManager.kt`
around lines 138 - 149, The AIA fetch path in AiaCompletingTrustManager is
reading an unbounded response body via conn.inputStream.use { it.readBytes() },
which can trigger OOM during handshake. Update the certificate download logic in
the helper that parses the AIA “CA Issuers” response to enforce a small maximum
byte limit while reading, stop once the limit is exceeded, and return null for
oversized responses before passing bytes to certFactory.generateCertificate.
1 participant
A self-hosted instance behind a QNAP/Synology reverse proxy often serves an incomplete TLS chain (leaf only, or leaf + an intermediate that does not reach a trusted root). Browsers — and Firefox with its own trust store — paper over this by fetching the missing intermediate from the certificate's AIA "CA Issuers" URL. The app, using the Android system trust store with no AIA fetching, instead failed the handshake with
Trust anchor for certification path not found, so onboarding never got past Discover library for a perfectly valid Let's Encrypt certificate.Reproduction (emulator, real user QNAP host)
letsencrypt.org(standard LE)Trust anchor for certification path not foundAIA-completed chain (2 -> 4) validated→ 200, discovery + login proceedWhat it does
AiaCompletingTrustManagerwraps the platformX509ExtendedTrustManager: on a validation failure it walks the chain, downloads the missing issuer(s) from the AIA URL, and re-validates. It extendsX509ExtendedTrustManager(not the plainX509TrustManager) because a domain-specificnetwork-security-configmakes the platform trust manager reject the hostname-unaware two-argcheckServerTrusted.Security
Validation is never weakened. The platform trust manager still makes the final decision, with the real peer hostname, against the system trust store. Only intermediates that cryptographically chain to an already-trusted root are added; on any failure the ORIGINAL exception is re-thrown, so genuinely untrusted certs fail identically. Wired with
runCatchingso any TLS setup issue falls back to default behaviour.Summary by CodeRabbit