From 77dc081ead3a060128ddc63404ad92623b4916b2 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 15 Oct 2015 13:35:34 -0700 Subject: [PATCH 01/13] Add a utility which generates a tls configuration for you given the requisite certs. Signed-off-by: Ying Li --- utils/tls_config.go | 52 ++++++++++++++++++++++++ utils/tls_config_test.go | 86 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 138 insertions(+) create mode 100644 utils/tls_config.go create mode 100644 utils/tls_config_test.go diff --git a/utils/tls_config.go b/utils/tls_config.go new file mode 100644 index 0000000000..6b44b6ea2e --- /dev/null +++ b/utils/tls_config.go @@ -0,0 +1,52 @@ +package utils + +import ( + "crypto/rand" + "crypto/tls" + "crypto/x509" + + "github.com/docker/notary/trustmanager" +) + +// ConfigureServerTLS specifies a set of ciphersuites, the server cert and key, +// and optionally client authentication. Note that a tls configuration is +// constructed that either requires and verifies client authentication or +// doesn't deal with client certs at all. Nothing in the middle. +func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, caCert string) (*tls.Config, error) { + keypair, err := tls.LoadX509KeyPair(serverCert, serverKey) + if err != nil { + return nil, err + } + + tlsConfig := &tls.Config{ + MinVersion: tls.VersionTLS12, + PreferServerCipherSuites: true, + CipherSuites: []uint16{ + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + }, + Certificates: []tls.Certificate{keypair}, + Rand: rand.Reader, + } + + if clientAuth { + tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert + if caCert != "" { + cert, err := trustmanager.LoadCertFromFile(caCert) + if err != nil { + return nil, err + } + caPool := x509.NewCertPool() + caPool.AddCert(cert) + tlsConfig.ClientCAs = caPool + } + } + + return tlsConfig, nil +} diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go new file mode 100644 index 0000000000..ee4e1f75d5 --- /dev/null +++ b/utils/tls_config_test.go @@ -0,0 +1,86 @@ +package utils + +import ( + "crypto/tls" + "testing" + + "github.com/stretchr/testify/assert" +) + +const ( + ServerCert = "../fixtures/notary-server.crt" + ServerKey = "../fixtures/notary-server.key" + CACert = "../fixtures/root-ca.crt" + FakeFile = "../fixtures/not-a-file" +) + +// TestTLSConfigFailsIfUnableToLoadCerts fails if unable to load either of the +// server files or the client cert info +func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { + for i := 0; i < 3; i++ { + files := []string{ServerCert, ServerKey, CACert} + files[i] = FakeFile + + result, err := ConfigureServerTLS(files[0], files[1], true, files[2]) + assert.Nil(t, result) + assert.Error(t, err) + } +} + +// TestConfigServerTLSServerCertsOnly returns a valid tls config with the +// provided server certificate, and since clientAuth was false, no client auth +// or CAs configured. +func TestConfigServerTLSServerCertsOnly(t *testing.T) { + keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) + assert.NoError(t, err) + + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, "") + assert.NoError(t, err) + assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) + assert.True(t, tlsConfig.PreferServerCipherSuites) + assert.Equal(t, tls.NoClientCert, tlsConfig.ClientAuth) + assert.Nil(t, tlsConfig.ClientCAs) +} + +// TestConfigServerTLSNoCACertsIfNoClientAuth returns a valid tls config with +// the provided server certificate, and since clientAuth was false, no client +// auth or CAs configured even though a client CA cert was provided. +func TestConfigServerTLSNoCACertsIfNoClientAuth(t *testing.T) { + keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) + assert.NoError(t, err) + + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, CACert) + assert.NoError(t, err) + assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) + assert.True(t, tlsConfig.PreferServerCipherSuites) + assert.Equal(t, tls.NoClientCert, tlsConfig.ClientAuth) + assert.Nil(t, tlsConfig.ClientCAs) +} + +// TestTLSConfigClientAuthEnabledNoCACerts returns a valid tls config with the +// provided server certificate client auth enabled, but no CAs configured. +func TestTLSConfigClientAuthEnabledNoCACerts(t *testing.T) { + keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) + assert.NoError(t, err) + + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, "") + assert.NoError(t, err) + assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) + assert.True(t, tlsConfig.PreferServerCipherSuites) + assert.Equal(t, tls.RequireAndVerifyClientCert, tlsConfig.ClientAuth) + assert.Nil(t, tlsConfig.ClientCAs) +} + +// TestTLSConfigClientAuthEnabledWithCACert returns a valid tls config with the +// provided server certificate, client auth enabled, and a client CA. +func TestTLSConfigClientAuthEnabledWithCACert(t *testing.T) { + keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) + assert.NoError(t, err) + + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, CACert) + assert.NoError(t, err) + assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) + assert.True(t, tlsConfig.PreferServerCipherSuites) + assert.Equal(t, tls.RequireAndVerifyClientCert, tlsConfig.ClientAuth) + assert.Equal(t, 1, len(tlsConfig.ClientCAs.Subjects())) +} From e50cc2c9cda5813b1057746e7e05453b7db3b44d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 16 Oct 2015 12:02:26 -0700 Subject: [PATCH 02/13] Add test to ensure that x509filestore loads existing certs from the directory without modifying/overwriting them. Signed-off-by: Ying Li --- trustmanager/x509filestore_test.go | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/trustmanager/x509filestore_test.go b/trustmanager/x509filestore_test.go index 21b1a0b36b..26efaf1c76 100644 --- a/trustmanager/x509filestore_test.go +++ b/trustmanager/x509filestore_test.go @@ -5,6 +5,7 @@ import ( "encoding/pem" "io/ioutil" "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -22,6 +23,47 @@ func TestNewX509FileStore(t *testing.T) { } } +// NewX509FileStore loads any existing certs from the directory, and does +// not overwrite any of the. +func TestNewX509FileStoreLoadsExistingCerts(t *testing.T) { + tempDir, err := ioutil.TempDir("", "cert-test") + assert.NoError(t, err, "couldn't open temp directory") + defer os.RemoveAll(tempDir) + + certBytes, err := ioutil.ReadFile("../fixtures/root-ca.crt") + assert.NoError(t, err, "couldn't read fixtures/root-ca.crt") + out, err := os.Create(filepath.Join(tempDir, "root-ca.crt")) + assert.NoError(t, err, "couldn't create a file in the temp dir") + + // to distinguish it from the canonical format + distinguishingBytes := []byte{'\n', '\n', '\n', '\n', '\n', '\n'} + nBytes, err := out.Write(distinguishingBytes) + assert.NoError(t, err, "could not write newlines to the temporary file") + assert.Equal(t, len(distinguishingBytes), nBytes, + "didn't write all bytes to temporary file") + + nBytes, err = out.Write(certBytes) + assert.NoError(t, err, "could not write cert to the temporary file") + assert.Equal(t, len(certBytes), nBytes, + "didn't write all bytes to temporary file") + + err = out.Close() + assert.NoError(t, err, "could not close temporary file") + + store, err := NewX509FileStore(tempDir) + assert.NoError(t, err, "failed to create a new X509FileStore") + + expectedCert, err := LoadCertFromFile("../fixtures/root-ca.crt") + assert.NoError(t, err, "could not load root-ca.crt") + assert.Equal(t, store.GetCertificates(), []*x509.Certificate{expectedCert}, + "did not load certificate already in the directory") + + outBytes, err := ioutil.ReadFile(filepath.Join(tempDir, "root-ca.crt")) + assert.NoError(t, err, "couldn't read temporary file") + assert.Equal(t, distinguishingBytes, outBytes[:6], "original file overwritten") + assert.Equal(t, certBytes, outBytes[6:], "original file overwritten") +} + func TestAddCertX509FileStore(t *testing.T) { // Read certificate from file b, err := ioutil.ReadFile("../fixtures/root-ca.crt") From 7356dfd2732141f222595ee466784e70a45c16c7 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 16 Oct 2015 14:13:05 -0700 Subject: [PATCH 03/13] Change ConfigServerTLS to take a client CA directory instead of certs Signed-off-by: Ying Li --- trustmanager/x509filestore.go | 6 +++++ trustmanager/x509filestore_test.go | 15 ++++++++++++ utils/tls_config.go | 25 ++++++++++++++------ utils/tls_config_test.go | 37 +++++++++++++++++++++++++----- 4 files changed, 70 insertions(+), 13 deletions(-) diff --git a/trustmanager/x509filestore.go b/trustmanager/x509filestore.go index 1df6233191..8417805583 100644 --- a/trustmanager/x509filestore.go +++ b/trustmanager/x509filestore.go @@ -260,6 +260,12 @@ func (s *X509FileStore) GetVerifyOptions(dnsName string) (x509.VerifyOptions, er return opts, nil } +// Empty returns true if there are no certificates in the X509FileStore, false +// otherwise. +func (s *X509FileStore) Empty() bool { + return len(s.fingerprintMap) == 0 +} + func fileName(cert *x509.Certificate) (string, CertID, error) { certID, err := fingerprintCert(cert) if err != nil { diff --git a/trustmanager/x509filestore_test.go b/trustmanager/x509filestore_test.go index 26efaf1c76..95be916107 100644 --- a/trustmanager/x509filestore_test.go +++ b/trustmanager/x509filestore_test.go @@ -124,6 +124,21 @@ func TestAddCertFromFileX509FileStore(t *testing.T) { } } +// TestNewX509FileStoreEmpty verifies the behavior of the Empty function +func TestNewX509FileStoreEmpty(t *testing.T) { + tempDir, err := ioutil.TempDir("", "cert-test") + assert.NoError(t, err) + defer os.RemoveAll(tempDir) + + store, err := NewX509FileStore(tempDir) + assert.NoError(t, err, "failed to create a new X509FileStore: %v", store) + assert.True(t, store.Empty()) + + err = store.AddCertFromFile("../fixtures/root-ca.crt") + assert.NoError(t, err, "failed to add certificate from file") + assert.False(t, store.Empty()) +} + func TestAddCertFromPEMX509FileStore(t *testing.T) { b, err := ioutil.ReadFile("../fixtures/root-ca.crt") if err != nil { diff --git a/utils/tls_config.go b/utils/tls_config.go index 6b44b6ea2e..57905d2faa 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -3,7 +3,8 @@ package utils import ( "crypto/rand" "crypto/tls" - "crypto/x509" + "fmt" + "os" "github.com/docker/notary/trustmanager" ) @@ -12,7 +13,7 @@ import ( // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or // doesn't deal with client certs at all. Nothing in the middle. -func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, caCert string) (*tls.Config, error) { +func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, caCertDir string) (*tls.Config, error) { keypair, err := tls.LoadX509KeyPair(serverCert, serverKey) if err != nil { return nil, err @@ -37,14 +38,24 @@ func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, ca if clientAuth { tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert - if caCert != "" { - cert, err := trustmanager.LoadCertFromFile(caCert) + if caCertDir != "" { + // Check to see if the given directory exists + fi, err := os.Stat(caCertDir) if err != nil { return nil, err } - caPool := x509.NewCertPool() - caPool.AddCert(cert) - tlsConfig.ClientCAs = caPool + if !fi.IsDir() { + return nil, fmt.Errorf("No such directory: %s", caCertDir) + } + + certStore, err := trustmanager.NewX509FileStore(caCertDir) + if err != nil { + return nil, err + } + if certStore.Empty() { + return nil, fmt.Errorf("No certificates in %s", caCertDir) + } + tlsConfig.ClientCAs = certStore.GetCertificatePool() } } diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index ee4e1f75d5..4d738a15c6 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -2,6 +2,10 @@ package utils import ( "crypto/tls" + "io" + "io/ioutil" + "os" + "path/filepath" "testing" "github.com/stretchr/testify/assert" @@ -10,16 +14,35 @@ import ( const ( ServerCert = "../fixtures/notary-server.crt" ServerKey = "../fixtures/notary-server.key" - CACert = "../fixtures/root-ca.crt" - FakeFile = "../fixtures/not-a-file" + RootCA = "../fixtures/root-ca.crt" ) +// copies the provided certificate into a temporary directory +func makeTempCertDir(t *testing.T) string { + tempDir, err := ioutil.TempDir("/tmp", "cert-test") + assert.NoError(t, err, "couldn't open temp directory") + + in, err := os.Open(RootCA) + assert.NoError(t, err, "cannot open %s", RootCA) + defer in.Close() + copiedCert := filepath.Join(tempDir, filepath.Base(RootCA)) + out, err := os.Create(copiedCert) + assert.NoError(t, err, "cannot open %s", copiedCert) + defer out.Close() + _, err = io.Copy(out, in) + assert.NoError(t, err, "cannot copy %s to %s", RootCA, copiedCert) + + return tempDir +} + // TestTLSConfigFailsIfUnableToLoadCerts fails if unable to load either of the // server files or the client cert info func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { + tempDir := makeTempCertDir(t) + for i := 0; i < 3; i++ { - files := []string{ServerCert, ServerKey, CACert} - files[i] = FakeFile + files := []string{ServerCert, ServerKey, tempDir} + files[i] = "not-real-file" result, err := ConfigureServerTLS(files[0], files[1], true, files[2]) assert.Nil(t, result) @@ -46,10 +69,11 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { // the provided server certificate, and since clientAuth was false, no client // auth or CAs configured even though a client CA cert was provided. func TestConfigServerTLSNoCACertsIfNoClientAuth(t *testing.T) { + tempDir := makeTempCertDir(t) keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, CACert) + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -74,10 +98,11 @@ func TestTLSConfigClientAuthEnabledNoCACerts(t *testing.T) { // TestTLSConfigClientAuthEnabledWithCACert returns a valid tls config with the // provided server certificate, client auth enabled, and a client CA. func TestTLSConfigClientAuthEnabledWithCACert(t *testing.T) { + tempDir := makeTempCertDir(t) keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, CACert) + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, tempDir) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) From 8d96cf0c1fd7054d74ecf1e39c3e265dc304c089 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 16 Oct 2015 15:09:40 -0700 Subject: [PATCH 04/13] Use ConfigureServerTLS for notary-server and notary-signer Signed-off-by: Ying Li --- cmd/notary-signer/main.go | 19 ++++--------------- server/server.go | 21 ++------------------- 2 files changed, 6 insertions(+), 34 deletions(-) diff --git a/cmd/notary-signer/main.go b/cmd/notary-signer/main.go index 83dc413c2b..23f03b3f17 100644 --- a/cmd/notary-signer/main.go +++ b/cmd/notary-signer/main.go @@ -1,8 +1,6 @@ package main import ( - "crypto/rand" - "crypto/tls" "database/sql" "errors" _ "expvar" @@ -22,6 +20,7 @@ import ( "github.com/docker/notary/cryptoservice" "github.com/docker/notary/signer" "github.com/docker/notary/signer/api" + "github.com/docker/notary/utils" "github.com/docker/notary/version" "github.com/endophage/gotuf/data" _ "github.com/go-sql-driver/mysql" @@ -103,20 +102,10 @@ func main() { log.Fatalf("Certificate and key are mandatory") } - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - PreferServerCipherSuites: true, - CipherSuites: []uint16{ - tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_CBC_SHA}, + tlsConfig, err := utils.ConfigureServerTLS(certFile, keyFile, false, "") + if err != nil { + logrus.Fatalf("Unable to set up TLS: %s", err.Error()) } - tlsConfig.Rand = rand.Reader cryptoServices := make(signer.CryptoServiceIndex) diff --git a/server/server.go b/server/server.go index bee1b7017d..e265ae3e88 100644 --- a/server/server.go +++ b/server/server.go @@ -1,7 +1,6 @@ package server import ( - "crypto/rand" "crypto/tls" "fmt" "net" @@ -42,27 +41,11 @@ func Run(ctx context.Context, addr, tlsCertFile, tlsKeyFile string, trust signed } if tlsCertFile != "" && tlsKeyFile != "" { - keypair, err := tls.LoadX509KeyPair(tlsCertFile, tlsKeyFile) + tlsConfig, err := utils.ConfigureServerTLS( + tlsCertFile, tlsKeyFile, false, "") if err != nil { return err } - tlsConfig := &tls.Config{ - MinVersion: tls.VersionTLS12, - PreferServerCipherSuites: true, - CipherSuites: []uint16{ - tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_CBC_SHA, - }, - Certificates: []tls.Certificate{keypair}, - Rand: rand.Reader, - } - logrus.Info("Enabling TLS") lsnr = tls.NewListener(lsnr, tlsConfig) } else if tlsCertFile != "" || tlsKeyFile != "" { From b399783eeef9e5cb79cd0bb0fafc772d906cd800 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 16 Oct 2015 18:31:03 -0700 Subject: [PATCH 05/13] Slight refactoring of ConfigureServerTLS and added a ConfigureClientTLS as well. Signed-off-by: Ying Li --- utils/tls_config.go | 66 ++++++++++++++++++++++++--------- utils/tls_config_test.go | 79 +++++++++++++++++++++++++++++----------- 2 files changed, 107 insertions(+), 38 deletions(-) diff --git a/utils/tls_config.go b/utils/tls_config.go index 57905d2faa..3b3885b7de 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -3,6 +3,7 @@ package utils import ( "crypto/rand" "crypto/tls" + "crypto/x509" "fmt" "os" @@ -38,25 +39,56 @@ func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, ca if clientAuth { tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert - if caCertDir != "" { - // Check to see if the given directory exists - fi, err := os.Stat(caCertDir) - if err != nil { - return nil, err - } - if !fi.IsDir() { - return nil, fmt.Errorf("No such directory: %s", caCertDir) - } + } - certStore, err := trustmanager.NewX509FileStore(caCertDir) - if err != nil { - return nil, err - } - if certStore.Empty() { - return nil, fmt.Errorf("No certificates in %s", caCertDir) - } - tlsConfig.ClientCAs = certStore.GetCertificatePool() + if caCertDir != "" { + // Check to see if the given directory exists + fi, err := os.Stat(caCertDir) + if err != nil { + return nil, err } + if !fi.IsDir() { + return nil, fmt.Errorf("No such directory: %s", caCertDir) + } + + certStore, err := trustmanager.NewX509FileStore(caCertDir) + if err != nil { + return nil, err + } + if certStore.Empty() { + return nil, fmt.Errorf("No certificates in %s", caCertDir) + } + tlsConfig.ClientCAs = certStore.GetCertificatePool() + } + + return tlsConfig, nil +} + +// ConfigureClientTLS generates a tls configuration for clients using the +// provided. +func ConfigureClientTLS(rootCA string, skipVerify bool, clientCert string, clientKey string) (*tls.Config, error) { + tlsConfig := &tls.Config{ + InsecureSkipVerify: skipVerify, + MinVersion: tls.VersionTLS12, + } + + if rootCA != "" { + rootCert, err := trustmanager.LoadCertFromFile(rootCA) + if err != nil { + return nil, fmt.Errorf( + "Could not load root ca file. %s", err.Error()) + } + rootPool := x509.NewCertPool() + rootPool.AddCert(rootCert) + tlsConfig.RootCAs = rootPool + } + + if clientCert != "" && clientKey != "" { + keypair, err := tls.LoadX509KeyPair(clientCert, clientKey) + if err != nil { + return nil, err + } + tlsConfig.Certificates = []tls.Certificate{keypair} } return tlsConfig, nil diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index 4d738a15c6..c86ddcf12c 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -35,8 +35,8 @@ func makeTempCertDir(t *testing.T) string { return tempDir } -// TestTLSConfigFailsIfUnableToLoadCerts fails if unable to load either of the -// server files or the client cert info +// If the cert files and directory are provided but are invalid, an error is +// returned. func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { tempDir := makeTempCertDir(t) @@ -50,9 +50,8 @@ func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { } } -// TestConfigServerTLSServerCertsOnly returns a valid tls config with the -// provided server certificate, and since clientAuth was false, no client auth -// or CAs configured. +// If server cert and key are provided, and client auth is disabled, then +// a valid tls.Config is returned with ClientAuth set to NoClientCert func TestConfigServerTLSServerCertsOnly(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) @@ -65,10 +64,10 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { assert.Nil(t, tlsConfig.ClientCAs) } -// TestConfigServerTLSNoCACertsIfNoClientAuth returns a valid tls config with -// the provided server certificate, and since clientAuth was false, no client -// auth or CAs configured even though a client CA cert was provided. -func TestConfigServerTLSNoCACertsIfNoClientAuth(t *testing.T) { +// If server cert and key are provided, and client cert directory is provided, +// a valid tls.Config is returned with the clientCAs set to the certs in that +// directory. +func TestConfigServerTLSWithCACerts(t *testing.T) { tempDir := makeTempCertDir(t) keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) @@ -78,12 +77,13 @@ func TestConfigServerTLSNoCACertsIfNoClientAuth(t *testing.T) { assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) assert.Equal(t, tls.NoClientCert, tlsConfig.ClientAuth) - assert.Nil(t, tlsConfig.ClientCAs) + assert.Len(t, tlsConfig.ClientCAs.Subjects(), 1) } -// TestTLSConfigClientAuthEnabledNoCACerts returns a valid tls config with the -// provided server certificate client auth enabled, but no CAs configured. -func TestTLSConfigClientAuthEnabledNoCACerts(t *testing.T) { +// If server cert and key are provided, and client auth is disabled, then +// a valid tls.Config is returned with ClientAuth set to +// RequireAndVerifyClientCert +func TestConfigServerTLSClientAuthEnabled(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) @@ -95,17 +95,54 @@ func TestTLSConfigClientAuthEnabledNoCACerts(t *testing.T) { assert.Nil(t, tlsConfig.ClientCAs) } -// TestTLSConfigClientAuthEnabledWithCACert returns a valid tls config with the -// provided server certificate, client auth enabled, and a client CA. -func TestTLSConfigClientAuthEnabledWithCACert(t *testing.T) { - tempDir := makeTempCertDir(t) +// The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean +func TestConfigClientTLSNoVerify(t *testing.T) { + for _, skip := range []bool{true, false} { + tlsConfig, err := ConfigureClientTLS("", skip, "", "") + assert.NoError(t, err) + assert.Nil(t, tlsConfig.Certificates) + assert.Equal(t, skip, tlsConfig.InsecureSkipVerify) + assert.Nil(t, tlsConfig.RootCAs) + } +} + +// The RootCA is set if it is provided and valid +func TestConfigClientTLSValidRootCA(t *testing.T) { + tlsConfig, err := ConfigureClientTLS(RootCA, false, "", "") + assert.NoError(t, err) + assert.Nil(t, tlsConfig.Certificates) + assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Len(t, tlsConfig.RootCAs.Subjects(), 1) +} + +// An error is returned if a root CA is provided but not valid +func TestConfigClientTLSInValidRootCA(t *testing.T) { + tlsConfig, err := ConfigureClientTLS("not-a-file.crt", false, "", "") + assert.Error(t, err) + assert.Nil(t, tlsConfig) +} + +// An error is returned if either the client cert or the key are provided +// but invalid. +func TestConfigClientTLSClientCertOrKeyInvalid(t *testing.T) { + for i := 0; i < 2; i++ { + files := []string{ServerCert, ServerKey} + files[i] = "not-a-file.crt" + tlsConfig, err := ConfigureClientTLS("", false, files[0], files[1]) + assert.Error(t, err) + assert.Nil(t, tlsConfig) + } +} + +// The certificate is set if the client cert and client key are provided and +// valid. +func TestConfigClientTLSValidClientCertAndKey(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, tempDir) + tlsConfig, err := ConfigureClientTLS("", false, ServerCert, ServerKey) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) - assert.True(t, tlsConfig.PreferServerCipherSuites) - assert.Equal(t, tls.RequireAndVerifyClientCert, tlsConfig.ClientAuth) - assert.Equal(t, 1, len(tlsConfig.ClientCAs.Subjects())) + assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Nil(t, tlsConfig.RootCAs) } From fb1013b997f9fcce7c80e1bcdc0c6b443d703c9d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 19 Oct 2015 13:01:58 -0700 Subject: [PATCH 06/13] Add servername to the client TLS config, and use it to build notary-server's TLS connection to notary-signer. Signed-off-by: Ying Li --- signer/signer_trust.go | 7 +++++-- utils/tls_config.go | 5 +++-- utils/tls_config_test.go | 25 ++++++++++++++++++++----- 3 files changed, 28 insertions(+), 9 deletions(-) diff --git a/signer/signer_trust.go b/signer/signer_trust.go index 07ad56ecc0..1454597297 100644 --- a/signer/signer_trust.go +++ b/signer/signer_trust.go @@ -7,6 +7,7 @@ import ( "github.com/Sirupsen/logrus" pb "github.com/docker/notary/proto" + "github.com/docker/notary/utils" "github.com/endophage/gotuf/data" "golang.org/x/net/context" "google.golang.org/grpc" @@ -30,10 +31,12 @@ type NotarySigner struct { func NewNotarySigner(hostname string, port string, tlscafile string) *NotarySigner { var opts []grpc.DialOption netAddr := net.JoinHostPort(hostname, port) - creds, err := credentials.NewClientTLSFromFile(tlscafile, hostname) + tlsConfig, err := utils.ConfigureClientTLS( + tlscafile, hostname, false, "", "") if err != nil { - logrus.Fatal("fail to read: ", err) + logrus.Fatal("Unable to set up TLS: ", err) } + creds := credentials.NewTLS(tlsConfig) opts = append(opts, grpc.WithTransportCredentials(creds)) conn, err := grpc.Dial(netAddr, opts...) diff --git a/utils/tls_config.go b/utils/tls_config.go index 3b3885b7de..3196a9831c 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -14,7 +14,7 @@ import ( // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or // doesn't deal with client certs at all. Nothing in the middle. -func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, caCertDir string) (*tls.Config, error) { +func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir string) (*tls.Config, error) { keypair, err := tls.LoadX509KeyPair(serverCert, serverKey) if err != nil { return nil, err @@ -66,10 +66,11 @@ func ConfigureServerTLS(serverCert string, serverKey string, clientAuth bool, ca // ConfigureClientTLS generates a tls configuration for clients using the // provided. -func ConfigureClientTLS(rootCA string, skipVerify bool, clientCert string, clientKey string) (*tls.Config, error) { +func ConfigureClientTLS(rootCA, serverName string, skipVerify bool, clientCert, clientKey string) (*tls.Config, error) { tlsConfig := &tls.Config{ InsecureSkipVerify: skipVerify, MinVersion: tls.VersionTLS12, + ServerName: serverName, } if rootCA != "" { diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index c86ddcf12c..7b7eebf409 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -98,26 +98,40 @@ func TestConfigServerTLSClientAuthEnabled(t *testing.T) { // The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean func TestConfigClientTLSNoVerify(t *testing.T) { for _, skip := range []bool{true, false} { - tlsConfig, err := ConfigureClientTLS("", skip, "", "") + tlsConfig, err := ConfigureClientTLS("", "", skip, "", "") assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, skip, tlsConfig.InsecureSkipVerify) + assert.Equal(t, "", tlsConfig.ServerName) + assert.Nil(t, tlsConfig.RootCAs) + } +} + +// The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean +func TestConfigClientServerName(t *testing.T) { + for _, name := range []string{"", "myname"} { + tlsConfig, err := ConfigureClientTLS("", name, false, "", "") + assert.NoError(t, err) + assert.Nil(t, tlsConfig.Certificates) + assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Equal(t, name, tlsConfig.ServerName) assert.Nil(t, tlsConfig.RootCAs) } } // The RootCA is set if it is provided and valid func TestConfigClientTLSValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS(RootCA, false, "", "") + tlsConfig, err := ConfigureClientTLS(RootCA, "", false, "", "") assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Equal(t, "", tlsConfig.ServerName) assert.Len(t, tlsConfig.RootCAs.Subjects(), 1) } // An error is returned if a root CA is provided but not valid func TestConfigClientTLSInValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS("not-a-file.crt", false, "", "") + tlsConfig, err := ConfigureClientTLS("not-a-file.crt", "", false, "", "") assert.Error(t, err) assert.Nil(t, tlsConfig) } @@ -128,7 +142,7 @@ func TestConfigClientTLSClientCertOrKeyInvalid(t *testing.T) { for i := 0; i < 2; i++ { files := []string{ServerCert, ServerKey} files[i] = "not-a-file.crt" - tlsConfig, err := ConfigureClientTLS("", false, files[0], files[1]) + tlsConfig, err := ConfigureClientTLS("", "", false, files[0], files[1]) assert.Error(t, err) assert.Nil(t, tlsConfig) } @@ -140,9 +154,10 @@ func TestConfigClientTLSValidClientCertAndKey(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureClientTLS("", false, ServerCert, ServerKey) + tlsConfig, err := ConfigureClientTLS("", "", false, ServerCert, ServerKey) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Equal(t, "", tlsConfig.ServerName) assert.Nil(t, tlsConfig.RootCAs) } From fc389b7bc3ba220df11e278bf356268acba8077a Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 19 Oct 2015 13:11:58 -0700 Subject: [PATCH 07/13] Use tls client config utility in notary as well. Signed-off-by: Ying Li --- cmd/notary/tuf.go | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 40f5191ca5..7df093f585 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -3,8 +3,6 @@ package main import ( "bufio" "crypto/sha256" - "crypto/tls" - "crypto/x509" "fmt" "io/ioutil" "net" @@ -22,7 +20,7 @@ import ( "github.com/docker/distribution/registry/client/transport" "github.com/docker/docker/pkg/term" notaryclient "github.com/docker/notary/client" - "github.com/docker/notary/trustmanager" + "github.com/docker/notary/utils" "github.com/spf13/cobra" ) @@ -360,7 +358,6 @@ func (ps passwordStore) Basic(u *url.URL) (string, string) { func getTransport(gun string, readOnly bool) http.RoundTripper { // Attempt to get a root CA from the config file. Nil is the host defaults. - rootPool := x509.NewCertPool() rootCAFile := mainViper.GetString("remote_server.root_ca") if rootCAFile != "" { // If we haven't been given an Absolute path, we assume it's relative @@ -368,19 +365,13 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { if !filepath.IsAbs(rootCAFile) { rootCAFile = filepath.Join(configPath, rootCAFile) } - rootCert, err := trustmanager.LoadCertFromFile(rootCAFile) - if err != nil { - fatalf("could not load root ca file. %s", err.Error()) - } - rootPool.AddCert(rootCert) } - // skipTLSVerify is false by default so verification will - // be performed. - tlsConfig := &tls.Config{ - InsecureSkipVerify: mainViper.GetBool("remote_server.skipTLSVerify"), - MinVersion: tls.VersionTLS10, - RootCAs: rootPool, + tlsConfig, err := utils.ConfigureClientTLS( + rootCAFile, "", mainViper.GetBool("remote_server.skipTLSVerify"), + "", "") + if err != nil { + logrus.Fatal("Unable to configure TLS: ", err.Error()) } base := &http.Transport{ From fb81aaed10ded21ac0857fff88e5957ea42f4453 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 19 Oct 2015 13:40:38 -0700 Subject: [PATCH 08/13] Add test for if the client CA dir is empty Signed-off-by: Ying Li --- utils/tls_config_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index 7b7eebf409..70821e2664 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -64,6 +64,17 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { assert.Nil(t, tlsConfig.ClientCAs) } +// If a valid client cert directory is provided, but it contains no client +// certs, an error is returned. +func TestConfigServerTLSWithEmptyCACertDir(t *testing.T) { + tempDir, err := ioutil.TempDir("/tmp", "cert-test") + assert.NoError(t, err, "couldn't open temp directory") + + tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) + assert.Nil(t, tlsConfig) + assert.Error(t, err) +} + // If server cert and key are provided, and client cert directory is provided, // a valid tls.Config is returned with the clientCAs set to the certs in that // directory. From 5cdb46a9da9be9802615e7cbf97a0a54314c6c33 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Mon, 19 Oct 2015 16:18:12 -0700 Subject: [PATCH 09/13] Accept the same ciphersuites in the client and server as docker. Signed-off-by: Ying Li --- utils/tls_config.go | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/utils/tls_config.go b/utils/tls_config.go index 3196a9831c..e41ab1e25d 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -10,6 +10,22 @@ import ( "github.com/docker/notary/trustmanager" ) +// Client TLS cipher suites (dropping CBC ciphers for client preferred suite set) +var clientCipherSuites = []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, + tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, +} + +// Server TLS cipher suites +var serverCipherSuites = append(clientCipherSuites, []uint16{ + tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, + tls.TLS_RSA_WITH_AES_256_CBC_SHA, + tls.TLS_RSA_WITH_AES_128_CBC_SHA, +}...) + // ConfigureServerTLS specifies a set of ciphersuites, the server cert and key, // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or @@ -23,18 +39,9 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir tlsConfig := &tls.Config{ MinVersion: tls.VersionTLS12, PreferServerCipherSuites: true, - CipherSuites: []uint16{ - tls.TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256, - tls.TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA, - tls.TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_128_CBC_SHA, - tls.TLS_RSA_WITH_AES_256_CBC_SHA, - }, - Certificates: []tls.Certificate{keypair}, - Rand: rand.Reader, + CipherSuites: serverCipherSuites, + Certificates: []tls.Certificate{keypair}, + Rand: rand.Reader, } if clientAuth { @@ -65,11 +72,12 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir } // ConfigureClientTLS generates a tls configuration for clients using the -// provided. +// provided parameters. func ConfigureClientTLS(rootCA, serverName string, skipVerify bool, clientCert, clientKey string) (*tls.Config, error) { tlsConfig := &tls.Config{ InsecureSkipVerify: skipVerify, MinVersion: tls.VersionTLS12, + CipherSuites: clientCipherSuites, ServerName: serverName, } From 412e0facc84af93307817a83bd9b4540cc88af06 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 21 Oct 2015 10:38:48 -0700 Subject: [PATCH 10/13] Explicitly check the skip tls verify boolean in notary client Signed-off-by: Ying Li --- cmd/notary/tuf.go | 7 +++++-- utils/tls_config.go | 4 ++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 7df093f585..1a80c8464e 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -367,9 +367,12 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { } } + insecureSkipVerify := false + if mainViper.IsSet("remote_server.skipTLSVerify") { + insecureSkipVerify = mainViper.GetBool("remote_server.skipTLSVerify") + } tlsConfig, err := utils.ConfigureClientTLS( - rootCAFile, "", mainViper.GetBool("remote_server.skipTLSVerify"), - "", "") + rootCAFile, "", insecureSkipVerify, "", "") if err != nil { logrus.Fatal("Unable to configure TLS: ", err.Error()) } diff --git a/utils/tls_config.go b/utils/tls_config.go index e41ab1e25d..9e9347d087 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -73,9 +73,9 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir // ConfigureClientTLS generates a tls configuration for clients using the // provided parameters. -func ConfigureClientTLS(rootCA, serverName string, skipVerify bool, clientCert, clientKey string) (*tls.Config, error) { +func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clientCert, clientKey string) (*tls.Config, error) { tlsConfig := &tls.Config{ - InsecureSkipVerify: skipVerify, + InsecureSkipVerify: insecureSkipVerify, MinVersion: tls.VersionTLS12, CipherSuites: clientCipherSuites, ServerName: serverName, From 61f9f8425408559be2ed7bd3c1c9049c9ac3b6cc Mon Sep 17 00:00:00 2001 From: Ying Li Date: Wed, 21 Oct 2015 18:43:33 -0700 Subject: [PATCH 11/13] Use configuration option structures to set up client TLS and server TLS. Test for if client cert is passed without a client key and vice versa. Fail in ConfigureClientTLS if only one of client cert/key is passed. Lint fixes. Signed-off-by: Ying Li --- cmd/notary-signer/main.go | 5 ++- cmd/notary/tuf.go | 6 ++- server/server.go | 6 ++- signer/signer_trust.go | 6 ++- signer/signer_trust_test.go | 2 +- trustmanager/x509filestore_test.go | 31 +++++++-------- utils/tls_config.go | 51 +++++++++++++++++-------- utils/tls_config_test.go | 60 +++++++++++++++++++++--------- 8 files changed, 110 insertions(+), 57 deletions(-) diff --git a/cmd/notary-signer/main.go b/cmd/notary-signer/main.go index 23f03b3f17..4a86dde90b 100644 --- a/cmd/notary-signer/main.go +++ b/cmd/notary-signer/main.go @@ -102,7 +102,10 @@ func main() { log.Fatalf("Certificate and key are mandatory") } - tlsConfig, err := utils.ConfigureServerTLS(certFile, keyFile, false, "") + tlsConfig, err := utils.ConfigureServerTLS(&utils.ServerTLSOpts{ + ServerCertFile: certFile, + ServerKeyFile: keyFile, + }) if err != nil { logrus.Fatalf("Unable to set up TLS: %s", err.Error()) } diff --git a/cmd/notary/tuf.go b/cmd/notary/tuf.go index 1a80c8464e..b3cd58e348 100644 --- a/cmd/notary/tuf.go +++ b/cmd/notary/tuf.go @@ -371,8 +371,10 @@ func getTransport(gun string, readOnly bool) http.RoundTripper { if mainViper.IsSet("remote_server.skipTLSVerify") { insecureSkipVerify = mainViper.GetBool("remote_server.skipTLSVerify") } - tlsConfig, err := utils.ConfigureClientTLS( - rootCAFile, "", insecureSkipVerify, "", "") + tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{ + RootCAFile: rootCAFile, + InsecureSkipVerify: insecureSkipVerify, + }) if err != nil { logrus.Fatal("Unable to configure TLS: ", err.Error()) } diff --git a/server/server.go b/server/server.go index e265ae3e88..cc8e29ded6 100644 --- a/server/server.go +++ b/server/server.go @@ -41,8 +41,10 @@ func Run(ctx context.Context, addr, tlsCertFile, tlsKeyFile string, trust signed } if tlsCertFile != "" && tlsKeyFile != "" { - tlsConfig, err := utils.ConfigureServerTLS( - tlsCertFile, tlsKeyFile, false, "") + tlsConfig, err := utils.ConfigureServerTLS(&utils.ServerTLSOpts{ + ServerCertFile: tlsCertFile, + ServerKeyFile: tlsKeyFile, + }) if err != nil { return err } diff --git a/signer/signer_trust.go b/signer/signer_trust.go index 1454597297..448a1767da 100644 --- a/signer/signer_trust.go +++ b/signer/signer_trust.go @@ -31,8 +31,10 @@ type NotarySigner struct { func NewNotarySigner(hostname string, port string, tlscafile string) *NotarySigner { var opts []grpc.DialOption netAddr := net.JoinHostPort(hostname, port) - tlsConfig, err := utils.ConfigureClientTLS( - tlscafile, hostname, false, "", "") + tlsConfig, err := utils.ConfigureClientTLS(&utils.ClientTLSOpts{ + RootCAFile: tlscafile, + ServerName: hostname, + }) if err != nil { logrus.Fatal("Unable to set up TLS: ", err) } diff --git a/signer/signer_trust_test.go b/signer/signer_trust_test.go index bcbba05bd6..4b5ccef40d 100644 --- a/signer/signer_trust_test.go +++ b/signer/signer_trust_test.go @@ -40,7 +40,7 @@ func stubHealthFunction(t *testing.T, status map[string]string, err error) rpcHe _, withDeadline := ctx.Deadline() assert.True(t, withDeadline) - return &pb.HealthStatus{status}, err + return &pb.HealthStatus{Status: status}, err } } diff --git a/trustmanager/x509filestore_test.go b/trustmanager/x509filestore_test.go index 95be916107..58decfc748 100644 --- a/trustmanager/x509filestore_test.go +++ b/trustmanager/x509filestore_test.go @@ -27,39 +27,36 @@ func TestNewX509FileStore(t *testing.T) { // not overwrite any of the. func TestNewX509FileStoreLoadsExistingCerts(t *testing.T) { tempDir, err := ioutil.TempDir("", "cert-test") - assert.NoError(t, err, "couldn't open temp directory") + assert.NoError(t, err) defer os.RemoveAll(tempDir) certBytes, err := ioutil.ReadFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "couldn't read fixtures/root-ca.crt") + assert.NoError(t, err) out, err := os.Create(filepath.Join(tempDir, "root-ca.crt")) - assert.NoError(t, err, "couldn't create a file in the temp dir") + assert.NoError(t, err) // to distinguish it from the canonical format distinguishingBytes := []byte{'\n', '\n', '\n', '\n', '\n', '\n'} nBytes, err := out.Write(distinguishingBytes) - assert.NoError(t, err, "could not write newlines to the temporary file") - assert.Equal(t, len(distinguishingBytes), nBytes, - "didn't write all bytes to temporary file") + assert.NoError(t, err) + assert.Len(t, distinguishingBytes, nBytes) nBytes, err = out.Write(certBytes) - assert.NoError(t, err, "could not write cert to the temporary file") - assert.Equal(t, len(certBytes), nBytes, - "didn't write all bytes to temporary file") + assert.NoError(t, err) + assert.Len(t, certBytes, nBytes) err = out.Close() - assert.NoError(t, err, "could not close temporary file") + assert.NoError(t, err) store, err := NewX509FileStore(tempDir) - assert.NoError(t, err, "failed to create a new X509FileStore") + assert.NoError(t, err) expectedCert, err := LoadCertFromFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "could not load root-ca.crt") - assert.Equal(t, store.GetCertificates(), []*x509.Certificate{expectedCert}, - "did not load certificate already in the directory") + assert.NoError(t, err) + assert.Equal(t, []*x509.Certificate{expectedCert}, store.GetCertificates()) outBytes, err := ioutil.ReadFile(filepath.Join(tempDir, "root-ca.crt")) - assert.NoError(t, err, "couldn't read temporary file") + assert.NoError(t, err) assert.Equal(t, distinguishingBytes, outBytes[:6], "original file overwritten") assert.Equal(t, certBytes, outBytes[6:], "original file overwritten") } @@ -131,11 +128,11 @@ func TestNewX509FileStoreEmpty(t *testing.T) { defer os.RemoveAll(tempDir) store, err := NewX509FileStore(tempDir) - assert.NoError(t, err, "failed to create a new X509FileStore: %v", store) + assert.NoError(t, err) assert.True(t, store.Empty()) err = store.AddCertFromFile("../fixtures/root-ca.crt") - assert.NoError(t, err, "failed to add certificate from file") + assert.NoError(t, err) assert.False(t, store.Empty()) } diff --git a/utils/tls_config.go b/utils/tls_config.go index 9e9347d087..0b1e70eac5 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -26,12 +26,22 @@ var serverCipherSuites = append(clientCipherSuites, []uint16{ tls.TLS_RSA_WITH_AES_128_CBC_SHA, }...) +// ServerTLSOpts generates a tls configuration for servers using the +// provided parameters. +type ServerTLSOpts struct { + ServerCertFile string + ServerKeyFile string + RequireClientAuth bool + ClientCADirectory string +} + // ConfigureServerTLS specifies a set of ciphersuites, the server cert and key, // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or // doesn't deal with client certs at all. Nothing in the middle. -func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir string) (*tls.Config, error) { - keypair, err := tls.LoadX509KeyPair(serverCert, serverKey) +func ConfigureServerTLS(opts *ServerTLSOpts) (*tls.Config, error) { + keypair, err := tls.LoadX509KeyPair( + opts.ServerCertFile, opts.ServerKeyFile) if err != nil { return nil, err } @@ -44,26 +54,26 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir Rand: rand.Reader, } - if clientAuth { + if opts.RequireClientAuth { tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert } - if caCertDir != "" { + if opts.ClientCADirectory != "" { // Check to see if the given directory exists - fi, err := os.Stat(caCertDir) + fi, err := os.Stat(opts.ClientCADirectory) if err != nil { return nil, err } if !fi.IsDir() { - return nil, fmt.Errorf("No such directory: %s", caCertDir) + return nil, fmt.Errorf("No such directory: %s", opts.ClientCADirectory) } - certStore, err := trustmanager.NewX509FileStore(caCertDir) + certStore, err := trustmanager.NewX509FileStore(opts.ClientCADirectory) if err != nil { return nil, err } if certStore.Empty() { - return nil, fmt.Errorf("No certificates in %s", caCertDir) + return nil, fmt.Errorf("No certificates in %s", opts.ClientCADirectory) } tlsConfig.ClientCAs = certStore.GetCertificatePool() } @@ -71,18 +81,28 @@ func ConfigureServerTLS(serverCert, serverKey string, clientAuth bool, caCertDir return tlsConfig, nil } +// ClientTLSOpts is a struct that contains options to pass to +// ConfigureClientTLS +type ClientTLSOpts struct { + RootCAFile string + ServerName string + InsecureSkipVerify bool + ClientCertFile string + ClientKeyFile string +} + // ConfigureClientTLS generates a tls configuration for clients using the // provided parameters. -func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clientCert, clientKey string) (*tls.Config, error) { +func ConfigureClientTLS(opts *ClientTLSOpts) (*tls.Config, error) { tlsConfig := &tls.Config{ - InsecureSkipVerify: insecureSkipVerify, + InsecureSkipVerify: opts.InsecureSkipVerify, MinVersion: tls.VersionTLS12, CipherSuites: clientCipherSuites, - ServerName: serverName, + ServerName: opts.ServerName, } - if rootCA != "" { - rootCert, err := trustmanager.LoadCertFromFile(rootCA) + if opts.RootCAFile != "" { + rootCert, err := trustmanager.LoadCertFromFile(opts.RootCAFile) if err != nil { return nil, fmt.Errorf( "Could not load root ca file. %s", err.Error()) @@ -92,8 +112,9 @@ func ConfigureClientTLS(rootCA, serverName string, insecureSkipVerify bool, clie tlsConfig.RootCAs = rootPool } - if clientCert != "" && clientKey != "" { - keypair, err := tls.LoadX509KeyPair(clientCert, clientKey) + if opts.ClientCertFile != "" || opts.ClientKeyFile != "" { + keypair, err := tls.LoadX509KeyPair( + opts.ClientCertFile, opts.ClientKeyFile) if err != nil { return nil, err } diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index 70821e2664..328ebc49d9 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -44,7 +44,12 @@ func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { files := []string{ServerCert, ServerKey, tempDir} files[i] = "not-real-file" - result, err := ConfigureServerTLS(files[0], files[1], true, files[2]) + result, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: files[0], + ServerKeyFile: files[1], + RequireClientAuth: true, + ClientCADirectory: files[2], + }) assert.Nil(t, result) assert.Error(t, err) } @@ -56,7 +61,10 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, "") + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -70,7 +78,11 @@ func TestConfigServerTLSWithEmptyCACertDir(t *testing.T) { tempDir, err := ioutil.TempDir("/tmp", "cert-test") assert.NoError(t, err, "couldn't open temp directory") - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCADirectory: tempDir, + }) assert.Nil(t, tlsConfig) assert.Error(t, err) } @@ -83,7 +95,11 @@ func TestConfigServerTLSWithCACerts(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, false, tempDir) + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCADirectory: tempDir, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -98,7 +114,11 @@ func TestConfigServerTLSClientAuthEnabled(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureServerTLS(ServerCert, ServerKey, true, "") + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + RequireClientAuth: true, + }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.True(t, tlsConfig.PreferServerCipherSuites) @@ -109,7 +129,8 @@ func TestConfigServerTLSClientAuthEnabled(t *testing.T) { // The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean func TestConfigClientTLSNoVerify(t *testing.T) { for _, skip := range []bool{true, false} { - tlsConfig, err := ConfigureClientTLS("", "", skip, "", "") + tlsConfig, err := ConfigureClientTLS( + &ClientTLSOpts{InsecureSkipVerify: skip}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, skip, tlsConfig.InsecureSkipVerify) @@ -121,7 +142,7 @@ func TestConfigClientTLSNoVerify(t *testing.T) { // The skipVerify boolean gets set on the tls.Config's InsecureSkipBoolean func TestConfigClientServerName(t *testing.T) { for _, name := range []string{"", "myname"} { - tlsConfig, err := ConfigureClientTLS("", name, false, "", "") + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ServerName: name}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) @@ -132,7 +153,7 @@ func TestConfigClientServerName(t *testing.T) { // The RootCA is set if it is provided and valid func TestConfigClientTLSValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS(RootCA, "", false, "", "") + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{RootCAFile: RootCA}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) @@ -141,21 +162,25 @@ func TestConfigClientTLSValidRootCA(t *testing.T) { } // An error is returned if a root CA is provided but not valid -func TestConfigClientTLSInValidRootCA(t *testing.T) { - tlsConfig, err := ConfigureClientTLS("not-a-file.crt", "", false, "", "") +func TestConfigClientTLSInvalidRootCA(t *testing.T) { + tlsConfig, err := ConfigureClientTLS( + &ClientTLSOpts{RootCAFile: "not-a-file"}) assert.Error(t, err) assert.Nil(t, tlsConfig) } // An error is returned if either the client cert or the key are provided -// but invalid. +// but invalid or blank. func TestConfigClientTLSClientCertOrKeyInvalid(t *testing.T) { for i := 0; i < 2; i++ { - files := []string{ServerCert, ServerKey} - files[i] = "not-a-file.crt" - tlsConfig, err := ConfigureClientTLS("", "", false, files[0], files[1]) - assert.Error(t, err) - assert.Nil(t, tlsConfig) + for _, invalid := range []string{"not-a-file", ""} { + files := []string{ServerCert, ServerKey} + files[i] = invalid + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ + ClientCertFile: files[0], ClientKeyFile: files[1]}) + assert.Error(t, err) + assert.Nil(t, tlsConfig) + } } } @@ -165,7 +190,8 @@ func TestConfigClientTLSValidClientCertAndKey(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) - tlsConfig, err := ConfigureClientTLS("", "", false, ServerCert, ServerKey) + tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{ + ClientCertFile: ServerCert, ClientKeyFile: ServerKey}) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) assert.Equal(t, false, tlsConfig.InsecureSkipVerify) From 09dc607bef96feafdd7cbce1194cc39614366b6d Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 23 Oct 2015 15:56:47 -0700 Subject: [PATCH 12/13] Read multiple CA certs from a single PEM file - thanks @mtrmac! Signed-off-by: Ying Li --- utils/tls_config.go | 50 +++++++------- utils/tls_config_test.go | 143 ++++++++++++++++++++++++++++----------- 2 files changed, 129 insertions(+), 64 deletions(-) diff --git a/utils/tls_config.go b/utils/tls_config.go index 0b1e70eac5..b457335181 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -5,9 +5,7 @@ import ( "crypto/tls" "crypto/x509" "fmt" - "os" - - "github.com/docker/notary/trustmanager" + "io/ioutil" ) // Client TLS cipher suites (dropping CBC ciphers for client preferred suite set) @@ -26,13 +24,30 @@ var serverCipherSuites = append(clientCipherSuites, []uint16{ tls.TLS_RSA_WITH_AES_128_CBC_SHA, }...) +func poolFromFile(filename string) (*x509.CertPool, error) { + pemBytes, err := ioutil.ReadFile(filename) + if err != nil { + return nil, err + } + pool := x509.NewCertPool() + if ok := pool.AppendCertsFromPEM(pemBytes); !ok { + return nil, fmt.Errorf( + "Unable to parse certificates from %s", filename) + } + if len(pool.Subjects()) == 0 { + return nil, fmt.Errorf( + "No certificates parsed from %s", filename) + } + return pool, nil +} + // ServerTLSOpts generates a tls configuration for servers using the // provided parameters. type ServerTLSOpts struct { ServerCertFile string ServerKeyFile string RequireClientAuth bool - ClientCADirectory string + ClientCAFile string } // ConfigureServerTLS specifies a set of ciphersuites, the server cert and key, @@ -58,24 +73,12 @@ func ConfigureServerTLS(opts *ServerTLSOpts) (*tls.Config, error) { tlsConfig.ClientAuth = tls.RequireAndVerifyClientCert } - if opts.ClientCADirectory != "" { - // Check to see if the given directory exists - fi, err := os.Stat(opts.ClientCADirectory) + if opts.ClientCAFile != "" { + pool, err := poolFromFile(opts.ClientCAFile) if err != nil { return nil, err } - if !fi.IsDir() { - return nil, fmt.Errorf("No such directory: %s", opts.ClientCADirectory) - } - - certStore, err := trustmanager.NewX509FileStore(opts.ClientCADirectory) - if err != nil { - return nil, err - } - if certStore.Empty() { - return nil, fmt.Errorf("No certificates in %s", opts.ClientCADirectory) - } - tlsConfig.ClientCAs = certStore.GetCertificatePool() + tlsConfig.ClientCAs = pool } return tlsConfig, nil @@ -102,14 +105,11 @@ func ConfigureClientTLS(opts *ClientTLSOpts) (*tls.Config, error) { } if opts.RootCAFile != "" { - rootCert, err := trustmanager.LoadCertFromFile(opts.RootCAFile) + pool, err := poolFromFile(opts.RootCAFile) if err != nil { - return nil, fmt.Errorf( - "Could not load root ca file. %s", err.Error()) + return nil, err } - rootPool := x509.NewCertPool() - rootPool.AddCert(rootCert) - tlsConfig.RootCAs = rootPool + tlsConfig.RootCAs = pool } if opts.ClientCertFile != "" || opts.ClientKeyFile != "" { diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index 328ebc49d9..f6b2e73a59 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -1,13 +1,18 @@ package utils import ( + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" "crypto/tls" - "io" + "crypto/x509" "io/ioutil" "os" - "path/filepath" "testing" + "github.com/docker/notary/trustmanager" "github.com/stretchr/testify/assert" ) @@ -17,38 +22,57 @@ const ( RootCA = "../fixtures/root-ca.crt" ) -// copies the provided certificate into a temporary directory -func makeTempCertDir(t *testing.T) string { - tempDir, err := ioutil.TempDir("/tmp", "cert-test") - assert.NoError(t, err, "couldn't open temp directory") +// generates a multiple-certificate file with both RSA and ECDSA certs and +// some garbage, returns filename. +func generateMultiCert(t *testing.T) string { + tempFile, err := ioutil.TempFile("/tmp", "cert-test") + defer tempFile.Close() + assert.NoError(t, err) - in, err := os.Open(RootCA) - assert.NoError(t, err, "cannot open %s", RootCA) - defer in.Close() - copiedCert := filepath.Join(tempDir, filepath.Base(RootCA)) - out, err := os.Create(copiedCert) - assert.NoError(t, err, "cannot open %s", copiedCert) - defer out.Close() - _, err = io.Copy(out, in) - assert.NoError(t, err, "cannot copy %s to %s", RootCA, copiedCert) + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + assert.NoError(t, err) + ecKey, err := ecdsa.GenerateKey(elliptic.P224(), rand.Reader) + assert.NoError(t, err) + template, err := trustmanager.NewCertificate("gun") + assert.NoError(t, err) - return tempDir + for _, key := range []crypto.Signer{rsaKey, ecKey} { + derBytes, err := x509.CreateCertificate( + rand.Reader, template, template, key.Public(), key) + assert.NoError(t, err) + + cert, err := x509.ParseCertificate(derBytes) + assert.NoError(t, err) + + pemBytes := trustmanager.CertToPEM(cert) + nBytes, err := tempFile.Write(pemBytes) + assert.NoError(t, err) + assert.Equal(t, nBytes, len(pemBytes)) + + assert.NoError(t, err) + } + + _, err = tempFile.WriteString(`\n + -----BEGIN CERTIFICATE----- + This is some garbage that isnt a cert + -----END CERTIFICATE----- + `) + + return tempFile.Name() } // If the cert files and directory are provided but are invalid, an error is // returned. func TestConfigServerTLSFailsIfUnableToLoadCerts(t *testing.T) { - tempDir := makeTempCertDir(t) - for i := 0; i < 3; i++ { - files := []string{ServerCert, ServerKey, tempDir} + files := []string{ServerCert, ServerKey, RootCA} files[i] = "not-real-file" result, err := ConfigureServerTLS(&ServerTLSOpts{ ServerCertFile: files[0], ServerKeyFile: files[1], RequireClientAuth: true, - ClientCADirectory: files[2], + ClientCAFile: files[2], }) assert.Nil(t, result) assert.Error(t, err) @@ -72,33 +96,34 @@ func TestConfigServerTLSServerCertsOnly(t *testing.T) { assert.Nil(t, tlsConfig.ClientCAs) } -// If a valid client cert directory is provided, but it contains no client +// If a valid client cert file is provided, but it contains no client // certs, an error is returned. -func TestConfigServerTLSWithEmptyCACertDir(t *testing.T) { - tempDir, err := ioutil.TempDir("/tmp", "cert-test") - assert.NoError(t, err, "couldn't open temp directory") +func TestConfigServerTLSWithEmptyCACertFile(t *testing.T) { + tempFile, err := ioutil.TempFile("/tmp", "cert-test") + assert.NoError(t, err) + defer os.RemoveAll(tempFile.Name()) + tempFile.Close() tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ - ServerCertFile: ServerCert, - ServerKeyFile: ServerKey, - ClientCADirectory: tempDir, + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCAFile: tempFile.Name(), }) assert.Nil(t, tlsConfig) assert.Error(t, err) } -// If server cert and key are provided, and client cert directory is provided, -// a valid tls.Config is returned with the clientCAs set to the certs in that -// directory. -func TestConfigServerTLSWithCACerts(t *testing.T) { - tempDir := makeTempCertDir(t) +// If server cert and key are provided, and client cert file is provided with +// one cert, a valid tls.Config is returned with the clientCAs set to that +// cert. +func TestConfigServerTLSWithOneCACert(t *testing.T) { keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) assert.NoError(t, err) tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ - ServerCertFile: ServerCert, - ServerKeyFile: ServerKey, - ClientCADirectory: tempDir, + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCAFile: RootCA, }) assert.NoError(t, err) assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) @@ -107,6 +132,30 @@ func TestConfigServerTLSWithCACerts(t *testing.T) { assert.Len(t, tlsConfig.ClientCAs.Subjects(), 1) } +// If server cert and key are provided, and client cert file is provided with +// multiple certs (and garbage), a valid tls.Config is returned with the +// clientCAs set to the valid cert and the garbage is ignored (but only +// because the garbage is at the end - actually CertPool.AppendCertsFromPEM +// aborts as soon as it finds an invalid cert) +func TestConfigServerTLSWithMultipleCACertsAndGarbage(t *testing.T) { + tempFilename := generateMultiCert(t) + defer os.RemoveAll(tempFilename) + + keypair, err := tls.LoadX509KeyPair(ServerCert, ServerKey) + assert.NoError(t, err) + + tlsConfig, err := ConfigureServerTLS(&ServerTLSOpts{ + ServerCertFile: ServerCert, + ServerKeyFile: ServerKey, + ClientCAFile: tempFilename, + }) + assert.NoError(t, err) + assert.Equal(t, []tls.Certificate{keypair}, tlsConfig.Certificates) + assert.True(t, tlsConfig.PreferServerCipherSuites) + assert.Equal(t, tls.NoClientCert, tlsConfig.ClientAuth) + assert.Len(t, tlsConfig.ClientCAs.Subjects(), 2) +} + // If server cert and key are provided, and client auth is disabled, then // a valid tls.Config is returned with ClientAuth set to // RequireAndVerifyClientCert @@ -151,8 +200,8 @@ func TestConfigClientServerName(t *testing.T) { } } -// The RootCA is set if it is provided and valid -func TestConfigClientTLSValidRootCA(t *testing.T) { +// The RootCA is set if the file provided has a single CA cert. +func TestConfigClientTLSRootCAFileWithOneCert(t *testing.T) { tlsConfig, err := ConfigureClientTLS(&ClientTLSOpts{RootCAFile: RootCA}) assert.NoError(t, err) assert.Nil(t, tlsConfig.Certificates) @@ -161,8 +210,24 @@ func TestConfigClientTLSValidRootCA(t *testing.T) { assert.Len(t, tlsConfig.RootCAs.Subjects(), 1) } -// An error is returned if a root CA is provided but not valid -func TestConfigClientTLSInvalidRootCA(t *testing.T) { +// If the root CA file provided has multiple CA certs and garbage, only the +// valid certs are read (but only because the garbage is at the end - actually +// CertPool.AppendCertsFromPEM aborts as soon as it finds an invalid cert) +func TestConfigClientTLSRootCAFileMultipleCertsAndGarbage(t *testing.T) { + tempFilename := generateMultiCert(t) + defer os.RemoveAll(tempFilename) + + tlsConfig, err := ConfigureClientTLS( + &ClientTLSOpts{RootCAFile: tempFilename}) + assert.NoError(t, err) + assert.Nil(t, tlsConfig.Certificates) + assert.Equal(t, false, tlsConfig.InsecureSkipVerify) + assert.Equal(t, "", tlsConfig.ServerName) + assert.Len(t, tlsConfig.RootCAs.Subjects(), 2) +} + +// An error is returned if a root CA is provided but the file doesn't exist. +func TestConfigClientTLSNonexistentRootCAFile(t *testing.T) { tlsConfig, err := ConfigureClientTLS( &ClientTLSOpts{RootCAFile: "not-a-file"}) assert.Error(t, err) From 15c3bbeb9c3b7bb4d8e7d0116cb3e3c3bf98ee53 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Fri, 23 Oct 2015 20:55:59 -0700 Subject: [PATCH 13/13] Remove explicit test for parsing garbage in certs. Signed-off-by: Ying Li --- utils/tls_config.go | 8 ++++++++ utils/tls_config_test.go | 26 +++++++------------------- 2 files changed, 15 insertions(+), 19 deletions(-) diff --git a/utils/tls_config.go b/utils/tls_config.go index b457335181..15050f84ae 100644 --- a/utils/tls_config.go +++ b/utils/tls_config.go @@ -54,6 +54,10 @@ type ServerTLSOpts struct { // and optionally client authentication. Note that a tls configuration is // constructed that either requires and verifies client authentication or // doesn't deal with client certs at all. Nothing in the middle. +// +// Also note that if the client CA file contains invalid data, behavior is not +// guaranteed. Currently (as of Go 1.5.1) only the valid certificates up to +// the bad data will be parsed and added the client CA pool. func ConfigureServerTLS(opts *ServerTLSOpts) (*tls.Config, error) { keypair, err := tls.LoadX509KeyPair( opts.ServerCertFile, opts.ServerKeyFile) @@ -96,6 +100,10 @@ type ClientTLSOpts struct { // ConfigureClientTLS generates a tls configuration for clients using the // provided parameters. +/// +// Note that if the root CA file contains invalid data, behavior is not +// guaranteed. Currently (as of Go 1.5.1) only the valid certificates up to +// the bad data will be parsed and added the root CA pool. func ConfigureClientTLS(opts *ClientTLSOpts) (*tls.Config, error) { tlsConfig := &tls.Config{ InsecureSkipVerify: opts.InsecureSkipVerify, diff --git a/utils/tls_config_test.go b/utils/tls_config_test.go index f6b2e73a59..75c53e8809 100644 --- a/utils/tls_config_test.go +++ b/utils/tls_config_test.go @@ -23,7 +23,7 @@ const ( ) // generates a multiple-certificate file with both RSA and ECDSA certs and -// some garbage, returns filename. +// returns the filename so that cleanup can be deferred. func generateMultiCert(t *testing.T) string { tempFile, err := ioutil.TempFile("/tmp", "cert-test") defer tempFile.Close() @@ -48,16 +48,7 @@ func generateMultiCert(t *testing.T) string { nBytes, err := tempFile.Write(pemBytes) assert.NoError(t, err) assert.Equal(t, nBytes, len(pemBytes)) - - assert.NoError(t, err) } - - _, err = tempFile.WriteString(`\n - -----BEGIN CERTIFICATE----- - This is some garbage that isnt a cert - -----END CERTIFICATE----- - `) - return tempFile.Name() } @@ -133,11 +124,9 @@ func TestConfigServerTLSWithOneCACert(t *testing.T) { } // If server cert and key are provided, and client cert file is provided with -// multiple certs (and garbage), a valid tls.Config is returned with the -// clientCAs set to the valid cert and the garbage is ignored (but only -// because the garbage is at the end - actually CertPool.AppendCertsFromPEM -// aborts as soon as it finds an invalid cert) -func TestConfigServerTLSWithMultipleCACertsAndGarbage(t *testing.T) { +// multiple certs, a valid tls.Config is returned with the clientCAs set to +// the valid cert. +func TestConfigServerTLSWithMultipleCACerts(t *testing.T) { tempFilename := generateMultiCert(t) defer os.RemoveAll(tempFilename) @@ -210,10 +199,9 @@ func TestConfigClientTLSRootCAFileWithOneCert(t *testing.T) { assert.Len(t, tlsConfig.RootCAs.Subjects(), 1) } -// If the root CA file provided has multiple CA certs and garbage, only the -// valid certs are read (but only because the garbage is at the end - actually -// CertPool.AppendCertsFromPEM aborts as soon as it finds an invalid cert) -func TestConfigClientTLSRootCAFileMultipleCertsAndGarbage(t *testing.T) { +// If the root CA file provided has multiple CA certs, only the valid certs +// are read. +func TestConfigClientTLSRootCAFileMultipleCerts(t *testing.T) { tempFilename := generateMultiCert(t) defer os.RemoveAll(tempFilename)