VAULT-35838: advance deprecation of duplicate HCL attributes to pending removal stage

api/cliconfig/config_test.go

@@ -61,17 +61,43 @@ nope = "true"
 	}
 }
 
+// TestParseConfig_HclDuplicateKey tests the parsing of HCL files with duplicate keys.
+// TODO (HCL_DUP_KEYS_DEPRECATION): on full removal change this test to ensure that duplicate attributes cannot be parsed
+// under any circumstances.
 func TestParseConfig_HclDuplicateKey(t *testing.T) {
-	_, duplicate, err := parseConfig(`
+	t.Run("fail parsing without env var", func(t *testing.T) {
+		_, _, err := parseConfig(`
 token_helper = "/token"
 token_helper = "/token"
 `)
-	// TODO (HCL_DUP_KEYS_DEPRECATION): change this to expect an error once support for duplicate keys is fully removed
-	if err != nil {
-		t.Fatal("expected no error")
-	}
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	})
 
-	if !duplicate {
-		t.Fatal("expected duplicate")
-	}
+	t.Run("fail parsing with env var set to false", func(t *testing.T) {
+		t.Setenv(allowHclDuplicatesEnvVar, "false")
+		_, _, err := parseConfig(`
+token_helper = "/token"
+token_helper = "/token"
+`)
+		if err == nil {
+			t.Fatal("expected error")
+		}
+	})
+
+	t.Run("succeed parsing with env var set to true", func(t *testing.T) {
+		t.Setenv(allowHclDuplicatesEnvVar, "true")
+		_, duplicate, err := parseConfig(`
+token_helper = "/token"
+token_helper = "/token"
+`)
+		if err != nil {
+			t.Fatal("expected no error")
+		}
+
+		if !duplicate {
+			t.Fatal("expected duplicate")
+		}
+	})
 }

api/cliconfig/hcl_dup_attr_deprecation.go

@@ -4,20 +4,42 @@
 package cliconfig
 
 import (
+	"fmt"
+	"os"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
 	hclParser "github.com/hashicorp/hcl/hcl/parser"
 )
 
+// allowHclDuplicatesEnvVar is an environment variable that allows Vault to revert back to accepting HCL files with
+// duplicate attributes. It's temporary until we finish the deprecation process, at which point this will be removed
+const allowHclDuplicatesEnvVar = "VAULT_ALLOW_PENDING_REMOVAL_DUPLICATE_HCL_ATTRIBUTES"
+
 // parseAndCheckForDuplicateHclAttributes parses the input JSON/HCL file and if it is HCL it also checks
-// for duplicate keys in the HCL file, allowing callers to handle the issue accordingly. In a future release we'll
-// change the behavior to treat duplicate keys as an error and eventually remove this helper altogether.
-// TODO (HCL_DUP_KEYS_DEPRECATION): remove once not used anymore
+// for duplicate keys in the HCL file, allowing callers to handle the issue accordingly. It now only accepts duplicate
+// // keys if the environment variable VAULT_ALLOW_PENDING_REMOVAL_DUPLICATE_HCL_ATTRIBUTES is set to true. In a future
+// // release we'll remove this function entirely and there will be no way to parse HCL files with duplicate keys.
+// // TODO (HCL_DUP_KEYS_DEPRECATION): remove once not used anymore
 func parseAndCheckForDuplicateHclAttributes(input string) (res *ast.File, duplicate bool, err error) {
 	res, err = hcl.Parse(input)
 	if err != nil && strings.Contains(err.Error(), "Each argument can only be defined once") {
+		allowHclDuplicatesRaw := os.Getenv(allowHclDuplicatesEnvVar)
+		if allowHclDuplicatesRaw == "" {
+			// default is to not allow duplicates
+			return nil, false, err
+		}
+		allowHclDuplicates, envParseErr := strconv.ParseBool(allowHclDuplicatesRaw)
+		if envParseErr != nil {
+			return nil, false, fmt.Errorf("error parsing %q environment variable: %w", allowHclDuplicatesEnvVar, err)
+		}
+		if !allowHclDuplicates {
+			return nil, false, err
+		}
+
+		// if allowed by the environment variable, parse again without failing on duplicate attributes
 		duplicate = true
 		res, err = hclParser.ParseDontErrorOnDuplicateKeys([]byte(input))
 	}

api/hcl_dup_attr_deprecation.go

@@ -4,20 +4,42 @@
 package api
 
 import (
+	"fmt"
+	"os"
+	"strconv"
 	"strings"
 
 	"github.com/hashicorp/hcl"
 	"github.com/hashicorp/hcl/hcl/ast"
 	hclParser "github.com/hashicorp/hcl/hcl/parser"
 )
 
+// allowHclDuplicatesEnvVar is an environment variable that allows Vault to revert back to accepting HCL files with
+// duplicate attributes. It's temporary until we finish the deprecation process, at which point this will be removed
+const allowHclDuplicatesEnvVar = "VAULT_ALLOW_PENDING_REMOVAL_DUPLICATE_HCL_ATTRIBUTES"
+
 // parseAndCheckForDuplicateHclAttributes parses the input JSON/HCL file and if it is HCL it also checks
-// for duplicate keys in the HCL file, allowing callers to handle the issue accordingly. In a future release we'll
-// change the behavior to treat duplicate keys as an error and eventually remove this helper altogether.
+// for duplicate keys in the HCL file, allowing callers to handle the issue accordingly. It now only accepts duplicate
+// keys if the environment variable VAULT_ALLOW_PENDING_REMOVAL_DUPLICATE_HCL_ATTRIBUTES is set to true. In a future
+// release we'll remove this function entirely and there will be no way to parse HCL files with duplicate keys.
 // TODO (HCL_DUP_KEYS_DEPRECATION): remove once not used anymore
 func parseAndCheckForDuplicateHclAttributes(input string) (res *ast.File, duplicate bool, err error) {
 	res, err = hcl.Parse(input)
 	if err != nil && strings.Contains(err.Error(), "Each argument can only be defined once") {
+		allowHclDuplicatesRaw := os.Getenv(allowHclDuplicatesEnvVar)
+		if allowHclDuplicatesRaw == "" {
+			// default is to not allow duplicates
+			return nil, false, err
+		}
+		allowHclDuplicates, envParseErr := strconv.ParseBool(allowHclDuplicatesRaw)
+		if envParseErr != nil {
+			return nil, false, fmt.Errorf("error parsing %q environment variable: %w", allowHclDuplicatesEnvVar, err)
+		}
+		if !allowHclDuplicates {
+			return nil, false, err
+		}
+
+		// if allowed by the environment variable, parse again without failing on duplicate attributes
 		duplicate = true
 		res, err = hclParser.ParseDontErrorOnDuplicateKeys([]byte(input))
 	}

api/ssh_agent_test.go

@@ -14,12 +14,25 @@ import (
 
 // TestSSH_CanLoadDuplicateKeys verifies that during the deprecation process of duplicate HCL attributes this function
 // will still allow them.
+// TODO (HCL_DUP_KEYS_DEPRECATION): on full removal change this test to ensure that duplicate attributes cannot be parsed
+// under any circumstances.
 func TestSSH_CanLoadDuplicateKeys(t *testing.T) {
-	_, err := LoadSSHHelperConfig("./test-fixtures/agent_config_duplicate_keys.hcl")
-	require.NoError(t, err)
-	// TODO (HCL_DUP_KEYS_DEPRECATION): Change test to expect an error
-	// require.Error(t, err)
-	// require.Contains(t, err.Error(), "Each argument can only be defined once")
+	t.Run("fail parsing without env var", func(t *testing.T) {
+		_, err := LoadSSHHelperConfig("./test-fixtures/agent_config_duplicate_keys.hcl")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "Each argument can only be defined once")
+	})
+	t.Run("fail parsing with env var set to false", func(t *testing.T) {
+		t.Setenv(allowHclDuplicatesEnvVar, "false")
+		_, err := LoadSSHHelperConfig("./test-fixtures/agent_config_duplicate_keys.hcl")
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "Each argument can only be defined once")
+	})
+	t.Run("succeed parsing with env var set to true", func(t *testing.T) {
+		t.Setenv(allowHclDuplicatesEnvVar, "true")
+		_, err := LoadSSHHelperConfig("./test-fixtures/agent_config_duplicate_keys.hcl")
+		require.NoError(t, err)
+	})
 }
 
 func TestSSH_CreateTLSClient(t *testing.T) {

changelog/31215.txt

@@ -0,0 +1,5 @@
+```release-note:deprecation
+core: disallow usage of duplicate attributes in HCL configuration files and policy definitions, which were already
+deprecated. For now those errors can be suppressed back to warnings by setting the environment variable
+VAULT_ALLOW_PENDING_REMOVAL_DUPLICATE_HCL_ATTRIBUTES.
+```

command/agent_test.go

@@ -30,6 +30,7 @@ import (
 	credAppRole "github.com/hashicorp/vault/builtin/credential/approle"
 	"github.com/hashicorp/vault/command/agent"
 	agentConfig "github.com/hashicorp/vault/command/agent/config"
+	"github.com/hashicorp/vault/helper/random"
 	"github.com/hashicorp/vault/helper/testhelpers/minimal"
 	"github.com/hashicorp/vault/helper/useragent"
 	vaulthttp "github.com/hashicorp/vault/http"
@@ -363,6 +364,7 @@ listener "tcp" {
 	)
 	configPath := makeTempFile(t, "config.hcl", config)
 
+	t.Setenv(random.AllowHclDuplicatesEnvVar, "true")
 	// Start the agent
 	ui, cmd := testAgentCommand(t, logger)
 	cmd.client = serverClient
@@ -400,7 +402,7 @@ listener "tcp" {
 	//----------------------------------------------------
 
 	// TODO (HCL_DUP_KEYS_DEPRECATION): Eventually remove this check together with the duplicate attribute in this
-	// test's configuration, create separate test ensuring such a config is not valid
+	// test's configuration
 	require.Contains(t, ui.ErrorWriter.String(),
 		"WARNING: Duplicate keys found")
 
@@ -3115,16 +3117,37 @@ func TestAgent_Config_ReloadTls(t *testing.T) {
 }
 
 // TestAgent_Config_HclDuplicateKey checks that a log warning is printed when the agent config has duplicate attributes
+// TODO (HCL_DUP_KEYS_DEPRECATION): always expect error once deprecation is done
 func TestAgent_Config_HclDuplicateKey(t *testing.T) {
-	configFile := populateTempFile(t, "agent-config.hcl", `
+	t.Run("duplicate error with env unset", func(t *testing.T) {
+		configFile := populateTempFile(t, "agent-config.hcl", `
 log_level = "trace"
 log_level = "debug"
 `)
-	_, duplicate, err := agentConfig.LoadConfigFileCheckDuplicates(configFile.Name())
-	// TODO (HCL_DUP_KEYS_DEPRECATION): expect error on duplicates once deprecation is done
-	require.NoError(t, err)
-	require.True(t, duplicate)
-	// require.Contains(t, err.Error(), "Each argument can only be defined once")
+		_, _, err := agentConfig.LoadConfigFileCheckDuplicates(configFile.Name())
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "Each argument can only be defined once")
+	})
+	t.Run("duplicate error with env set to false", func(t *testing.T) {
+		configFile := populateTempFile(t, "agent-config.hcl", `
+log_level = "trace"
+log_level = "debug"
+`)
+		t.Setenv(random.AllowHclDuplicatesEnvVar, "false")
+		_, _, err := agentConfig.LoadConfigFileCheckDuplicates(configFile.Name())
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "Each argument can only be defined once")
+	})
+	t.Run("duplicate warning with env set to true", func(t *testing.T) {
+		configFile := populateTempFile(t, "agent-config.hcl", `
+log_level = "trace"
+log_level = "debug"
+`)
+		t.Setenv(random.AllowHclDuplicatesEnvVar, "true")
+		_, duplicate, err := agentConfig.LoadConfigFileCheckDuplicates(configFile.Name())
+		require.NoError(t, err)
+		require.True(t, duplicate)
+	})
 }
 
 // TestAgent_NonTLSListener_SIGHUP tests giving a SIGHUP signal to a listener

