From 2b8da952a80ab0676d33bfecb34ee3a66589950e Mon Sep 17 00:00:00 2001
From: Pierre Souchay <pierresouchay@users.noreply.github.com>
Date: Thu, 2 Apr 2020 09:59:23 +0200
Subject: [PATCH] agent: show warning when enable_script_checks is enabled
 without safty net (#7437)

In order to enforce a bit security on Consul agents, add a new method in agent
to highlight possible security issues.

This does not return an error for now, but might in the future.

For now, it detects issues such as:

https://www.hashicorp.com/blog/protecting-consul-from-rce-risk-in-specific-configurations/

This would display this kind of messages:

```
2020-03-11T18:27:49.873+0100 [ERROR] agent: [SECURITY] issue: error="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/"
```
---
 agent/agent.go      | 24 ++++++++++++++++++++++++
 agent/agent_test.go | 15 +++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/agent/agent.go b/agent/agent.go
index 2ac009ef5..256785774 100644
--- a/agent/agent.go
+++ b/agent/agent.go
@@ -384,6 +384,11 @@ func (a *Agent) Start() error {
 
 	c := a.config
 
+	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 "+
@@ -3841,6 +3846,21 @@ 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()
 
@@ -4022,6 +4042,10 @@ func (a *Agent) loadLimits(conf *config.RuntimeConfig) {
 // including all services, checks, tokens, metadata, dnsServer configs, etc.
 // It will also reload all ongoing watches.
 func (a *Agent) ReloadConfig(newCfg *config.RuntimeConfig) error {
+	if err := a.CheckSecurity(newCfg); err != nil {
+		a.logger.Error("Security error while reloading configuration: %#v", err)
+		return err
+	}
 	// Bulk update the services and checks
 	a.PauseSync()
 	defer a.ResumeSync()
diff --git a/agent/agent_test.go b/agent/agent_test.go
index 89b6d5a38..8261f6520 100644
--- a/agent/agent_test.go
+++ b/agent/agent_test.go
@@ -3383,6 +3383,21 @@ func TestAgent_loadTokens(t *testing.T) {
 	})
 }
 
+func TestAgent_SecurityChecks(t *testing.T) {
+	t.Parallel()
+	hcl := `
+		enable_script_checks = true
+	`
+	a := &TestAgent{Name: t.Name(), HCL: hcl}
+	defer a.Shutdown()
+
+	data := make([]byte, 0, 8192)
+	bytesBuffer := bytes.NewBuffer(data)
+	a.LogOutput = bytesBuffer
+	assert.NoError(t, a.Start(t))
+	assert.Contains(t, bytesBuffer.String(), "using enable-script-checks without ACLs and without allow_write_http_from is DANGEROUS")
+}
+
 func TestAgent_ReloadConfigOutgoingRPCConfig(t *testing.T) {
 	t.Parallel()
 	dataDir := testutil.TempDir(t, "agent") // we manage the data dir