Merge pull request #92 from xmidt-org/fix/verify-peer-certificate

Fix/verify peer certificate

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 - Migrated to github.com/golang-jwt/jwt to address a security vulnerability. [#78](https://github.com/xmidt-org/themis/pull/78)
 - Updated spec file and rpkg version macro to be able to choose when the 'v' is included in the version. [#80](https://github.com/xmidt-org/themis/pull/80)
 - Updated transport.go to send a 400 error if there is an issue parsing the URL. [#47](https://github.com/xmidt-org/themis/issues/47)
+- Allow any peer certificate to pass validation, instead of requiring all of them to pass.  [#91](https://github.com/xmidt-org/themis/issues/91)
 
 
 ## [v0.4.7]

xhttp/xhttpserver/mocks_test.go

@@ -4,9 +4,9 @@ import (
 	"bufio"
 	"context"
 	"crypto/x509"
-	"math/big"
 	"net"
 	"net/http"
+	"testing"
 
 	"github.com/stretchr/testify/mock"
 )
@@ -134,18 +134,50 @@ func (m *mockServer) ExpectShutdown(p ...interface{}) *mock.Call {
 	return m.On("Shutdown", p...)
 }
 
-func stubPeerCert(serialNumber int64) *x509.Certificate {
-	return &x509.Certificate{
-		SerialNumber: big.NewInt(serialNumber),
+func newCertificateMatcher(t *testing.T, commonName string, dnsNames ...string) func(*x509.Certificate) bool {
+	return func(actual *x509.Certificate) bool {
+		t.Logf("Testing cert: Subject.CommonName=%s, DNSNames=%s", actual.Subject.CommonName, actual.DNSNames)
+
+		switch {
+		case commonName != actual.Subject.CommonName:
+			return false
+
+		case len(dnsNames) != len(actual.DNSNames):
+			return false
+
+		default:
+			for i := 0; i < len(dnsNames); i++ {
+				if dnsNames[i] != actual.DNSNames[i] {
+					return false
+				}
+			}
+		}
+
+		return true
 	}
 }
 
-func stubChain(serialNumber int64) [][]*x509.Certificate {
-	return [][]*x509.Certificate{
-		[]*x509.Certificate{
-			&x509.Certificate{
-				SerialNumber: big.NewInt(serialNumber),
-			},
-		},
+type mockPeerVerifier struct {
+	mock.Mock
+}
+
+func (m *mockPeerVerifier) Verify(peerCert *x509.Certificate, verifiedChains [][]*x509.Certificate) error {
+	return m.Called(peerCert, verifiedChains).Error(0)
+}
+
+// ExpectVerify sets up the a mocked call to Verify with a peer certificate with the given
+// subject common name and dns names.  Since this package doesn't use any other fields,
+// this expectation suffices for tests.
+func (m *mockPeerVerifier) ExpectVerify(certificateMatcher func(*x509.Certificate) bool) *mock.Call {
+	return m.On(
+		"Verify",
+		mock.MatchedBy(certificateMatcher),
+		[][]*x509.Certificate(nil), // we always pass nil in tests, since we don't use this parameter
+	)
+}
+
+func assertPeerVerifierExpectations(t *testing.T, pvs ...PeerVerifier) {
+	for _, pv := range pvs {
+		pv.(*mockPeerVerifier).AssertExpectations(t)
 	}
 }

xhttp/xhttpserver/tls.go

@@ -6,6 +6,8 @@ import (
 	"errors"
 	"io/ioutil"
 	"strings"
+
+	"go.uber.org/multierr"
 )
 
 var (
@@ -122,23 +124,34 @@ func (pvs PeerVerifiers) Verify(peerCert *x509.Certificate, verifiedChains [][]*
 }
 
 // VerifyPeerCertificate may be used as the closure for crypto/tls.Config.VerifyPeerCertificate
-func (pvs PeerVerifiers) VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
+//
+// If any of the rawCerts passes verification, this method returns nil to indicate that the
+// client has supplied a valid certificate.  If all rawCerts fail verification or if any certificates
+// fail to parse, this method returns an error.
+func (pvs PeerVerifiers) VerifyPeerCertificate(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) (err error) {
 	if len(pvs) == 0 {
-		return nil
+		return
 	}
 
 	for _, rawCert := range rawCerts {
-		peerCert, err := x509.ParseCertificate(rawCert)
-		if err == nil {
-			err = pvs.Verify(peerCert, verifiedChains)
+		peerCert, parseErr := x509.ParseCertificate(rawCert)
+		if parseErr != nil {
+			// any parse error invalidates the entire sequence of certificates
+			err = parseErr
+			break
 		}
 
-		if err != nil {
-			return err
+		verifyErr := pvs.Verify(peerCert, verifiedChains)
+		err = multierr.Append(err, verifyErr)
+
+		if verifyErr == nil {
+			// we found (1) cert that passes verification, so we're good
+			err = nil
+			break
 		}
 	}
 
-	return nil
+	return
 }
 
 // NewPeerVerifiers constructs a chain of verification strategies merged from a set of options with an extra