crypto/tls: Make TLS Client Authentication work according to the spec

crypto/tls: Improve TLS Client Authentication

Fix incorrect marshal/unmarshal of certificateRequest.
Add support for configuring client-auth on the server side.
Fix the certificate selection in the client side.
Update generate_cert.go to new time package

Fixes issue 2521.

[jra, 2011-12-03 01:08:16]

Hello agl@chromium.org (cc: golang-dev@googlegroups.com, mikkel@krautz.dk),

I'd like you to review this change to
https://go.googlecode.com/hg/

[mkrautz, 2011-12-03 02:07:35]

A few comments.

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:162: AuthenticateClientRequired bool
This does not require a valid certificate if AuthenticateClientCAs == nil.

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:166: // client certificates are not verified.
Maybe: "If nil, client certificates are not verified, but are all deemed valid."
to remedy the issue above.

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:167: AuthenticateClientCAs *x509.CertPool
Maybe just 'ClientCAs' (in line with RootCAs)?

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
File src/pkg/crypto/tls/handshake_messages.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_messages.go:884: tot := 0
Maybe call this caLen like in the unmarshal case?

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
File src/pkg/crypto/tls/handshake_server.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_server.go:176: // See if there is a client
certificate
I don't like the extra code, but...

Per RFC 4346, 7.4.6:

If no suitable certificate is available, the client SHOULD send a certificate
message containingno certificates.  That is, the certificate_list structure has
a length of zero.

So this is correct. I assume you've run into something that doesn't send a
certificateMsg in response? A browser? Just curious.

[jra, 2011-12-03 02:42:44]

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:162: AuthenticateClientRequired bool
Maybe I should just s/valid//. Because what AuthenticateClientRequired triggers
is the check that the client really gave us something, and what
AuthenticateClientCAs triggers is a validation on what they gave.

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
File src/pkg/crypto/tls/handshake_messages.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_messages.go:884: tot := 0
On 2011/12/03 02:07:35, mkrautz wrote:
> Maybe call this caLen like in the unmarshal case?

Done.

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
File src/pkg/crypto/tls/handshake_server.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/handshake_...
src/pkg/crypto/tls/handshake_server.go:176: // See if there is a client
certificate
No, honestly, I didn't see that SHOULD in the spec. I made it this way out of
defensive principles: if they forget to send it, and we are allowed to handshake
with no client cert, then why not keep going?

[agl1, 2011-12-05 17:29:30]

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5448093/diff/5001/src/pkg/crypto/tls/common.go#...
src/pkg/crypto/tls/common.go:156: AuthenticateClient bool
I think the API should look something like this:

type ClientAuthType int

const (
  NoClientCert ClientAuthType = iota
  RequestClientCert
  RequireAnyClientCert
  VerifyClientCertIfGiven
  RequireAndVerifyClientCert
)

type Config struct {
  ...
  ClientAuth ClientAuthType
  ClientCAs *x509.CertPool
}

Where "Request" means send a CertificateRequest, "Require" means, "Request" +
reject a connection where the Certificate is empty. "Verify" means "Request" +
reject a connection where a non-empty certificate doesn't match "ClientCAs" (but
an empty certificate is still possible.) "RequireAndVerify" means that every
connection must have a verified certificate.

Having lead the SSL code at Google, the various types of client-auth have lead
to confusion and explicit names like this help a lot.

[agl1, 2011-12-10 21:47:17]

Please let me know when I should take another look.

Cheers

AGL

[jra, 2011-12-22 16:02:18]

Hello krautz@gmail.com, agl@golang.org (cc: golang-dev@googlegroups.com,
mikkel@krautz.dk),

Please take another look.

[jra, 2011-12-22 16:03:09]

Hello krautz@gmail.com, agl@golang.org (cc: golang-dev@googlegroups.com,
mikkel@krautz.dk),

Please take another look.

[bradfitz, 2011-12-22 19:02:03]

Could you change this CL's subject line to be more descriptive?  We've
already had 11,046 CLs which "improve" things.

On Thu, Dec 22, 2011 at 8:02 AM, <jra@nella.org> wrote:

> Hello krautz@gmail.com, agl@golang.org (cc: golang-dev@googlegroups.com,
> mikkel@krautz.dk),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**5448093/<http://codereview.appspot.com/5448093/>
>

[bradfitz, 2011-12-22 19:13:06]

(Drive-by comments.  Consider agl the real reviewer still.)

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:164: // TLS Client Authentication.
What's the default?

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
File src/pkg/crypto/tls/generate_cert.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:34: now := time.Now().In(time.UTC)
Why In(time.UTC)?

