From 27b36bfc4e12f4ff3c843a12e9b7eb22237fa89e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 17 Aug 2020 17:24:49 -0400 Subject: [PATCH 1/2] config: move NodeName validation to config validation Previsouly it was done in Agent.Start, which is much later then it needs to be. The new 'dns' package was required, because otherwise there would be an import cycle. In the future we should move more of the dns server into the dns package. --- agent/agent.go | 24 +++----------- agent/config/builder.go | 14 ++++++++- agent/config/builder_test.go | 61 ++++++++++++++++++++++++++++++++++++ agent/config/runtime_test.go | 29 +++++------------ agent/dns.go | 10 ++---- agent/dns/dns.go | 10 ++++++ agent/dns_test.go | 3 +- 7 files changed, 102 insertions(+), 49 deletions(-) create mode 100644 agent/dns/dns.go diff --git a/agent/agent.go b/agent/agent.go index 8dfd6450ca..7b483e9b57 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -17,6 +17,7 @@ import ( "sync" "time" + "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/go-connlimit" "github.com/hashicorp/go-hclog" "github.com/hashicorp/go-memdb" @@ -642,21 +643,6 @@ func (a *Agent) Start(ctx context.Context) error { return err } - // Warn if the node name is incompatible with DNS - if InvalidDnsRe.MatchString(a.config.NodeName) { - a.logger.Warn("Node name will not be discoverable "+ - "via DNS due to invalid characters. Valid characters include "+ - "all alpha-numerics and dashes.", - "node_name", a.config.NodeName, - ) - } else if len(a.config.NodeName) > MaxDNSLabelLength { - a.logger.Warn("Node name will not be discoverable "+ - "via DNS due to it being too long. Valid lengths are between "+ - "1 and 63 bytes.", - "node_name", a.config.NodeName, - ) - } - // load the tokens - this requires the logger to be setup // which is why we can't do this in New a.loadTokens(a.config) @@ -2484,13 +2470,13 @@ func (a *Agent) validateService(service *structs.NodeService, chkTypes []*struct } // Warn if the service name is incompatible with DNS - if InvalidDnsRe.MatchString(service.Service) { + if dns.InvalidNameRe.MatchString(service.Service) { a.logger.Warn("Service name will not be discoverable "+ "via DNS due to invalid characters. Valid characters include "+ "all alpha-numerics and dashes.", "service", service.Service, ) - } else if len(service.Service) > MaxDNSLabelLength { + } else if len(service.Service) > dns.MaxLabelLength { a.logger.Warn("Service name will not be discoverable "+ "via DNS due to it being too long. Valid lengths are between "+ "1 and 63 bytes.", @@ -2500,13 +2486,13 @@ func (a *Agent) validateService(service *structs.NodeService, chkTypes []*struct // Warn if any tags are incompatible with DNS for _, tag := range service.Tags { - if InvalidDnsRe.MatchString(tag) { + if dns.InvalidNameRe.MatchString(tag) { a.logger.Debug("Service tag will not be discoverable "+ "via DNS due to invalid characters. Valid characters include "+ "all alpha-numerics and dashes.", "tag", tag, ) - } else if len(tag) > MaxDNSLabelLength { + } else if len(tag) > dns.MaxLabelLength { a.logger.Debug("Service tag will not be discoverable "+ "via DNS due to it being too long. Valid lengths are between "+ "1 and 63 bytes.", diff --git a/agent/config/builder.go b/agent/config/builder.go index 4b11a2e337..93e94b4ea3 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -19,6 +19,7 @@ import ( "github.com/hashicorp/consul/agent/connect/ca" "github.com/hashicorp/consul/agent/consul" "github.com/hashicorp/consul/agent/consul/authmethod/ssoauth" + "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/ipaddr" "github.com/hashicorp/consul/lib" @@ -1117,9 +1118,20 @@ func (b *Builder) Validate(rt RuntimeConfig) error { return fmt.Errorf("data_dir %q is not a directory", rt.DataDir) } } - if rt.NodeName == "" { + + switch { + case rt.NodeName == "": return fmt.Errorf("node_name cannot be empty") + case dns.InvalidNameRe.MatchString(rt.NodeName): + b.warn("Node name %q will not be discoverable "+ + "via DNS due to invalid characters. Valid characters include "+ + "all alpha-numerics and dashes.", rt.NodeName) + case len(rt.NodeName) > dns.MaxLabelLength: + b.warn("Node name %q will not be discoverable "+ + "via DNS due to it being too long. Valid lengths are between "+ + "1 and 63 bytes.", rt.NodeName) } + if ipaddr.IsAny(rt.AdvertiseAddrLAN.IP) { return fmt.Errorf("Advertise address cannot be 0.0.0.0, :: or [::]") } diff --git a/agent/config/builder_test.go b/agent/config/builder_test.go index 01e58b8385..9be12a4d34 100644 --- a/agent/config/builder_test.go +++ b/agent/config/builder_test.go @@ -3,8 +3,10 @@ package config import ( "fmt" "io/ioutil" + "net" "os" "path/filepath" + "strings" "testing" "time" @@ -121,3 +123,62 @@ func setupConfigFiles(t *testing.T) []string { subpath, } } + +func TestBuilder_BuildAndValidate_NodeName(t *testing.T) { + type testCase struct { + name string + nodeName string + expectedWarn string + } + + fn := func(t *testing.T, tc testCase) { + b, err := NewBuilder(BuilderOpts{ + Config: Config{ + NodeName: pString(tc.nodeName), + DataDir: pString("dir"), + }, + }) + patchBuilderShims(b) + require.NoError(t, err) + _, err = b.BuildAndValidate() + require.NoError(t, err) + require.Len(t, b.Warnings, 1) + require.Contains(t, b.Warnings[0], tc.expectedWarn) + } + + var testCases = []testCase{ + { + name: "invalid character - unicode", + nodeName: "🐼", + expectedWarn: `Node name "🐼" will not be discoverable via DNS due to invalid characters`, + }, + { + name: "invalid character - slash", + nodeName: "thing/other/ok", + expectedWarn: `Node name "thing/other/ok" will not be discoverable via DNS due to invalid characters`, + }, + { + name: "too long", + nodeName: strings.Repeat("a", 66), + expectedWarn: "due to it being too long.", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fn(t, tc) + }) + } +} + +func patchBuilderShims(b *Builder) { + b.hostname = func() (string, error) { + return "thehostname", nil + } + b.getPrivateIPv4 = func() ([]*net.IPAddr, error) { + return []*net.IPAddr{ipAddr("10.0.0.1")}, nil + } + b.getPublicIPv6 = func() ([]*net.IPAddr, error) { + return []*net.IPAddr{ipAddr("dead:beef::1")}, nil + } +} diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 1a7edb29ab..819dce8979 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -4280,27 +4280,16 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { t.Fatal("NewBuilder", err) } - // mock the hostname function unless a mock is provided - b.hostname = tt.hostname - if b.hostname == nil { - b.hostname = func() (string, error) { return "nodex", nil } + patchBuilderShims(b) + if tt.hostname != nil { + b.hostname = tt.hostname } - - // mock the ip address detection - privatev4 := tt.privatev4 - if privatev4 == nil { - privatev4 = func() ([]*net.IPAddr, error) { - return []*net.IPAddr{ipAddr("10.0.0.1")}, nil - } + if tt.privatev4 != nil { + b.getPrivateIPv4 = tt.privatev4 } - publicv6 := tt.publicv6 - if publicv6 == nil { - publicv6 = func() ([]*net.IPAddr, error) { - return []*net.IPAddr{ipAddr("dead:beef::1")}, nil - } + if tt.publicv6 != nil { + b.getPublicIPv6 = tt.publicv6 } - b.getPrivateIPv4 = privatev4 - b.getPublicIPv6 = publicv6 // read the source fragements for i, data := range srcs { @@ -4332,12 +4321,10 @@ func testConfig(t *testing.T, tests []configTest, dataDir string) { if err != nil && tt.err != "" && !strings.Contains(err.Error(), tt.err) { t.Fatalf("error %q does not contain %q", err.Error(), tt.err) } - require.Equal(t, tt.warns, b.Warnings, "warnings") - - // stop if we expected an error if tt.err != "" { return } + require.Equal(t, tt.warns, b.Warnings, "warnings") // build a default configuration, then patch the fields we expect to change // and compare it with the generated configuration. Since the expected diff --git a/agent/dns.go b/agent/dns.go index 187b0463c5..686ce8b20b 100644 --- a/agent/dns.go +++ b/agent/dns.go @@ -5,17 +5,17 @@ import ( "encoding/hex" "fmt" "net" + "regexp" "strings" "sync/atomic" "time" - "regexp" - metrics "github.com/armon/go-metrics" radix "github.com/armon/go-radix" "github.com/coredns/coredns/plugin/pkg/dnsutil" cachetype "github.com/hashicorp/consul/agent/cache-types" "github.com/hashicorp/consul/agent/config" + agentdns "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/ipaddr" @@ -38,12 +38,8 @@ const ( staleCounterThreshold = 5 * time.Second defaultMaxUDPSize = 512 - - MaxDNSLabelLength = 63 ) -var InvalidDnsRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - type dnsSOAConfig struct { Refresh uint32 // 3600 by default Retry uint32 // 600 @@ -539,7 +535,7 @@ func (d *DNSServer) nameservers(cfg *dnsConfig, maxRecursionLevel int) (ns []dns for _, o := range out.Nodes { name, dc := o.Node.Node, o.Node.Datacenter - if InvalidDnsRe.MatchString(name) { + if agentdns.InvalidNameRe.MatchString(name) { d.logger.Warn("Skipping invalid node for NS records", "node", name) continue } diff --git a/agent/dns/dns.go b/agent/dns/dns.go new file mode 100644 index 0000000000..8744eb351f --- /dev/null +++ b/agent/dns/dns.go @@ -0,0 +1,10 @@ +package dns + +import "regexp" + +// MaxLabelLength is the maximum length for a name that can be used in DNS. +const MaxLabelLength = 63 + +// InvalidNameRe is a regex that matches characters which can not be included in +// a DNS name. +var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) diff --git a/agent/dns_test.go b/agent/dns_test.go index 07c4211871..a82cc9c7a3 100644 --- a/agent/dns_test.go +++ b/agent/dns_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/hashicorp/consul/agent/config" + agentdns "github.com/hashicorp/consul/agent/dns" "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/lib" @@ -6976,7 +6977,7 @@ func TestDNSInvalidRegex(t *testing.T) { } for _, test := range tests { t.Run(test.desc, func(t *testing.T) { - if got, want := InvalidDnsRe.MatchString(test.in), test.invalid; got != want { + if got, want := agentdns.InvalidNameRe.MatchString(test.in), test.invalid; got != want { t.Fatalf("Expected %v to return %v", test.in, want) } }) From 35f1ecee0b689e156e82c9f7a75a95f06099a27e Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 17 Aug 2020 17:39:49 -0400 Subject: [PATCH 2/2] config: Move remote-script-checks warning to config Previously it was done in Agent.Start, but it can be done much earlier --- agent/agent.go | 25 ------------------------- agent/config/builder.go | 18 ++++++++++++++++++ agent/config/runtime_test.go | 1 + 3 files changed, 19 insertions(+), 25 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index 7b483e9b57..fc41059372 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -638,11 +638,6 @@ func (a *Agent) Start(ctx context.Context) error { return fmt.Errorf("Failed to load TLS configurations after applying auto-config settings: %w", err) } - if err := a.CheckSecurity(c); err != nil { - a.logger.Error("Security error while parsing configuration: %#v", err) - return err - } - // load the tokens - this requires the logger to be setup // which is why we can't do this in New a.loadTokens(a.config) @@ -3700,21 +3695,6 @@ func (a *Agent) getPersistedTokens() (*persistedTokens, error) { return persistedTokens, nil } -// CheckSecurity Performs security checks in Consul Configuration -// It might return an error if configuration is considered too dangerous -func (a *Agent) CheckSecurity(conf *config.RuntimeConfig) error { - if conf.EnableRemoteScriptChecks { - if !conf.ACLsEnabled { - if len(conf.AllowWriteHTTPFrom) == 0 { - err := fmt.Errorf("using enable-script-checks without ACLs and without allow_write_http_from is DANGEROUS, use enable-local-script-checks instead, see https://www.hashicorp.com/blog/protecting-consul-from-rce-risk-in-specific-configurations/") - a.logger.Error("[SECURITY] issue", "error", err) - // TODO: return the error in future Consul versions - } - } - } - return nil -} - func (a *Agent) loadTokens(conf *config.RuntimeConfig) error { persistedTokens, persistenceErr := a.getPersistedTokens() @@ -3912,11 +3892,6 @@ func (a *Agent) ReloadConfig() error { // the configuration using CLI flags and on disk config, this just takes a // runtime configuration and applies it. func (a *Agent) reloadConfigInternal(newCfg *config.RuntimeConfig) error { - if err := a.CheckSecurity(newCfg); err != nil { - a.logger.Error("Security error while reloading configuration: %#v", err) - return err - } - // Change the log level and update it if logging.ValidateLogLevel(newCfg.LogLevel) { a.logger.SetLevel(logging.LevelFromString(newCfg.LogLevel)) diff --git a/agent/config/builder.go b/agent/config/builder.go index 93e94b4ea3..216237e6f0 100644 --- a/agent/config/builder.go +++ b/agent/config/builder.go @@ -3,6 +3,7 @@ package config import ( "encoding/base64" "encoding/json" + "errors" "fmt" "io/ioutil" "net" @@ -1350,6 +1351,11 @@ func (b *Builder) Validate(rt RuntimeConfig) error { return err } + if err := validateRemoteScriptsChecks(rt); err != nil { + // TODO: make this an error in a future version + b.warn(err.Error()) + } + return nil } @@ -2191,3 +2197,15 @@ func UIPathBuilder(UIContentString string) string { } return "/ui/" } + +const remoteScriptCheckSecurityWarning = "using enable-script-checks without ACLs and without allow_write_http_from is DANGEROUS, use enable-local-script-checks instead, see https://www.hashicorp.com/blog/protecting-consul-from-rce-risk-in-specific-configurations/" + +// validateRemoteScriptsChecks returns an error if EnableRemoteScriptChecks is +// enabled without other security features, which mitigate the risk of executing +// remote scripts. +func validateRemoteScriptsChecks(conf RuntimeConfig) error { + if conf.EnableRemoteScriptChecks && !conf.ACLsEnabled && len(conf.AllowWriteHTTPFrom) == 0 { + return errors.New(remoteScriptCheckSecurityWarning) + } + return nil +} diff --git a/agent/config/runtime_test.go b/agent/config/runtime_test.go index 819dce8979..dee77376cc 100644 --- a/agent/config/runtime_test.go +++ b/agent/config/runtime_test.go @@ -423,6 +423,7 @@ func TestBuilder_BuildAndValide_ConfigFlagsAndEdgecases(t *testing.T) { rt.EnableRemoteScriptChecks = true rt.DataDir = dataDir }, + warns: []string{remoteScriptCheckSecurityWarning}, }, { desc: "-encrypt",