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

Default to TLS 1.2 as minimum version #62

Merged
merged 3 commits into from
Sep 9, 2021

Conversation

jmcampanini
Copy link
Member

The stdlib's HTTP server allows TLS 1.0 / 1.1 connections by default. This library should be secure by default and require TLS 1.2 as the minimum TSL version.

This PR does the following:

  • adds a new Param called WithTLSConfig which allows the user to set the TLS configuration
  • adds a call to WithTLSConfig to the DefaultParams that sets the MinVersion to TLS 1.2

Downstream owners can opt out of using TLS 1.2 by adding their own Param or configuring their http/server themselves.

@jmcampanini jmcampanini requested a review from a team September 8, 2021 17:22
@@ -47,6 +48,7 @@ func DefaultParams(logger zerolog.Logger, metricsPrefix string) []Param {
WithUTCNanoTime(),
WithErrorLogging(RichErrorMarshalFunc),
WithMetrics(),
WithTLSConfig(&tls.Config{MinVersion: tls.VersionTLS12}),

Choose a reason for hiding this comment

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

Do we need to add MinVersion to the TLSConfig struct? Perhaps adding these options set to their defaults in the example/config.yaml may help with discoverability as well?

Copy link
Member Author

@jmcampanini jmcampanini Sep 8, 2021

Choose a reason for hiding this comment

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

hrmm. i guess it depends on how we want to treat server.TLSConfig (as opposed to our YAML object) configuration: is it runtime configurable, specifically: will a single binary be deployed to different scenarios where we want to set the minimum TLS version.

considering TLS 1.0 and 1.1 have been widely deprecated since early 2020, i feel okay with the cost of downstream consumers that require TLS 1.0/1.1 support to explicitly add code to set the server.TLSConfig.MinVersion in their services.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this as a code-only option until there are two or more places where having it as part of the configuration file would be useful.

@dreamlibrarian
Copy link
Contributor

👍

Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Setting the minimum TLS version to 1.2 when TLS is enabled sounds reasonable to me. A few things to consider:

  1. Is the tls.Config struct ignored when calling ListenAndServe instead of ListenAndServeTLS?
  2. WithTLSConfig() sounds reasonable but introduces a third way to modify these settings. Clients can currently use WithHTTPServer to provide a pre-configured server or use HTTPServer() to access and modify the underlying server instance before calling Start(). While modifying the server after creation always wins, it might not be clear how WithHTTPServer and WithTLSConfig interact if both are provided.

@@ -47,6 +48,7 @@ func DefaultParams(logger zerolog.Logger, metricsPrefix string) []Param {
WithUTCNanoTime(),
WithErrorLogging(RichErrorMarshalFunc),
WithMetrics(),
WithTLSConfig(&tls.Config{MinVersion: tls.VersionTLS12}),
Copy link
Member

Choose a reason for hiding this comment

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

I think we should leave this as a code-only option until there are two or more places where having it as part of the configuration file would be useful.


func WithTLSConfig(tlsConfig *tls.Config) Param {
return func(s *Server) error {
s.server.TLSConfig = tlsConfig
Copy link
Member

Choose a reason for hiding this comment

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

s.server is nil when Param functions are called, unless WithHTTPServer appears earlier in the parameter list.

@jmcampanini
Copy link
Member Author

jmcampanini commented Sep 9, 2021

Is the tls.Config struct ignored when calling ListenAndServe instead of ListenAndServeTLS?

ListenAndServeTLS calls srv.ServeTLS, while ListenAndServe calls srv.Serve. they end up using different listeners (and the listener is what is constructed with the TLSConfig), so the ListenAndServe doesn't need/use the TLSConfig.

WithTLSConfig() sounds reasonable but introduces a third way to modify these settings... it might not be clear how WithHTTPServer and WithTLSConfig interact if both are provided.

this makes sense to me. I've changed the impl to just create the http.Server with the TLSConfig.MinVersion already set. there is now only 1 Params way of modifying the http.Server.

@jmcampanini jmcampanini merged commit c1d302b into develop Sep 9, 2021
@jmcampanini jmcampanini deleted the jc/add-tls12-min-support branch September 9, 2021 18:02
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.

6 participants