buildkite / agent

The Buildkite Agent is an open-source toolkit written in Go for securely running build jobs on any device or network
https://buildkite.com/
MIT License
809 stars 298 forks source link

Do not spam logs about skipped env var redactions for known false positives #2020

Open goodspark opened 1 year ago

goodspark commented 1 year ago

Is your feature request related to a problem? Please describe. To be overly cautious, we added *KEY* to our redacted-vars config. Soon CI job logs were spammed with:

⚠️ Warning: Value of BUILDKITE_SSH_KEYSCAN below minimum length (6 bytes) and will not be redacted

That env var will only ever be true or false and is immutable.

But carving out a bunch of possible patterns for redacted-vars is not a good experience.

Describe the solution you'd like Instead, the agent should know if an env var is known and will never contain actual secret values and never spam warning messages in logs about them.

Describe alternatives you've considered Constantly tuning redacted-vars patterns to workaround every single possibility.

triarius commented 1 year ago

Thanks for getting back to us about the usability of the redact-vars feature @goodspark. I suppose catching things like this is part and parcel of being cautious and using a wide filter like *KEY*.

Instead, the agent should know if an env var is known and will never contain actual secret values and never spam warning messages in logs about them.

Arguably, that variable shouldn't be redacted anyway. But I think it might surprise the user to extend the allowlist to cover the redaction and not just the warning. We're having a bit of an internal discussion about a maintainable and least surprising course to take here.

Keen to hear your thoughts on this matter too, though.

goodspark commented 1 year ago

Perhaps one compromise (though not the best) would be:

That's kind of annoying bc the agent needs to keep track of what was warned about and not.

As a bonus:

One of the pain points of this warning log is that it causes all the hooks' log sections to get expanded and cause CI logs to be extremely spammy. When people are trying to find out why CI failed on their PRs, seeing a bunch of yellow warning logs for false positives and a bunch of previously unseen hook logs quickly leads to log fatigue and exasperation.

triarius commented 1 year ago

Sorry, we didn't respond earlier @goodspark, but we've taken another look at this.

How about if we add a flag that allows skipping warnings for the variables that are too short to be redacted? Would that remove most of the spam for you?