code review 1975042: Client certificate support for crypto/tls

Client certificate support for crypto/tls

This changeset implements client certificate support in crypto/tls
for both handshake_server.go and handshake_client.go

The updated server implementation sends an empty certificateAuthorities
field in the certificateRequestMsg, thus allowing clients to send any
certificates they wish. Likewise, the client code will only respond
with its certificate when the server requests a certificate with this
field empty.
The reason for not implementing this is that I'm not sure how widely
used this list of allowed DNs of CAs is in practice. I tried a couple of
applications to see if they would send me something back in this field,
one being Apache HTTPD. They all sent an empty CA list in the
certificateRequestMsg, so it would seem that the decision of accepting
a certificate or not is often delegated to until after the TLS handshake
is done. I'd like some feedback on this if someone knows more about it.

The scripted server handshake test in handshake_server_test.go has also
been updated to include the extra handshake messages this changeset
implements.

[mkrautz, 2010-08-11 20:45:06]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

I'd like you to review this change.

[agl, 2010-08-11 22:12:52]

http://codereview.appspot.com/1975042/diff/1/2
File src/pkg/crypto/rsa/pkcs1v15.go (right):

http://codereview.appspot.com/1975042/diff/1/2#newcode149
src/pkg/crypto/rsa/pkcs1v15.go:149: HashMD5SHA1
Needs a comment here about why we need to have this.

http://codereview.appspot.com/1975042/diff/1/4
File src/pkg/crypto/tls/conn.go (right):

