luxolus
/
open-consul
2
0
Fork 0
Code Issues 1 Pull Requests Packages Releases Wiki Activity

Merge pull request #8528 from hashicorp/dnephin/move-node-name-validation 

config: Move some config validation from Agent.Start to config.Builder.Validate
This commit is contained in:
Daniel Nephin 2020-08-18 18:25:41 -04:00 committed by GitHub
parent 87fb6b9339 7078ca07fa
commit ab0d206eac
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 121 additions and 74 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

49
agent/agent.go
View File

@ -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"
@ -637,26 +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
}
// 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 +2465,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 +2481,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.",
@ -3714,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()
@ -3926,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))

32
agent/config/builder.go
View File

@ -3,6 +3,7 @@ package config
import (
"encoding/base64"
"encoding/json"
"errors"
"fmt"
"io/ioutil"
"net"
@ -19,6 +20,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 +1119,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 [::]")
}
@ -1338,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
}
@ -2179,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
}

61
agent/config/builder_test.go
View File

@ -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
}
}

30
agent/config/runtime_test.go
View File

@ -423,6 +423,7 @@ func TestBuilder_BuildAndValide_ConfigFlagsAndEdgecases(t *testing.T) {
rt.EnableRemoteScriptChecks = true
rt.DataDir = dataDir
},
warns: []string{remoteScriptCheckSecurityWarning},
},
{
desc: "-encrypt",
@ -4280,27 +4281,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 +4322,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

10
agent/dns.go
View File

@ -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
}

10
agent/dns/dns.go Normal file
View File

@ -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\\-]+`)

3
agent/dns_test.go
View File

@ -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)
}
})