[go] crypto/tls: add VerifyPeerCertificate to tls.Config

[Joshua Boelter (Gerrit)]

Joshua Boelter uploaded a change:
https://go-review.googlesource.com/26654

crypto/tls: add VerifyPeerCertificate to tls.Config

VerifyPeerCertificate returns an error if the peer should not be
trusted. It will be called after the initial handshake and before
any other verification checks on the cert or chain are performed.
This provides the callee an opportunity to augment the certificate
verification.

If VerifyPeerCertificate is not nil and returns an error,
then the handshake will fail.

Fixes #16363

Change-Id: I6a22f199f0e81b6f5d5f37c54d85ab878216bb22
---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go
M src/crypto/tls/handshake_server_test.go
M src/crypto/tls/tls_test.go
M src/net/http/transport.go
7 files changed, 121 insertions(+), 1 deletion(-)



diff --git a/src/crypto/tls/common.go b/src/crypto/tls/common.go
index 9fc7420..32df896 100644
--- a/src/crypto/tls/common.go
+++ b/src/crypto/tls/common.go
@@ -304,6 +304,16 @@
// first element of Certificates will be used.
GetCertificate func(clientHello *ClientHelloInfo) (*Certificate, error)

+ // VerifyPeerCertificate returns an error if the peer should not be
+ // trusted. It will be called after the initial handshake and before
+ // any other verification checks on the cert or chain are performed.
+ // This provides the callee an opportunity to augment the certificate
+ // verification.
+ //
+ // If VerifyPeerCertificate is not nil and returns an error,
+ // then the handshake will fail.
+ VerifyPeerCertificate func(peer []*x509.Certificate) error
+
// RootCAs defines the set of root certificate authorities
// that clients use when verifying server certificates.
// If RootCAs is nil, TLS uses the host's root CA set.
@@ -430,6 +440,7 @@
Certificates: c.Certificates,
NameToCertificate: c.NameToCertificate,
GetCertificate: c.GetCertificate,
+ VerifyPeerCertificate: c.VerifyPeerCertificate,
RootCAs: c.RootCAs,
NextProtos: c.NextProtos,
ServerName: c.ServerName,
diff --git a/src/crypto/tls/handshake_client.go
b/src/crypto/tls/handshake_client.go
index f789e6f..01c6e60 100644
--- a/src/crypto/tls/handshake_client.go
+++ b/src/crypto/tls/handshake_client.go
@@ -283,6 +283,14 @@
certs[i] = cert
}

+ if c.config.VerifyPeerCertificate != nil {
+ err := c.config.VerifyPeerCertificate(certs)
+ if err != nil {
+ c.sendAlert(alertBadCertificate)
+ return err
+ }
+ }
+
if !c.config.InsecureSkipVerify {
opts := x509.VerifyOptions{
Roots: c.config.RootCAs,
diff --git a/src/crypto/tls/handshake_client_test.go
b/src/crypto/tls/handshake_client_test.go
index ce987f1..a33a833 100644
--- a/src/crypto/tls/handshake_client_test.go
+++ b/src/crypto/tls/handshake_client_test.go
@@ -8,9 +8,11 @@
"bytes"
"crypto/ecdsa"
"crypto/rsa"
+ "crypto/sha256"
"crypto/x509"
"encoding/base64"
"encoding/binary"
+ "encoding/hex"
"encoding/pem"
"errors"
"fmt"
@@ -956,6 +958,35 @@
}
}

+func TestHandshakeClientVerifyPeerCertificate(t *testing.T) {
+ config := *testConfig
+ config.VerifyPeerCertificate = func(peer []*x509.Certificate) error {
+ if len(peer) != 1 {
+ t.Fatalf("VerifyPeerCertificate: got:%x want:%x", len(peer), 1)
+ }
+
+ hash := sha256.Sum256(peer[0].Raw)
+ hashstr := hex.EncodeToString(hash[:])
+
+ if
hashstr != "bf348f30a3c02342f5dca297b66516879bc673f5656f81e18dc061418b119197"
{
+ t.Fatalf("VerifyPeerCertificate: got:%x want:%x",
hashstr, "bf348f30a3c02342f5dca297b66516879bc673f5656f81e18dc061418b119197")
+ }
+
+ return nil
+ }
+
+ test := &clientTest{
+ name: "ECDHE-ECDSA-AES",
+ command:
[]string{"openssl", "s_server", "-cipher", "ECDHE-ECDSA-AES128-SHA"},
+ cert: testECDSACertificate,
+ key: testECDSAPrivateKey,
+ config: &config,
+ }
+ // runClientTestTLS10(t, test)
+ // runClientTestTLS11(t, test)
+ runClientTestTLS12(t, test)
+}
+
// brokenConn wraps a net.Conn and causes all Writes after a certain
number to
// fail with brokenConnErr.
type brokenConn struct {
diff --git a/src/crypto/tls/handshake_server.go
b/src/crypto/tls/handshake_server.go
index 1aac729..73c80d5 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -709,6 +709,14 @@
}
}

