Open
Conversation
…and update HttpClient and tests
Contributor
|
I agree that this PR makes sense. Let's see.. |
Contributor
|
@Scemp Could you try the APK from https://github.com/opencloud-eu/android/releases/tag/build-tester-apk-9 ? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the regression from 0c72514, reported in #125.
The short version: 0c72514 (correctly) dropped the all-accepting HostnameVerifier that was effectively disabling hostname checks for every connection - a real MITM hole. Problem is, it also broke every self-hosted setup where the URL hostname doesn't match the cert's CN/SAN. Classic case from the issue:
https://truenas:30259with a self-signed TrueNAS cert that's issued fortruenas.localor an IP. On 1.2.0 this silently worked because the check was a no-op; on 1.2.1 it's a dead end with no way forward.Why it's a dead end, exactly
Walking the flow on 1.2.1 for the affected user:
AdvancedX509TrustManager.checkServerTrusted()throwsCertificateCombinedExceptionknownServers.bksonSavedCertificate()retries the requesttruenasagainst the cert's SANSSLPeerUnverifiedException-> wrapped into the newSSL_RECOVERABLE_PEER_UNVERIFIEDcode pathnewInstanceForFullSslErrorseessslPeerUnverifiedException != nulland setsm509Certificate = null, which in turn hides Cancel and relabels Trust as plain OKmHandler == null->onCancelCertificate()-> login abortedReinstall doesn't help: cert stays in the store so the retry hits step 4 immediately without ever going through the trust dialog.
The fix
New
KnownServersHostnameVerifier. It runs OkHttp's defaultOkHostnameVerifier(RFC 2818/6125) first. If that passes, done. If it fails, it checks whether the peer cert is already inknownServers.bks. If yes, the hostname mismatch is accepted - the rationale being that the user has already actively opted into trusting exactly this certificate.Per-scenario breakdown of what that does:
Also added
NetworkUtils.isCertInKnownServersStore()for the lookup.Security note
The 1.2.0 bypass was blanket - every TLS connection effectively had hostname verification disabled. This one is gated per-certificate on explicit user trust: someone pulling off a MITM needs both a copy of a cert the user has already accepted and network position. For self-hosted users the effective risk is equivalent to 1.2.0, but without the global bypass for all TLS traffic to regular servers.
Dead code I left in place
The
sslPeerUnverifiedExceptionbranch inRemoteOperationResultand the "hide Cancel + relabel OK" branch inSslUntrustedCertDialogare both unreachable in the normal flow now, because the HostnameVerifier handles the known-cert case before it ever bubbles up. I didn't rip them out in this PR - they still act as a safety net if aSSLPeerUnverifiedExceptionever comes through from a cert that isn't in the store (e.g. a misconfigured public server), so we'd still get a mapped result code and a sensible dialog instead of a generic network error. Happy to clean them up in a follow-up if you'd rather have lean code than defense in depth.Tests
HttpClientTlsTestupdated to match the new contract:accepts user-trusted certificate despite hostname mismatch- the regression case, inverted from the previous "rejects" testrejects certificate with hostname mismatch when not in known servers- guards the security propertywraps hostname mismatch as certificate combined exception- unchanged, still covers the fallback mappingAll three pass locally. Detekt and the app compile are also clean.
Not tested by me
I don't have a TrueNAS box handy, so I didn't run the full login end-to-end on a real device against the affected setup. Confirmation from the reporter in #125 would be appreciated before merging.
Closes #125.