search

ricoberger / script_exporter

Prometheus exporter to execute scripts and collect metrics from the output or the exit status.
MIT License
354 stars 82 forks source link

Logger dumps sensitive env vars into logs #91

Closed diversario closed 1 year ago

diversario commented 1 year ago

Hello @ricoberger !

Thank you for maintaining this great tool!

I recently discovered that the script executor dumps all environment variables into logs on errors: https://github.com/ricoberger/script_exporter/blob/a504422cc28d02ec4ff7d2380c2a7fe8cf5d16e1/pkg/exporter/scripts.go#L92

which can be very helpful for debugging, but it can also dump sensitive values such as passwords/access tokens/etc, for example if a script is something like

curl -s -u $GITHUB_USERNAME:$GITHUB_TOKEN $somegithublink

and thanks to log shipping that immediately ends up exposed to anyone with access to logs.

It would be great to (conditionally?) avoid dumping env vars to logs.

ricoberger commented 1 year ago

Hi @diversario, thanks for raising this issue.

I think the best option would be to only log the environment variables when the log level is set to debug and exclude them from the error log. What do you think?

diversario commented 1 year ago

That would be better, but that would still make it unfeasible to use debug level as it would leak secrets (and, it would make sense that one would want to run the exporter with real credentials + debug logs to figure out what's going on).

Not sure what the right fix here would be. Maybe... a blocklist of string to not log, or log but with the value redacted?.. Even a toggle to just not log env vars at all, regardless of the level, would be better, tho not as flexible.

lufik commented 1 year ago

@diversario it makes sense to me log everything in debug mode (sometimes you need to check even secrets). It's admin/user responsibility to log this debug mode to e.g. stdout and not to log shipping part.

ricoberger commented 1 year ago

Hi, this should be fixed by #93 were I added a -log.env flag which must be set to true to log the environment variables.

diversario commented 1 year ago

@diversario it makes sense to me log everything in debug mode (sometimes you need to check even secrets). It's admin/user responsibility to log this debug mode to e.g. stdout and not to log shipping part.

Log shipping picks up both stdout and stderr since otherwise it would've been incomplete.

For inspecting env of a process a workaround is to inspect its /proc/X/environ in the container.