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.

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

Reload tls config #5419

Merged
merged 31 commits into from
Mar 13, 2019
Merged
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
d881d63
copy config
hanshasselberg Feb 27, 2019
8bcab53
wip
hanshasselberg Feb 28, 2019
76249dc
GetConfigForClient
hanshasselberg Feb 28, 2019
12076fa
better test
hanshasselberg Feb 28, 2019
f76c13e
guard config reading
hanshasselberg Feb 28, 2019
a064f14
getclientconfigfor
hanshasselberg Feb 28, 2019
c996dd6
remove comment
hanshasselberg Mar 4, 2019
6017229
fix tests
hanshasselberg Mar 4, 2019
066acf6
check config
hanshasselberg Mar 4, 2019
61b6c50
add test for failing tlsconfig reload
hanshasselberg Mar 4, 2019
cd35331
extra test for test
hanshasselberg Mar 4, 2019
dd93d73
add logger so that we can debug log
hanshasselberg Mar 5, 2019
b111993
that refactor....
hanshasselberg Mar 5, 2019
10aef9c
woot
hanshasselberg Mar 6, 2019
fccacc7
fix small things
hanshasselberg Mar 6, 2019
a8c3f05
improvements
hanshasselberg Mar 6, 2019
cb3c407
small things
hanshasselberg Mar 6, 2019
ae321db
correct locking in wrap
hanshasselberg Mar 6, 2019
b789253
small things
hanshasselberg Mar 6, 2019
93f9adc
fix that test :tada:
hanshasselberg Mar 6, 2019
b2f7f19
remove that name
hanshasselberg Mar 6, 2019
16c1c80
merge conflict.
hanshasselberg Mar 6, 2019
792889a
another bad merge
hanshasselberg Mar 6, 2019
967fb90
improve locking
hanshasselberg Mar 6, 2019
b25c9df
more tests
hanshasselberg Mar 8, 2019
cab155a
more tests
hanshasselberg Mar 8, 2019
3d7fc9e
add note about tls reloading to the docs.
hanshasselberg Mar 8, 2019
7f78eb8
add comments to explain these functions are only used for testing.
hanshasselberg Mar 11, 2019
2efe7d2
actually check the cas and not only for values in the configuration
hanshasselberg Mar 11, 2019
282bd93
set next protos, but do not accept everything from the client
hanshasselberg Mar 12, 2019
421cacf
fix test :)
hanshasselberg Mar 12, 2019
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 12 additions & 17 deletions agent/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,11 @@ func (a *Agent) Start() error {
// waiting to discover a consul server
consulCfg.ServerUp = a.sync.SyncFull.Trigger

a.tlsConfigurator = tlsutil.NewConfigurator(c.ToTLSUtilConfig())
tlsConfigurator, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), a.logger)
if err != nil {
return err
}
a.tlsConfigurator = tlsConfigurator