http://codereview.appspot.com/1975042/diff/1/4#newcode48
src/pkg/crypto/tls/conn.go:48: Certificate *x509.Certificate
This should go into peerCertificates. (peerCertificates is the server's cert for
a client connection, and the client's certs for a server connection.)

http://codereview.appspot.com/1975042/diff/1/5
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/1975042/diff/1/5#newcode180
src/pkg/crypto/tls/handshake_client.go:180: if transmitCert &&
len(c.config.Certificates) > 0 && len(c.config.Certificates[0].Certificate) > 0
{
the 'transmitCert' clause here is superfluous.

http://codereview.appspot.com/1975042/diff/1/5#newcode183
src/pkg/crypto/tls/handshake_client.go:183: certMsg.certificates =
c.config.Certificates[0].Certificate
Clients can have certificate chains. It seems that the code should support that
since it's almost more fiddly not to.

http://codereview.appspot.com/1975042/diff/1/6
File src/pkg/crypto/tls/handshake_messages.go (right):

http://codereview.appspot.com/1975042/diff/1/6#newcode683
src/pkg/crypto/tls/handshake_messages.go:683: var i int
i := 0

http://codereview.appspot.com/1975042/diff/1/6#newcode700
src/pkg/crypto/tls/handshake_messages.go:700: for _, octet := range
m.certificateTypes {
looks like you can use copy() here.

http://codereview.appspot.com/1975042/diff/1/6#newcode739
src/pkg/crypto/tls/handshake_messages.go:739: for i, _ := range
m.certificateTypes {
copy() again?

http://codereview.appspot.com/1975042/diff/1/6#newcode749
src/pkg/crypto/tls/handshake_messages.go:749: m.certificateAuthorities =
make([][]byte, numCA)
move inside the if on the next line.

http://codereview.appspot.com/1975042/diff/1/6#newcode752
src/pkg/crypto/tls/handshake_messages.go:752: if len(y) < 3 {
I think you can delete this check.

http://codereview.appspot.com/1975042/diff/1/6#newcode757
src/pkg/crypto/tls/handshake_messages.go:757: CAlen := uint16(y[0])<<16 |
uint16(y[1])
if len(y) < 2 {
  return false
}

http://codereview.appspot.com/1975042/diff/1/6#newcode758
src/pkg/crypto/tls/handshake_messages.go:758: if uint16(len(y[2:])) < CAlen {
y = y[2:] first. (Then just test len(y).)

http://codereview.appspot.com/1975042/diff/1/6#newcode763
src/pkg/crypto/tls/handshake_messages.go:763: copy(dst, y[2:2+CAlen])
remove the '2's now

http://codereview.appspot.com/1975042/diff/1/6#newcode765
src/pkg/crypto/tls/handshake_messages.go:765: y = y[2+CAlen:]
and here.

http://codereview.appspot.com/1975042/diff/1/6#newcode768
src/pkg/crypto/tls/handshake_messages.go:768: 
s/y/x/ in the above and then:

if len(x) > 0 {
  return false
}

to catch trailing garbage.

http://codereview.appspot.com/1975042/diff/1/6#newcode786
src/pkg/crypto/tls/handshake_messages.go:786: x[1] = uint8(length << 16)
these shifts are in the wrong direction.

http://codereview.appspot.com/1975042/diff/1/6#newcode789
src/pkg/crypto/tls/handshake_messages.go:789: x[4] = uint8(siglength << 8)
ditto.

http://codereview.appspot.com/1975042/diff/1/8
File src/pkg/crypto/tls/handshake_server.go (right):

http://codereview.appspot.com/1975042/diff/1/8#newcode116
src/pkg/crypto/tls/handshake_server.go:116: // Request a client certificate
We can't always send a client certificate request.

http://codereview.appspot.com/1975042/diff/1/8#newcode135
src/pkg/crypto/tls/handshake_server.go:135: certMsg, ok = msg.(*certificateMsg)
Nor can we always expect the client to send a certificate.

[mkrautz, 2010-08-12 11:57:56]

On 2010/08/11 22:12:52, agl wrote:
> http://codereview.appspot.com/1975042/diff/1/2
> File src/pkg/crypto/rsa/pkcs1v15.go (right):
> 
> http://codereview.appspot.com/1975042/diff/1/2#newcode149
> src/pkg/crypto/rsa/pkcs1v15.go:149: HashMD5SHA1
> Needs a comment here about why we need to have this.
> 
> http://codereview.appspot.com/1975042/diff/1/4
> File src/pkg/crypto/tls/conn.go (right):
> 
> http://codereview.appspot.com/1975042/diff/1/4#newcode48
> src/pkg/crypto/tls/conn.go:48: Certificate *x509.Certificate
> This should go into peerCertificates. (peerCertificates is the server's cert
for
> a client connection, and the client's certs for a server connection.)

OK. For client connections, this field could previously be accessed at the end
of the handshake
to check whether it actually sent a certificate in response to a certificate
request.
(I.e. conn.Certificate == nil meant that it didn't.)

> 
> http://codereview.appspot.com/1975042/diff/1/5
> File src/pkg/crypto/tls/handshake_client.go (right):
> 
> http://codereview.appspot.com/1975042/diff/1/5#newcode180
> src/pkg/crypto/tls/handshake_client.go:180: if transmitCert &&
> len(c.config.Certificates) > 0 && len(c.config.Certificates[0].Certificate) >
0
> {
> the 'transmitCert' clause here is superfluous.
> 
> http://codereview.appspot.com/1975042/diff/1/5#newcode183
> src/pkg/crypto/tls/handshake_client.go:183: certMsg.certificates =
> c.config.Certificates[0].Certificate
> Clients can have certificate chains. It seems that the code should support
that
> since it's almost more fiddly not to.

This was the intention to begin with.

I'm a little confused at the structure of config.Certificates. In the Config
struct, it's a slice of Certificates. That's fine.
But the Certificate struct's Certificate field is a multi-dimensional slice, so
I assumed that in the presence of a certificate chain, they would simply be
included after the main certificate in the Certificate struct. This also seems
to be the logic that handshake_server is using.  What's the Right Way?

> 
> http://codereview.appspot.com/1975042/diff/1/6
> File src/pkg/crypto/tls/handshake_messages.go (right):
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode683
> src/pkg/crypto/tls/handshake_messages.go:683: var i int
> i := 0
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode700
> src/pkg/crypto/tls/handshake_messages.go:700: for _, octet := range
> m.certificateTypes {
> looks like you can use copy() here.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode739
> src/pkg/crypto/tls/handshake_messages.go:739: for i, _ := range
> m.certificateTypes {
> copy() again?
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode749
> src/pkg/crypto/tls/handshake_messages.go:749: m.certificateAuthorities =
> make([][]byte, numCA)
> move inside the if on the next line.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode752
> src/pkg/crypto/tls/handshake_messages.go:752: if len(y) < 3 {
> I think you can delete this check.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode757
> src/pkg/crypto/tls/handshake_messages.go:757: CAlen := uint16(y[0])<<16 |
> uint16(y[1])
> if len(y) < 2 {
>   return false
> }
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode758
> src/pkg/crypto/tls/handshake_messages.go:758: if uint16(len(y[2:])) < CAlen {
> y = y[2:] first. (Then just test len(y).)
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode763
> src/pkg/crypto/tls/handshake_messages.go:763: copy(dst, y[2:2+CAlen])
> remove the '2's now
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode765
> src/pkg/crypto/tls/handshake_messages.go:765: y = y[2+CAlen:]
> and here.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode768
> src/pkg/crypto/tls/handshake_messages.go:768: 
> s/y/x/ in the above and then:
> 
> if len(x) > 0 {
>   return false
> }
> 
> to catch trailing garbage.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode786
> src/pkg/crypto/tls/handshake_messages.go:786: x[1] = uint8(length << 16)
> these shifts are in the wrong direction.
> 
> http://codereview.appspot.com/1975042/diff/1/6#newcode789
> src/pkg/crypto/tls/handshake_messages.go:789: x[4] = uint8(siglength << 8)
> ditto.
> 
> http://codereview.appspot.com/1975042/diff/1/8
> File src/pkg/crypto/tls/handshake_server.go (right):
> 
> http://codereview.appspot.com/1975042/diff/1/8#newcode116
> src/pkg/crypto/tls/handshake_server.go:116: // Request a client certificate
> We can't always send a client certificate request.

RFC 4346 (p. 43): A non-anonymous server can optionally request a certificate
from
the client, if it is appropriate for the selected cipher suite.

I interpret that as saying "any server that itself has a certificate may request
a certificate from a client".
However, if the intention is to reduce the chatter on the wire for cases where
client certificates are
irrelevant, I'm all for it.  Should it be configurable in the Config struct?

> 
> http://codereview.appspot.com/1975042/diff/1/8#newcode135
> src/pkg/crypto/tls/handshake_server.go:135: certMsg, ok =
msg.(*certificateMsg)
> Nor can we always expect the client to send a certificate.

In the code that this changeset adds, we always send a certificate request.
Thus, the way I interpret
the RFC, the client should respond with a certificate message (albeit empty, if
it doesn't have a
suitable certificate.)

RFC 4346 (p. 45): This is the first message the client can send after receiving
a
server hello done message.  This message is only sent if the server requests a
certificate.  If no suitable certificate is available, the client SHOULD send a
certificate message containing no certificates.  That is, the certificate_list
structure
has a length of zero.  If client authentication is required by the server for
the handshake
to continue, it may respond with a fatal handshake failure alert.

Then again, the word is *SHOULD*, so I'm not sure what to make of it.

[mkrautz, 2010-08-12 13:48:00]

On 2010/08/11 22:12:52, agl wrote:
> http://codereview.appspot.com/1975042/diff/1/6#newcode749
> src/pkg/crypto/tls/handshake_messages.go:749: m.certificateAuthorities =
> make([][]byte, numCA)
> move inside the if on the next line.

The file handshake_messages_test.go seems to re-use messages in its tests. If I
don't re-allocate, even for the numCA == 0 case, the test fails because
m.certificateAuthorities is full of data that shouldn't be there.  I've let it
stay outside the if.

I've fixed the other issues (except the ones I've commented on above)...

[mkrautz, 2010-08-12 13:49:01]

Hello agl, rsc (cc: golang-dev@googlegroups.com),

Please take another look.

[agl, 2010-08-12 23:42:57]

This is looking pretty good, although I'll want to patch it in locally before
landing.

As for the CertificateRequest message: yes, the RFC says that you may send it.
However, in practice, if you do, then browsers and other TLS clients will
usually pop up a dialog asking the user to select a client-side cert. That's
obviously bad for the typical use of this code, which is as an HTTPS server.

I'll get around to this tomorrow (EST) and will grab whatever patch is current
on this change list at the time.

[mkrautz, 2010-08-13 13:15:44]

On 2010/08/12 23:42:57, agl wrote:
> This is looking pretty good, although I'll want to patch it in locally before
> landing.
> 
> As for the CertificateRequest message: yes, the RFC says that you may send it.
> However, in practice, if you do, then browsers and other TLS clients will
> usually pop up a dialog asking the user to select a client-side cert. That's
> obviously bad for the typical use of this code, which is as an HTTPS server.

Right. I actually hadn't considered the dialog.  My own use of this code is not
mainly HTTPS, so it hadn't struck me :)

I'll add a field to Config to make it configurable. 

> 
> I'll get around to this tomorrow (EST) and will grab whatever patch is current
> on this change list at the time.

Great, thanks.

[agl, 2010-08-13 18:44:09]

I've fixed up the patch and am ready to land. However, I don't see you
in the CONTRIBUTORS list. Have you signed the CLA?
http://golang.org/doc/contribute.html#copyright


AGL

[mkrautz, 2010-08-13 19:10:48]

On 2010/08/13 18:44:09, agl wrote:
> I've fixed up the patch and am ready to land. However, I don't see you
> in the CONTRIBUTORS list. Have you signed the CLA?
> http://golang.org/doc/contribute.html#copyright
> 
> 
> AGL
> 

Yes, I signed it before submitting the changeset.  However, I'm curious whether
you've gotten an HTTPS client to send you a certificate?  After the realization
that it would be a good idea to make the certificate request optional, I went on
to check it out in a couple of browsers...

Chrome dev channel crashes when attempting to access sites that require
certificate authentication.  Safari and Firefox won't send me any certificates
(certMsg is empty.)

[agl, 2010-08-13 19:13:55]

On Fri, Aug 13, 2010 at 3:10 PM,  <krautz@gmail.com> wrote:
> Yes, I signed it before submitting the changeset.

Thanks for that. rsc and I are processing it now.

>  However, I'm curious
> whether you've gotten an HTTPS client to send you a certificate?  After
> the realization that it would be a good idea to make the certificate
> request optional, I went on to check it out in a couple of browsers...
>
> Chrome dev channel crashes when attempting to access sites that require
> certificate authentication.  Safari and Firefox won't send me any
> certificates (certMsg is empty.)

Chrome crashes for you? Crap. I'll see if I can repo that.

I didn't get Firefox to send a certificate, but it was upset about the
self-signed cert and I didn't bother to go through all the dialogs.
I've verified the messages by hand and openssl s_client works. It may
well be that extra magic is needed to trigger browser behaviour, but
this information isn't plumbed into http yet so it wouldn't be useful
as is anyway.


Cheers

AGL

[agl1, 2010-08-16 15:22:30]

*** Submitted as http://code.google.com/p/go/source/detail?r=ca3b155f1eaa ***

crypto/tls: client certificate support.

This changeset implements client certificate support in crypto/tls
for both handshake_server.go and handshake_client.go

The updated server implementation sends an empty CertificateAuthorities
field in the CertificateRequest, thus allowing clients to send any
certificates they wish. Likewise, the client code will only respond
with its certificate when the server requests a certificate with this
field empty.

R=agl, rsc, agl1
CC=golang-dev
http://codereview.appspot.com/1975042

Committer: Adam Langley <agl@golang.org>