VAULT-32657 deprecate duplicate attributes in HCL configs and policies

api/cliconfig/config.go

@@ -35,7 +35,7 @@ type defaultConfig struct {
 // loadConfig reads the configuration from the given path. If path is
 // empty, then the default path will be used, or the environment variable
 // if set.
-func loadConfig(path string) (*defaultConfig, error) {
+func loadConfig(path string) (config *defaultConfig, duplicate bool, err error) {
 	if path == "" {
 		path = defaultConfigPath
 	}
@@ -44,35 +44,37 @@ func loadConfig(path string) (*defaultConfig, error) {
 	}
 
 	// NOTE: requires HOME env var to be set
-	path, err := homedir.Expand(path)
+	path, err = homedir.Expand(path)
 	if err != nil {
-		return nil, fmt.Errorf("error expanding config path %q: %w", path, err)
+		return nil, false, fmt.Errorf("error expanding config path %q: %w", path, err)
 	}
 
 	contents, err := os.ReadFile(path)
 	if err != nil && !os.IsNotExist(err) {
-		return nil, err
+		return nil, false, err
 	}
 
-	conf, err := parseConfig(string(contents))
+	conf, duplicate, err := parseConfig(string(contents))
 	if err != nil {
-		return nil, fmt.Errorf("error parsing config file at %q: %w; ensure that the file is valid; Ansible Vault is known to conflict with it", path, err)
+		return nil, duplicate, fmt.Errorf("error parsing config file at %q: %w; ensure that the file is valid; Ansible Vault is known to conflict with it", path, err)
 	}
 
-	return conf, nil
+	return conf, duplicate, nil
 }
 
 // parseConfig parses the given configuration as a string.
-func parseConfig(contents string) (*defaultConfig, error) {
-	root, err := hcl.Parse(contents)
+func parseConfig(contents string) (config *defaultConfig, duplicate bool, err error) {
+	// TODO (HCL_DUP_KEYS_DEPRECATION): on removal stage change this to a simple hcl.Parse, effectively treating
+	// duplicate keys as an error. Also get rid of all of these "duplicate" named return values
+	root, duplicate, err := parseAndCheckForDuplicateHclAttributes(contents)
 	if err != nil {
-		return nil, err
+		return nil, duplicate, err
 	}
 
 	// Top-level item should be the object list
 	list, ok := root.Node.(*ast.ObjectList)
 	if !ok {
-		return nil, fmt.Errorf("failed to parse config; does not contain a root object")
+		return nil, duplicate, fmt.Errorf("failed to parse config; does not contain a root object")
 	}
 
 	valid := map[string]struct{}{
@@ -88,12 +90,12 @@ func parseConfig(contents string) (*defaultConfig, error) {
 	}
 
 	if validationErrors != nil {
-		return nil, validationErrors
+		return nil, duplicate, validationErrors
 	}
 
 	var c defaultConfig
 	if err := hcl.DecodeObject(&c, list); err != nil {
-		return nil, err
+		return nil, duplicate, err
 	}
-	return &c, nil
+	return &c, duplicate, nil
 }

api/cliconfig/config_test.go

@@ -11,7 +11,7 @@ import (
 )
 
 func TestLoadConfig(t *testing.T) {
-	config, err := loadConfig(filepath.Join("testdata", "config.hcl"))
+	config, duplicate, err := loadConfig(filepath.Join("testdata", "config.hcl"))
 	if err != nil {
 		t.Fatalf("err: %s", err)
 	}
@@ -22,21 +22,29 @@ func TestLoadConfig(t *testing.T) {
 	if !reflect.DeepEqual(expected, config) {
 		t.Fatalf("bad: %#v", config)
 	}
+
+	if duplicate {
+		t.Fatal("expected no duplicate")
+	}
 }
 
 func TestLoadConfig_noExist(t *testing.T) {
-	config, err := loadConfig("nope/not-once/.never")
+	config, duplicate, err := loadConfig("nope/not-once/.never")
 	if err != nil {
 		t.Fatal(err)
 	}
 
 	if config.TokenHelper != "" {
 		t.Errorf("expected %q to be %q", config.TokenHelper, "")
 	}
+
+	if duplicate {
+		t.Fatal("expected no duplicate")
+	}
 }
 
 func TestParseConfig_badKeys(t *testing.T) {
-	_, err := parseConfig(`
+	_, duplicate, err := parseConfig(`
 token_helper = "/token"
 nope = "true"
 `)
@@ -47,4 +55,23 @@ nope = "true"
 	if !strings.Contains(err.Error(), `invalid key "nope" on line 3`) {
 		t.Errorf("bad error: %s", err.Error())
 	}
+
+	if duplicate {
+		t.Fatal("expected no duplicate")
+	}
+}
+
+func TestParseConfig_HclDuplicateKey(t *testing.T) {
+	_, duplicate, 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 !duplicate {
+		t.Fatal("expected duplicate")
+	}
 }

api/cliconfig/hcl_dup_attr_deprecation.go

@@ -0,0 +1,25 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package cliconfig
+
+import (
+	"strings"
+
+	"github.com/hashicorp/hcl"
+	"github.com/hashicorp/hcl/hcl/ast"
+	hclParser "github.com/hashicorp/hcl/hcl/parser"
+)
+
+// 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
+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") {
+		duplicate = true
+		res, err = hclParser.ParseDontErrorOnDuplicateKeys([]byte(input))
+	}
+	return res, duplicate, err
+}

api/cliconfig/util.go

@@ -10,19 +10,27 @@ import (
 // DefaultTokenHelper returns the token helper that is configured for Vault.
 // This helper should only be used for non-server CLI commands.
 func DefaultTokenHelper() (tokenhelper.TokenHelper, error) {
-	config, err := loadConfig("")
+	config, _, err := DefaultTokenHelperCheckDuplicates()
+	return config, err
+}
+
+// TODO (HCL_DUP_KEYS_DEPRECATION): eventually make this consider duplicates an error. Ideally we should remove it but
+// maybe we can't since it's become part of the API pkg.
+func DefaultTokenHelperCheckDuplicates() (helper tokenhelper.TokenHelper, duplicate bool, err error) {
+	config, duplicate, err := loadConfig("")
 	if err != nil {
-		return nil, err
+		return nil, duplicate, err
 	}
 
 	path := config.TokenHelper
 	if path == "" {
-		return tokenhelper.NewInternalTokenHelper()
+		helper, err = tokenhelper.NewInternalTokenHelper()
+		return helper, duplicate, err
 	}
 
 	path, err = tokenhelper.ExternalTokenHelperPath(path)
 	if err != nil {
-		return nil, err
+		return nil, duplicate, err
 	}
-	return &tokenhelper.ExternalTokenHelper{BinaryPath: path}, nil
+	return &tokenhelper.ExternalTokenHelper{BinaryPath: path}, duplicate, nil
 }

api/go.mod

@@ -21,7 +21,7 @@ require (
 	github.com/hashicorp/go-rootcerts v1.0.2
 	github.com/hashicorp/go-secure-stdlib/parseutil v0.1.6
 	github.com/hashicorp/go-secure-stdlib/strutil v0.1.2
-	github.com/hashicorp/hcl v1.0.0
+	github.com/hashicorp/hcl v1.0.1-vault-7
 	github.com/mitchellh/go-homedir v1.1.0
 	github.com/mitchellh/mapstructure v1.5.0
 	github.com/natefinch/atomic v1.0.1

api/go.sum

@@ -38,6 +38,8 @@ github.com/hashicorp/go-sockaddr v1.0.2 h1:ztczhD1jLxIRjVejw8gFomI1BQZOe2WoVOu0S
 github.com/hashicorp/go-sockaddr v1.0.2/go.mod h1:rB4wwRAUzs07qva3c5SdrY/NEtAUjGlgmH/UkBUC97A=
 github.com/hashicorp/hcl v1.0.0 h1:0Anlzjpi4vEasTeNFn2mLJgTSwt0+6sfsiTG8qcWGx4=
 github.com/hashicorp/hcl v1.0.0/go.mod h1:E5yfLk+7swimpb2L/Alb/PJmXilQ/rhwaUYs4T20WEQ=
+github.com/hashicorp/hcl v1.0.1-vault-7 h1:ag5OxFVy3QYTFTJODRzTKVZ6xvdfLLCA1cy/Y6xGI0I=
+github.com/hashicorp/hcl v1.0.1-vault-7/go.mod h1:XYhtn6ijBSAj6n4YqAaf7RBPS4I06AItNorpy+MoQNM=
 github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
 github.com/mattn/go-colorable v0.1.9/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
 github.com/mattn/go-colorable v0.1.12/go.mod h1:u5H1YNBxpqRaxsYJYSkiCWKzEfiAb1Gb520KVy5xxl4=

api/hcl_dup_attr_deprecation.go

@@ -0,0 +1,25 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package api
+
+import (
+	"strings"
+
+	"github.com/hashicorp/hcl"
+	"github.com/hashicorp/hcl/hcl/ast"
+	hclParser "github.com/hashicorp/hcl/hcl/parser"
+)
+
+// 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
+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") {
+		duplicate = true
+		res, err = hclParser.ParseDontErrorOnDuplicateKeys([]byte(input))
+	}
+	return res, duplicate, err
+}

api/ssh_agent.go

@@ -150,7 +150,9 @@ func LoadSSHHelperConfig(path string) (*SSHHelperConfig, error) {
 // ParseSSHHelperConfig parses the given contents as a string for the SSHHelper
 // configuration.
 func ParseSSHHelperConfig(contents string) (*SSHHelperConfig, error) {
-	root, err := hcl.Parse(string(contents))
+	// TODO (HCL_DUP_KEYS_DEPRECATION): replace with simple call to hcl.Parse once deprecation of duplicate attributes
+	// is over, for now just ignore duplicates
+	root, _, err := parseAndCheckForDuplicateHclAttributes(contents)
 	if err != nil {
 		return nil, errwrap.Wrapf("error parsing config: {{err}}", err)
 	}

api/ssh_agent_test.go

@@ -8,8 +8,20 @@ import (
 	"net/http"
 	"strings"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
+// TestSSH_CanLoadDuplicateKeys verifies that during the deprecation process of duplicate HCL attributes this function
+// will still allow them.
+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")
+}
+
 func TestSSH_CreateTLSClient(t *testing.T) {
 	// load the default configuration
 	config, err := LoadSSHHelperConfig("./test-fixtures/agent_config.hcl")

api/test-fixtures/agent_config_duplicate_keys.hcl

@@ -0,0 +1,6 @@
+# Copyright (c) HashiCorp, Inc.
+# SPDX-License-Identifier: MPL-2.0
+
+vault_addr="http://127.0.0.1:8200"
+vault_addr="http://127.0.0.1:8201"
+ssh_mount_point="ssh"

changelog/30386.txt

@@ -0,0 +1,3 @@
+```release-note:deprecation
+core: deprecate duplicate attributes in HCL configuration files and policy definitions
+```

command/agent.go

@@ -1211,12 +1211,17 @@ func (c *AgentCommand) loadConfig(paths []string) (*agentConfig.Config, error) {
 	cfg := agentConfig.NewConfig()
 
 	for _, configPath := range paths {
-		configFromPath, err := agentConfig.LoadConfig(configPath)
+		// TODO (HCL_DUP_KEYS_DEPRECATION): go back to agentConfig.LoadConfig and remove duplicate when deprecation is done
+		configFromPath, duplicate, err := agentConfig.LoadConfigCheckDuplicates(configPath)
 		if err != nil {
 			errs = multierror.Append(errs, fmt.Errorf("error loading configuration from %s: %w", configPath, err))
 		} else {
 			cfg = cfg.Merge(configFromPath)
 		}
+		if duplicate {
+			c.UI.Warn(fmt.Sprintf(
+				"WARNING: Duplicate keys found in the Vault agent configuration file %q, duplicate keys in HCL files are deprecated and will be forbidden in a future release.", configPath))
+		}
 	}
 
 	if errs != nil {