Re: [pve-devel] [PATCH v3 proxmox-websocket-tunnel 3/4] add fingerprint validation

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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 <f.gruenbichler@proxmox.com>
>> ---
>> 
>> 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.

> 
>> +
>> +                    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::<Vec<&str>>()
>> +                        .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);
>>           }
>