Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

return auto_encrypt cert for listeners #6489

Merged
merged 8 commits into from Nov 12, 2019
Merged

return auto_encrypt cert for listeners #6489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
531fda6
return auto_encrypt cert for listeners
hanshasselberg Sep 16, 2019
4888a4a
fix cert check
hanshasselberg Sep 17, 2019
53778c4
fix lock
hanshasselberg Sep 17, 2019
d1be52e
add tests
hanshasselberg Sep 17, 2019
b867448
improve tests
hanshasselberg Sep 23, 2019
13be5f2
remove strange condition
hanshasselberg Sep 23, 2019
d28773e
test: add comments describing expected behavior for auto-encrypt and …
mikemorris Nov 12, 2019
9f18bc4
test: expect nil *tls.Certificate
mikemorris Nov 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
21 changes: 13 additions & 8 deletions tlsutil/config.go
View file
Open in desktop
Expand Up @@ -326,7 +326,7 @@ func (c *Configurator) check(config Config, pool *x509.CertPool, cert *tls.Certi
if pool == nil {
return fmt.Errorf("VerifyIncoming set, and no CA certificate provided!")
}
if cert == nil || cert.Certificate == nil {
if cert == nil {
return fmt.Errorf("VerifyIncoming set, and no Cert/Key pair provided!")
}
}
Expand All @@ -351,7 +351,7 @@ func (c *Config) baseVerifyIncoming() bool {

func loadKeyPair(certFile, keyFile string) (*tls.Certificate, error) {
if certFile == "" || keyFile == "" {
return &tls.Certificate{}, nil
return nil, nil
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning an empty cert here led to all kinds of strange things. This is gone now for good.

}
cert, err := tls.LoadX509KeyPair(certFile, keyFile)
if err != nil {
Expand Down Expand Up @@ -428,11 +428,16 @@ func (c *Configurator) commonTLSConfig(verifyIncoming bool) *tls.Config {
tlsConfig.PreferServerCipherSuites = c.base.PreferServerCipherSuites

// GetCertificate is used when acting as a server and responding to
// client requests. Always return the manually configured cert, because
// on the server this is all we have. And on the client, this is the
// only sensitive option.
// client requests. Default to the manually configured cert, but allow
// autoEncrypt cert too so that a client can encrypt incoming
// connections without having a manual cert configured.
tlsConfig.GetCertificate = func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
return c.manual.cert, nil
cert := c.manual.cert
if cert == nil {
cert = c.autoEncrypt.cert
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can c.autoEncrypt ever be nil? If it is this will panic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewConfigurator initializes it:

consul/tlsutil/config.go

Lines 169 to 176 in ef52147

func NewConfigurator(config Config, logger *log.Logger) (*Configurator, error) {
c := &Configurator{logger: logger, manual: &manual{}, autoEncrypt: &autoEncrypt{}}
err := c.Update(config)
if err != nil {
return nil, err
}
return c, nil
}
.

}

return cert, nil
}

// GetClientCertificate is used when acting as a client and responding
Expand Down Expand Up @@ -630,9 +635,9 @@ func (c *Configurator) OutgoingRPCWrapper() DCWrapper {
// there is no cert, it will return a time in the past.
func (c *Configurator) AutoEncryptCertNotAfter() time.Time {
c.RLock()
defer c.RUnlock()
tlsCert := c.autoEncrypt.cert
c.RUnlock()
if tlsCert == nil {
if tlsCert == nil || tlsCert.Certificate == nil {
return time.Now().AddDate(0, 0, -1)
}
cert, err := x509.ParseCertificate(tlsCert.Certificate[0])
Expand Down
59 changes: 42 additions & 17 deletions tlsutil/config_test.go
View file
Open in desktop
Expand Up @@ -293,17 +293,16 @@ func TestConfigurator_loadKeyPair(t *testing.T) {
cert, key string
shoulderr bool
isnil bool
isempty bool
}
variants := []variant{
{"", "", false, false, true},
{"bogus", "", false, false, true},
{"", "bogus", false, false, true},
{"../test/key/ourdomain.cer", "", false, false, true},
{"", "../test/key/ourdomain.key", false, false, true},
{"bogus", "bogus", true, true, false},
{"", "", false, true},
{"bogus", "", false, true},
{"", "bogus", false, true},
{"../test/key/ourdomain.cer", "", false, true},
{"", "../test/key/ourdomain.key", false, true},
{"bogus", "bogus", true, true},
{"../test/key/ourdomain.cer", "../test/key/ourdomain.key",
false, false, false},
false, false},
}
for i, v := range variants {
info := fmt.Sprintf("case %d", i)
Expand All @@ -317,10 +316,6 @@ func TestConfigurator_loadKeyPair(t *testing.T) {
require.NoError(t, err1, info)
require.NoError(t, err2, info)
}
if v.isempty {
require.Empty(t, cert1.Certificate, info)
require.Empty(t, cert2.Certificate, info)
}
if v.isnil {
require.Nil(t, cert1, info)
require.Nil(t, cert2, info)
Expand Down Expand Up @@ -538,17 +533,47 @@ func TestConfigurator_CommonTLSConfigGetClientCertificate(t *testing.T) {
c, err := NewConfigurator(Config{}, nil)
require.NoError(t, err)

cert, err := c.commonTLSConfig(false).GetCertificate(nil)
cert, err := c.commonTLSConfig(false).GetClientCertificate(nil)
require.NoError(t, err)
require.Nil(t, cert.Certificate)
require.Nil(t, cert)

c.manual.cert = &tls.Certificate{}
cert, err = c.commonTLSConfig(false).GetCertificate(nil)
c1, err := loadKeyPair("../test/key/something_expired.cer", "../test/key/something_expired.key")
require.NoError(t, err)
c.manual.cert = c1
cert, err = c.commonTLSConfig(false).GetClientCertificate(nil)
require.NoError(t, err)
require.Equal(t, c.manual.cert, cert)

c2, err := loadKeyPair("../test/key/ourdomain.cer", "../test/key/ourdomain.key")
require.NoError(t, err)
c.autoEncrypt.cert = c2
cert, err = c.commonTLSConfig(false).GetClientCertificate(nil)
require.NoError(t, err)
require.Equal(t, c.autoEncrypt.cert, cert)
}

func TestConfigurator_CommonTLSConfigGetCertificate(t *testing.T) {
c, err := NewConfigurator(Config{}, nil)
require.NoError(t, err)

cert, err := c.commonTLSConfig(false).GetCertificate(nil)
require.NoError(t, err)
require.Nil(t, cert)

// Setting a certificate as the auto-encrypt cert will return it as the regular server certificate
c1, err := loadKeyPair("../test/key/something_expired.cer", "../test/key/something_expired.key")
require.NoError(t, err)
c.autoEncrypt.cert = c1
cert, err = c.commonTLSConfig(false).GetCertificate(nil)
require.NoError(t, err)
require.Equal(t, c.autoEncrypt.cert, cert)

// Setting a different certificate as a manual cert will override the auto-encrypt cert and instead return the manual cert
c2, err := loadKeyPair("../test/key/ourdomain.cer", "../test/key/ourdomain.key")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this test looks good now but it might not be obvious to future us exactly what the signficance f this behaviour is.

Maybe add comments that adding one cert as the auto_encrypt one gets returned as the regular server cert above and then that loading a different one as well as the "manual" cert means we still server the manual one not the auto-encrypt one.

require.NoError(t, err)
c.manual.cert = c2
cert, err = c.commonTLSConfig(false).GetCertificate(nil)
require.NoError(t, err)
require.Equal(t, c.manual.cert, cert)
}

Expand Down Expand Up @@ -715,7 +740,7 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) {
c, err := NewConfigurator(Config{}, nil)
require.NoError(t, err)
require.Nil(t, c.caPool)
require.Nil(t, c.manual.cert.Certificate)
require.Nil(t, c.manual.cert)
require.Equal(t, c.base, &Config{})
require.Equal(t, 1, c.version)

Expand Down