From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 9B9E61FF13C for ; Thu, 25 Jun 2026 13:19:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 95367D508; Thu, 25 Jun 2026 13:19:45 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 25 Jun 2026 13:19:40 +0200 Message-Id: Subject: Re: [PATCH proxmox v3 2/6] http: tls: use legacy behavior when PROXMOX_NEW_TLS_CHECK is not set To: "Dominik Csapak" , , X-Mailer: aerc 0.20.0 References: <20260617085949.1528300-1-d.csapak@proxmox.com> <20260617085949.1528300-3-d.csapak@proxmox.com> In-Reply-To: <20260617085949.1528300-3-d.csapak@proxmox.com> From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1782386376339 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.106 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: 32XE5VVREDSMN2MG5J2C3WPN4SWAVG3B X-Message-ID-Hash: 32XE5VVREDSMN2MG5J2C3WPN4SWAVG3B 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 VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Jun 17, 2026 at 10:59 AM CEST, Dominik Csapak wrote: > if that environment variable is not set to "1", give the openssl result > priority, and potentially ignore a given fingerprint that is not > matching. If that's the case, print a warning. > > Signed-off-by: Dominik Csapak -->8 snip 8<-- > /// Intended as an openssl verification callback. > /// > -/// The following things are checked: > +/// If the 'PROXMOX_NEW_TLS_CHECK' environment variable is set to "1", > +/// the following things are checked: > /// > /// * If no fingerprint is given, return the openssl verification result > -/// * If a fingerprint is given, do: > -/// * Ignore all non-leaf certificates/ > +/// * If a fingerprint is given, ignore all non-leaf certificates > +/// > +/// Otherwise, we trust the openssl result if the whole chain was truste= d > pub fn openssl_verify_callback( > openssl_valid: bool, > ctx: &mut X509StoreContextRef, > expected_fp: Option<&str>, > ) -> Result<(), SslVerifyError> { > let trust_openssl =3D ctx.error() !=3D X509VerifyResult::APPLICATION= _VERIFICATION; > - if expected_fp.is_none() && openssl_valid && trust_openssl { > - return Ok(()); > + > + let new_check =3D matches!(std::env::var("PROXMOX_NEW_TLS_CHECK").as= _deref(), Ok("1")); > + > + if openssl_valid && trust_openssl { > + if new_check && expected_fp.is_none() { > + return Ok(()); > + } > + > + // legacy mode: skip all valid certs except the leaf, so we can = warn if fingerprint does not match > + if !new_check && ctx.error_depth() > 0 { this check is probably wrong from what i can tell. assume a certificate chain with the certificate of the root ca being in the systems trust store and no fingerprint was provided: 1. the root ca's certificate is provided. since it is in the systems trust store `openssl_valid` and `trust_openssl` are true. we are in the old verification logic, so `new_check` is `false`. `ctx.error_depth()` is `> 0` as we aren't at the leaf yet. 2. the facts outlined hold true for all certificates in the chain until we reach the leaf certificate. `trust_openssl` is never set to false, as that would happen further down the callback (not that setting an error here would *not* make the behavior correct). 3. the leaf certificate will not enter this block, as `ctx.error_depth() > 0` no longer holds true. > + return Ok(()); > + } > } > > let cert =3D match ctx.current_cert() { > @@ -50,7 +62,7 @@ pub fn openssl_verify_callback( > }; > > if ctx.error_depth() > 0 { 3. (cont.) same here > - // openssl was not valid, but we want to continue, so save that = we don't trust openssl > + // if openssl is not valid, and we want to continue, save that w= e don't trust openssl > ctx.set_error(X509VerifyResult::APPLICATION_VERIFICATION); > return Ok(()); > } > @@ -65,6 +77,13 @@ pub fn openssl_verify_callback( > ctx.set_error(X509VerifyResult::OK); > Ok(()) > } else { > + if !new_check && openssl_valid && trust_openssl { 4. since no fingerprint was provided, we won't get here > + log::warn!( > + "Certificate chain valid, but fingerprint does not m= atch, ignoring fingerprint! To prioritize the fingerprint, set `PROXMOX_NEW= _TLS_CHECK=3D1` in your environment." > + ); > + return Ok(()); > + } > + > Err(SslVerifyError::FingerprintMismatch { > fingerprint, > expected: expected_fp.to_string(), 5. instead the `else` branch of the outer-most `if` statement here will be taken, reporting the certificate as `SslVerifyError::UntrustedCertificate`. however, that is wrong. the certificate should be trusted as openssl validated the certificate and no fingerprint was provided.