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

crypto/tls: Allow sending unrecognized_name alert from GetCertificate #18377

Open
titanous opened this Issue Dec 19, 2016 · 12 comments

Comments

Projects
None yet
7 participants
@titanous
Copy link
Member

titanous commented Dec 19, 2016

RFC 6066 says:

If the server understood the ClientHello extension but does not recognize the server name, the server SHOULD take one of two actions: either abort the handshake by sending a fatal-level unrecognized_name(112) alert or continue the handshake.

Currently returning an error from the GetCertificate hook results in a generic internal_error fatal alert. To implement the first action in the RFC, there should be a way to return an unrecognized_name fatal alert to the client when a GetCertificate hook is unable to find a certificate for the server name specified in the ClientHello. I propose the addition of a special error variable to the crypto/tls package that triggers this alert:

// ErrUnrecognizedName sends an unrecognized_name fatal alert to the client
// when returned from a GetCertificate hook function call.
var ErrUnrecognizedName = errors.New("crypto/tls: unrecognized server name")
@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 19, 2016

Seems reasonable. @agl?

@bradfitz bradfitz added this to the Go1.9Maybe milestone Dec 19, 2016

@bradfitz

This comment has been minimized.

Copy link
Member

bradfitz commented Dec 19, 2016

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

@mholt

This comment has been minimized.

Copy link

mholt commented Dec 19, 2016

They will: mholt/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.)

@agl

This comment has been minimized.

Copy link
Contributor

agl commented Dec 19, 2016

We could just make a callback error always trigger unrecognized_name.

@mholt

This comment has been minimized.

Copy link

mholt commented Dec 19, 2016

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)

@agl

This comment has been minimized.

Copy link
Contributor

agl commented Dec 20, 2016

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.

@titanous

This comment has been minimized.

Copy link
Member Author

titanous commented Dec 20, 2016

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.

@agl

This comment has been minimized.

Copy link
Contributor

agl commented Dec 20, 2016

(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.)

@estark37

This comment has been minimized.

Copy link

estark37 commented Dec 20, 2016

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).

@Luit

This comment has been minimized.

Copy link

Luit commented Feb 28, 2017

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.

tmthrgd added a commit to tmthrgd/tls-tris that referenced this issue Mar 24, 2017

Send an “unrecognized name” alert (112) when no certificate is found
This sends an “unrecognized name” alert (112) instead
of an “internal error” (80) when no certificate is found.

This addresses golang/go#18377.

tmthrgd added a commit to tmthrgd/tls-tris that referenced this issue Mar 25, 2017

Send an “unrecognized name” alert (112) when no certificate is found
This sends an “unrecognized name” alert (112) instead
of an “internal error” (80) when no certificate is found.

This addresses golang/go#18377.
@tmthrgd

This comment has been minimized.

Copy link
Contributor

tmthrgd commented Apr 11, 2017

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.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.9Maybe Jun 28, 2017

@mholt

This comment has been minimized.

Copy link

mholt commented Aug 9, 2017

@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).

@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Nov 16, 2017

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