// Setup either the client or the server.
if c.ServerMode {
Expand Down Expand Up @@ -662,10 +666,7 @@ func (a *Agent) listenHTTP() ([]*HTTPServer, error) {
var tlscfg *tls.Config
_, isTCP := l.(*tcpKeepAliveListener)
if isTCP && proto == "https" {
tlscfg, err = a.tlsConfigurator.IncomingHTTPSConfig()
if err != nil {
return err
}
tlscfg = a.tlsConfigurator.IncomingHTTPSConfig()
l = tls.NewListener(l, tlscfg)
}
srv := &HTTPServer{
Expand Down Expand Up @@ -2232,11 +2233,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType,
chkType.Interval = checks.MinInterval
}

a.tlsConfigurator.AddCheck(string(check.CheckID), chkType.TLSSkipVerify)
tlsClientConfig, err := a.tlsConfigurator.OutgoingTLSConfigForCheck(string(check.CheckID))
if err != nil {
return fmt.Errorf("Failed to set up TLS: %v", err)
}
tlsClientConfig := a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify)

http := &checks.CheckHTTP{
Notify: a.State,
Expand Down Expand Up @@ -2287,12 +2284,7 @@ func (a *Agent) addCheck(check *structs.HealthCheck, chkType *structs.CheckType,

var tlsClientConfig *tls.Config
if chkType.GRPCUseTLS {
var err error
a.tlsConfigurator.AddCheck(string(check.CheckID), chkType.TLSSkipVerify)
tlsClientConfig, err = a.tlsConfigurator.OutgoingTLSConfigForCheck(string(check.CheckID))
if err != nil {
return fmt.Errorf("Failed to set up TLS: %v", err)
}
tlsClientConfig = a.tlsConfigurator.OutgoingTLSConfigForCheck(chkType.TLSSkipVerify)
}

grpc := &checks.CheckGRPC{
Expand Down Expand Up @@ -2431,7 +2423,6 @@ func (a *Agent) removeCheckLocked(checkID types.CheckID, persist bool) error {
return fmt.Errorf("CheckID missing")
}

a.tlsConfigurator.RemoveCheck(string(checkID))
a.cancelCheckMonitors(checkID)
a.State.RemoveCheck(checkID)

Expand Down Expand Up @@ -3559,6 +3550,10 @@ func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error {
// the checks and service registrations.
a.loadTokens(newCfg)

if err := a.tlsConfigurator.Update(newCfg.ToTLSUtilConfig()); err != nil {
return fmt.Errorf("Failed reloading tls configuration: %s", err)
}
Copy link
Member Author

@hanshasselberg hanshasselberg Mar 6, 2019

Choose a reason for hiding this comment

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

This is one of the cornerstones of this PR, the config is being updated here! And every tls.Config created afterwards will have the updates.


// Reload service/check definitions and metadata.
if err := a.loadServices(newCfg); err != nil {
return fmt.Errorf("Failed reloading services: %s", err)
Expand Down
117 changes: 107 additions & 10 deletions agent/agent_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package agent

import (
"bytes"
"crypto/tls"
"encoding/json"
"fmt"
"io/ioutil"
Expand Down Expand Up @@ -3366,23 +3367,15 @@ func TestAgent_SetupProxyManager(t *testing.T) {
ports { http = -1 }
data_dir = "` + dataDir + `"
`
c := TestConfig(
// randomPortsSource(false),
config.Source{Name: t.Name(), Format: "hcl", Data: hcl},
)
a, err := New(c)
a, err := NewUnstartedAgent(t, t.Name(), hcl)
require.NoError(t, err)
require.Error(t, a.setupProxyManager(), "setupProxyManager should fail with invalid HTTP API config")

hcl = `
ports { http = 8001 }
data_dir = "` + dataDir + `"
`
c = TestConfig(
// randomPortsSource(false),
config.Source{Name: t.Name(), Format: "hcl", Data: hcl},
)
a, err = New(c)
a, err = NewUnstartedAgent(t, t.Name(), hcl)
require.NoError(t, err)
require.NoError(t, a.setupProxyManager())
}
Expand Down Expand Up @@ -3543,3 +3536,107 @@ func TestAgent_loadTokens(t *testing.T) {
require.Equal("foxtrot", a.tokens.ReplicationToken())
})
}

func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) {
t.Parallel()
dataDir := testutil.TempDir(t, "agent") // we manage the data dir
defer os.RemoveAll(dataDir)
hcl := `
data_dir = "` + dataDir + `"
verify_outgoing = true
ca_file = "../test/ca/root.cer"
cert_file = "../test/key/ourdomain.cer"
key_file = "../test/key/ourdomain.key"
verify_server_hostname = false
`
a, err := NewUnstartedAgent(t, t.Name(), hcl)
require.NoError(t, err)
tlsConf := a.tlsConfigurator.OutgoingRPCConfig()
require.True(t, tlsConf.InsecureSkipVerify)
require.Len(t, tlsConf.ClientCAs.Subjects(), 1)
require.Len(t, tlsConf.RootCAs.Subjects(), 1)

hcl = `
data_dir = "` + dataDir + `"
verify_outgoing = true
ca_path = "../test/ca_path"
cert_file = "../test/key/ourdomain.cer"
key_file = "../test/key/ourdomain.key"
verify_server_hostname = true
`
c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl})
require.NoError(t, a.ReloadConfig(c))
tlsConf = a.tlsConfigurator.OutgoingRPCConfig()
require.False(t, tlsConf.InsecureSkipVerify)
require.Len(t, tlsConf.RootCAs.Subjects(), 2)
require.Len(t, tlsConf.ClientCAs.Subjects(), 2)
}

func TestAgent_ReloadConfigIncomingRPCConfig(t *testing.T) {
t.Parallel()
dataDir := testutil.TempDir(t, "agent") // we manage the data dir
defer os.RemoveAll(dataDir)
hcl := `
data_dir = "` + dataDir + `"
verify_outgoing = true
ca_file = "../test/ca/root.cer"
cert_file = "../test/key/ourdomain.cer"
key_file = "../test/key/ourdomain.key"
verify_server_hostname = false
`
a, err := NewUnstartedAgent(t, t.Name(), hcl)
require.NoError(t, err)
tlsConf := a.tlsConfigurator.IncomingRPCConfig()
require.NotNil(t, tlsConf.GetConfigForClient)
tlsConf, err = tlsConf.GetConfigForClient(nil)
require.NoError(t, err)
require.NotNil(t, tlsConf)
require.True(t, tlsConf.InsecureSkipVerify)
require.Len(t, tlsConf.ClientCAs.Subjects(), 1)
require.Len(t, tlsConf.RootCAs.Subjects(), 1)

hcl = `
data_dir = "` + dataDir + `"
verify_outgoing = true
ca_path = "../test/ca_path"
cert_file = "../test/key/ourdomain.cer"
key_file = "../test/key/ourdomain.key"
verify_server_hostname = true
`
c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl})
require.NoError(t, a.ReloadConfig(c))
tlsConf, err = tlsConf.GetConfigForClient(nil)
require.NoError(t, err)
require.False(t, tlsConf.InsecureSkipVerify)
require.Len(t, tlsConf.ClientCAs.Subjects(), 2)
require.Len(t, tlsConf.RootCAs.Subjects(), 2)
}

func TestAgent_ReloadConfigTLSConfigFailure(t *testing.T) {
t.Parallel()
dataDir := testutil.TempDir(t, "agent") // we manage the data dir
defer os.RemoveAll(dataDir)
hcl := `
data_dir = "` + dataDir + `"
verify_outgoing = true
ca_file = "../test/ca/root.cer"
cert_file = "../test/key/ourdomain.cer"
key_file = "../test/key/ourdomain.key"
verify_server_hostname = false
`
a, err := NewUnstartedAgent(t, t.Name(), hcl)
require.NoError(t, err)
tlsConf := a.tlsConfigurator.IncomingRPCConfig()

hcl = `
data_dir = "` + dataDir + `"
verify_incoming = true
`
c := TestConfig(config.Source{Name: t.Name(), Format: "hcl", Data: hcl})
require.Error(t, a.ReloadConfig(c))
tlsConf, err = tlsConf.GetConfigForClient(nil)
require.NoError(t, err)
require.Equal(t, tls.NoClientCert, tlsConf.ClientAuth)
require.Len(t, tlsConf.ClientCAs.Subjects(), 1)
require.Len(t, tlsConf.RootCAs.Subjects(), 1)
}
5 changes: 3 additions & 2 deletions agent/config/runtime.go
Original file line number Diff line number Diff line change
Expand Up @@ -1580,12 +1580,13 @@ func (c *RuntimeConfig) Sanitized() map[string]interface{} {
return sanitize("rt", reflect.ValueOf(c)).Interface().(map[string]interface{})
}

func (c *RuntimeConfig) ToTLSUtilConfig() *tlsutil.Config {
return &tlsutil.Config{
func (c *RuntimeConfig) ToTLSUtilConfig() tlsutil.Config {
return tlsutil.Config{
VerifyIncoming: c.VerifyIncoming,
VerifyIncomingRPC: c.VerifyIncomingRPC,
VerifyIncomingHTTPS: c.VerifyIncomingHTTPS,
VerifyOutgoing: c.VerifyOutgoing,
VerifyServerHostname: c.VerifyServerHostname,
Copy link
Member Author

Choose a reason for hiding this comment

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

Ups, how could I forget that?!

CAFile: c.CAFile,
CAPath: c.CAPath,
CertFile: c.CertFile,
Expand Down
2 changes: 2 additions & 0 deletions agent/config/runtime_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5434,6 +5434,7 @@ func TestRuntime_ToTLSUtilConfig(t *testing.T) {
VerifyIncomingRPC: true,
VerifyIncomingHTTPS: true,
VerifyOutgoing: true,
VerifyServerHostname: true,
CAFile: "a",
CAPath: "b",
CertFile: "c",
Expand All @@ -5450,6 +5451,7 @@ func TestRuntime_ToTLSUtilConfig(t *testing.T) {
require.Equal(t, c.VerifyIncomingRPC, r.VerifyIncomingRPC)
require.Equal(t, c.VerifyIncomingHTTPS, r.VerifyIncomingHTTPS)
require.Equal(t, c.VerifyOutgoing, r.VerifyOutgoing)
require.Equal(t, c.VerifyServerHostname, r.VerifyServerHostname)
require.Equal(t, c.CAFile, r.CAFile)
require.Equal(t, c.CAPath, r.CAPath)
require.Equal(t, c.CertFile, r.CertFile)
Expand Down
21 changes: 11 additions & 10 deletions agent/consul/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,16 @@ type Client struct {
EnterpriseClient
}

// NewClient is used to construct a new Consul client from the
// configuration, potentially returning an error
// NewClient is used to construct a new Consul client from the configuration,
// potentially returning an error.
// NewClient only used to help setting up a client for testing. Normal code
// exercises NewClientLogger.
func NewClient(config *Config) (*Client, error) {
return NewClientLogger(config, nil, tlsutil.NewConfigurator(config.ToTLSUtilConfig()))
c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)
Copy link
Member

Choose a reason for hiding this comment

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

Might be useful to note here that a normal Consul Agent doesn't use this method and instead will pass in its own TLS configurator.

At first I was thinking that the agent and server/client would have different configurators (and thus reloading would not work) but realized that you have it passing them to NewClientLogger and NewServerLogger. Adding a comment here to mention whats going on would probably be good.

Copy link
Member Author

Choose a reason for hiding this comment

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

✔️

if err != nil {
return nil, err
}
return NewClientLogger(config, nil, c)
}

func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsutil.Configurator) (*Client, error) {
Expand All @@ -113,12 +119,6 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti
config.LogOutput = os.Stderr
}

// Create the tls Wrapper
tlsWrap, err := tlsConfigurator.OutgoingRPCWrapper()
if err != nil {
return nil, err
}

// Create a logger
if logger == nil {
logger = log.New(config.LogOutput, "", log.LstdFlags)
Expand All @@ -129,7 +129,7 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti
LogOutput: config.LogOutput,
MaxTime: clientRPCConnMaxIdle,
MaxStreams: clientMaxStreams,
TLSWrapper: tlsWrap,
TLSWrapper: tlsConfigurator.OutgoingRPCWrapper(),
ForceTLS: config.VerifyOutgoing,
}

Expand Down Expand Up @@ -158,6 +158,7 @@ func NewClientLogger(config *Config, logger *log.Logger, tlsConfigurator *tlsuti
CacheConfig: clientACLCacheConfig,
Sentinel: nil,
}
var err error
if c.acls, err = NewACLResolver(&aclConfig); err != nil {
c.Shutdown()
return nil, fmt.Errorf("Failed to create ACL resolver: %v", err)
Expand Down
4 changes: 2 additions & 2 deletions agent/consul/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -379,8 +379,8 @@ type Config struct {
CAConfig *structs.CAConfiguration
}

func (c *Config) ToTLSUtilConfig() *tlsutil.Config {
return &tlsutil.Config{
func (c *Config) ToTLSUtilConfig() tlsutil.Config {
return tlsutil.Config{
VerifyIncoming: c.VerifyIncoming,
VerifyOutgoing: c.VerifyOutgoing,
CAFile: c.CAFile,
Expand Down
28 changes: 11 additions & 17 deletions agent/consul/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -252,11 +252,17 @@ type Server struct {
EnterpriseServer
}

// NewServer is only used to help setting up a server for testing. Normal code
// exercises NewServerLogger.
func NewServer(config *Config) (*Server, error) {
return NewServerLogger(config, nil, new(token.Store), tlsutil.NewConfigurator(config.ToTLSUtilConfig()))
c, err := tlsutil.NewConfigurator(config.ToTLSUtilConfig(), nil)
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here. A comment about this configurator not being used for a normal Consul agent would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

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

✔️

if err != nil {
return nil, err
}
return NewServerLogger(config, nil, new(token.Store), c)
}

// NewServer is used to construct a new Consul server from the
// NewServerLogger is used to construct a new Consul server from the
// configuration, potentially returning an error
func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tlsConfigurator *tlsutil.Configurator) (*Server, error) {
// Check the protocol version.
Expand Down Expand Up @@ -296,18 +302,6 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl
}
}

// Create the TLS wrapper for outgoing connections.
tlsWrap, err := tlsConfigurator.OutgoingRPCWrapper()
if err != nil {
return nil, err
}

// Get the incoming TLS config.
incomingTLS, err := tlsConfigurator.IncomingRPCConfig()
if err != nil {
return nil, err
}

// Create the tombstone GC.
gc, err := state.NewTombstoneGC(config.TombstoneTTL, config.TombstoneTTLGranularity)
if err != nil {
Expand All @@ -322,7 +316,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl
LogOutput: config.LogOutput,
MaxTime: serverRPCCache,
MaxStreams: serverMaxStreams,
TLSWrapper: tlsWrap,
TLSWrapper: tlsConfigurator.OutgoingRPCWrapper(),
ForceTLS: config.VerifyOutgoing,
}

Expand All @@ -338,7 +332,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl
reconcileCh: make(chan serf.Member, reconcileChSize),
router: router.NewRouter(logger, config.Datacenter),
rpcServer: rpc.NewServer(),
rpcTLS: incomingTLS,
rpcTLS: tlsConfigurator.IncomingRPCConfig(),
reassertLeaderCh: make(chan chan error),
segmentLAN: make(map[string]*serf.Serf, len(config.Segments)),
sessionTimers: NewSessionTimers(),
Expand Down Expand Up @@ -373,7 +367,7 @@ func NewServerLogger(config *Config, logger *log.Logger, tokens *token.Store, tl
}

// Initialize the RPC layer.
if err := s.setupRPC(tlsWrap); err != nil {
if err := s.setupRPC(tlsConfigurator.OutgoingRPCWrapper()); err != nil {
s.Shutdown()
return nil, fmt.Errorf("Failed to start RPC layer: %v", err)
}
Expand Down
6 changes: 5 additions & 1 deletion agent/consul/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,11 @@ func newServer(c *Config) (*Server, error) {
w = os.Stderr
}
logger := log.New(w, c.NodeName+" - ", log.LstdFlags|log.Lmicroseconds)
srv, err := NewServerLogger(c, logger, new(token.Store), tlsutil.NewConfigurator(c.ToTLSUtilConfig()))
tlsConf, err := tlsutil.NewConfigurator(c.ToTLSUtilConfig(), logger)
if err != nil {
return nil, err
}
srv, err := NewServerLogger(c, logger, new(token.Store), tlsConf)
if err != nil {
return nil, err
}
Expand Down
Loading