VAULT-32657 deprecate duplicate attributes in HCL configs and policies (#30386)

* upgrade hcl dependency on api pkg

This upgrades the hcl dependency for the API pkg,
and adapts its usage so users of our API pkg are
not affected. There's no good way of communicating
a warning via a library call so we don't.

The tokenHelper which is used by all Vault CLI
commands in order to create the Vault client, as
well as directly used by the login and server
commands, is implemented on the api pkg, so this
upgrade also affects all of those commands. Seems
like this was only moved to the api pkg because
the Terraform provider uses it, and I thought
creating a full copy of all those files back under
command would be too much spaghetti.

Also leaving some TODOs to make next deprecation
steps easier.

* upgrade hcl dependency in vault and sdk pkgs

* upgrade hcl dependency in vault and sdk pkgs

* add CLI warnings to commands that take a config

- vault agent (unit test on CMD warning)
- vault proxy (unit test on CMD warning)
- vault server (no test for the warning)
- vault operator diagnose (no tests at all, uses the
same function as vault server

* ignore duplicates on ParseKMSes function

* Extend policy parsing functions and warn on policy store

* Add warning on policy fmt with duplicate attributes

* Add warnings when creating/updating policy with duplicate HCL attrs

* Add log warning when switchedGetPolicy finds duplicate attrs

Following operations can trigger this warning when they run into a policy
with duplicate attributes:
* replication filtered path namespaces invalidation
* policy read API
* building an ACL (for many different purposes like most authZ operations)
* looking up DR token policies
* creating a token with named policies
* when caching the policies for all namespaces during unseal

* Print log warnings when token inline policy has duplicate attrs

No unit tests on these as new test infra would have to be built on all.
Operations affected, which will now print a log warning when the retrieved
token has an inline policy with duplicate attributes:
* capabilities endpoints in sys mount
* handing events under a subscription with a token with duplicate
attrs in inline policies
* token used to create another token has duplicate attrs in inline
policies (sudo check)
* all uses of fetchACLTokenEntryAndEntity when the request uses a
token with inline policies with duplicate attrs. Almost all reqs
are subject to this
* when tokens are created with inline policies (unclear exactly how that
can happen)

* add changelog and deprecation notice

* add missing copywrite notice

* fix copy-paste mistake

good thing it was covered by unit tests

* Fix manual parsing of telemetry field in SharedConfig

This commit in the hcl library was not in the
v1.0.1-vault-5 version we're using but is
included in v1.0.1-vault-7:
hashicorp/hcl@e80118a

This thing of reusing when parsing means that
our approach of manually re-parsing fields
on top of fields that have already been parsed
by the hcl annotation causes strings (maybe
more?) to concatenate.

Fix that by removing annotation. There's
actually more occurrences of this thing of
automatically parsing something that is also
manually parsing. In some places we could
just remove the boilerplate manual parsing, in
others we better remove the auto parsing, but
I don't wanna pull at that thread right now. I
just checked that all places at least fully
overwrite the automatically parsed field
instead of reusing it as the target of the
decode call. The only exception is the AOP
field on ent but that doesn't have maps or
slices, so I think it's fine.

An alternative approach would be to ensure
that the auto-parsed value is discarded,
like the current parseCache function does

note how it's template not templates

* Fix linter complaints

* Update command/base_predict.go

Co-authored-by: Mike Palmiotto <mike.palmiotto@hashicorp.com>

* address review

* remove copywrite headers

* re-add copywrite headers

* make fmt

* Update website/content/partials/deprecation/duplicate-hcl-attributes.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/partials/deprecation/duplicate-hcl-attributes.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* Update website/content/partials/deprecation/duplicate-hcl-attributes.mdx

Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

* undo changes to deprecation.mdx

* remove deprecation doc

* fix conflict with changes from main

---------

Co-authored-by: Mike Palmiotto <mike.palmiotto@hashicorp.com>
Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>

Author: 3 people

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"