+ if c.config.VerifyPeerCertificate != nil {
+ err := c.config.VerifyPeerCertificate(certs)
+ if err != nil {
+ c.sendAlert(alertBadCertificate)
+ return nil, err
+ }
+ }
+
if c.config.ClientAuth >= VerifyClientCertIfGiven && len(certs) > 0 {
opts := x509.VerifyOptions{
Roots: c.config.ClientCAs,
diff --git a/src/crypto/tls/handshake_server_test.go
b/src/crypto/tls/handshake_server_test.go
index 9ae5d11..8caf8c1 100644
--- a/src/crypto/tls/handshake_server_test.go
+++ b/src/crypto/tls/handshake_server_test.go
@@ -9,6 +9,8 @@
"crypto/ecdsa"
"crypto/elliptic"
"crypto/rsa"
+ "crypto/sha256"
+ "crypto/x509"
"encoding/hex"
"encoding/pem"
"errors"
@@ -976,6 +978,64 @@
runServerTestTLS11(t, test)
}

+func TestClientAuthVerifyPeerCertificate(t *testing.T) {
+ var certPath, keyPath, ecdsaCertPath, ecdsaKeyPath string
+
+ if *update {
+ certPath = tempFile(clientCertificatePEM)
+ defer os.Remove(certPath)
+ keyPath = tempFile(clientKeyPEM)
+ defer os.Remove(keyPath)
+ ecdsaCertPath = tempFile(clientECDSACertificatePEM)
+ defer os.Remove(ecdsaCertPath)
+ ecdsaKeyPath = tempFile(clientECDSAKeyPEM)
+ defer os.Remove(ecdsaKeyPath)
+ }
+
+ config := *testConfig
+ config.ClientAuth = RequestClientCert
+ config.VerifyPeerCertificate = func(peer []*x509.Certificate) error {
+ t.Log("VerifyPeerCertificate")
+
+ // we expect exactly one certificate with the following sha256 thumbprint
+ // bf348f30a3c02342f5dca297b66516879bc673f5656f81e18dc061418b119197
+
+ for _, c := range peer {
+ t.Log("Subject", c.Subject)
+ t.Log("Issuer ", c.Issuer)
+
+ hash := sha256.Sum256(c.Raw)
+ hashstr := hex.EncodeToString(hash[:])
+
+ t.Log("SHA256 ", hashstr)
+ }
+
+ return nil
+ }
+ test := &serverTest{
+ name: "ClientAuthRequestedNotGiven",
+ command:
[]string{"openssl", "s_client", "-no_ticket", "-cipher", "RC4-SHA"},
+ config: &config,
+ }
+ runServerTestTLS12(t, test)
+
+ test = &serverTest{
+ name: "ClientAuthRequestedAndGiven",
+ command:
[]string{"openssl", "s_client", "-no_ticket", "-cipher", "RC4-SHA", "-cert",
certPath, "-key", keyPath},
+ config: &config,
+ expectedPeerCerts: []string{clientCertificatePEM},
+ }
+ runServerTestTLS12(t, test)
+
+ test = &serverTest{
+ name: "ClientAuthRequestedAndECDSAGiven",
+ command:
[]string{"openssl", "s_client", "-no_ticket", "-cipher", "RC4-SHA", "-cert",
ecdsaCertPath, "-key", ecdsaKeyPath},
+ config: &config,
+ expectedPeerCerts: []string{clientECDSACertificatePEM},
+ }
+ runServerTestTLS12(t, test)
+}
+
// cert.pem and key.pem were generated with generate_cert.go
// Thus, they have no ExtKeyUsage fields and trigger an error
// when verification is turned on.
diff --git a/src/crypto/tls/tls_test.go b/src/crypto/tls/tls_test.go
index 48b46a0..e38c5e5 100644
--- a/src/crypto/tls/tls_test.go
+++ b/src/crypto/tls/tls_test.go
@@ -477,7 +477,7 @@
case "Rand":
f.Set(reflect.ValueOf(io.Reader(os.Stdin)))
continue
- case "Time", "GetCertificate":
+ case "Time", "GetCertificate", "VerifyPeerCertificate":
// DeepEqual can't compare functions.
continue
case "Certificates":
diff --git a/src/net/http/transport.go b/src/net/http/transport.go
index 3046de5..b12f748 100644
--- a/src/net/http/transport.go
+++ b/src/net/http/transport.go
@@ -2076,6 +2076,7 @@
Certificates: cfg.Certificates,
NameToCertificate: cfg.NameToCertificate,
GetCertificate: cfg.GetCertificate,
+ VerifyPeerCertificate: cfg.VerifyPeerCertificate,
RootCAs: cfg.RootCAs,
NextProtos: cfg.NextProtos,
ServerName: cfg.ServerName,
@@ -2109,6 +2110,7 @@
Certificates: cfg.Certificates,
NameToCertificate: cfg.NameToCertificate,
GetCertificate: cfg.GetCertificate,
+ VerifyPeerCertificate: cfg.VerifyPeerCertificate,
RootCAs: cfg.RootCAs,
NextProtos: cfg.NextProtos,
ServerName: cfg.ServerName,

--
https://go-review.googlesource.com/26654

[Joshua Boelter (Gerrit)]

Joshua Boelter has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Unit tests need some work if this is a reasonable approach. #16363 has an
example usage that works with https/http2 but it's not clear if that
approach is best practice.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Joshua Boelter <joshua....@gmail.com>
Gerrit-HasComments: No

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

(These changes are on hold until the tree opens for Go 1.8.)

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-HasComments: No

[Emmanuel Odeke (Gerrit)]

Emmanuel Odeke has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

(6 comments)

Thanks for the CL @joshua. I've only made a few stylistic change comments.

I was curious could we add a test in net/http? IMO, we could make a less
broad CL by first getting the code for crypto/tls reviewed, merged and then
after you could send a followup CL for net/http that'll include tests and
with more focus with a review from Brad and other net/http folks. In the
follow up CL, we'll refer to the new feature's CL.

https://go-review.googlesource.com/#/c/26654/1/src/crypto/tls/common.go
File src/crypto/tls/common.go:

Line 309: // any other verification checks on the cert or chain are
performed.
s/cert /certificate /


https://go-review.googlesource.com/#/c/26654/1/src/crypto/tls/handshake_client.go
File src/crypto/tls/handshake_client.go:

Line 287: err := c.config.VerifyPeerCertificate(certs)
if err := c.config.VerifyPeerCertificate(certs); err != nil {


https://go-review.googlesource.com/#/c/26654/1/src/crypto/tls/handshake_client_test.go
File src/crypto/tls/handshake_client_test.go:

Line 985: // runClientTestTLS10(t, test)
Did you mean to leave these commented?


https://go-review.googlesource.com/#/c/26654/1/src/crypto/tls/handshake_server.go
File src/crypto/tls/handshake_server.go:

Line 713: err := c.config.VerifyPeerCertificate(certs)
if err := c.config.VerifyPeerCertificate(certs); err != nil {


https://go-review.googlesource.com/#/c/26654/1/src/crypto/tls/handshake_server_test.go
File src/crypto/tls/handshake_server_test.go:

Line 1004: t.Log("Subject", c.Subject)
t.Logf("Subject: %s", c.Subject)
t.Logf("Issue: %s", c.Issuer)


Line 1007: hash := sha256.Sum256(c.Raw)
We could simplify the sha256 summing to this, and also use a format
specifier and formatter.

t.Logf("SHA256: %x", sha256.Sum256(c.Raw))

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>

Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>
Gerrit-HasComments: Yes

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

What's the situation where this is useful to you? Generally people who want
to do additional verification can just get the ConnectionState after the
handshake is complete and decide whether they want to use the connection or
not. There might be an argument that you don't want to send a client
certificate to a server without running the verification first, but since
the client cert is sent in the clear anyway, it's not that compelling.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>

Gerrit-HasComments: No

[Joshua Boelter (Gerrit)]

Joshua Boelter has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

> What's the situation where this is useful to you? Generally people
> who want to do additional verification can just get the
> ConnectionState after the handshake is complete and decide whether
> they want to use the connection or not. There might be an argument
> that you don't want to send a client certificate to a server
> without running the verification first, but since the client cert
> is sent in the clear anyway, it's not that compelling.

I'm using this to do certificate pinning for HTTP REST calls. Also
referenced from #9451 for pinning SMTP certs.

Last I tried, I could not get down to the ConnectionState from a
http.Transport if the connection is proxied. The DialTLS func in the
Transport isn't used when proxied.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>

Gerrit-Reviewer: Joshua Boelter <joshua....@intel.com>
Gerrit-HasComments: No

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Brad: does this seem like a reasonable use-case (i.e. that it's difficult
to get the ConnectionState from an http.Transport when using proxying?)

If so, then I can polish and land this.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>

Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Yeah, the use case mentioned does ring a bell.

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Thanks Brad.

Ok, if the use-case is reasonable then the question becomes what should the
callback look like? At the moment it's called before normal verification
and with the server-sent chain.

That isn't very helpful for implementing pinning (at least, not of the HPKP
variety) because for that you want the verified chain.

So I'd be inclined to make this:

func VerifyPeerCertificate(rawCerts [][]byte, verifiedChains
[][]*x509.Certificate)

And have it be called after normal verification (and not at all if normal
verification fails).

If InsecureSkipVerify was set then it would be called in all cases, but
verifiedChains would be nil.

If that meets everyone's needs then I can take care of it and land it.

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

SGTM

[Joshua Boelter (Gerrit)]

Joshua Boelter has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Adan - Perfect. You can take over and close this change. thx.

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 1:

Ack. Just waiting for the backlog of crypto/tls changes to land so that I
don't get a big conflict.

[Adam Langley (Gerrit)]

Reviewers: Emmanuel Odeke, Joshua Boelter, Brad Fitzpatrick

Adam Langley uploaded a new patch set:

https://go-review.googlesource.com/26654

crypto/tls: add VerifyPeerCertificate to tls.Config

VerifyPeerCertificate returns an error if the peer should not be
trusted. It will be called after the initial handshake and before
any other verification checks on the cert or chain are performed.
This provides the callee an opportunity to augment the certificate
verification.

If VerifyPeerCertificate is not nil and returns an error,
then the handshake will fail.

Fixes #16363

Change-Id: I6a22f199f0e81b6f5d5f37c54d85ab878216bb22
---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go

M src/crypto/tls/tls_test.go
5 files changed, 182 insertions(+), 1 deletion(-)

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 2: Run-TryBot+1

Brad: would you mind taking a look at this? Technically I'm reviewing a
change so could +2 and land it, but I've essentially rewritten it so that
feels a little like cheating.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>
Gerrit-Reviewer: Joshua Boelter <joshua....@intel.com>

Gerrit-HasComments: No

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 2:

TryBots beginning. Status page: http://farmer.golang.org/try?commit=91e124f1

[Gobot Gobot (Gerrit)]

Gobot Gobot has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 2: TryBot-Result+1

TryBots are happy.

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>

Gerrit-Reviewer: Gobot Gobot <go...@golang.org>

[Brad Fitzpatrick (Gerrit)]

Brad Fitzpatrick has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 2: Code-Review+2

(1 comment)

https://go-review.googlesource.com/#/c/26654/2/src/crypto/tls/common.go
File src/crypto/tls/common.go:

Line 330: // receives the raw ASN.1 certificates provided by the server
and also
Should "by the server" be "by the peer"?

--
https://go-review.googlesource.com/26654
Gerrit-Reviewer: Adam Langley <a...@golang.org>
Gerrit-Reviewer: Brad Fitzpatrick <brad...@golang.org>
Gerrit-Reviewer: Emmanuel Odeke <emm....@gmail.com>
Gerrit-Reviewer: Gobot Gobot <go...@golang.org>
Gerrit-Reviewer: Joshua Boelter <joshua....@intel.com>

Gerrit-HasComments: Yes

[Adam Langley (Gerrit)]

Adam Langley has posted comments on this change.

crypto/tls: add VerifyPeerCertificate to tls.Config

Patch Set 3:

(1 comment)

https://go-review.googlesource.com/#/c/26654/2/src/crypto/tls/common.go
File src/crypto/tls/common.go:

Line 330: // receives the raw ASN.1 certificates provided by the peer and

also
> Should "by the server" be "by the peer"?

Done

[Adam Langley (Gerrit)]

Adam Langley has submitted this change and it was merged.

crypto/tls: add VerifyPeerCertificate to tls.Config

VerifyPeerCertificate returns an error if the peer should not be
trusted. It will be called after the initial handshake and before
any other verification checks on the cert or chain are performed.
This provides the callee an opportunity to augment the certificate
verification.

If VerifyPeerCertificate is not nil and returns an error,
then the handshake will fail.

Fixes #16363

Change-Id: I6a22f199f0e81b6f5d5f37c54d85ab878216bb22

Reviewed-on: https://go-review.googlesource.com/26654
Reviewed-by: Brad Fitzpatrick <brad...@golang.org>

---
M src/crypto/tls/common.go
M src/crypto/tls/handshake_client.go
M src/crypto/tls/handshake_client_test.go
M src/crypto/tls/handshake_server.go

M src/crypto/tls/tls_test.go
5 files changed, 182 insertions(+), 1 deletion(-)

Approvals:
Brad Fitzpatrick: Looks good to me, approved