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

x/crypto/acme/autocert: assume default domain for non-SNI connections? #18785

Open
rsc opened this issue Jan 25, 2017 · 11 comments

Comments

@rsc
Copy link
Contributor

commented Jan 25, 2017

If you're running a one-domain site, autocert could potentially handle
non-SNI requests (GetCertificate("")) by picking a domain from its cached
cert set - most commonly used, most recently used, first used, shortest, some policy.
Today it returns an error, which probably turns into a TLS hangup.

This affects clients with non-SNI browsers, such as IE 8.

See discussion at end of #13523 for more.

/cc @bradfitz @agl @einthusan

@rsc rsc added this to the Unreleased milestone Jan 25, 2017

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jan 25, 2017

/cc @x1ddos

@einthusan

This comment has been minimized.

Copy link

commented Jan 26, 2017

@rsc Thanks for opening this issue. We indeed use autocert at the moment. Unfortunately I'm not good enough of a developer to help out, but thanks for the consideration of this 👍

@x1ddos

This comment has been minimized.

Copy link
Member

commented Jan 26, 2017

Interesting. As an alternative, it should be fairly easy to setup a fallback independently of what acme/autocert does. For instance:

m := autocert.Manager{
	Prompt: autocert.AcceptTOS,
	HostPolicy: autocert.HostWhitelist("example.org"),
}

getCert := func(hello *tls.ClientHelloInfo) (*tls.Certificate, error) {
  if hello.ServerName == "" {
    // Deal with this non-SNI client.
  }
  // Otherwise, let autocert proceed as usual.
  return m.GetCertificate(hello) 
}

s := &http.Server{
	Addr: ":https",
	TLSConfig: &tls.Config{GetCertificate: getCert},
}
s.ListenAndServeTLS("", "")
@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I can't think of a policy I like for multi-domain scenarios. For single domain all policies work, but if it stops being applied when a new domain is observed it will cause unexpected breakage once deployed.

We could type-assert the HostPolicy to HostWhitelist, but it's magic.

How about adding a helper to autocert?

// FallbackServerName is a GetCertificate wrapper that sets
// tls.ClientHelloInfo.ServerName to a default value when it's missing.
// It's meant to support legacy clients that don't send a server name
// indication, which would cause an error otherwise.
func FallbackServerName(name string, getCertificate func(*tls.ClientHelloInfo) (*tls.Certificate, error)) func(*tls.ClientHelloInfo) (*tls.Certificate, error)
@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

IE 8 is at 1.3% market share. Do we need to do anything here? Maybe we can just ignore it for a few more months until it's even closer to 0%.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

Command line tools are unfortunately far behind and very relevant for some applications. Which is part of why I don't think we need to solve the general problem, but just provide a documented workaround.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I'd prefer to not add new func hooks if possible.

If something like what @x1ddos said works for the power user case, I'd prefer if we add anything for it to just be a optional string field, like:

     LegacyDefaultServerName string

Or maybe something less verbose and gross, but I kinda like throwing "Legacy" in there. Maybe saying "legacy" in the docs in the enoguh.

@x1ddos

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

+1 for legacy. it's much simpler. more advanced cases can implement their own GetCertificate.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I liked a separate helper that doesn't pollute the Manager, and can be used with any GetCertificate implementation, not just the autocert one.

If we go for the field, LegacyServerName with a good doc is probably enough.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I liked a separate helper that doesn't pollute the Manager, and can be used with any GetCertificate implementation, not just the autocert one.

I see. I thought you wanted to add such a thing to x/crypto/acme/autocert then. If it lives elsewhere, I'm fine with it. Then we could just add a new Example to autocert referencing the helper's package in a comment.

@FiloSottile

This comment has been minimized.

Copy link
Member

commented Jun 6, 2018

I did mean to add it to x/crypto/acme/autocert, just not to Manager, but I accept suggestions as to where it could live. I don't have good ideas off the top of my head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.