Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate issues in the custom tonic TLS handling used in anemo #3255

Closed
muXxer opened this issue Oct 14, 2024 · 1 comment
Closed

Investigate issues in the custom tonic TLS handling used in anemo #3255

muXxer opened this issue Oct 14, 2024 · 1 comment
Assignees
Labels
node Issues related to the Core Node team

Comments

@muXxer
Copy link
Contributor

muXxer commented Oct 14, 2024

Investigate issues in the custom TLS handling that surfaced during the re-fork, documenting findings and suggesting potential improvements.

#2882

@muXxer muXxer added the node Issues related to the Core Node team label Oct 14, 2024
@semenov-vladyslav
Copy link
Contributor

semenov-vladyslav commented Oct 25, 2024

The issue stems in tonic not very nicely handling tls connections with custom connectors.

ChannelPool's clients connect using tonic::transport::channel::Endpoint::connect_with_connector function passing hyper_rustls::HttpsConnector which handles TLS connection. But connect_with_connector wraps this connector into service::Connector (not a public struct) which cannot understand that the inner connector (HttpsConnector) already handles TLS and does handle TLS if feature = "tls" is enabled: if the URI scheme is "https" and no tls config is set then service::Connector fails with HttpsUriWithoutTlsSupport. Note, it feature = "tls" is not enabled then service::Connector doesn't care about URI scheme being "https" which is weird and inconsistent IMHO. This issue has been known for a while now.

A recent PR makes it possible to workaround this issue: just reimplement connect_with_connector but without any service::Connector wrapping, this is now possible as the PR makes tonic::transport::Channel::connect function public and which is essential in reimplementing connect_with_connector. Unfortunately, it is not yet available in tonic-0.12.3 and will be available in the upcoming tonic-0.13.0.

So, the solutions seems as follows:

  1. wait for tonic-0.13.0
  2. replace the following lines in tonic_network:
match endpoint
    .connect_with_connector(https_connector.clone())
    .await

with

match {
    let mut connector = hyper_timeout::TimeoutConnector::new(https_connector.clone());
    connector.set_connect_timeout(Some(timeout));
    tonic::transport::Channel::connect(connector, endpoint.clone()).await
}

Also, don't forget to add hyper-timeout = "0.5" as dependency.

This solution has been tested with the tonic's master branch that has PR merged as follows:

  1. run iota start command (built with tonic's "tls" feature enabled);
  2. observe that HttpsUriWithoutTlsSupport error is not reported in the log output.

@semenov-vladyslav semenov-vladyslav self-assigned this Oct 25, 2024
@muXxer muXxer closed this as completed Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node Issues related to the Core Node team
Projects
None yet
Development

No branches or pull requests

2 participants