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

proposal: x/crypto/acme/autocert: Tell users that Manager.TLSConfig() returns a config that allows TLS 1.0 #63890

Closed
1f604 opened this issue Nov 1, 2023 · 1 comment
Labels
Milestone

Comments

@1f604
Copy link

1f604 commented Nov 1, 2023

I am using the example code from here: https://pkg.go.dev/golang.org/x/crypto/acme/autocert#example-Manager

Currently the Manager.TLSConfig() function takes no parameters and returns a TLSConfig that allows TLS 1.0.

The example code suggests that we should pass the return value directly into the server struct like this:

	s := &http.Server{
		Addr:      ":https",
		TLSConfig: m.TLSConfig(),
	}

Now, if this proposal goes through then this won't be as big of a problem: #62459

However, what if I want to set MinVersion to tls.VersionTLS13 instead of the proposed TLS 1.2?

So I created my own wrapper function for this:

type SafeTLSAutoCertManager struct {
	_private *autocert.Manager
}

// SecureTLSConfig creates a new secure TLS config suitable for net/http.Server servers
func (m *SafeTLSAutoCertManager) GetSecureTLSConfig() *tls.Config {
	return &tls.Config{ //nolint:exhaustruct // I'm using Valsorda's example config: https://blog.cloudflare.com/exposing-go-on-the-internet/
		GetCertificate: m._private.GetCertificate,
		NextProtos: []string{
			"h2", "http/1.1", // enable HTTP/2
			acme.ALPNProto, // enable tls-alpn ACME challenges
		},
		PreferServerCipherSuites: true,
		CurvePreferences: []tls.CurveID{
			tls.X25519, // Go 1.8 only
			tls.CurveP256,
		},
		MinVersion: tls.VersionTLS13,
	}
}

But then I thought that maybe the library itself could be more user-friendly and provide some kind of hint to the user that they should not blindly use the return value from TLSConfig() and that they need to make modifications to the return value before they use it in their server.

Maybe TLSConfig() could take some parameters? Or maybe the docs could be improved?

What do you think?

@1f604 1f604 added the Proposal label Nov 1, 2023
@gopherbot gopherbot added this to the Proposal milestone Nov 1, 2023
@FiloSottile
Copy link
Contributor

We generally believe in making the defaults safe any time we can, rather than telling the user they should change them. That's why we are doing #62459.

Note that if you want to limit to TLS 1.3 you can do something like

config := m.TLSConfig()
config.MinVersion = tls.VersionTLS13

without reimplementing the whole config.

Still, I think there is nothing autocert-specific in the choice of MinVersion, so it's something which we should get right at the crypto/tls level (which indeed is #62459), rather than in the autocert docs.

@1f604 1f604 closed this as completed Nov 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants