fedimint / tonic_lnd

Rust implementation of LND RPC client using async GRPC library `tonic`
6 stars 16 forks source link

Version bumps #5

Closed okjodom closed 1 year ago

okjodom commented 1 year ago
okjodom commented 1 year ago

With tonic v0.6, tonic::transport::ClientTlsConfig can take a raw rustls client config built with custom cert verifier. We establish a http transport Channel connection with the following code snippet:

async fn config(
        path: impl AsRef<Path> + Into<PathBuf>,
        address: String,
    ) -> Result<tonic::transport::Channel, ConnectError> {
        let mut tls_config = rustls::ClientConfig::new();
        tls_config
            .dangerous()
            .set_certificate_verifier(std::sync::Arc::new(CertVerifier::load(path).await?));
        tls_config.set_protocols(&["h2".into()]);

        let chan = tonic::transport::Endpoint::from_shared(address.clone())
            .map_err(|e| InternalConnectError::InvalidAddress {
                address: address.clone(),
                error: Box::new(e),
            })?
            .tls_config(tonic::transport::ClientTlsConfig::new().rustls_client_config(tls_config)) // BLOCKER: since tonic v0.7, we cannot use ClientTlsConfig directly
            .map_err(InternalConnectError::TlsConfig)?
            .connect()
            .await
            .map_err(InternalConnectError::TlsConfig)?;

        Ok(chan)
    }

After breaking change, tonic::transport::ClientTlsConfig no longer allows direct rustls client config, so we need an alternative path provided we are still using a custom cert verifier.

async fn config(
        path: impl AsRef<Path> + Into<PathBuf>,
        address: String,
    ) -> Result<tonic::transport::Channel, ConnectError> {
        // let mut tls_config = rustls::ClientConfig::new();
        // tls_config
        //     .dangerous()
        //     .set_certificate_verifier(std::sync::Arc::new(CertVerifier::load(path).await?));
        // tls_config.set_protocols(&["h2".into()]);

        let tls_config = ClientConfig::builder()
            .with_safe_defaults()
            .with_custom_certificate_verifier(Arc::new(CertVerifier::load(path).await?))
            .with_no_client_auth();

        let mut hc = HttpConnector::new();
        hc.enforce_http(false);

        let conn = tower::ServiceBuilder::new()
            .layer_fn(move |s| {
                hyper_rustls::HttpsConnectorBuilder::new()
                    .with_tls_config(tls_config.clone())
                    .https_or_http()
                    .enable_http2()
                    .wrap_connector(s)
            })
            .service(hc);

        // let chan = tonic::transport::Endpoint::from_shared(address.clone())
        //     .map_err(|e| InternalConnectError::InvalidAddress {
        //         address: address.clone(),
        //         error: Box::new(e),
        //     })?
        //     .tls_config(tonic::transport::ClientTlsConfig::new().rustls_client_config(tls_config)) // BLOCKER: since tonic v0.7, we cannot use ClientTlsConfig directly
        //     .map_err(InternalConnectError::TlsConfig)?
        //     .connect_with_connector(connector)
        //     .await
        //     .map_err(InternalConnectError::TlsConfig)?;

        let chan = tonic::transport::Endpoint::from_shared(address.clone())
            .map_err(|e| InternalConnectError::InvalidAddress {
                address: address.clone(),
                error: Box::new(e),
            })?
            // WIP: Create a HHTP/2 transport channel with TLS
            .connect_with_connector(conn)
            .await
            .map_err(InternalConnectError::TlsConfig)?;

        Ok(chan)
    }
Kixunil commented 1 year ago

Oh, crap, removing config is terrible.

okjodom commented 1 year ago

@sr-gi I believe we cracked it! Please take a look at the latest iteration in PR 🥳

okjodom commented 1 year ago

Additional concerns with this version bump:

cc @Kixunil

dpc commented 1 year ago

I'm a bit out of context here, but on my level of understanding LGTM. Please ping me once you want to land and there are no outstanding issues, if you need someone to approve.

sr-gi commented 1 year ago

For context, I think we could create our own custom certificate verifier based on: https://github.com/briansmith/webpki/pull/127, which allows self-signed certs, and then pass it to rustls via with_custom_certificate_verifier

sr-gi commented 1 year ago

For context, I think we could create our own custom certificate verifier based on: briansmith/webpki#127, which allows self-signed certs, and then pass it to rustls via with_custom_certificate_verifier

I think this may actually be harder than I though. It requires copying a lot of functionality from webpky given most of it is not public. I think we should, at least, verify that the end_entity cert is within the ones presented.