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

Expose granular control of TLS configuration for OpenSSL and Rustls #47

Closed
jen20 opened this issue Oct 5, 2019 · 2 comments
Closed

Comments

@jen20
Copy link
Contributor

jen20 commented Oct 5, 2019

Feature Request

Crates

  • tonic

Motivation

Currently a small subset of of the configuration options available for OpenSSL or Rustls are exposed by the builder API, via the openssl_tls and rustls_tls functions:

  • Certificate and corresponding private key for servers (via openssl_tls for servers and rustls_tls for clients)
  • A root certificate and domain name for client endpoints (via openssl_tls for servers and rustls_tls for clients)

Clearly many more configuration points exist in each of the supported TLS libraries which are not exposed. If these are needed, users lose the benefit of the 'batteries included' nature of the transport implementations which are part of Tonic.

Proposal

Currently the TLS library in use can be switched out in Tonic by enabling the appropriate Cargo feature, and changing the method name called from the builder - the arguments are compatible for each.

Wrapping every available option in a cross-TLS library compatible way in this way could lead to a sprawling API, and there may still be divergence between libraries. Instead, it would be better to allow the native configuration structure from each library to be passed into the builders:

This does add considerable complexity though, and there are some cases where the existing mechanism is sufficient - so it is desirable to keep the simpler interface for those that prefer that.

It's probably better for these different mechanisms for configuring TLS to be mutually exclusive with one another - that is, it should not be possible to pass in both a ServerConfig and an Identity. Consequently, I propose making the existing methods on the builder take an enumeration instead, with separate variants for each configuration mechanism.

Alternatives

An alternative to the enumeration is to have builder methods for each configuration mechanism and ensure via some other mechanism that they are not used together. This seems more complex to me, though there is currently not much in the way of precedent for the style suggested above in the builder API so this might be preferable.

@LucioFranco
Copy link
Member

@jen20 Thank you for writing this detailed issue up! I think you are 100% correct on your ideas. This would be super handy to include.

Just thinking about this now, something that is kinda done in reqwest is that Identity and Certifiate both contain the logic to know how to apply themselves against the specific type of tls raw config. This could mean we could store an Option<Identity> and Option<Certificate> in the builder as functions that are not related to rustls or openssl, then use the same method as in reqwest to expose enabling openssl or rustls. Then we can provide a separate enable function that allows you to pass a raw config. This would mix the batteres included concept very well since we can still do the work for you for adding the basic certificate but you can enable additional features. If you want to completely by pass how certs are added to say support RSA you could pass in a custom config.

What do you think about that?

@jen20
Copy link
Contributor Author

jen20 commented Oct 8, 2019

Fixed by #48.

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

No branches or pull requests

2 participants