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.