Seems reasonable. @agl?

I suppose the bigger question is why this matters to you. How did this come up? Do clients & servers implement this in the wild?

They will: caddyserver/caddy#1303 (comment)

With this change, the browser will have a specific kind of error message to display for the user rather than making it look like TLS is broken on the server. Mostly a "nice to have" feature. :)

(To answer your question, it just came up in a discussion about changing how Caddy handles handshakes with server names it does not have a certificate for; right now, like nginx, it will serve a 'default' certificate, but we were thinking of changing it so that it returns a TLS alert rather than what looks like an attack.)

We could just make a callback error always trigger unrecognized_name.

We could just make a callback error always trigger unrecognized_name.

True, but I can also think of countless other types of problems that could occur in more advanced situations of getting a certificate, even if the name is recognized. For example, Caddy is subject to rate limits and, even if its configuration recognizes the name in the handshake, it may not be able to get a certificate for that hostname. In my opinion, that's different than the server not being configured at all to connect with that name.

The special error value fundamentally says, "I don't know this identity/name, what are you even doing??" (client's mistake) as opposed to "I know this name, but [something went wrong] and I can't prove it..." (server's mistake)

But why expose that difference to a client? The fatal alert values are very rough already and few of them are terribly meaningful. If unrecognized_name could mean to a client "you might have better luck with a different SNI value" then maybe that's of value, although I'm skeptical. You seem to assume that Chrome would display a different error message for this but I consider that unlikely.

Thus I can see that perhaps unrecognized_name is a better alert when this callback fails, but I'm unsure that we should add complexity to let the callback try and control the alert.

You seem to assume that Chrome would display a different error message for this but I consider that unlikely.

@estark37 from the Chrome team suggests in this comment that the correct alert value could be used instead of the existing heuristic that triggers for some mismatched certificates.

(I think Emily was just saying that a fatal alert might be better for users than returning a certificate that's not going to match. But I've asked her to be sure.)

My main feeling is that an alert is better than a certificate error, and I don't feel too strongly about when unrecognized_name is sent vs internal_error. Hypothetically, I could imagine a future in which Chrome treats unrecognized_name specially for certain names. But I wouldn't make any plans based on that, as I haven't really thought it through and it would be pretty low-priority (the existing heuristic only fires for a very small percentage of certificate errors).

I knew about the unrecognized_name alert from a server (which appears to be Apache) configured to deny connections for unrecognized names. The Chrome error I got was the generic SSL protocol error page though, only showing the internal name for the protocol error. I'd prefer to see that alert over an internal_error one though.

In my opinion, if GetCertificates is configured to fetch files and fails at that, internal_error is still the "right" alert, so I'd vote for having just a specific marker error to trigger an unrecognized_name as described in this issue.

While I am strongly in favour of this proposal, I don't think a sentinel error is the right way to go about this.

The GetCertificate callback has three return cases:

1. (certificate, nil) selects a certificate to use for the handshake,
2. (nil, error) emits a fatal internal_error alert,
3. (nil, nil) continues with the existing certificate selection logic using Certificates or NameToCertificate - essentially stating that the callback doesn't have a certificate to match.

The current behaviour is to emit an internal_error alert with tls: no certificates configured if GetCertificate returns (nil, nil) and Certificates is empty. I would propose that in this case, and only in this case, an unrecognized_name alert be sent to the client instead.

This means the current behaviour of "no certificates configured" + internal_error is maintained for people who fail to set either GetCertificate or Certificates, and an unrecognized_name can be signalled by returning (nil, nil) - passing the buck - and leaving Certificates empty.

Not only do I think this is much cleaner than using a sentinel error, it avoids exporting any of the crypto/tls alert code or internal TLS implementation details - and I think that's actually a really strong positive.

It also doesn't require any sort of compatibility guarantee. Obviously a ErrUnrecognizedName error would be bound by the Go1 compatibility guarantee, which seems like an unnecessary negative here in my mind.

I'm happy to take this on and submit a change for review, if others agree about doing it this way. It's a relatively minor change.

@tmthrgd I like that proposal. At least, from the perspective of my use case (Caddy), it should fit nicely since we don't use the Certificates field at all. (We make heavy use of GetCertificate).

GetCertificate is not the only place to process the SNI from the client. Another place is GetConfigForClient where returning (nil, nil) results in using the default configuration.

Context: toying with a dumb proxy server that uses GetConfigForClient instead of plain GetCertificate due to additional side-effects based on the SNI.

We can just make unrecognized_name the alert we send whenever there are no certificates available. That can be reached easily both from GetCertificate and GetConfigForClient by leaving Certificates empty. (Let's not document it for now not to commit to it under the compatibility promise.)

I would accept a CL that implements that. (Once the tree opens.)

I'll implement this while reworking certificate selection in #32426.

Change https://golang.org/cl/205059 mentions this issue: crypto/tls: select only compatible chains from Certificates