From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D8BE562094 for ; Wed, 19 Jan 2022 13:17:01 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CDB02184C7 for ; Wed, 19 Jan 2022 13:16:31 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 62B5D184BA for ; Wed, 19 Jan 2022 13:16:30 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 30CFF4506F for ; Wed, 19 Jan 2022 13:16:30 +0100 (CET) Message-ID: <8735485c-517c-e70f-02f7-3a970b27f575@proxmox.com> Date: Wed, 19 Jan 2022 13:16:24 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Content-Language: en-US To: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= , pve-devel@lists.proxmox.com References: <20211222135257.3242938-1-f.gruenbichler@proxmox.com> <20211222135257.3242938-7-f.gruenbichler@proxmox.com> <1642582327.0fpxeqo696.astroid@nora.none> From: Fabian Ebner In-Reply-To: <1642582327.0fpxeqo696.astroid@nora.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.136 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v3 proxmox-websocket-tunnel 3/4] add fingerprint validation X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 19 Jan 2022 12:17:01 -0000 Am 19.01.22 um 11:34 schrieb Fabian Grünbichler: > On January 4, 2022 12:37 pm, Fabian Ebner wrote: >> Am 22.12.21 um 14:52 schrieb Fabian Grünbichler: >>> in case we have no explicit fingerprint, we use openssl's regular "PEER" >>> verification. if we have a fingerprint, we ignore openssl altogether and >>> just verify the fingerprint of the presented leaf certificate. >>> >>> Signed-off-by: Fabian Grünbichler >>> --- >>> >>> Notes: >>> v3: switch to using hex instead of no-longer-existing digest_to_hex >>> v2: new >>> >>> src/main.rs | 47 ++++++++++++++++++++++++++++++++++++++++++++--- >>> 1 file changed, 44 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/main.rs b/src/main.rs >>> index 582214c..49d6ffe 100644 >>> --- a/src/main.rs >>> +++ b/src/main.rs >>> @@ -134,9 +134,50 @@ impl CtrlTunnel { >>> } >>> >>> let mut ssl_connector_builder = SslConnector::builder(SslMethod::tls())?; >>> - if fingerprint.is_some() { >>> - // FIXME actually verify fingerprint via callback! >>> - ssl_connector_builder.set_verify(openssl::ssl::SslVerifyMode::NONE); >>> + if let Some(expected) = fingerprint { >>> + ssl_connector_builder.set_verify_callback( >>> + openssl::ssl::SslVerifyMode::NONE, >>> + move |_valid, ctx| { >>> + let cert = match ctx.current_cert() { >>> + Some(cert) => cert, >>> + None => { >>> + eprintln!("SSL context lacks current certificate."); >>> + return false; >>> + } >>> + }; >>> + >>> + let depth = ctx.error_depth(); >>> + if depth != 0 { >>> + return true; >>> + } >> >> Sorry about my ignorance. Does using SslVerifyMode::NONE imply that >> there is an error? At depth 0? Why is it fine to return true if not? > > this is a bit.. tricky (did I mention I really really dislike openssl's > API? ;)) > > basically what we do in this branch (if we have a pinned fingerprint to > check - the regular 'connect iff trusted by system' is the else branch > below) we set our own callback that gets called for each cert along the > chain (starting at the top, ending with the leaf/end certificate, but > the order is not relevant since a single failed callback fails the whole > verification). > > for each cert (== element of the chain == depth value) we get the result > of openssl's check (`_valid`) and the X509 store context (ctx). > > the context (among other things ;)) contains information about where > (depth) in the chain we currently are: > - depth 0 == peer certificate (the one we are interested in) > - depth 1 == CA certificate (signer of peer cert, not interesting) > - depth 2 == higher CA certificate (signer of CA at 1, not interesting) > - depth X == higher CA certificate (signer of CA at X-1, not > interesting) > > all but the peer certificate are optional (peer could give us just a > self-signed certificate, or an incomplete chain). > > that the methods here are all referring to 'error' is an OpenSSL > peculiarity - it basically gives us a cert store with the current cert > and error depth set to values that are valid if we fail (error) the > verification. > > for each cert/call we do the following: > > - ensure there is a current cert in the context or fail verification > - continue verification with next element of the chain if we are not > (yet) at the peer certificate (depth != 0) > - calculate fingerprint for current (== peer) cert, or fail > - compare fingerprint with pinned/expected one, fail if not expected > > since the verification fails as soon as single callback fails, we need > to > - return false if we fail some assumption (like ctx having a current > cert, or being able to calculate a cert's FP) > - return true if the current call is at a depth we are not interested in > verifying > - return true/false depending on result of FP check if current call is at > a depth we are interested in > > I'll add a comment to the depth part that it is for skipping the CA > certs! also verify mode should technically be PEER, so I'll fix that up > as well. > Thanks for the thorough explanation! I think I get it now (especially knowing that the callback can be called multiple times helps a lot). >> >>> + >>> + let fp = match cert.digest(openssl::hash::MessageDigest::sha256()) { >>> + Ok(fp) => fp, >>> + Err(err) => { >>> + // should not happen >>> + eprintln!("failed to calculate certificate FP - {}", err); >>> + return false; >>> + } >>> + }; >>> + let fp_string = hex::encode(&fp); >>> + let fp_string = fp_string >>> + .as_bytes() >>> + .chunks(2) >>> + .map(|v| std::str::from_utf8(v).unwrap()) >>> + .collect::>() >>> + .join(":"); >>> + >>> + let expected = expected.to_lowercase(); >>> + if expected == fp_string { >>> + true >>> + } else { >>> + eprintln!("certificate fingerprint does not match expected fingerprint!"); >>> + eprintln!("expected: {}", expected); >>> + eprintln!("encountered: {}", fp_string); >>> + false >>> + } >>> + }, >>> + ); >>> } else { >>> ssl_connector_builder.set_verify(openssl::ssl::SslVerifyMode::PEER); >>> } >>