From 9fcf50f3e507e83c0b12301d2427ad60806b1f80 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Wed, 24 May 2017 11:53:01 -0400 Subject: [PATCH 1/3] Cert verification for non-CA certs --- builtin/credential/cert/path_login.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 43e90532d0c2..69c79c33ddf4 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -175,6 +175,15 @@ func (b *backend) verifyCredentials(req *logical.Request, d *framework.FieldData if tCert.SerialNumber.Cmp(clientCert.SerialNumber) == 0 && bytes.Equal(tCert.AuthorityKeyId, clientCert.AuthorityKeyId) && b.matchesConstraints(clientCert, trustedNonCA.Certificates, trustedNonCA) { + + // We are not looking for trusted chains here since this is a + // non-CA cert. But validating the connection state detects + // expired certificates. + _, err := validateConnState(roots, connState) + if err != nil { + return nil, nil, err + } + return trustedNonCA, nil, nil } } From 28664517d280d55a68159919cb05511317c9e5b1 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Wed, 24 May 2017 21:39:01 -0400 Subject: [PATCH 2/3] Added test case to ensure login fails with expired non-CA cert --- builtin/credential/cert/backend_test.go | 224 ++++++++++++++++++++++++ 1 file changed, 224 insertions(+) diff --git a/builtin/credential/cert/backend_test.go b/builtin/credential/cert/backend_test.go index f96c9cb868c8..fa188b141f8b 100644 --- a/builtin/credential/cert/backend_test.go +++ b/builtin/credential/cert/backend_test.go @@ -1,14 +1,22 @@ package cert import ( + "crypto/rand" + "crypto/rsa" "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" "fmt" "io/ioutil" + "math/big" + "net" + "os" "reflect" "testing" "time" "github.com/hashicorp/go-rootcerts" + "github.com/hashicorp/vault/helper/certutil" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" logicaltest "github.com/hashicorp/vault/logical/testing" @@ -101,6 +109,222 @@ func connectionState(t *testing.T, serverCAPath, serverCertPath, serverKeyPath, return connState } +func TestBackend_NonCAExpiry(t *testing.T) { + var resp *logical.Response + var err error + + // Create a self-signed certificate and issue a leaf certificate using the + // CA cert + template := &x509.Certificate{ + SerialNumber: big.NewInt(1234), + Subject: pkix.Name{ + CommonName: "localhost", + Organization: []string{"hashicorp"}, + OrganizationalUnit: []string{"vault"}, + }, + BasicConstraintsValid: true, + NotBefore: time.Now().Add(-30 * time.Second), + NotAfter: time.Now().Add(50 * time.Second), + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth, x509.ExtKeyUsageServerAuth}, + KeyUsage: x509.KeyUsage(x509.KeyUsageCertSign | x509.KeyUsageCRLSign), + } + + // Set IP SAN + parsedIP := net.ParseIP("127.0.0.1") + if parsedIP == nil { + t.Fatalf("failed to create parsed IP") + } + template.IPAddresses = []net.IP{parsedIP} + + // Private key for CA cert + caPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + + // Marshalling to be able to create PEM file + caPrivateKeyBytes := x509.MarshalPKCS1PrivateKey(caPrivateKey) + + caPublicKey := &caPrivateKey.PublicKey + + template.IsCA = true + + caCertBytes, err := x509.CreateCertificate(rand.Reader, template, template, caPublicKey, caPrivateKey) + if err != nil { + t.Fatal(err) + } + + caCert, err := x509.ParseCertificate(caCertBytes) + if err != nil { + t.Fatal(err) + } + + parsedCaBundle := &certutil.ParsedCertBundle{ + Certificate: caCert, + CertificateBytes: caCertBytes, + PrivateKeyBytes: caPrivateKeyBytes, + PrivateKeyType: certutil.RSAPrivateKey, + } + + caCertBundle, err := parsedCaBundle.ToCertBundle() + if err != nil { + t.Fatal(err) + } + + caCertFile, err := ioutil.TempFile("", "caCert") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(caCertFile.Name()) + + if _, err := caCertFile.Write([]byte(caCertBundle.Certificate)); err != nil { + t.Fatal(err) + } + if err := caCertFile.Close(); err != nil { + t.Fatal(err) + } + + caKeyFile, err := ioutil.TempFile("", "caKey") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(caKeyFile.Name()) + + if _, err := caKeyFile.Write([]byte(caCertBundle.PrivateKey)); err != nil { + t.Fatal(err) + } + if err := caKeyFile.Close(); err != nil { + t.Fatal(err) + } + + // Prepare template for non-CA cert + + template.IsCA = false + template.SerialNumber = big.NewInt(5678) + + template.KeyUsage = x509.KeyUsage(x509.KeyUsageDigitalSignature | x509.KeyUsageCertSign) + issuedPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048) + if err != nil { + t.Fatal(err) + } + + issuedPrivateKeyBytes := x509.MarshalPKCS1PrivateKey(issuedPrivateKey) + + issuedPublicKey := &issuedPrivateKey.PublicKey + + // Keep a short certificate lifetime so logins can be tested both when + // cert is valid and when it gets expired + template.NotBefore = time.Now().Add(-2 * time.Second) + template.NotAfter = time.Now().Add(3 * time.Second) + + issuedCertBytes, err := x509.CreateCertificate(rand.Reader, template, caCert, issuedPublicKey, caPrivateKey) + if err != nil { + t.Fatal(err) + } + + issuedCert, err := x509.ParseCertificate(issuedCertBytes) + if err != nil { + t.Fatal(err) + } + + parsedIssuedBundle := &certutil.ParsedCertBundle{ + Certificate: issuedCert, + CertificateBytes: issuedCertBytes, + PrivateKeyBytes: issuedPrivateKeyBytes, + PrivateKeyType: certutil.RSAPrivateKey, + } + + issuedCertBundle, err := parsedIssuedBundle.ToCertBundle() + if err != nil { + t.Fatal(err) + } + + issuedCertFile, err := ioutil.TempFile("", "issuedCert") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(issuedCertFile.Name()) + + if _, err := issuedCertFile.Write([]byte(issuedCertBundle.Certificate)); err != nil { + t.Fatal(err) + } + if err := issuedCertFile.Close(); err != nil { + t.Fatal(err) + } + + issuedKeyFile, err := ioutil.TempFile("", "issuedKey") + if err != nil { + t.Fatal(err) + } + + defer os.Remove(issuedKeyFile.Name()) + + if _, err := issuedKeyFile.Write([]byte(issuedCertBundle.PrivateKey)); err != nil { + t.Fatal(err) + } + if err := issuedKeyFile.Close(); err != nil { + t.Fatal(err) + } + + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + + b, err := Factory(config) + if err != nil { + t.Fatal(err) + } + + // Register the Non-CA certificate of the client key pair + certData := map[string]interface{}{ + "certificate": issuedCertBundle.Certificate, + "policies": "abc", + "display_name": "cert1", + "ttl": 10000, + } + certReq := &logical.Request{ + Operation: logical.UpdateOperation, + Path: "certs/cert1", + Storage: storage, + Data: certData, + } + + resp, err = b.HandleRequest(certReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + // Create connection state using the certificates generated + connState := connectionState(t, caCertFile.Name(), caCertFile.Name(), caKeyFile.Name(), issuedCertFile.Name(), issuedKeyFile.Name()) + + loginReq := &logical.Request{ + Operation: logical.UpdateOperation, + Storage: storage, + Path: "login", + Connection: &logical.Connection{ + ConnState: &connState, + }, + } + + // Login when the certificate is still valid. Login should succeed. + resp, err = b.HandleRequest(loginReq) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + + // Wait until the certificate expires + time.Sleep(5 * time.Second) + + // Login attempt after certificate expiry should fail + resp, err = b.HandleRequest(loginReq) + if err == nil { + t.Fatalf("expected error due to expired certificate") + } +} + func TestBackend_RegisteredNonCA_CRL(t *testing.T) { config := logical.TestBackendConfig() storage := &logical.InmemStorage{} From 18aca03f9d202ba063e180c5ad3f91717f18b545 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Wed, 24 May 2017 21:39:17 -0400 Subject: [PATCH 3/3] Address review feedback --- builtin/credential/cert/path_login.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/builtin/credential/cert/path_login.go b/builtin/credential/cert/path_login.go index 69c79c33ddf4..2faecd3afdca 100644 --- a/builtin/credential/cert/path_login.go +++ b/builtin/credential/cert/path_login.go @@ -166,6 +166,12 @@ func (b *backend) verifyCredentials(req *logical.Request, d *framework.FieldData // Load the trusted certificates roots, trusted, trustedNonCAs := b.loadTrustedCerts(req.Storage, certName) + // Get the list of full chains matching the connection + trustedChains, err := validateConnState(roots, connState) + if err != nil { + return nil, nil, err + } + // If trustedNonCAs is not empty it means that client had registered a non-CA cert // with the backend. if len(trustedNonCAs) != 0 { @@ -175,26 +181,11 @@ func (b *backend) verifyCredentials(req *logical.Request, d *framework.FieldData if tCert.SerialNumber.Cmp(clientCert.SerialNumber) == 0 && bytes.Equal(tCert.AuthorityKeyId, clientCert.AuthorityKeyId) && b.matchesConstraints(clientCert, trustedNonCA.Certificates, trustedNonCA) { - - // We are not looking for trusted chains here since this is a - // non-CA cert. But validating the connection state detects - // expired certificates. - _, err := validateConnState(roots, connState) - if err != nil { - return nil, nil, err - } - return trustedNonCA, nil, nil } } } - // Get the list of full chains matching the connection - trustedChains, err := validateConnState(roots, connState) - if err != nil { - return nil, nil, err - } - // If no trusted chain was found, client is not authenticated if len(trustedChains) == 0 { return nil, logical.ErrorResponse("invalid certificate or no client certificate supplied"), nil