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

🐛 Handle pinning by Docker URLs for GitHub actions workflows #2594

Merged
merged 5 commits into from
Jan 24, 2023

Conversation

raghavkaul
Copy link
Contributor

What kind of change does this PR introduce?

Handle Docker/other container registry URLs for GitHub actions workflows. [jobs.<job_id>.steps[*].uses](https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idstepsuses) in GitHub actions can take GitHub repos, docker containers, and local filepaths. This change supports repos and containers but not local filepaths.

What is the current behavior?

Only public action pinning was detected.

What is the new behavior (if this is a feature change)?**

Handle Docker image pinning. Stricter regex.

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #2580.

Does this PR introduce a user-facing change?

NONE

@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #2594 (3c078e4) into main (99398db) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 3c078e4 differs from pull request most recent head 70b193f. Consider uploading reports for the commit 70b193f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2594      +/-   ##
==========================================
+ Coverage   40.40%   40.45%   +0.05%     
==========================================
  Files         122      122              
  Lines        9908     9917       +9     
==========================================
+ Hits         4003     4012       +9     
  Misses       5624     5624              
  Partials      281      281              

@github-actions
Copy link

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Long regexes can get tricky. Would it be simpler to use the current one but add a fall back or an ? group for the cases that start with @sha256:?

checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

LGTM with some additional tests and length verification to the right size.

Thanks!

checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies.go Outdated Show resolved Hide resolved
@raghavkaul
Copy link
Contributor Author

raghavkaul commented Jan 11, 2023

Thanks for the review, I'll revisit/simplify the regex and add some tests.

@laurentsimon laurentsimon self-requested a review January 11, 2023 22:52
Signed-off-by: Raghav Kaul <raghavkaul@google.com>
@raghavkaul raghavkaul temporarily deployed to integration-test January 17, 2023 16:48 — with GitHub Actions Inactive
@github-actions
Copy link

@raghavkaul raghavkaul temporarily deployed to integration-test January 18, 2023 15:29 — with GitHub Actions Inactive
@github-actions
Copy link

@raghavkaul raghavkaul temporarily deployed to integration-test January 24, 2023 16:00 — with GitHub Actions Inactive
@github-actions
Copy link

Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Looks good thanks

@laurentsimon laurentsimon enabled auto-merge (squash) January 24, 2023 19:04
@laurentsimon laurentsimon temporarily deployed to integration-test January 24, 2023 19:04 — with GitHub Actions Inactive
@laurentsimon laurentsimon merged commit e6a900d into ossf:main Jan 24, 2023
@github-actions
Copy link

raghavkaul added a commit to raghavkaul/scorecard that referenced this pull request Feb 9, 2023
Signed-off-by: Raghav Kaul <raghavkaul@google.com>

Signed-off-by: Raghav Kaul <raghavkaul@google.com>
Co-authored-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Shofiya2003 pushed a commit to Shofiya2003/scorecard that referenced this pull request Mar 10, 2023
Signed-off-by: Raghav Kaul <raghavkaul@google.com>

Signed-off-by: Raghav Kaul <raghavkaul@google.com>
Co-authored-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: Shofiya2003 <shofiyabootwala@gmail.com>
Shofiya2003 pushed a commit to Shofiya2003/scorecard that referenced this pull request Mar 10, 2023
Signed-off-by: Raghav Kaul <raghavkaul@google.com>

Signed-off-by: Raghav Kaul <raghavkaul@google.com>
Co-authored-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
Signed-off-by: Shofiya2003 <shofiyabootwala@gmail.com>
raghavkaul added a commit to raghavkaul/scorecard that referenced this pull request Apr 4, 2023
Signed-off-by: Raghav Kaul <raghavkaul@google.com>

Signed-off-by: Raghav Kaul <raghavkaul@google.com>
Co-authored-by: laurentsimon <64505099+laurentsimon@users.noreply.github.com>
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

Successfully merging this pull request may close these issues.

BUG Pinned-Dependencies check does not handle direct docker GitHub Actions correctly
3 participants