-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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.
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
consul-template
: revert function_denylist
logic
#12071
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
```release-note:bug | ||
template: Fixed a bug where the default `function_denylist` would be appended to a specified list | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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{""}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
client { | ||
enabled = true | ||
|
||
template { | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
client { | ||
enabled = true | ||
|
||
template { | ||
function_denylist = ["foo"] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
client { | ||
enabled = true | ||
|
||
template { | ||
function_denylist = [] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
client { | ||
enabled = true | ||
|
||
template { | ||
disable_file_sandbox = true | ||
function_denylist = [""] | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
{ | ||
"client": { | ||
"enabled": true, | ||
"template": { | ||
"disable_file_sandbox": true, | ||
"function_denylist": [ | ||
"" | ||
] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
client { | ||
enabled = true | ||
|
||
template { | ||
disable_file_sandbox = true | ||
} | ||
} |
There was a problem hiding this comment.
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).