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

Vastly improve secrets scanning & false-positives #21570

Closed

Conversation

cevich
Copy link
Member

@cevich cevich commented Feb 8, 2024

  • Fix new-pr secret-scan review URL JQ format
    The before jq filter was using GHA context elements in it's filter
    (.github.event) that don't actually exist in the event JSON. Removing
    them resulted in successful jq before/after commands when run against
    event JSON exported from a new-pr workflow.

  • Secret scanning: Move debugging block
    Previously, it was possible for one of several steps to throw an error,
    leaving the operator with no debugging details. Relocate the "important
    details" step earlier in the workflow to mitigate a risk of unavailable
    debugging info.

  • Wave secrets protections on select files/scripts
    The baseline gitleaks data is unfortunately commit-locked, meaning small
    changes to files due to (for example) rebases, can render them useless.
    Manually go through all findings and where possible mark lines to be
    ignored directly. In a few cases where secrets are used in tests, mark
    them to be ignored via a new .gitleaksignore file. This will
    hopefully cut way down on the number of false-positive alerts that
    require review.

Note: I intentionally did not wave checks in the .cirrus.yml file as
it's currently going through a large number of changes. I'll leave it
up to a future followup commit to mark known/approved secret references
in this file.

Does this PR introduce a user-facing change?

None

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none labels Feb 8, 2024
Copy link
Contributor

openshift-ci bot commented Feb 8, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: cevich
Once this PR has been reviewed and has the lgtm label, please assign flouthoc for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

The `before` jq filter was using GHA context elements in it's filter
(`.github.event`) that don't actually exist in the event JSON.  Removing
them resulted in successful `jq` before/after commands when run against
event JSON exported from a new-pr workflow.

[NO NEW TESTS NEEDED]

Signed-off-by: Chris Evich <[email protected]>
Previously, it was possible for one of several steps to throw an error,
leaving the operator with no debugging details.  Relocate the "important
details" step earlier in the workflow to mitigate a risk of unavailable
debugging info.

[NO NEW TESTS NEEDED]

Signed-off-by: Chris Evich <[email protected]>
The baseline gitleaks data is unfortunately commit-locked, meaning small
changes to files due to (for example) rebases, can render them useless.
Manually go through all findings and where possible mark lines to be
ignored directly.  In a few cases where secrets are used in tests, mark
them to be ignored via a new `.gitleaksignore` file.  This will
hopefully cut way down on the number of false-positive alerts that
require review.

Note: I intentionally did not wave checks in the `.cirrus.yml` file as
it's currently going through a large number of changes.  I'll leave it
up to a future followup commit to mark known/approved secret references
in this file.

[NO NEW TESTS NEEDED]

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the fix_secret_scanning_again_again branch from bd5b824 to ae7a074 Compare February 8, 2024 19:10
Copy link

Cockpit tests failed for commit ae7a074. @martinpitt, @jelly, @mvollmer please check.

@mheon
Copy link
Member

mheon commented Feb 8, 2024

Potential concern: someone could use .gitleaksignore to hide the fact that an actual leak was introduced. Maybe label PRs with changes to that file so they get extra attention?

@cevich
Copy link
Member Author

cevich commented Feb 8, 2024

someone could use .gitleaksignore to hide the fact that an actual leak was introduced.

Yes, I anticipated this. This check runs from the main tree against the subject PR tree, checking specifically for what you describe. In fact, it's why I had to add the BypassLeakNotification label, otherwise this PR would fire off the warning e-mails every time I push.

@cevich
Copy link
Member Author

cevich commented Feb 8, 2024

Case in point: The scanner for this PR will never ever go green. https://github.com/containers/podman/actions/runs/7834697427/job/21378478408?pr=21570#step:12:77

But once merged, if somebody merges into their PR, hopefully it shouldn't fire off so many damn false-positive alerts.

The other idea I had was to whitelist any PR authored by a maintainer.

If none of this works, and the false-positives continue. I'll likely give up and yank the scanner - more trouble than it's maintenance-worth.

@cevich
Copy link
Member Author

cevich commented Feb 8, 2024

Leaving this as a draft until all the activity on main slows down, then I can rebase a final time.

@Luap99
Copy link
Member

Luap99 commented Feb 9, 2024

The other idea I had was to whitelist any PR authored by a maintainer.

Yeah this is not an option IMO, having false positives for any contributor is not acceptable to me. For outside people who do not know the CI setup they get greeted with bogus errors they cannot understand and fix which is a frustrating experience, i.e. #21492

@cevich
Copy link
Member Author

cevich commented Feb 9, 2024

Worse...AFAIK there isn't a good way in github actions to determine if an action is running on a fork or upstream. At least without hard-coding it. Maybe let's just get rid of this damn thing. It's really intended to be run from a pre-commit anyway, so I guess it will be each maintainers responsibility to do that.

cevich added a commit to cevich/podman that referenced this pull request Feb 9, 2024
Ref:
containers#21570 (comment)

This tool is really intended/best used from git pre-commit on developers
local machines, to prevent addition of secret leaks.  When used as a
check against PRs, it tends to turn up more false-positives than helpful
warnings.  There's no good way to fix this, and maintaining the scanner
is an additional burden.  Rather than continue struggling to improve/fix
the situation, let's just remove the tool entirely.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Feb 9, 2024

Opened #21588

@cevich cevich closed this Feb 9, 2024
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 10, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
BypassLeakNotification do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants