From 35f1ecee0b689e156e82c9f7a75a95f06099a27e Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Mon, 17 Aug 2020 17:39:49 -0400
Subject: [PATCH] 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",