command/login_test.go

@@ -16,6 +16,7 @@ import (
 	credToken "github.com/hashicorp/vault/builtin/credential/token"
 	credUserpass "github.com/hashicorp/vault/builtin/credential/userpass"
 	"github.com/hashicorp/vault/command/token"
+	"github.com/hashicorp/vault/helper/random"
 	"github.com/hashicorp/vault/helper/testhelpers"
 	"github.com/hashicorp/vault/vault"
 	"github.com/stretchr/testify/require"
@@ -639,18 +640,33 @@ token_helper = ""
 	}
 	token := secret.Auth.ClientToken
 
-	ui, cmd := testLoginCommand(t)
-	cmd.tokenHelper = nil // cause default one to be used
-	cmd.client = client
+	t.Run("fail if duplicates are not allowed", func(t *testing.T) {
+		_, cmd := testLoginCommand(t)
+		cmd.tokenHelper = nil // cause default one to be used
+		cmd.client = client
 
-	code := cmd.Run([]string{
-		token,
+		code := cmd.Run([]string{
+			token,
+		})
+		if exp := 1; code != exp {
+			t.Errorf("expected %d to be %d", code, exp)
+		}
 	})
-	if exp := 0; code != exp {
-		t.Errorf("expected %d to be %d", code, exp)
-	}
 
-	// TODO (HCL_DUP_KEYS_DEPRECATION): Instead ensure that login command fails if the config file contains duplicate keys
-	require.Contains(t, ui.ErrorWriter.String(),
-		"WARNING: Duplicate keys found in the Vault token helper configuration file, duplicate keys in HCL files are deprecated and will be forbidden in a future release.")
+	t.Run("succeed if duplicates are allowed", func(t *testing.T) {
+		t.Setenv(random.AllowHclDuplicatesEnvVar, "true")
+		ui, cmd := testLoginCommand(t)
+		cmd.tokenHelper = nil // cause default one to be used
+		cmd.client = client
+
+		code := cmd.Run([]string{
+			token,
+		})
+		if exp := 0; code != exp {
+			t.Errorf("expected %d to be %d", code, exp)
+		}
+
+		require.Contains(t, ui.ErrorWriter.String(),
+			"WARNING: Duplicate keys found in the Vault token helper configuration file, duplicate keys in HCL files are deprecated and will be forbidden in a future release.")
+	})
 }

command/operator_migrate_test.go

@@ -22,6 +22,7 @@ import (
 	log "github.com/hashicorp/go-hclog"
 	"github.com/hashicorp/go-secure-stdlib/base62"
 	"github.com/hashicorp/vault/command/server"
+	"github.com/hashicorp/vault/helper/random"
 	"github.com/hashicorp/vault/sdk/physical"
 	"github.com/hashicorp/vault/vault"
 )
@@ -219,16 +220,17 @@ storage_destination "raft" {
 				},
 			},
 		}
