diff --git a/packages/core/src/http/client/server_cert_verifier.rs b/packages/core/src/http/client/server_cert_verifier.rs index f617dda6..5d405c76 100644 --- a/packages/core/src/http/client/server_cert_verifier.rs +++ b/packages/core/src/http/client/server_cert_verifier.rs @@ -3,11 +3,45 @@ use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, Server use rustls::client::WebPkiServerVerifier; use rustls::pki_types::pem::PemObject; use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; -use rustls::{CertificateError, DigitallySignedStruct, Error, RootCertStore, SignatureScheme}; -use std::fmt::{Debug, Formatter}; +use rustls::{ + CertificateError, DigitallySignedStruct, Error, OtherError, RootCertStore, SignatureScheme, +}; +use std::fmt::{Debug, Display, Formatter}; use std::sync::Arc; use x509_parser::nom::AsBytes; +/// The reason a peer certificate was rejected, as an error that rustls carries +/// along. +/// +/// Without this, every check in this file collapses into rustls' +/// [`CertificateError::ApplicationVerificationFailure`], which surfaces in the +/// app as a bare "error sending request for url (...)" with no hint that the +/// certificate was the problem. +struct RejectedCert(String); + +impl Display for RejectedCert { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +/// rustls renders `InvalidCertificate` with `Debug`, so the derived +/// `RejectedCert("…")` would end up in the message. +impl Debug for RejectedCert { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.0) + } +} + +impl std::error::Error for RejectedCert {} + +fn rejected(reason: String) -> Error { + tracing::warn!("{reason}"); + Error::InvalidCertificate(CertificateError::Other(OtherError(Arc::new(RejectedCert( + reason, + ))))) +} + /// Verifies the certificate of the remote peer during the TLS handshake. /// /// LocalSend peers use self-signed certificates, so there is no authority to @@ -65,20 +99,15 @@ impl ServerCertVerifier for PinnedServerCertVerifier { // The hostname is deliberately ignored: peers are addressed by IP and // their certificates carry no matching SAN. The fingerprint below is // what identifies the peer. - crate::crypto::cert::verify_cert_from_der(end_entity.as_bytes(), None).map_err(|e| { - tracing::warn!("Server certificate verification failed: {e:#}"); - Error::InvalidCertificate(CertificateError::ApplicationVerificationFailure) - })?; + crate::crypto::cert::verify_cert_from_der(end_entity.as_bytes(), None) + .map_err(|e| rejected(format!("server certificate is not valid: {e:#}")))?; if let Some(expected) = &self.expected_fingerprint { let actual = fingerprint_from_cert_der(end_entity.as_bytes()); if &actual != expected { - tracing::warn!( - "Server certificate fingerprint mismatch: expected {expected}, got {actual}" - ); - return Err(Error::InvalidCertificate( - CertificateError::ApplicationVerificationFailure, - )); + return Err(rejected(format!( + "server certificate fingerprint mismatch: expected {expected}, got {actual}" + ))); } } @@ -167,7 +196,11 @@ VRus1zGVD8IVpIdPMyz01WJyS7M0fWaHXKWo+Bo= #[test] fn rejects_mismatching_fingerprint() { let other = "0".repeat(64); - assert!(verify(Some(&other)).is_err()); + let error = verify(Some(&other)).unwrap_err(); + let message = error.to_string(); + assert!(message.contains("fingerprint mismatch"), "{message}"); + assert!(message.contains(&other), "{message}"); + assert!(message.contains(FINGERPRINT), "{message}"); } #[test] diff --git a/packages/localsend_isolates/lib/src/isolate/child/http_provider.dart b/packages/localsend_isolates/lib/src/isolate/child/http_provider.dart index 374d38f5..6eeb88b9 100644 --- a/packages/localsend_isolates/lib/src/isolate/child/http_provider.dart +++ b/packages/localsend_isolates/lib/src/isolate/child/http_provider.dart @@ -14,10 +14,15 @@ class HttpClientCollection { /// on that address. final RsHttpClient discovery; + /// The request timeout of the [discovery] client. + /// Also apply it to short-lived pinned requests made during discovery. + final int discoveryTimeout; + HttpClientCollection({ required String privateKey, required String certificate, required this.discovery, + required this.discoveryTimeout, }) : _privateKey = privateKey, _certificate = certificate; @@ -27,12 +32,17 @@ class HttpClientCollection { /// The check happens during the TLS handshake, so a different peer never /// receives the request. Create one per upload task and reuse it for all /// files of that task, so the connection is kept alive between them. - RsHttpClient pinnedTo(String fingerprint) { + /// + /// [timeoutMs] bounds each request. Leave it out for uploads, which may + /// legitimately run for a long time; set it when the peer is unverified and + /// could keep the request open forever. + RsHttpClient pinnedTo(String fingerprint, {int? timeoutMs}) { return createClient( privateKey: _privateKey, cert: _certificate, version: LsHttpClientVersion.v2, expectedFingerprint: fingerprint, + timeoutMs: timeoutMs, ); } } @@ -51,5 +61,6 @@ final httpProvider = ViewProvider((ref) { expectedFingerprint: null, timeoutMs: discoveryTimeout, ), + discoveryTimeout: discoveryTimeout, ); }); diff --git a/packages/localsend_isolates/lib/src/task/discovery/multicast_discovery.dart b/packages/localsend_isolates/lib/src/task/discovery/multicast_discovery.dart index 3de34ec3..9e625a0f 100644 --- a/packages/localsend_isolates/lib/src/task/discovery/multicast_discovery.dart +++ b/packages/localsend_isolates/lib/src/task/discovery/multicast_discovery.dart @@ -26,16 +26,22 @@ class MulticastService { bool _listening = false; /// Binds the UDP sockets and listens to multicast announcements. - /// Announcements of other devices are answered with an HTTP register - /// request while the server is running. - Stream startListener() async* { + Stream startListener() { if (_listening) { _logger.info('Already listening to multicast'); - return; + return const Stream.empty(); } _listening = true; + // Verified devices are pushed in as [_answerAnnouncement] resolves, which + // runs concurrently: one unreachable peer must not hold up the others. + final devices = StreamController(); + unawaited(_runListener(devices)); + return devices.stream; + } + + Future _runListener(StreamController devices) async { while (true) { final syncState = _ref.read(syncProvider); @@ -68,13 +74,17 @@ class MulticastService { unawaited(multicast.announce()); await for (final event in multicast.listen()) { - final device = event.message.toDevice(event.ip); - yield device; - - if (_ref.read(syncProvider).serverRunning) { + if (!_ref.read(syncProvider).serverRunning) { // only respond when server is running - unawaited(_answerAnnouncement(device)); + continue; } + + unawaited(() async { + final device = await _answerAnnouncement(event.message.toDevice(event.ip)); + if (device != null && !devices.isClosed) { + devices.add(device); + } + }()); } // The stream ended because [restartListener] stopped the discovery. @@ -106,21 +116,28 @@ class MulticastService { await multicast.announce(); } - /// Responds to an announcement over HTTP. - Future _answerAnnouncement(Device peer) async { + /// Responds to an announcement and returns the peer. + /// Data from the multicast announcement is ignored. + /// + /// Returns null if the peer could not be reached or could not be verified. + Future _answerAnnouncement(Device peer) async { + final clients = _ref.read(httpProvider); try { - await _ref - .read(httpProvider) - .discovery + final response = await clients + .pinnedTo(peer.fingerprint, timeoutMs: clients.discoveryTimeout) .register( protocol: peer.getProtocolType(), ip: peer.ip!, port: peer.port, payload: _ref.read(syncProvider).toRegisterDto(), ); + _logger.info('Respond to announcement of ${peer.alias} (${peer.ip}, model: ${peer.deviceModel}) via TCP'); + + return response.body.toDevice(peer.ip!, peer.port, peer.https, const MulticastDiscovery()); } catch (e) { _logger.warning('Could not respond to announcement of ${peer.alias} (${peer.ip})', e); + return null; } } } diff --git a/packages/localsend_isolates/rust/src/api/http.rs b/packages/localsend_isolates/rust/src/api/http.rs index c476bbd1..3f546176 100644 --- a/packages/localsend_isolates/rust/src/api/http.rs +++ b/packages/localsend_isolates/rust/src/api/http.rs @@ -7,6 +7,7 @@ pub use localsend::http::dto::{ PrepareUploadRequestDto, PrepareUploadResponseDto, PrepareUploadResult, ProtocolType, RegisterDto, RegisterResponseDto, }; +use localsend::reqwest; pub struct RsHttpClient { inner: localsend::http::client::LsHttpClient, @@ -195,7 +196,7 @@ impl From for RsHttpClientError { status: e.status, message: e.message, }, - ClientError::Reqwest(e) => RsHttpClientError::Reqwest(e.to_string()), + ClientError::Reqwest(e) => RsHttpClientError::Reqwest(error_chain(&e)), ClientError::Json(e) => RsHttpClientError::Json(e.to_string()), ClientError::Io(e) => RsHttpClientError::Io(e.to_string()), ClientError::Other(e) => RsHttpClientError::Other(e.to_string()), @@ -204,6 +205,21 @@ impl From for RsHttpClientError { } } +/// Renders an error together with everything that caused it. +/// +/// [`reqwest::Error`] alone only says "error sending request for url (...)". +pub fn error_chain(e: &dyn std::error::Error) -> String { + use std::fmt::Write; + + let mut message = e.to_string(); + let mut source = e.source(); + while let Some(current) = source { + let _ = write!(message, ": {current}"); + source = current.source(); + } + message +} + #[frb(mirror(LsHttpClientVersion))] pub enum _LsHttpClientVersion { V2,