Mask password discovered via module autodiscover hint

[ycombinator]

Follow up to #15349.

With this PR any string representations of a Metricbeat module configuration constructed by autodiscovery will mask the password. For instance, this will prevent the password from being output verbatim in Metricbeat logs.

Manual Testing

1. Enable autodiscover in metricbeat.yml

metricbeat:
  autodiscover.providers:
    - type: docker
      hints.enabled: true

2. Start metricbeat with log enabled: ./metricbeat -e -d "hints.builder"

3. docker run -l co.elastic.metrics/module=nats -l co.elastic.metrics/username=user42 -l co.elastic.metrics/password=SUPERSECRET --name nats nats

4. Check in Metricbeat logs for :

   2019-12-16T15:53:18.712+0200	DEBUG	[hints.builder]	hints/metrics.go:144	generated config: {"enabled":true,"hosts":"xxxxx","metricsets":["connections","routes","stats","subscriptions"],"module":"nats","password":"xxxxx","period":"1m","processors":null,"ssl":{},"timeout":"3s","username":"user42"}
   

   Specifically make sure the password field is masked in the generated config that's emitted in the log.

[ycombinator]

jenkins, test this

[ChrsMark]

It looks good to me! This handles better the situation on our side since we know that we received a password setting and we mask it in our logs.

However we should have in mind that when the Docker event arrives and is logged then the password will be displayed again in the container's labels.

[exekias]

I wonder if we should forbid this kind of configuration, by telling users they must explicitly use some reference to an env var, docker labels / kubernetes annotations are not well suited for storing password

[exekias]

btw, they could input passwords already by using the raw hint

[urso]

check /libbeat/common/config.go (DebugString, PrintDebugf). There we have a blacklist of keys we don't want to show.

[ycombinator]

Updated code to use common.DebugString.

[ycombinator]

I wonder if we should forbid this kind of configuration, by telling users they must explicitly use some reference to an env var, docker labels / kubernetes annotations are not well suited for storing password

I think we are doing that here: https://github.com/elastic/beats/blob/master/metricbeat/docs/autodiscover-hints.asciidoc#coelasticmetricspassword. Were you thinking of some other way?

[urso]

Hm, it's a pitty to loose this debug string. The object is created dynamically. If something goes wrong in NewConfigFrom one needs this debug output in order to tell what the input was.

MapStr has a few serialization methods: String, StringToPrint, and MarshalLogObject (used by zap loggers in case MapStr is passed directly). I wonder if we should: a) always apply the blacklist in these methods (there might be other places we write this debug string), or b) provide a special function/mapping serializing MapStr to a string.

We can also add a 'mixture' of a) and b) by implementing a fmt.Formatter. E.g. if %v or %s is given we apply the blacklist. But if the + or # flag is passed we do not apply the blacklist. For MarshalLogObject I would always prefer to apply the blacklist, as this is part of the logger.

@ycombinator @exekias WDYT?

[ycombinator]

We can also add a 'mixture' of a) and b) by implementing a fmt.Formatter. E.g. if %v or %s is given we apply the blacklist. But if the + or # flag is passed we do not apply the blacklist. For MarshalLogObject I would always prefer to apply the blacklist, as this is part of the logger.

I like this option the best.

[ycombinator]

@urso I've implemented your suggestion and done a bit of related minor cleanup as well. Please re-review when you get a chance. Thanks!

[ycombinator]

Chatting with @urso off-PR, we decided to make a follow up issue to try and reduce the memory allocations introduced in this PR due to cloning of the common.MapStr object: #15653.

[ycombinator]

CI failures are unrelated and failing on master as well. Merging.

[pySilver]

@ycombinator sorry to bother you here, docs says

co.elastic.metrics/password
The password to use for authentication. It is recommended to retrieve this sensitive information from an ENV variable or a keystore and avoid placing passwords in plain text

How exactly it works? How do you distinguish if that ENV variable and not a password? Should this ENV variable has to be defined in metricbeat environment or observed docker container environment? Should it be denoted somehow so system knows it's an ENV reference?

I've added the following label to my container:

co.elastic.metrics/password: ELASTIC_PASSWORD and defined ELASTIC_PASSWORD env at metricbeat host (docker container) but it doesn't work this way (401).

[ChrsMark]

@pySilver hi! It should be like co.elastic.metrics/password: ${ELASTIC_PASSWORD}.

See more on #12597 about how to leverage local keystore
and #18096 for leveraging k8s secrets. These are quite fresh and would be awsome if you want to give them a shot :).

[pySilver]

@ChrsMark thank you! This can be a valuable addition to the docs :)

Unfortunately (fortunately?) I'm using Nomad/Vault/Consul instead of k8s so I cannot provide any feedback on k8s secrets.