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.

Sign up for GitHub

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

Jump to bottom

[CI:DOCS] Implement secrets/credential scanning #20136

Merged
merged 1 commit into from
Oct 30, 2023
Merged

[CI:DOCS] Implement secrets/credential scanning #20136

merged 1 commit into from
Oct 30, 2023

Conversation

cevich
Copy link
Member

@cevich cevich commented Sep 25, 2023

As an effort to catch potential secrets and/or credential leaks, add a github-actions workflow which is untouchable in a PR context. To additionally guard against accidents, also check recent branch history. This is especially important on newly created release-branches, which may begin with content from who-knows-where.

Finally, since the new workflow bypasses PR-level changes to the scanner config and base-line. Add a Cirrus-CI invocation of the scanning tool to help catch tool-breaking changes from being merged.

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 Sep 25, 2023
@cevich cevich force-pushed the credential_scanning_config branch 12 times, most recently from bcbf6e5 to 61c4d0a Compare September 26, 2023 17:16
@cevich cevich changed the title [WIP] [CI:DOCS] Implement secrets/credential scanning [CI:DOCS] Implement secrets/credential scanning Sep 26, 2023
@cevich cevich marked this pull request as ready for review September 26, 2023 17:16
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2023
@cevich cevich marked this pull request as draft September 26, 2023 17:19
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2023
@cevich cevich changed the title [CI:DOCS] Implement secrets/credential scanning [WIP] [CI:DOCS] Implement secrets/credential scanning Sep 26, 2023
@cevich cevich force-pushed the credential_scanning_config branch 3 times, most recently from 8c9713b to afc8942 Compare September 26, 2023 20:06
@cevich cevich changed the title [WIP] [CI:DOCS] Implement secrets/credential scanning [CI:DOCS] Implement secrets/credential scanning Sep 26, 2023
@cevich cevich marked this pull request as ready for review September 26, 2023 20:11
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2023
@cevich
Copy link
Member Author

cevich commented Sep 26, 2023

This is ready for initial review.

@rhatdan
Copy link
Member

rhatdan commented Sep 29, 2023

LGTM

flouthoc
flouthoc reviewed Sep 29, 2023
View reviewed changes
Copy link
Collaborator

@flouthoc flouthoc left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are bunch of CirrusCI warning in changes are they relevant ?

@flouthoc
Copy link
Collaborator

See: https://github.com/containers/podman/pull/20136/checks?check_run_id=17155277272

@cevich
Copy link
Member Author

cevich commented Sep 29, 2023

There are bunch of CirrusCI warning in changes are they relevant ?

The but their only_if conditions are different things are very minor warnings. Most of them are related to support for magic PR description strings like [CI:DOCS], [CI:BUILD], etc. Fixing them all is probably not worth the effort needed.

When the only_if vs deps. /are/ genuinely broken, the effect will be seen as artifact-passing breakage (curl 404 errors). I'm not aware of us hitting those with any frequency though, so it's probably fine.

Thanks for checking.

@cevich cevich changed the title [CI:DOCS] Implement secrets/credential scanning [WIP] [CI:DOCS] Implement secrets/credential scanning Sep 29, 2023
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2023
@cevich
Copy link
Member Author

cevich commented Sep 29, 2023

Flipping this back to [WIP] so it doesn't get merged yet. Review can continue. I need to ask an internal group some questions related to GitLeaks, before this can get merged.

@cevich cevich changed the title [WIP] [CI:DOCS] Implement secrets/credential scanning [CI:DOCS] Implement secrets/credential scanning Oct 5, 2023
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 5, 2023
@cevich
Copy link
Member Author

cevich commented Oct 5, 2023

Update: Concluded necessary internal discussion. Removed [WIP], this can be merged when appropriate (maybe not right before I go on PTO?). It covers several important security-scanning contexts not protected by other tooling.

Assuming/once this is working, I plan to make it into a reusable workflow that can be called from Buildah and Skopeo.

@cevich
Implement secrets/credential scanning
6cb1042 
As an effort to catch potential secrets and/or credential leaks, add a
github-actions workflow which is untouchable in a PR context.
To additionally guard against accidents, also check recent branch
history.  This is especially important on newly created
release-branches, which may begin with content from who-knows-where.

Finally, since the new workflow bypasses PR-level changes to the scanner
config and base-line.  Add a Cirrus-CI invocation of the scanning tool
to help catch tool-breaking changes from being merged.

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

cevich commented Oct 5, 2023

Force-push: Fixed missing recipient notification address var. Rebased on upstream.

@cevich cevich force-pushed the credential_scanning_config branch from afc8942 to 6cb1042 Compare October 5, 2023 15:17
@TomSweeneyRedHat
Copy link
Member

LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm
/hold # for @cevich to cancel the hold upon his return

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2023
@rhatdan
Copy link
Member

rhatdan commented Oct 12, 2023

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 12, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cevich, rhatdan

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 12, 2023
@cevich
Copy link
Member Author

cevich commented Oct 19, 2023

Note: Intending to merge this around Wednesday the 25th, assuming that's a good time to quickly fix any mass-PR CI breakage.

@cevich
Copy link
Member Author

cevich commented Oct 30, 2023

@containers/podman-maintainers I intended to merge this last week, but got distracted. I'm going to merge this now. I'll try to watch, but also PLMK if you notice any PRs getting hung up on secret scanning so I can fix ASAP.

@cevich cevich merged commit 1146f2c into containers:main Oct 30, 2023
43 checks passed
@cevich
Copy link
Member Author

cevich commented Oct 30, 2023 •
edited
Loading

Argh, of course it broke. Looks like Ubuntu hasn't updated podman to support --userns=keep-id yet. I'll open a PR to fix this.

@cevich
Copy link
Member Author

cevich commented Oct 30, 2023

Opened #20533

@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Jan 29, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@flouthoc flouthoc flouthoc left review comments

Assignees

@TomSweeneyRedHat TomSweeneyRedHat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cevich @rhatdan @flouthoc @TomSweeneyRedHat