I'm setting up a server now to try run a fuzzer on this package and see if I can uncover anything. I'll report back if I discover anything related to this bug.

FYI, you can reproduce this without fuzzing just by sending a cert that's not valid for some reason (e.g. use SHA1 for the signature algorithm).

I think the question is what the behavior should be when an invalid certificate is received. I'm sure the client certificate in this case was being rejected for a good reason.

Hmm. Normally, we try not to be lenient on peers sending invalid data. Why would a client legitimately send a broken certificate? Is there a compatibility case to be made for it? (Like, if we consider invalid certificates that clients have to serve to certain legacy servers, or something like that.)

There's also the fact that if we ignore it, we have no way to report that, so it would make for pretty hard to debug scenarios.

BTW, I feel like if we ignore invalid certificates we should do so also for VerifyClientCertIfGiven.

I wish I knew the story of how this fellow came to install this cert. The problem was explained by him to me as "some websites block Brave" and he had no idea why that was. (I don't think the fact that he was using Brave has anything to do with it.)

I can understand not wanting to follow Postel's Law in the default case, but the alternative is going to be that I have to set up a completely separate HTTPS endpoint for API requests, so that I can accept client certificates without locking out important users. Or patch the stdlib, which is probably less operational overhead than the former.

If we set NoClientAuth, we're already ignoring client certificates. So I fail to see that doing so is somehow dangerous. For reporting warnings, http.Server contains a Logger, maybe tls.Config should as well?

At a minimum, I feel the consequences of setting RequestClientCert (some users will be unable to connect) should be documented somewhere. I would have never figured out the problem from the error message without grepping for it in the source; the words "client certificate" appear in the pre-TLS1.3 error, but not in the newer.

Is there any reason to believe this will happen again? It's the first time it's reported here, as far as I know.

Also, if a client sends an invalid client certificate, I suspect they would rather learn that there is an issue rather than have it be silently ignored. Most clients don't send certificates, and the ones that do should be presumed to know what they are doing.

BTW, if you are setting ClientCAs, clients should only send certificates that chain to acceptable CAs. It's kind of surprising Brave would send a random certificate.

We should 100% make the error message better, marking this NeedsFix for improving the error message.

I just checked, and Nginx ignores client certificate errors when ssl_verify_client is set to optional. Apache appears to do likewise. Go's web server appears to be the outlier here.

I understand the client may want to know that their certificate isn't being used, but "why isn't my certificate being accepted?" is a very different debugging problem than "why can't I connect?" In this case, I can't tell the user "sorry, your browser is broken," since they're able to use it to connect to most other sites. From their perspective, my web site is "always down."

It occurs to me that maybe Github isn't the best place to debate this; should I cross-post this somewhere for broader input?

An important detail here is whether the certificate is completely invalid or not. If not, then we might have a compatibility problem of the shape "I need to send this client certificate for these legacy servers but Go thinks it's invalid", and we might have to accommodate it. Otherwise, why not tell the user to drop the broken client certificate?

(GitHub issues is where we have most technical discussions.)

I don't have access to the original certificate, but let me see if I can come up with a plausible test case. The returned error message indicates verifyHandshakeSignature failed, but doesn't bubble up the error message from that function. That function can fail with a valid signature because an unsupported algorithm was specified.

Telling the user to "drop the broken client certificate" would probably work in cases where one wasn't providing phone support to a very important non-technical customer in a different organization, i.e. if I had any access to their computer. As it stands, they seem unlikely to be convinced that this isn't a vast conspiracy against Brave, since their browser only fails on some small subset of sites.

Re: forum, was just making sure. :)

Mostly for my own edification, I looked up what RFC 8446 has to say about this:

Also, if some aspect of the certificate chain was unacceptable (e.g., it was not signed by a known, trusted CA), the server MAY at its discretion either continue the handshake (considering the client unauthenticated) or abort the handshake.

Any endpoint receiving any certificate which it would need to validate using any signature algorithm using an MD5 hash MUST abort the handshake with a "bad_certificate" alert. SHA-1 is deprecated, and it is RECOMMENDED that any endpoint receiving any certificate which it would need to validate using any signature algorithm using a SHA-1 hash abort the handshake with a "bad_certificate" alert. For clarity, this means that endpoints can accept these algorithms for certificates that are self-signed or are trust anchors.

Thus either Go's current behavior (reject the handshake) or Apache/Nginx/etc.'s (consider the client unauthenticated) is acceptable. Even if it's not the default, I would really like a configuration parameter for the latter. I would be more than happy to submit a patch to this effect, if it has any chance of being accepted.

Sorry, but the existence of a single client with an unspecified broken configuration is not enough grounds to extend the API surface or make the stack less strict. The cases in which I can see us making a different decision are A) if the client turns out to be sending that certificate for a legitimate (even if incompatible) reason or B) if this proves a much more common issue than we've seen so far. The part of the spec you quoted suggests that even A) is not absolute.

No worries, I'll just fork crypto/tls on my end to mimic the Nginx behavior. Adding RequestClientCertIgnoreInvalid to ClientAuthType turned out to be pretty easy.

Appreciate your looking into this, thanks!

As I got into the code to improve the error messages, I realized that this is not an invalid certificate, but an invalid signature by that certificate on the handshake transcript.

This should not be possible if both stacks are behaving correctly (as by then the signature algorithms have been negotiated successfully), so there is a bug either in our stack or in theirs. Maybe even in the Brave or OS code that generates the signatures.

If you can get more details (exact browser version, OS, certificate type, a PCAP with a keylog...) please open a new issue (this one will be fixed by my error message CL) and we should investigate.

Change https://golang.org/cl/204157 mentions this issue: crypto/tls: improve error messages for invalid certificates and signatures