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

feat(transport): Expose more granular control of TLS configuration #48

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

jen20
Copy link
Contributor

@jen20 jen20 commented Oct 5, 2019

This is the start of an implementation of the solution for exposing more granular control over TLS as described in #47.

It is currently incomplete, but does not lose any functionality over the previous API and should help inform whether this is the path we want to go down or not.

@jen20 jen20 changed the title Jen20/expose tls configuration Expose more granular control of TLS configuration Oct 5, 2019
@jen20 jen20 force-pushed the jen20/expose-tls-configuration branch 3 times, most recently from 43f60fa to f624e8d Compare October 7, 2019 12:22
@jen20
Copy link
Contributor Author

jen20 commented Oct 7, 2019

After playing with this some more, and the suggestions in #47 I've reworked this to use a new builder for TLS options. This allows us to expose several different levels of abstraction for configuring TLS while not creating sprawl in the Server and Endpoint builders.

@jen20 jen20 force-pushed the jen20/expose-tls-configuration branch 4 times, most recently from cb010c9 to 87d6549 Compare October 7, 2019 13:02
@jen20 jen20 marked this pull request as ready for review October 7, 2019 13:09
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks fantastic! Really happy with how it turned out. I left some comments inline.

.gitignore Outdated Show resolved Hide resolved
tonic/src/transport/endpoint.rs Outdated Show resolved Hide resolved
tonic/src/transport/endpoint.rs Outdated Show resolved Hide resolved
tonic/src/transport/endpoint.rs Outdated Show resolved Hide resolved
}

/// Sets the CA Certificate against which to verify the server's TLS certificate.
pub fn ca_certificate(mut self, ca_certificate: Certificate) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about this taking &mut self and returning &mut Self like the other builders?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a look at this, but we have the issue that we want to take the SslConnector and ClientConfig values from the builder? Happy to look at alternatives though.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the configs should be cheaply clonable right? So that means we can take a &self and get a full version.

tonic/src/transport/endpoint.rs Outdated Show resolved Hide resolved
tonic/src/transport/endpoint.rs Outdated Show resolved Hide resolved
tonic/src/transport/service/tls.rs Outdated Show resolved Hide resolved
tonic/src/transport/service/tls.rs Outdated Show resolved Hide resolved
tonic/src/transport/service/tls.rs Outdated Show resolved Hide resolved
@jen20 jen20 force-pushed the jen20/expose-tls-configuration branch 3 times, most recently from 6e520f5 to 7f0dfca Compare October 8, 2019 10:06
@jen20
Copy link
Contributor Author

jen20 commented Oct 8, 2019

Thanks for the feedback @LucioFranco - most of the issues you raised are now addressed, with a couple of additional questions inline.

This commit reworks TLS configuration of both servers and endpoints in
order to provide a more flexible API. We now add options to configure
the selected TLS library using the appropriate 'native' configuration
structures, as well as retaining the existing simplier interface which
is compatible with both.

The new API can also be easily extended to support simple interfaces for
configuring mTLS and a range of other options without creating sprawl
in the builders for `Server` and `Endpoint`.
@jen20 jen20 force-pushed the jen20/expose-tls-configuration branch from 7f0dfca to 95bc8ee Compare October 8, 2019 15:23
Copy link
Member

@LucioFranco LucioFranco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for this! Super happy to land this!

@LucioFranco LucioFranco changed the title Expose more granular control of TLS configuration feat(transport): Expose more granular control of TLS configuration Oct 8, 2019
@LucioFranco LucioFranco merged commit 8db3961 into hyperium:master Oct 8, 2019
@jen20
Copy link
Contributor Author

jen20 commented Oct 8, 2019

Thanks! Next up I’m going to look at mutual TLS, I’ll likely have a write up of the options some time tomorrow morning (EU time).

@jen20 jen20 deleted the jen20/expose-tls-configuration branch October 8, 2019 16:00
blittable pushed a commit to blittable/tonic that referenced this pull request Oct 22, 2019
…yperium#48)

This commit reworks TLS configuration of both servers and endpoints in
order to provide a more flexible API. We now add options to configure
the selected TLS library using the appropriate 'native' configuration
structures, as well as retaining the existing simplier interface which
is compatible with both.

The new API can also be easily extended to support simple interfaces for
configuring mTLS and a range of other options without creating sprawl
in the builders for `Server` and `Endpoint`.
brentalanmiller pushed a commit to brentalanmiller/tonic that referenced this pull request Oct 6, 2023
brentalanmiller pushed a commit to brentalanmiller/tonic that referenced this pull request Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants