all: secrets cannot contain $$ or ${SOMETEXT}

[efd6]

Relates:

• No way to escape variables in transpiler parsing elastic-agent#2177
• Escape ${foo} on strings when configuration is received from Elastic-Agent beats#35260

This was identified while working on a customer issue with the ti_threatconnect integration. In that issue we were seeing 400 status codes coming back from API queries. After tracking down the cause, it is seen to come from variable expansion when taking the provided config and rendering it for beats. The secret in question had an instance of the substring $$ which was being interpreted as a $-escaped $.

This causes confusion for users and is not ideal.

To reproduce this.

1. Start a stack with elastic-package.

2. Add the ThreatConnect integration with the Elastic-Agent (elastic-package) as the agent with the following config.
   

3. Save and deploy.

4. Collect diagnostics from the agent and examine the pre-config.yaml file for the package installation.

   inputs:
       - data_stream:
           namespace: default
         id: cel-threatconnect-dd614c67-bb84-47de-b23e-93c5f3ce6753
         meta:
           package:
               name: ti_threatconnect
               version: 0.4.0
         name: ti_threatconnect-1
         package_policy_id: dd614c67-bb84-47de-b23e-93c5f3ce6753
         revision: 1
         streams:
           - config_version: 2
             data_stream:
               dataset: ti_threatconnect.indicator
               type: logs
             fields:
               _conf:
                   ioc_expiration_duration: 90d
             fields_under_root: true
             id: cel-ti_threatconnect.indicator-dd614c67-bb84-47de-b23e-93c5f3ce6753
             interval: 24h
             program: |
   <snip>
            publisher_pipeline.disable_host: true
             redact:
               fields:
                   - secret_key
             resource.ssl: null
             resource.timeout: 2m
             resource.url: https://example.com
             state:
               access_id: user
               batch: 5000
               counter: 0
               event_list:
                   - associatedGroups
                   - associatedIndicators
                   - attributes
                   - securityLabels
                   - sightings
                   - tags
                   - threatAssess
               initial_interval: 168h
               secret_key: 1$$2$$$$3$$$$$$4
               want_more: false
             tags:
               - forwarded
               - threatconnect-indicator
         type: cel
         use_output: default
   

   Examine the corresponding components/cel-default/beat-rendered-config.yml file.

   inputs:
       - config_version: 2
         data_stream:
           dataset: ti_threatconnect.indicator
           type: logs
         fields:
           _conf:
               ioc_expiration_duration: 90d
         fields_under_root: true
         id: cel-ti_threatconnect.indicator-dd614c67-bb84-47de-b23e-93c5f3ce6753
         index: logs-ti_threatconnect.indicator-default
         interval: 24h
         processors:
           - add_fields:
               fields:
                   input_id: cel-threatconnect-dd614c67-bb84-47de-b23e-93c5f3ce6753
               target: '@metadata'
           - add_fields:
               fields:
                   dataset: ti_threatconnect.indicator
                   namespace: default
                   type: logs
               target: data_stream
           - add_fields:
               fields:
                   dataset: ti_threatconnect.indicator
               target: event
           - add_fields:
               fields:
                   stream_id: cel-ti_threatconnect.indicator-dd614c67-bb84-47de-b23e-93c5f3ce6753
               target: '@metadata'
           - add_fields:
               fields:
                   id: 07d963cd-5e9a-4ef1-a3b7-f4c3776c0389
                   snapshot: false
                   version: 8.13.0
               target: elastic_agent
           - add_fields:
               fields:
                   id: 07d963cd-5e9a-4ef1-a3b7-f4c3776c0389
               target: agent
         program: |
   <snip>
         publisher_pipeline:
           disable_host: true
         redact:
           fields:
               - secret_key
         resource:
           ssl: null
           timeout: 2m
           url: https://example.com
         state:
           access_id: user
           batch: 5000
           counter: 0
           event_list:
               - associatedGroups
               - associatedIndicators
               - attributes
               - securityLabels
               - sightings
               - tags
               - threatAssess
           initial_interval: 168h
           secret_key: 1$2$$3$$$4
           want_more: false
         tags:
           - forwarded
           - threatconnect-indicator
         type: cel
   

   Note that the secret_key has had each of the $$ resolved to $. This results in failure to authenticate to the API. This is confusing for customers who have values that include these sequences.

   The change is a result of variable expansion when the beats config is rendered and would also be expected to result in changes if the secret (or other field) included syntax that would be expected to expand, for example ${NOT_A_VAR}.
   
   This results in no error, but also no component being run
   
   and no components in the diags.
   
   The input shows up in the pre-config.yaml, but nothing is present in the computed-config.yaml.

   agent:
       download:
           sourceURI: https://artifacts.elastic.co/downloads/
       features: null
       monitoring:
           enabled: false
           logs: false
           metrics: false
       protection:
           enabled: false
           signing_key: <REDACTED>
           uninstall_token_hash: <REDACTED>
   fleet:
       hosts:
           - https://fleet-server:8220
   host:
       id: 8259e024976a406e8a54cdbffeb84fec
   id: elastic-agent-managed-ep
   inputs: []
   output_permissions:
       default:
           _elastic_agent_checks:
               cluster:
                   - monitor
           _elastic_agent_monitoring:
               indices: []
           642d500a-11a1-4f61-b28b-e8bd927a7c1f:
               indices:
                   - names:
                       - logs-ti_threatconnect.indicator-default
                     privileges:
                       - auto_configure
                       - create_doc
   outputs:
   <snip>
   

Related issues: elastic/elastic-agent#2177 elastic/elastic-agent#2261 elastic/beats#35260

[elasticmachine]

Pinging @elastic/elastic-agent (Team:Elastic-Agent)

[cmacknz]

Possibly related:

• No way to escape variables in transpiler parsing elastic-agent#2177
• Escape ${foo} on strings when configuration is received from Elastic-Agent beats#35260

[elasticmachine]

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

[cmacknz]

A simpler reproduction using just the output section of the policy:

outputs:
  default:
    type: elasticsearch
    hosts: [127.0.0.1:9200]
    api_key: "example-key"
    #username: "elastic"
    #password: "changeme"
    preset: balanced
    dollar_signs: $$$$

Gets transformed into the following in the output of elastic-agent inspect:

outputs:
  default:
    api_key: example-key
    hosts:
    - 127.0.0.1:9200
    preset: balanced
    dollar_signs: $$
    type: elasticsearch

Edit: switched use of secret_key to dollar_signs so it isn't redacted in diagnostics.

[cmacknz]

I had thought the transpiler and variable substation was to blame here but I think these are getting eaten by go-ucfg.

[cmacknz]

Reproduction isolated to a unit test:

diff --git a/internal/pkg/config/config_test.go b/internal/pkg/config/config_test.go
index 5d393d3486..0c02ea9079 100644
--- a/internal/pkg/config/config_test.go
+++ b/internal/pkg/config/config_test.go
@@ -20,6 +20,19 @@ func TestConfig(t *testing.T) {
        testLoadFiles(t)
 }

