From f88d1ccc366063595a18080c7c83b49507fd0f1c Mon Sep 17 00:00:00 2001 From: Matt Keeler Date: Mon, 15 Apr 2019 14:34:36 -0400 Subject: [PATCH] Handle rules translation when coming from the JSON compat HCL (#5662) We were not handling some object keys when they were strings instead of identifiers. Now both are handled. Fixes #5493 --- acl/policy.go | 34 ++++++--- acl/policy_test.go | 108 ++++++++++++++++++++++++++++ command/acl/rules/translate.go | 7 ++ command/acl/rules/translate_test.go | 52 +++++++++----- 4 files changed, 173 insertions(+), 28 deletions(-) diff --git a/acl/policy.go b/acl/policy.go index 7fefcc2b05..180676f1e2 100644 --- a/acl/policy.go +++ b/acl/policy.go @@ -4,11 +4,13 @@ import ( "bytes" "encoding/binary" "fmt" + "strconv" "github.com/hashicorp/consul/sentinel" "github.com/hashicorp/hcl" "github.com/hashicorp/hcl/hcl/ast" hclprinter "github.com/hashicorp/hcl/hcl/printer" + "github.com/hashicorp/hcl/hcl/token" "golang.org/x/crypto/blake2b" ) @@ -818,22 +820,36 @@ func TranslateLegacyRules(policyBytes []byte) ([]byte, error) { rewritten := ast.Walk(parsed, func(node ast.Node) (ast.Node, bool) { switch n := node.(type) { - case *ast.ObjectKey: - switch n.Token.Text { + case *ast.ObjectItem: + if len(n.Keys) < 1 { + return node, true + } + + txt := n.Keys[0].Token.Text + if n.Keys[0].Token.Type == token.STRING { + txt, err = strconv.Unquote(txt) + if err != nil { + return node, true + } + } + + switch txt { + case "policy": + n.Keys[0].Token.Text = "policy" case "agent": - n.Token.Text = "agent_prefix" + n.Keys[0].Token.Text = "agent_prefix" case "key": - n.Token.Text = "key_prefix" + n.Keys[0].Token.Text = "key_prefix" case "node": - n.Token.Text = "node_prefix" + n.Keys[0].Token.Text = "node_prefix" case "query": - n.Token.Text = "query_prefix" + n.Keys[0].Token.Text = "query_prefix" case "service": - n.Token.Text = "service_prefix" + n.Keys[0].Token.Text = "service_prefix" case "session": - n.Token.Text = "session_prefix" + n.Keys[0].Token.Text = "session_prefix" case "event": - n.Token.Text = "event_prefix" + n.Keys[0].Token.Text = "event_prefix" } } diff --git a/acl/policy_test.go b/acl/policy_test.go index cb36b86b92..f002a2f905 100644 --- a/acl/policy_test.go +++ b/acl/policy_test.go @@ -1407,6 +1407,114 @@ operator = "write" require.Equal(t, strings.Trim(expected, "\n"), string(output)) } +func TestRulesTranslate_GH5493(t *testing.T) { + input := ` +{ + "key": { + "": { + "policy": "read" + }, + "key": { + "policy": "read" + }, + "policy": { + "policy": "read" + }, + "privatething1/": { + "policy": "deny" + }, + "anapplication/private/": { + "policy": "deny" + }, + "privatething2/": { + "policy": "deny" + } + }, + "session": { + "": { + "policy": "write" + } + }, + "node": { + "": { + "policy": "read" + } + }, + "agent": { + "": { + "policy": "read" + } + }, + "service": { + "": { + "policy": "read" + } + }, + "event": { + "": { + "policy": "read" + } + }, + "query": { + "": { + "policy": "read" + } + } +}` + expected := ` +key_prefix "" { + policy = "read" +} + +key_prefix "key" { + policy = "read" +} + +key_prefix "policy" { + policy = "read" +} + +key_prefix "privatething1/" { + policy = "deny" +} + +key_prefix "anapplication/private/" { + policy = "deny" +} + +key_prefix "privatething2/" { + policy = "deny" +} + +session_prefix "" { + policy = "write" +} + +node_prefix "" { + policy = "read" +} + +agent_prefix "" { + policy = "read" +} + +service_prefix "" { + policy = "read" +} + +event_prefix "" { + policy = "read" +} + +query_prefix "" { + policy = "read" +} +` + output, err := TranslateLegacyRules([]byte(input)) + require.NoError(t, err) + require.Equal(t, strings.Trim(expected, "\n"), string(output)) +} + func TestPrecedence(t *testing.T) { type testCase struct { name string diff --git a/command/acl/rules/translate.go b/command/acl/rules/translate.go index a44091218c..cdd423943f 100644 --- a/command/acl/rules/translate.go +++ b/command/acl/rules/translate.go @@ -53,6 +53,11 @@ func (c *cmd) Run(args []string) int { return 1 } + if c.tokenSecret && c.tokenAccessor { + c.UI.Error(fmt.Sprintf("Error - cannot specify both -token-secret and -token-accessor")) + return 1 + } + data, err := c.dataFromArgs(c.flags.Args()) if err != nil { c.UI.Error(fmt.Sprintf("Error! %v", err)) @@ -69,6 +74,8 @@ func (c *cmd) Run(args []string) int { // Trim whitespace and newlines (e.g. from echo without -n) data = strings.TrimSpace(data) + // It is not a bug that this doesn't look at tokenAccessor. We already know that we want the rules from + // a token and just need to tell the helper function whether it should be retrieved by its secret or accessor if rules, err := aclhelpers.GetRulesFromLegacyToken(client, data, c.tokenSecret); err != nil { c.UI.Error(err.Error()) return 1 diff --git a/command/acl/rules/translate_test.go b/command/acl/rules/translate_test.go index 162a49b5f4..d37b4de703 100644 --- a/command/acl/rules/translate_test.go +++ b/command/acl/rules/translate_test.go @@ -9,10 +9,10 @@ import ( "github.com/hashicorp/consul/agent" "github.com/hashicorp/consul/logger" - "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/sdk/testutil" + "github.com/hashicorp/consul/testrpc" "github.com/mitchellh/cli" - "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestRulesTranslateCommand_noTabs(t *testing.T) { @@ -25,7 +25,6 @@ func TestRulesTranslateCommand_noTabs(t *testing.T) { func TestRulesTranslateCommand(t *testing.T) { t.Parallel() - assert := assert.New(t) testDir := testutil.TempDir(t, "acl") defer os.RemoveAll(testDir) @@ -53,9 +52,9 @@ func TestRulesTranslateCommand(t *testing.T) { expected := "service_prefix \"\" {\n policy = \"write\"\n}" // From a file - { + t.Run("file", func(t *testing.T) { err := ioutil.WriteFile(testDir+"/rules.hcl", []byte(rules), 0644) - assert.NoError(err) + require.NoError(t, err) args := []string{ "-http-addr=" + a.HTTPAddr(), @@ -64,13 +63,13 @@ func TestRulesTranslateCommand(t *testing.T) { } code := cmd.Run(args) - assert.Equal(code, 0) - assert.Empty(ui.ErrorWriter.String()) - assert.Contains(ui.OutputWriter.String(), expected) - } + require.Equal(t, code, 0) + require.Empty(t, ui.ErrorWriter.String()) + require.Contains(t, ui.OutputWriter.String(), expected) + }) // From stdin - { + t.Run("stdin", func(t *testing.T) { go func() { stdinW.Write([]byte(rules)) stdinW.Close() @@ -83,13 +82,13 @@ func TestRulesTranslateCommand(t *testing.T) { } code := cmd.Run(args) - assert.Equal(code, 0) - assert.Empty(ui.ErrorWriter.String()) - assert.Contains(ui.OutputWriter.String(), expected) - } + require.Equal(t, code, 0) + require.Empty(t, ui.ErrorWriter.String()) + require.Contains(t, ui.OutputWriter.String(), expected) + }) // From arg - { + t.Run("arg", func(t *testing.T) { args := []string{ "-http-addr=" + a.HTTPAddr(), "-token=root", @@ -97,8 +96,23 @@ func TestRulesTranslateCommand(t *testing.T) { } code := cmd.Run(args) - assert.Equal(code, 0) - assert.Empty(ui.ErrorWriter.String()) - assert.Contains(ui.OutputWriter.String(), expected) - } + require.Equal(t, code, 0) + require.Empty(t, ui.ErrorWriter.String()) + require.Contains(t, ui.OutputWriter.String(), expected) + }) + + // cannot specify both secret and accessor + t.Run("exclusive-options", func(t *testing.T) { + args := []string{ + "-http-addr=" + a.HTTPAddr(), + "-token=root", + "-token-secret", + "-token-accessor", + `token "" { policy = "write" }`, + } + + code := cmd.Run(args) + require.Equal(t, 1, code, 0) + require.Equal(t, "Error - cannot specify both -token-secret and -token-accessor\n", ui.ErrorWriter.String()) + }) }