agent icon indicating copy to clipboard operation
agent copied to clipboard

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

Open goodspark opened this issue 1 year ago • 3 comments

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.

goodspark avatar Mar 18 '23 05:03 goodspark

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.

triarius avatar Apr 06 '23 03:04 triarius

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

  • Always redact variables based on the configured values
  • But only warn about them once, when they're encountered in a hook
  • If a previously redacted env var changed in a subsequent hook or command, continue redacting it but still obey the warn-once rule

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

As a bonus:

  • Only warn once, at the end of the job logs.

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.

goodspark avatar Apr 06 '23 04:04 goodspark

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?

triarius avatar Jul 05 '23 01:07 triarius