diff --git a/connect/service.go b/connect/service.go index 7bdaf3c60f..74e6a52b5e 100644 --- a/connect/service.go +++ b/connect/service.go @@ -72,7 +72,7 @@ func NewServiceWithLogger(serviceName string, client *api.Client, service: serviceName, client: client, logger: logger, - tlsCfg: newDynamicTLSConfig(defaultTLSConfig()), + tlsCfg: newDynamicTLSConfig(defaultTLSConfig(), logger), httpResolverFromAddr: ConsulResolverFromAddrFunc(client), } @@ -121,7 +121,7 @@ func NewDevServiceWithTLSConfig(serviceName string, logger *log.Logger, s := &Service{ service: serviceName, logger: logger, - tlsCfg: newDynamicTLSConfig(tlsCfg), + tlsCfg: newDynamicTLSConfig(tlsCfg, logger), } return s, nil } diff --git a/connect/tls.go b/connect/tls.go index 7d679e3838..7c513dc611 100644 --- a/connect/tls.go +++ b/connect/tls.go @@ -15,6 +15,30 @@ import ( "github.com/hashicorp/consul/api" ) +// parseLeafX509Cert will parse an X509 certificate +// from the TLS certificate and store the parsed +// value in the TLS certificate as the Leaf field. +func parseLeafX509Cert(leaf *tls.Certificate) error { + if leaf == nil { + // nothing to parse for nil cert + return nil + } + + if leaf.Leaf != nil { + // leaf cert was already parsed + return nil + } + + cert, err := x509.ParseCertificate(leaf.Certificate[0]) + + if err != nil { + return err + } + + leaf.Leaf = cert + return nil +} + // verifierFunc is a function that can accept rawCertificate bytes from a peer // and verify them against a given tls.Config. It's called from the // tls.Config.VerifyPeerCertificate hook. @@ -267,12 +291,17 @@ type tlsCfgUpdate struct { // newDynamicTLSConfig returns a dynamicTLSConfig constructed from base. // base.Certificates[0] is used as the initial leaf and base.RootCAs is used as // the initial roots. -func newDynamicTLSConfig(base *tls.Config) *dynamicTLSConfig { +func newDynamicTLSConfig(base *tls.Config, logger *log.Logger) *dynamicTLSConfig { cfg := &dynamicTLSConfig{ base: base, } if len(base.Certificates) > 0 { cfg.leaf = &base.Certificates[0] + // If this does error then future calls to Ready will fail + // It is better to handle not-Ready rather than failing + if err := parseLeafX509Cert(cfg.leaf); err != nil && logger != nil { + logger.Printf("[ERR] Error parsing configured leaf certificate: %v", err) + } } if base.RootCAs != nil { cfg.roots = base.RootCAs @@ -334,19 +363,39 @@ func (cfg *dynamicTLSConfig) SetRoots(roots *x509.CertPool) error { func (cfg *dynamicTLSConfig) SetLeaf(leaf *tls.Certificate) error { cfg.Lock() defer cfg.Unlock() + if err := parseLeafX509Cert(leaf); err != nil { + return err + } cfg.leaf = leaf + cfg.notify() return nil } // notify is called under lock during an update to check if we are now ready. func (cfg *dynamicTLSConfig) notify() { - if cfg.readyCh != nil && cfg.leaf != nil && cfg.roots != nil { + if cfg.readyCh != nil && cfg.leaf != nil && cfg.roots != nil && cfg.leaf.Leaf != nil { close(cfg.readyCh) cfg.readyCh = nil } } +func (cfg *dynamicTLSConfig) VerifyLeafWithRoots() error { + cfg.RLock() + defer cfg.RUnlock() + + if cfg.roots == nil { + return fmt.Errorf("No roots are set") + } else if cfg.leaf == nil { + return fmt.Errorf("No leaf certificate is set") + } else if cfg.leaf.Leaf == nil { + return fmt.Errorf("Leaf certificate has not been parsed") + } + + _, err := cfg.leaf.Leaf.Verify(x509.VerifyOptions{Roots: cfg.roots}) + return err +} + // Roots returns the current CA root CertPool. func (cfg *dynamicTLSConfig) Roots() *x509.CertPool { cfg.RLock() @@ -364,19 +413,19 @@ func (cfg *dynamicTLSConfig) Leaf() *tls.Certificate { // Ready returns whether or not both roots and a leaf certificate are // configured. If both are non-nil, they are assumed to be valid and usable. func (cfg *dynamicTLSConfig) Ready() bool { - cfg.RLock() - defer cfg.RUnlock() - return cfg.leaf != nil && cfg.roots != nil + // not locking because VerifyLeafWithRoots will do that + return cfg.VerifyLeafWithRoots() == nil } // ReadyWait returns a chan that is closed when the the Service becomes ready // for use for the first time. Note that if the Service is ready when it is // called it returns a nil chan. Ready means that it has root and leaf -// certificates configured which we assume are valid. The service may -// subsequently stop being "ready" if it's certificates expire or are revoked -// and an error prevents new ones being loaded but this method will not stop -// returning a nil chan in that case. It is only useful for initial startup. For -// ongoing health Ready() should be used. +// certificates configured but not that the combination is valid nor that +// the current time is within the validity window of the certificate. The +// service may subsequently stop being "ready" if it's certificates expire +// or are revoked and an error prevents new ones from being loaded but this +// method will not stop returning a nil chan in that case. It is only useful +// for initial startup. For ongoing health Ready() should be used. func (cfg *dynamicTLSConfig) ReadyWait() <-chan struct{} { return cfg.readyCh } diff --git a/connect/tls_test.go b/connect/tls_test.go index 31dc7548f9..c637b2f43d 100644 --- a/connect/tls_test.go +++ b/connect/tls_test.go @@ -302,7 +302,7 @@ func TestDynamicTLSConfig(t *testing.T) { baseCfg := TestTLSConfig(t, "web", ca1) newCfg := TestTLSConfig(t, "web", ca2) - c := newDynamicTLSConfig(baseCfg) + c := newDynamicTLSConfig(baseCfg, nil) // Should set them from the base config require.Equal(c.Leaf(), &baseCfg.Certificates[0]) @@ -368,7 +368,7 @@ func TestDynamicTLSConfig_Ready(t *testing.T) { ca1 := connect.TestCA(t, nil) baseCfg := TestTLSConfig(t, "web", ca1) - c := newDynamicTLSConfig(defaultTLSConfig()) + c := newDynamicTLSConfig(defaultTLSConfig(), nil) readyCh := c.ReadyWait() assertBlocked(t, readyCh) require.False(c.Ready(), "no roots or leaf, should not be ready") @@ -382,6 +382,17 @@ func TestDynamicTLSConfig_Ready(t *testing.T) { require.NoError(err) assertNotBlocked(t, readyCh) require.True(c.Ready(), "should be ready") + + ca2 := connect.TestCA(t, nil) + ca2cfg := TestTLSConfig(t, "web", ca2) + + require.NoError(c.SetRoots(ca2cfg.RootCAs)) + assertNotBlocked(t, readyCh) + require.False(c.Ready(), "invalid leaf, should not be ready") + + require.NoError(c.SetRoots(baseCfg.RootCAs)) + assertNotBlocked(t, readyCh) + require.True(c.Ready(), "should be ready") } func assertBlocked(t *testing.T, ch <-chan struct{}) {