+func TestToMapStrWithDollarSigns(t *testing.T) {
+       in := map[string]interface{}{
+               "hello": map[string]interface{}{
+                       "what": "$$$$",
+               },
+       }
+
+       c := MustNewConfigFrom(in)
+       out, err := c.ToMapStr()
+       assert.NoError(t, err)
+       assert.Equal(t, in, out)
+}
+
 func TestInputsResolveNOOP(t *testing.T) {
        contents := map[string]interface{}{
                "outputs": map[string]interface{}{
(END)

Output:

❯ gotest ./internal/pkg/config/...
?       github.com/elastic/elastic-agent/internal/pkg/config/operations [no test files]
--- FAIL: TestToMapStrWithDollarSigns (0.00s)
    config_test.go:33:
                Error Trace:    /Users/cmackenzie/go/src/github.com/elastic/elastic-agent/internal/pkg/config/config_test.go:33
                Error:          Not equal:
                                expected: map[string]interface {}{"hello":map[string]interface {}{"what":"$$$$"}}
                                actual  : map[string]interface {}{"hello":map[string]interface {}{"what":"$$"}}

                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -2,3 +2,3 @@
                                  (string) (len=5) "hello": (map[string]interface {}) (len=1) {
                                -  (string) (len=4) "what": (string) (len=4) "$$$$"
                                +  (string) (len=4) "what": (string) (len=2) "$$"
                                  }
                Test:           TestToMapStrWithDollarSigns
FAIL
FAIL    github.com/elastic/elastic-agent/internal/pkg/config    0.206s
FAIL

[cmacknz]

Caused by go-ucfg variable expansion I believe, the following change makes the test above pass:

index b9524eafd1..85020664c1 100644
--- a/internal/pkg/config/config.go
+++ b/internal/pkg/config/config.go
@@ -36,7 +36,7 @@ func VarSkipKeys(keys ...string) Option {
 var DefaultOptions = []interface{}{
        ucfg.PathSep("."),
        ucfg.ResolveEnv,
-       ucfg.VarExp,
+       // ucfg.VarExp,
        VarSkipKeys("inputs"),
        ucfg.IgnoreCommas,
 }
(END)

[cmacknz]

This PR in go-ucfg seems to have introduced this behavior. https://github.com/elastic/go-ucfg/pull/120/files

I suspect this is a bug in the go-ucfg lexer which appears to be hand written.

[cmacknz]

I see a test case in https://github.com/elastic/go-ucfg/blob/5edd2c2d96ddaa8e5ba1b5fd48fd1ebe6ce25432/variables_test.go#L27-L46 that would break if we simply removed this.

{"string with escaped var", "escaped $${var}", str("escaped ${var}")},

I don't want to over focus on $$ sequences as the problem because realistically a secret value like ${@!} or other patterns that look like they could be variables should not be interpreted as a variable at all either.

Really what needs to happen is all secret values need to be treated as literal strings with none of go-ucfg's processing applied.

[cmacknz]

Something previously proposed is a dedicated secrets provider so the values only show up after variable substitution:

outputs:
  default:
    type: elasticsearch
    api_key: ${secrets.<uuid>}

providers:
  secrets:
    <uuid>: <value>

inputs:
  - id: 'asdf'
    type: httpjson
    headers:
      Authorization: Bearer ${secrets.<uuid>}

Having a secret provider like this would be a way around this, where we need a way to escape ${} and $$ and other combinations in the agent policy. The biggest con is this requires Fleet to generate a different agent policy dependent on the agent version. It also does nothing to help elastic/elastic-agent#2177.

[cmacknz]

To summarize the problem:

1. It is valid for the value of a secret to come from a variable, for example an environment variable like ${env.my_secret}.
2. It is valid for secrets to contain randomly generated strings that could look like a variable, but should not be substituted and no characters should not be removed.

We need to add proper escape sequences for each special character in our configuration parser and variable substitution to allow the second case.

[cmacknz]

If the solution does turn out to be adding an escape syntax, older agent versions aren't going to support it and we need to account for that in the implementation. There will need to be a companion issue for Fleet to implement escaping (or a secrets provider per above, or whatever else we may decide on).

[blakerouse]

providers:
  secrets:
    <uuid>: <value>

inputs:
  - id: 'asdf'
    type: httpjson
    headers:
      Authorization: Bearer ${secrets.<uuid>}

This is already supported on all versions of Elastic Agent (I bet you didn't know that??). Its just not called secrets, its called local.

providers:
  local:
    <uuid>: <value>

inputs:
  - id: 'asdf'
    type: httpjson
    headers:
      Authorization: Bearer ${local.<uuid>}

[michel-laterman]

Caused by go-ucfg variable expansion I believe, the following change makes the test above pass:
...

I added a test case to see what happens if we nested a value that contains $$ somewhere under inputs:
https://github.com/elastic/elastic-agent/pull/5097/files#diff-0c3e3dddfb4bb7bd65b3f673ba04e2c6ac89d6b4d45f0ba63b1e7bc5963aef78R152-R160
and it works correctly, it's not altered by go-ucfg like a non-inputs value in the top level case is

I think the agent is not unpacking/rendering that part of the input with the DefaultOptions, or it's just passing the inputs array through

[michel-laterman]

I think this may be caused by the fleet-ui.

If I create an example policy with only the custom logs integration (collecting /var/log/example), and add

custom.key: $$$$

under custom configuration:

The policy will render the input as

inputs:
  - id: logfile-logs-802a2527-fc49-496e-b7ba-349c9024e5a4
    name: log-test
    revision: 1
    type: logfile
    use_output: default
    meta:
      package:
        name: log
        version: 2.3.1
    data_stream:
      namespace: default
    package_policy_id: 802a2527-fc49-496e-b7ba-349c9024e5a4
    streams:
      - id: logfile-log.logs-802a2527-fc49-496e-b7ba-349c9024e5a4
        data_stream:
          dataset: generic
        paths:
          - /var/log/example
        ignore_older: 72h
        custom.key: $$

cc @kpollich

[kpollich]

The API request to create the policy contains the raw $$$$ text, but then it appears during Fleet's policy compilation the dollar signs are escaped, as the resulting saved object contains $$ instead:

When the code gets into compileTemplate (

kibana/x-pack/plugins/fleet/server/services/epm/agent/agent.ts

Lines 19 to 48 in b3dcc54

	export function compileTemplate(variables: PackagePolicyConfigRecord, templateStr: string) {
	const logger = appContextService.getLogger();
	const { vars, yamlValues } = buildTemplateVariables(logger, variables);
	let compiledTemplate: string;
	try {
	const template = handlebars.compile(templateStr, { noEscape: true });
	compiledTemplate = template(vars);
	} catch (err) {
	throw new PackageInvalidArchiveError(`Error while compiling agent template: ${err.message}`);
	}
	compiledTemplate = replaceRootLevelYamlVariables(yamlValues, compiledTemplate);
	const yamlFromCompiledTemplate = safeLoad(compiledTemplate, {});
	// Hack to keep empty string ('') values around in the end yaml because
	// `safeLoad` replaces empty strings with null
	const patchedYamlFromCompiledTemplate = Object.entries(yamlFromCompiledTemplate).reduce(
	(acc, [key, value]) => {
	if (value === null && typeof vars[key] === 'string' && vars[key].trim() === '') {
	acc[key] = '';
	} else {
	acc[key] = value;
	}
	return acc;
	},
	{} as { [k: string]: any }
	);
	return replaceVariablesInYaml(yamlValues, patchedYamlFromCompiledTemplate);
	}

) we have these values:

Diving deeper into the code it looks like the culprit is the safeLoad method from js-yaml here:

kibana/x-pack/plugins/fleet/server/services/epm/agent/agent.ts

Line 31 in b3dcc54

const yamlFromCompiledTemplate = safeLoad(compiledTemplate, {});

This method seems to escape $ such that $$ is compiled to $ in the resulting string.

I checked first if the safeDump method we call on the variable contents does any escaping and that doesn't seem to be the case. I think a remediation might be to update js-yaml and move to using load and dump which are safe-by-default in newer versions of the library and don't have the same escaping concerns.

{ key: '##processors##', val: null, safeDumpVersion: 'null\n' }
{
  key: '##custom##',
  val: { foo: '$$$$$' },
  safeDumpVersion: 'foo: $$$$$\n'
}
{
  paths: [ '/vas' ],
  ignore_older: '72h',
  data_stream: { dataset: 'generic' },
  foo: '$$$'
}

[kpollich]

This can probably move over to the UI team and we can pick it up from here if that makes sense to you @michel-laterman

cc @ycombinator

[ycombinator]

Thanks @michel-laterman and @kpollich for your investigations. Transferred issue to Kibana repo.

[michel-laterman]

Yes makes sense, do the same methods get called when a secret is specified as well?

[ycombinator]

@kpollich I'm re-assigning this issue to you for the moment so you can re-assign to the appropriate UI engineer.

[nimarezainia]

is safeload called anywhere else that might cause similar issues?