From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 0A1901FF13E for ; Wed, 01 Jul 2026 15:36:44 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id C53C5213BD; Wed, 01 Jul 2026 15:36:43 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 01 Jul 2026 15:36:07 +0200 Message-Id: To: "Dominik Csapak" , , Subject: Re: [PATCH proxmox-backup v4 6/8] pbs-client: use proxmox-https openssl callback X-Mailer: aerc 0.20.0 References: <20260701103120.1593265-1-d.csapak@proxmox.com> <20260701103120.1593265-7-d.csapak@proxmox.com> In-Reply-To: <20260701103120.1593265-7-d.csapak@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782912962854 X-SPAM-LEVEL: Spam detection results: 0 DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: WUWDF6F2HWCSQ43F5ZIX4U2HHR3V42UF X-Message-ID-Hash: WUWDF6F2HWCSQ43F5ZIX4U2HHR3V42UF X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Jul 1, 2026 at 12:30 PM CEST, Dominik Csapak wrote: > instead of implementing it here. This changes the behavior when giving a > fingerprint explicitly when the certificate chain is trusted by openssl. > Previously this would be accepted due to openssls checks, regardless if > the given fingerprint would match or not. > > With this patch, a given fingerprint has higher priority than openssls > validation. > > Signed-off-by: Dominik Csapak > --- > Cargo.toml | 2 +- > pbs-client/src/http_client.rs | 165 ++++++++++++++++------------------ > 2 files changed, 76 insertions(+), 91 deletions(-) > > diff --git a/Cargo.toml b/Cargo.toml > index a625370cf..9530e1ccb 100644 > --- a/Cargo.toml > +++ b/Cargo.toml > @@ -64,7 +64,7 @@ proxmox-config-digest =3D "1" > proxmox-daemon =3D "1" > proxmox-fuse =3D "3" > proxmox-docgen =3D "1" > -proxmox-http =3D { version =3D "1.0.2", features =3D [ "client", "http-h= elpers", "api-types", "websocket" ] } # see below > +proxmox-http =3D { version =3D "1.0.2", features =3D [ "client", "http-h= elpers", "api-types", "websocket", "tls" ] } # see below > proxmox-human-byte =3D "1" > proxmox-io =3D "1.0.1" # tools and client use "tokio" feature > proxmox-lang =3D "1.1" > diff --git a/pbs-client/src/http_client.rs b/pbs-client/src/http_client.r= s -->8 snip 8<-- > @@ -425,30 +431,47 @@ impl HttpClient { > let interactive =3D options.interactive; > let fingerprint_cache =3D options.fingerprint_cache; > let prefix =3D options.prefix.clone(); > - let trust_openssl_valid =3D Arc::new(Mutex::new(true)); > ssl_connector_builder.set_verify_callback( > openssl::ssl::SslVerifyMode::PEER, > - move |valid, ctx| match Self::verify_callback( > + move |valid, ctx| match openssl_verify_callback( > valid, > ctx, > - expected_fingerprint.as_ref(), > - interactive, > - Arc::clone(&trust_openssl_valid), > + expected_fingerprint.clone(), > ) { > - Ok(None) =3D> true, > - Ok(Some(fingerprint)) =3D> { > - if fingerprint_cache { > - if let Some(ref prefix) =3D prefix { > - if let Err(err) =3D store_fingerprint(pr= efix, &server, &fingerprint) { > - error!("{}", err); > - } > - } > + Ok(()) =3D> { > + if let Some(fp) =3D &expected_fingerprint { > + *verified_fingerprint.lock().unwrap() =3D So= me(fp.to_string()); > } > - *verified_fingerprint.lock().unwrap() =3D Some(f= ingerprint); > true > - } > + }, > Err(err) =3D> { > - error!("certificate validation failed - {}", err= ); > + match err { > + SslVerifyError::NoCertificate =3D> error!( > + "certificate validation failed - context= lacks current certificate" > + ), nit: i still think this could be misinterpreted and lead users to add certificates to the trust store. i think this could be made clearer with something like "certificate validation failed - could not get a certificate necessary for verifiction". though, this error case isn't likely to happen in real ussage anyway from what i can tell. -->8 snip 8<--