Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

consul-template: revert function_denylist logic #12071

Merged
merged 3 commits into from
Apr 18, 2022
Merged

consul-template: revert function_denylist logic #12071

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bf93cbd
consul-template: replace config rather than append
Mar 1, 2022
5abbf9c
Update changelog verbiage
DerekStrickland Mar 12, 2022
f8518ff
Update .changelog/12071.txt
DerekStrickland Apr 15, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions .changelog/12071.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:bug
template: Fixes a bug in config loading that resulted in the default `function_denylist`
always being appended to whatever the user specified.
DerekStrickland marked this conversation as resolved.
Show resolved Hide resolved
```
72 changes: 0 additions & 72 deletions client/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,78 +396,6 @@ func (c *ClientTemplateConfig) Copy() *ClientTemplateConfig {
return nc
}

// Merge merges the values of two ClientTemplateConfigs. If first copies the receiver
// instance, and then overrides those values with the instance to merge with.
func (c *ClientTemplateConfig) Merge(b *ClientTemplateConfig) *ClientTemplateConfig {
if c == nil {
return b
}

result := *c

if b == nil {
return &result
}

if b.BlockQueryWaitTime != nil {
result.BlockQueryWaitTime = b.BlockQueryWaitTime
}
if b.BlockQueryWaitTimeHCL != "" {
result.BlockQueryWaitTimeHCL = b.BlockQueryWaitTimeHCL
}

if b.ConsulRetry != nil {
result.ConsulRetry = result.ConsulRetry.Merge(b.ConsulRetry)
}

result.DisableSandbox = b.DisableSandbox

// Maintain backward compatibility for older clients
if len(b.FunctionBlacklist) > 0 {
for _, fn := range b.FunctionBlacklist {
if !helper.SliceStringContains(result.FunctionBlacklist, fn) {
result.FunctionBlacklist = append(result.FunctionBlacklist, fn)
}
}
Comment on lines -425 to -431
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we only ever call Merge when merging onto the default configuration? If not, we're potentially dropping the merged-onto result by doing this, and it doesn't seem to be protecting us from the [""] case (see the test below).

} else if b.FunctionBlacklist != nil {
// No funcs denied
result.FunctionBlacklist = []string{}
}

if len(b.FunctionDenylist) > 0 {
for _, fn := range b.FunctionDenylist {
if !helper.SliceStringContains(result.FunctionDenylist, fn) {
result.FunctionDenylist = append(result.FunctionDenylist, fn)
}
}
} else if b.FunctionDenylist != nil {
// No funcs denied
result.FunctionDenylist = []string{}
}

if b.MaxStale != nil {
result.MaxStale = b.MaxStale
}

if b.MaxStaleHCL != "" {
result.MaxStaleHCL = b.MaxStaleHCL
}

if b.Wait != nil {
result.Wait = result.Wait.Merge(b.Wait)
}

if b.WaitBounds != nil {
result.WaitBounds = result.WaitBounds.Merge(b.WaitBounds)
}

if b.VaultRetry != nil {
result.VaultRetry = result.VaultRetry.Merge(b.VaultRetry)
}

return &result
}

func (c *ClientTemplateConfig) IsEmpty() bool {
if c == nil {
return true
Expand Down
7 changes: 2 additions & 5 deletions command/agent/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1705,11 +1705,8 @@ func (a *ClientConfig) Merge(b *ClientConfig) *ClientConfig {
result.DisableRemoteExec = b.DisableRemoteExec
}

if result.TemplateConfig == nil && b.TemplateConfig != nil {
templateConfig := *b.TemplateConfig
result.TemplateConfig = &templateConfig
} else if b.TemplateConfig != nil {
result.TemplateConfig = result.TemplateConfig.Merge(b.TemplateConfig)
if b.TemplateConfig != nil {
result.TemplateConfig = b.TemplateConfig
}

// Add the servers
Expand Down
104 changes: 70 additions & 34 deletions command/agent/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1454,43 +1454,79 @@ func TestConfig_LoadConsulTemplateConfig(t *testing.T) {
require.Equal(t, 20*time.Second, *templateConfig.VaultRetry.MaxBackoff)
}

func TestConfig_LoadConsulTemplateBasic(t *testing.T) {
ci.Parallel(t)

defaultConfig := DefaultConfig()

// hcl
agentConfig, err := LoadConfig("test-resources/client_with_basic_template.hcl")
require.NoError(t, err)
require.NotNil(t, agentConfig.Client.TemplateConfig)

agentConfig = defaultConfig.Merge(agentConfig)
require.Len(t, agentConfig.Client.TemplateConfig.FunctionDenylist, 0)
require.NotNil(t, agentConfig.Client.TemplateConfig.FunctionDenylist)

clientAgent := Agent{config: agentConfig}
clientConfig, err := clientAgent.clientConfig()
require.NoError(t, err)

templateConfig := clientConfig.TemplateConfig
require.NotNil(t, templateConfig)
require.True(t, templateConfig.DisableSandbox)
require.Len(t, templateConfig.FunctionDenylist, 0)

// json
agentConfig, err = LoadConfig("test-resources/client_with_basic_template.json")
require.NoError(t, err)
func TestConfig_LoadConsulTemplate_FunctionDenylist(t *testing.T) {
cases := []struct {
File string
Expected *client.ClientTemplateConfig
}{
{
"test-resources/minimal_client.hcl",
nil,
},
{
"test-resources/client_with_basic_template.json",
&client.ClientTemplateConfig{
DisableSandbox: true,
FunctionDenylist: []string{},
},
},
{
"test-resources/client_with_basic_template.hcl",
&client.ClientTemplateConfig{
DisableSandbox: true,
FunctionDenylist: []string{},
},
},
{
"test-resources/client_with_function_denylist.hcl",
&client.ClientTemplateConfig{
DisableSandbox: false,
FunctionDenylist: []string{"foo"},
},
},
{
"test-resources/client_with_function_denylist_empty.hcl",
&client.ClientTemplateConfig{
DisableSandbox: false,
FunctionDenylist: []string{},
},
},
{
"test-resources/client_with_function_denylist_empty_string.hcl",
&client.ClientTemplateConfig{
DisableSandbox: true,
FunctionDenylist: []string{""},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like a bug to me: when would we ever want this to be the result after a merged configuration? Are we validating it elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it probably is, and I think you commented on the issue about that. Unfortunately, I pulled old code and confirmed that this currently does not fail validation. I added that comment to the issue thread as well.

My two thoughts other than continuing to allow this are either:

  • Remove this check and open an issue to validate empty strings (or remove them?)
  • Update this PR to validate empty strings (or remove them?)

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say let's fix this issue once and for all here instead of trying to do it in another issue and having to touch this code again. There are two issues in #11923, but we're really only discussing one of them.

This is the JSON client configuration block from #11923 (comment):

"template": {
  "function_denylist": [
    ""
  ]
}

This is an operator error: we should either correct it silently or return an error on startup. It's not clear to me at any point in the discussion in #11923 whether this configuration actually worked prior to 1.2.4. Generally speaking we don't silently correct things in the configuration, so my take is that this should blow up so the operator can fix their configuration. Especially because this is a security-related configuration.

The second issue is whether the configuration is being merged correctly with the defaults when we're done. It's not clear to me that the merging bug is a problem at all if the configuration were:

"template": {
  "function_denylist": []
}

Because in that scenario the configuration is an empty slice and there's nothing to iterate over during the merge.

},
},
{
"test-resources/client_with_function_denylist_empty_string.json",
&client.ClientTemplateConfig{
DisableSandbox: true,
FunctionDenylist: []string{""},
},
},
{
"test-resources/client_with_function_denylist_nil.hcl",
&client.ClientTemplateConfig{
DisableSandbox: true,
},
},
{
"test-resources/client_with_empty_template.hcl",
nil,
},
}

agentConfig = defaultConfig.Merge(agentConfig)
for _, tc := range cases {
t.Run(tc.File, func(t *testing.T) {
agentConfig, err := LoadConfig(tc.File)

clientAgent = Agent{config: agentConfig}
clientConfig, err = clientAgent.clientConfig()
require.NoError(t, err)
require.NoError(t, err)

templateConfig = clientConfig.TemplateConfig
require.NotNil(t, templateConfig)
require.True(t, templateConfig.DisableSandbox)
require.Len(t, templateConfig.FunctionDenylist, 0)
templateConfig := agentConfig.Client.TemplateConfig
require.Equal(t, tc.Expected, templateConfig)
})
}
}

func TestParseMultipleIPTemplates(t *testing.T) {
Expand Down
6 changes: 6 additions & 0 deletions command/agent/test-resources/client_with_empty_template.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
client {
enabled = true

template {
}
}
7 changes: 7 additions & 0 deletions command/agent/test-resources/client_with_function_denylist.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
client {
enabled = true

template {
function_denylist = ["foo"]
}
}
7 changes: 7 additions & 0 deletions command/agent/test-resources/client_with_function_denylist_empty.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
client {
enabled = true

template {
function_denylist = []
}
}
8 changes: 8 additions & 0 deletions command/agent/test-resources/client_with_function_denylist_empty_string.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
client {
enabled = true

template {
disable_file_sandbox = true
function_denylist = [""]
}
}
11 changes: 11 additions & 0 deletions command/agent/test-resources/client_with_function_denylist_empty_string.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"client": {
"enabled": true,
"template": {
"disable_file_sandbox": true,
"function_denylist": [
""
]
}
}
}
7 changes: 7 additions & 0 deletions command/agent/test-resources/client_with_function_denylist_nil.hcl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
client {
enabled = true

template {
disable_file_sandbox = true
}
}