+		t.Setenv(random.AllowHclDuplicatesEnvVar, "true")
 		cfg, err := cmd.loadMigratorConfig(cfgName)
 		if err != nil {
 			t.Fatal(cfg)
 		}
 		if diff := deep.Equal(cfg, expCfg); diff != nil {
 			t.Fatal(diff)
 		}
-		// TODO (HCL_DUP_KEYS_DEPRECATION): Remove warning and instead add one of these "verifyBad" tests down below
-		// to ensure that duplicate attributes fail to parse.
+		// TODO (HCL_DUP_KEYS_DEPRECATION): Remove warning and leave only this "verifyBad" test down below
 		strings.Contains(ui.ErrorWriter.String(), "WARNING: Duplicate keys found in migration configuration file, duplicate keys in HCL files are deprecated and will be forbidden in a future release.")
+		t.Setenv(random.AllowHclDuplicatesEnvVar, "false")
 
 		verifyBad := func(cfg string) {
 			os.WriteFile(cfgName, []byte(cfg), 0o644)
@@ -276,6 +278,16 @@ storage_destination "raft" {
 
 storage_destination "consul" {
   path = "dest_path"
+}`)
+		// duplicate hcl attribute
+		verifyBad(`
+storage_source "consul" {
+  path = "src_path"
+}
+
+storage_destination "raft" {
+  path = "dest_path"
+  path = "dest_path"
 }`)
 	})