Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
goodspark opened this issue Mar 18, 2023 · 3 comments

Comments

@goodspark
Copy link
Contributor

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
Copy link
Contributor

triarius commented Apr 6, 2023

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
Copy link
Contributor Author

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.

@triarius
Copy link
Contributor

triarius commented Jul 5, 2023

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants