Support self-signed Electrum certificates - #843
Open
kwsantiago wants to merge 11 commits into
Open
kwsantiago wants to merge 11 commits into
kwsantiago wants to merge 11 commits into
Conversation
Custom Electrum nodes could only be reached over TLS chains anchored in the bundled webpki roots, so a self hosted server was unreachable unless its certificate came from a public CA. Node now carries an optional TlsTrust describing how its certificate is verified. A custom CA validates the chain and still checks the hostname, so a privately issued leaf can rotate without the user pinning it again. A pinned SHA-256 fingerprint accepts one specific leaf and skips the hostname check, covering servers reached by IP whose certificate has no matching SAN. Leaving it unset keeps the previous behavior. electrum-client only accepts a caller supplied rustls session through RawClient, which has no reconnect loop of its own, so Transport pairs it with the url and trust settings needed to rebuild it. Without that a single dropped socket would break a pinned node until the app restarts, because the client is cached for the lifetime of a wallet. Reconnects track a generation so callers that failed on the same dead socket reuse the connection the first of them established. Only a rejected certificate counts as a certificate failure. An ssl:// url pointed at a plaintext port stays a connection error, so the user is never asked to trust their way out of an unrelated problem. Pinning still verifies the handshake signature, so the peer has to hold the pinned certificate's key. The handshake is completed while creating the client rather than on first use, so a rejected certificate is reported the way the default path reports one.
TlsTrust lives on Node, so it can be set for any api type, but only the Electrum client reads it. An Esplora node configured to trust a specific certificate would silently connect using the default roots instead. Honoring it here would mean building a custom reqwest client, so refuse the node rather than connect with weaker trust than it asked for.
The Electrum client honored TlsTrust, but nothing could set it: custom nodes were always built with no certificate settings, so the feature was unreachable. parse_custom_node now takes them, and fetch_node_certificate reads what a server presents so it can be shown to the user before it is trusted. That read verifies nothing, which is why the fingerprint has to be confirmed against the server rather than accepted on sight. certificate_decision answers whether a rejected certificate can be offered for confirmation at all. A url that already trusts a certificate is told the certificate changed rather than asked to accept a new one. Deciding it here rather than in each app keeps the rule in one place and lets it be tested; check_and_save_node also refuses to save a node that would drop the certificate a url already trusts, so it cannot be lost by omission either. Hosts are allowed to be IP addresses. Self hosted servers are often reached that way, and an IPv6 literal was rejected outright.
Saving a custom Electrum node whose certificate cannot be verified now shows that certificate's SHA-256 fingerprint and asks whether to trust it, instead of failing with a connection error. The prompt appears only after normal verification has already failed, so nothing changes for a node with a certificate from a public CA, and there is no setting to turn on before it is needed. Accepting pins that exact certificate for that node alone. Certificate settings are carried into every later save and restored with the rest of the node, so trusting a certificate once does not turn into answering the same question on each save. Whether a rejected certificate can be offered at all is decided in the core, so both apps apply the same rule. The prompt is deliberately not offered for preset nodes, which are public servers where an unrecognized certificate is a reason to stop.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 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.
🧹 Nitpick comments (1)
rust/src/node_connect.rs (1)
249-249: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
ResultExt::map_err_strfor these error conversions. These new sites use the.map_err(|e| Error::Variant(e.to_string()))pattern the project asks to avoid.♻️ Suggested change
- let certificate = - cove_tokio::unblock::run_blocking(move || transport::peer_certificate(&url)) - .await - .map_err(|error| Error::ReadCertificateError(error.to_string()))?; + let certificate = + cove_tokio::unblock::run_blocking(move || transport::peer_certificate(&url)) + .await + .map_err_str(Error::ReadCertificateError)?;-fn normalized_url(url: &str) -> Result<String, Error> { - let url = parse_node_url(url) - .map_err(|error| Error::ParseNodeUrlError(error.to_string()))? - .to_string(); +fn normalized_url(url: &str) -> Result<String, Error> { + let url = parse_node_url(url).map_err_str(Error::ParseNodeUrlError)?.to_string();As per coding guidelines: "Use
cove_util::ResultExt::map_err_strand..map_err_prefixinstead of.map_err(|e| Error::Variant(e.to_string()))".Also applies to: 358-361
🤖 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 `@rust/src/node_connect.rs` at line 249, Replace the inline error-to-string conversions in the certificate-reading flow, including the site around Error::ReadCertificateError and the additional sites around lines 358–361, with cove_util::ResultExt::map_err_str. Preserve each existing Error variant and conversion behavior while applying the project-standard extension method.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@rust/src/node_connect.rs`:
- Line 249: Replace the inline error-to-string conversions in the
certificate-reading flow, including the site around Error::ReadCertificateError
and the additional sites around lines 358–361, with
cove_util::ResultExt::map_err_str. Preserve each existing Error variant and
conversion behavior while applying the project-standard extension method.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a949b9b1-7537-4574-9ab9-edd704f514f8
⛔ Files ignored due to path filters (3)
android/app/src/main/java/org/bitcoinppl/cove_core/cove.ktis excluded by!android/app/src/main/java/org/bitcoinppl/cove_core/**ios/CoveCore/Sources/CoveCore/generated/cove.swiftis excluded by!**/generated/**,!ios/CoveCore/Sources/CoveCore/generated/**rust/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
android/app/src/main/java/org/bitcoinppl/cove/flows/SettingsFlow/NodeSettingsScreen.ktandroid/app/src/main/res/values/strings.xmlios/Cove/Flows/SettingsFlow/SettingsScreen/NodeSelectionView.swiftrust/Cargo.tomlrust/src/node.rsrust/src/node/client.rsrust/src/node/client/electrum.rsrust/src/node/client/electrum/test_server.rsrust/src/node/client/electrum/transport.rsrust/src/node/client/esplora.rsrust/src/node/tls.rsrust/src/node_connect.rs
Extract certificate alert actions/message builders and simplify node parse/save error handling in the node selection view. Standardize Rust node-connect error conversions through ResultExt helpers.
Increase the application versionCode to reflect the latest Android release increment.
Member
|
also bring these over 3607cf3 |
kwsantiago
force-pushed
the
electrum-self-signed-certs
branch
from
July 22, 2026 22:15
432f3b8 to
1692ad1
Compare
RawClient::from only wraps the stream, so server.version was never sent and block_headers was decoded as 1.4. Negotiate on connect and reconnect.
Editing the url or switching to Esplora reused the saved pin. parse_custom_node now inherits it, only for an unchanged Electrum url.
Keeps both sides of the node.rs tests. Cargo.lock taken from master.
kwsantiago
force-pushed
the
electrum-self-signed-certs
branch
from
July 22, 2026 22:30
1692ad1 to
b0cfa7a
Compare
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
2 participants
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.
Closes #298.
A custom Electrum node could only be reached over a chain anchored in the bundled webpki roots, so a self hosted server was unreachable unless its certificate came from a public CA.
Nodegains an optionalTlsTrust. Unset keeps today's behavior exactly. Saving a custom node whose certificate cannot be verified now shows that certificate's SHA-256 fingerprint and asks whether to trust it.Two rules make it opt in without adding a setting:
certificate_decisionrather than in each app, so both platforms apply the same rule and it can be tested.Pinning still verifies the handshake signature, so the peer must hold the pinned key. It authenticates against a certificate rather than a name, which is not the same as skipping verification.
Notes
electrum-clientonly takes a caller supplied rustls session throughRawClient, which has no reconnect loop, soTransportkeeps what it needs to rebuild the connection. The client is cached for a wallet's lifetime, so without that one dropped socket would break a pinned node until restart.ssl://urls refuse certificate settings rather than connecting with weaker trust than was asked for.TlsTrust::CustomCavalidates against a user supplied CA and still checks the hostname, so a privately issued leaf can rotate without re-pinning. It is tested, but only the fingerprint mode has UI here since that is what the issue asks for and it needs no file picker. Happy to wire up the paste-a-certificate field as a follow-up, or drop the variant if you would rather it landed with its own UI.rcgenis dev only.rustls-platform-verifieror equivalent.Testing
1617 Rust tests,
fmtandclippy -D warningsclean. Tests cover the pinned and CA paths, hostname mismatch by IP, IPv6, leaf rotation, that default trust still rejects self-signed, that a rejected certificate is distinguished from an unreachable node, and reconnect including concurrent callers.Verified on a Pixel 9a against a self-signed front end to a real Electrum server:
REJECTED: UnknownCAOK, 0 requestsOK, 1 requestOK, 0 requestsOK, 62 requestsOK, 1 requestREJECTED: AccessDeniedThe last row used no interaction: the app was relaunched and loaded the pin itself.
The Android flow above was exercised on device. The iOS side compiles but has not been run on a device, so the flow itself is unverified there.
Bindings are regenerated here; say the word if you would rather they came from the Regenerate Bindings workflow.
Known limitations