TLS cert validation can fail if with multiple certificates

fedimint / tonic_lnd

Rust implementation of LND RPC client using async GRPC library `tonic`

6 stars 16 forks source link

TLS cert validation can fail if with multiple certificates #12

Closed justinmoon closed 10 months ago

justinmoon commented 11 months ago

I was just trying to use this with a TLS cert provided by Voltage. The TLS cert they gave me looked like this:

-----BEGIN CERTIFICATE-----
<cert1>
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
<cert2>
-----END CERTIFICATE-----
-----BEGIN CERTIFICATE-----
<cert-3>
-----END CERTIFICATE-----

This check was failing https://github.com/fedimint/tonic_lnd/blob/1bcb98334fe5ea2d2255df693331d3ced6b298a6/src/lib.rs#L327-L333

With this hack to sort the certificates, then the check passed. So I think we need to do something like to sort the certificates in some deterministic way before checking that local and server certificates match. But it really feels like rustls should have a more official way of doing this.

elsirion commented 11 months ago

Could you post an example of these certificates? I think your solution is good, if anything it's still too strict, we should probably allow supplying more certificates in the file than the server returns and just check that all certs returned by the server are in the file.

douglaz commented 11 months ago

We could try to use https://docs.rs/rustls/latest/rustls/client/struct.WebPkiServerVerifier.html

But I'm not sure it will fit our use case. If we're going to implement ServerCertVerifier then there is no example or help function on rustls. Perhaps we can find something on webpki.

Anyway, I think the hack is fine for now.

elsirion commented 11 months ago

Why do we use that fork again in the first place? Would it be useful to upstream a change and then rebase on that?

douglaz commented 11 months ago

Why do we use that fork again in the first place? Would it be useful to upstream a change and then rebase on that?

I think there are different reasons, but I'm following this project for a long time and found out the upstream isn't agile enough. For instance this PR was a great improvement and it was never merged: https://github.com/Kixunil/tonic_lnd/pull/20

And now we have CI, which is still missing on the original repository.

elsirion commented 11 months ago

Thx for the context. Then I'd say the plan is:

Get @justinmoon's changes or a more sophisticated version into our fork
Release our fork
update tonic_lnd in Fedimint, possibly backporting to 0.2 for an eventual 0.2.1

justinmoon commented 10 months ago

Closed by https://github.com/fedimint/tonic_lnd/pull/18