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

[Fabian Ebner <f.ebner@proxmox.com>]

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 <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.
> 

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::<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);
>>>            }
>>