The time.Now() and time.Now().In(time.UTC) are always the exact same moment in
time.

The zone is only for printing.  I don't think this is ever printed anywhere;
just compared.

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:42: NotBefore: now.Add(-300 * time.Second),
Sub(5 * time.Minute)

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:44: NotAfter: now.Add(60 * 60 * 24 * 365 *
time.Second),
Add(365 * 24 * time.Hour)

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/x509/verify.go
File src/pkg/crypto/x509/verify.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/x509/verify.g...
src/pkg/crypto/x509/verify.go:163: if opts.Roots == nil {
this may not be needed anymore, since findVerifiedParents at tip now doesn't
crash if its receiver, Roots, is nil.

[jra, 2012-01-03 02:04:17]

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:164: // TLS Client Authentication.
On 2011/12/22 19:13:06, bradfitz wrote:
> What's the default?

Done.

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
File src/pkg/crypto/tls/generate_cert.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:42: NotBefore: now.Add(-300 * time.Second),
On 2011/12/22 19:13:06, bradfitz wrote:
> Sub(5 * time.Minute)

The docs say, "To compute t-d for a duration d, use t.Add(-d)". So I fixed it to
be more readable, but still use Add. ok?

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/tls/generate_...
src/pkg/crypto/tls/generate_cert.go:44: NotAfter: now.Add(60 * 60 * 24 * 365 *
time.Second),
On 2011/12/22 19:13:06, bradfitz wrote:
> Add(365 * 24 * time.Hour)

Done.

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/x509/verify.go
File src/pkg/crypto/x509/verify.go (right):

http://codereview.appspot.com/5448093/diff/13001/src/pkg/crypto/x509/verify.g...
src/pkg/crypto/x509/verify.go:163: if opts.Roots == nil {
On 2011/12/22 19:13:06, bradfitz wrote:
> this may not be needed anymore, since findVerifiedParents at tip now doesn't
> crash if its receiver, Roots, is nil.

Done.

[agl1, 2012-01-03 15:33:24]

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:192: c2, ok :=
x509.ParseCertificates(cert.Certificate[0])
I'd be happy to add a *x509.Certificate member to Certificate to save parsing it
each time. The member could be used if non-nil for those who wish to avoid the
parsing cost.

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:197: certToSend = &cert
do you want to label the outer for loop and break out of it here?

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server.go:177: var skipCert, skipRead bool
I think the logic here can be simplified. Remove these bools and then:

msg, err = c.readHandshake()
if err != nil {
  ...
}
if certMsg, ok = msg.(*certificateMsg); ok {
  ... handle cert message ...
 
  msg, err = c.readHandshake()
  if err != nil {
    ...
  }
} else {
  // The message wasn't a certificate...
}

... msg is ready to be processed here in either case ...

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server_test.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server_test.go:239: log.Print("Handling request
from client ",
no need to wrap lines.

[jra, 2012-01-05 00:28:51]

PTAL

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_client.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:192: c2, ok :=
x509.ParseCertificates(cert.Certificate[0])
On 2012/01/03 15:33:24, agl1 wrote:
> I'd be happy to add a *x509.Certificate member to Certificate to save parsing
it
> each time. The member could be used if non-nil for those who wish to avoid the
> parsing cost.

Done.

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_client.go:197: certToSend = &cert
On 2012/01/03 15:33:24, agl1 wrote:
> do you want to label the outer for loop and break out of it here?

Done.

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server.go:177: var skipCert, skipRead bool
Done. Thanks, I was scratching my head trying to make that part not suck, and I
just needed someone with distance to say, "make it not suck like this..." :)

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
File src/pkg/crypto/tls/handshake_server_test.go (right):

http://codereview.appspot.com/5448093/diff/13003/src/pkg/crypto/tls/handshake...
src/pkg/crypto/tls/handshake_server_test.go:239: log.Print("Handling request
from client ",
On 2012/01/03 15:33:24, agl1 wrote:
> no need to wrap lines.

Done.

[agl1, 2012-01-05 17:05:50]

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

crypto/tls: Improve TLS Client Authentication

Fix incorrect marshal/unmarshal of certificateRequest.
Add support for configuring client-auth on the server side.
Fix the certificate selection in the client side.
Update generate_cert.go to new time package

Fixes issue 2521.

R=krautz, agl, bradfitz
CC=golang-dev, mikkel
http://codereview.appspot.com/5448093

Committer: Adam Langley <agl@golang.org>