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

Split unpinned-uses into two separate checks #205

Merged

Conversation

funnelfiasco
Copy link
Contributor

@funnelfiasco funnelfiasco commented Nov 26, 2024

Here's my first pass at implementing #179. The code is there and functional, AFAICT. I've tested it against a real workflow file that uses repo paths. I don't have any handy that use Docker, but if I understand the model correctly, it's very straightforward to check it and so my code will Just Work™.

I will add tests and docs before I remove the "WIP" from this, but I wanted to make sure what I've done here is generally acceptable before I put effort into that part.

Since most of the "what to do if we find an unpinned uses of either flavor" is repetitive, I tossed in a flag instead of repeating that code.

There's a lot of "not" in the new function I created in the model. It could have been written a little more cleanly without the negations, but that kept the usage the same as in the existing unpinned(), which seemed more important.

Of note, this only looks to see if the pin looks like a SHA. It does not attempt to verify that such a SHA actually exists. You probably expect that, but I wanted to be explicit about it (and I will be in the docs updates, when I create them)

Edit to add: I have also not run fmt or clippy yet, since it seemed like something that could wait until I got a thumbs up on the general direction.

Fixes #179

@funnelfiasco funnelfiasco marked this pull request as draft November 26, 2024 02:45
src/models.rs Outdated Show resolved Hide resolved
src/models.rs Outdated Show resolved Hide resolved
src/models.rs Outdated Show resolved Hide resolved
@woodruffw
Copy link
Owner

Thanks @funnelfiasco! The overall approach here LGTM; I left a couple of nits. If you want to run cargo fmt and cargo clippy internally then I can do a full review pass.

@woodruffw woodruffw added the enhancement New feature or request label Nov 26, 2024
@funnelfiasco
Copy link
Contributor Author

funnelfiasco commented Nov 26, 2024

Updated based on feedback:

  • Used is_some() for the Docker hash instead of ! .is_none()
  • Renamed new function unhashed
  • Dropped use of pedantic in favor of marking unpinned as medium severity and unhashed as low severity
  • Used ref_is_commit and dropped regex
  • Ran fmt and clippy

Todo, pending approval of these changes:

  • Docs
  • Tests
  • Squash commits

@funnelfiasco funnelfiasco force-pushed the issue179-pedantic_unpinned_checks branch from 60afb21 to c4c86d6 Compare November 28, 2024 02:41
@funnelfiasco funnelfiasco marked this pull request as ready for review November 28, 2024 02:42
@funnelfiasco funnelfiasco changed the title [WIP] Basic functionality for checking for pinned SHAs Basic functionality for checking for pinned SHAs Nov 28, 2024
@funnelfiasco
Copy link
Contributor Author

Rested, tested, and ready for review

@funnelfiasco funnelfiasco changed the title Basic functionality for checking for pinned SHAs Split unpinned-uses into two separate checks Nov 28, 2024
@woodruffw
Copy link
Owner

Thanks! I'll do a review pass tomorrow.

funnelfiasco and others added 3 commits November 28, 2024 15:15
1. A Medium-severity for entirely unpinned
2. A Low-severity for pinned to branch or tag
(since this can be a vector for a supply chain attack)

Signed-off-by: Ben Cotton <[email protected]>
This updates several tests that broke in the process

Signed-off-by: Ben Cotton <[email protected]>
@funnelfiasco funnelfiasco force-pushed the issue179-pedantic_unpinned_checks branch from c4c86d6 to 0581fb9 Compare November 28, 2024 20:16
@funnelfiasco
Copy link
Contributor Author

Updated based on feedback

Copy link
Owner

@woodruffw woodruffw left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM! I thought about it a bit more and I'm mildly worried that having the unhashed() check by default (vs. pedantic) is going to cause too much alert fatigue for most CI users. But that's not a blocker here, since this is a good impetus for me to rethink the sensitivity defaults/flags.

@woodruffw woodruffw merged commit bb71076 into woodruffw:main Nov 28, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: split unpinned-uses into regular and pedantic options
2 participants