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

🐛 Pinned-Dependencies continues on error #3515

Merged
merged 21 commits into from
Nov 8, 2023

Conversation

pnacht
Copy link
Contributor

@pnacht pnacht commented Sep 25, 2023

What kind of change does this PR introduce?

What is the current behavior?

Whenever Pinned-Dependencies hits a runtime error (i.e. can't detect job OS, can't parse a Dockerfile, etc), the check crashes entirely and the project gets an inconclusive score for the check.

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

If Pinned-Dependencies hits a runtime error, the "element" that caused the error (i.e. workflow job, Dockerfile) is skipped and the check progresses as well as possible.

TO-DO

At this stage of the PR, the proposed change only applies to failures to detect a job's operating system (as happens with apache/beam). The idea is for this to cover other similar cases (i.e. apache/arrow, caused by an error parsing a Dockerfile).

Also, at this stage, the failure to detect a job's operating system isn't logged anywhere. The job is simply skipped "invisibly". This is useful information that should be displayed as a warning in the check's details. However, there's currently no place to store this information since PinningDependenciesData only stores data on problematic dependencies, not on problems encountered while investigating.

The most straightforward solution I see is to modify PinningDependenciesData to also contain new SkippedWorkflowJobs and SkippedFiles fields (each a struct containing the job/filename and skip reason). The data in these fields can later be added to the logs as warnings, i.e:

Warn: myWorkflow.yaml's job 'myJob' skipped: could not determine operating system

The job/filenames can be either parsed from the error message or a new error type can be defined which also includes this metadata. A type assertion can then be used to identify such cases and extract the data from the error. (I don't know where I'd store this new error type)

However, I'm no expert in either Go or the Scorecard codebase, so would like to get feedback on this solution before implementing it.

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

I have added the same test workflow to multiple test functions in raw/pinned-dependencies-test.go. The results show that a problematic job affects non-GHA-pinning scores, but GHA-pinning is unaffected.

Notes for the reviewer

I couldn't see where to neatly add anything regarding this change to the Pinned-Deps documentation (which is somewhat high-level, without much detail on how the score is calculated). I have therefore left the docs as-is, without changes. Let me know if you wish to add a comment on such "error-handling" to the check's docs.

Which issue(s) this PR fixes

Fixes #3316.

Does this PR introduce a user-facing change?

Pinned-Dependencies now continues after encountering runtime errors

@pnacht pnacht temporarily deployed to gitlab September 25, 2023 23:06 — with GitHub Actions Inactive
@pnacht pnacht temporarily deployed to integration-test September 25, 2023 23:06 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 25, 2023

Codecov Report

Merging #3515 (5705652) into main (e16d3e3) will decrease coverage by 5.62%.
The diff coverage is 93.22%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3515      +/-   ##
==========================================
- Coverage   76.01%   70.40%   -5.62%     
==========================================
  Files         206      206              
  Lines       14003    14053      +50     
==========================================
- Hits        10645     9894     -751     
- Misses       2727     3585     +858     
+ Partials      631      574      -57     

@spencerschrock
Copy link
Member

If Pinned-Dependencies hits a runtime error, the "element" that caused the error (i.e. workflow job, Dockerfile) is skipped and the check progresses as well as possible.

I think this is fine. There's the chance of missing dangerous workflows this way, but if we log it (either in the execution, or as a detail) then hopefully we still get bug reports.

The most straightforward solution I see is to modify PinningDependenciesData

I think the simplest is to just throw in a log that mentions it, but I can see the value in having it in the details. We currently handle some parse errors just as a silent debug statement:

if rr.Msg != nil {
dl.Debug(&checker.LogMessage{
Path: rr.Location.Path,
Type: rr.Location.Type,
Offset: rr.Location.Offset,
EndOffset: rr.Location.EndOffset,
Text: *rr.Msg,
Snippet: rr.Location.Snippet,
})
continue

A type assertion can then be used to identify such cases and extract the data from the error

These days, it's usually done with errors.As

Of course the other option is to fix the root cause of #3316, instead of skipping over it. But that may be a more complicated fix. Any thoughts @laurentsimon @raghavkaul ?

@github-actions
Copy link

github-actions bot commented Oct 8, 2023

Stale pull request message

@pnacht
Copy link
Contributor Author

pnacht commented Oct 20, 2023

I think the simplest is to just throw in a log that mentions it, but I can see the value in having it in the details. We currently handle some parse errors just as a silent debug statement:

if rr.Msg != nil {
dl.Debug(&checker.LogMessage{
Path: rr.Location.Path,
Type: rr.Location.Type,
Offset: rr.Location.Offset,
EndOffset: rr.Location.EndOffset,
Text: *rr.Msg,
Snippet: rr.Location.Snippet,
})
continue

I've just pushed some commits such that the skipped steps are displayed in the details.

Currently, scorecard --repo apache/beam --checks Pinned-Dependencies [...] | jq '.checks[0]' returns:

{
  "details": null,
  "score": -1,
  "reason": "internal error: internal error: unable to determine OS for job: Build python wheels on ${{matrix.arch}} for ${{ matrix.os_python.os }}",
}

With the proposed change, the score is now correctly calculated (skipping the problematic steps), and everything that had to be skipped is logged in the details.

{
  "details": [
    "Warn: Possibly incomplete results: error parsing job operating system: .github/workflows/build_wheels.yml's job 'Build python wheels on ${{matrix.arch}} for ${{ matrix.os_python.os }}' (step 5)",
    "Warn: Possibly incomplete results: error parsing job operating system: .github/workflows/build_wheels.yml's job 'Build python wheels on ${{matrix.arch}} for ${{ matrix.os_python.os }}' (step 7)",
    "Warn: GitHub-owned GitHubAction not pinned by hash: .github/workflows/assign_milestone.yml:34: update your workflow using https://app.stepsecurity.io/secureworkflow/apache/beam/assign_milestone.yml/master?enable=pin",
    "... 1000+ other warnings ..."
  ],
  "score": 0,
  "reason": "dependency not pinned by hash detected -- score normalized to 0",
}

It was also quite straightforward to do the same for the apache/arrow error, so I've implemented that as well.

scorecard --repo apache/arrow --checks Pinned-Dependencies [...] | jq '.checks[0]' before:

Error: check runtime error: Pinned-Dependencies: internal error: error parsing shell code: ci/docker/python-wheel-windows-test-vs2017.dockerfile:1:2: "if <cond>" must be followed by "then"
2023/10/20 23:06:16 error during command execution: check runtime error: Pinned-Dependencies: internal error: error parsing shell code: ci/docker/python-wheel-windows-test-vs2017.dockerfile:1:2: "if <cond>" must be followed by "then"
{
  "details": null,
  "score": -1,
  "reason": "internal error: error parsing shell code: ci/docker/python-wheel-windows-test-vs2017.dockerfile:1:2: \"if <cond>\" must be followed by \"then\"",
}

Now:

{
  "details": [
    "Warn: Possibly incomplete results: error parsing shell code: ci/docker/python-wheel-windows-test-vs2017.dockerfile:1:2: \"if \u003ccond\u003e\" must be followed by \"then\"",
    "Warn: Possibly incomplete results: error parsing shell code: ci/docker/python-wheel-windows-vs2017.dockerfile:1:2: \"if \u003ccond\u003e\" must be followed by \"then\"",
    "Warn: GitHub-owned GitHubAction not pinned by hash: .github/workflows/archery.yml:53: update your workflow using https://app.stepsecurity.io/secureworkflow/apache/arrow/archery.yml/main?enable=pin",
    "... 300+ warnings ..."
  ],
  "score": 0,
  "reason": "dependency not pinned by hash detected -- score normalized to 0",
}

I've seen a few other places where this maybe could also be applied, but I haven't dug into them to be sure yet.

These days, it's usually done with errors.As

Thanks for the tip, I used it! (I'm still getting my golang sea legs...)

Of course the other option is to fix the root cause of #3316, instead of skipping over it. But that may be a more complicated fix. Any thoughts @laurentsimon @raghavkaul ?

The issue here isn't necessarily in Scorecard, though:

  • for apache/beam, the problem is a failure to identify a job's OS if the matrix is too complex (matrix variables that are dictionaries). That could probably be figured out within Scorecard.
  • however, for apache/arrow, the error is thrown directly by mvdan.cc/sh/v3/syntax.

@spencerschrock
Copy link
Member

With the proposed change, the score is now correctly calculated (skipping the problematic steps), and everything that had to be skipped is logged in the details.

I'd lean towards Info instead of warn? just so it doesn't end up as something in the security dashboard?

for apache/beam, the problem is a failure to identify a job's OS if the matrix is too complex (matrix variables that are dictionaries). That could probably be figured out within Scorecard.

I have a change I was playing around with, but haven't had time to finish testing.

  • however, for apache/arrow, the error is thrown directly by mvdan.cc/sh/v3/syntax.

Because we don't have a shell parser for powershell. So partially scorecard's fault

checker/raw_result.go Outdated Show resolved Hide resolved
@laurentsimon
Copy link
Contributor

If Pinned-Dependencies hits a runtime error, the "element" that caused the error (i.e. workflow job, Dockerfile) is skipped and the check progresses as well as possible.

I think this is fine. There's the chance of missing dangerous workflows this way, but if we log it (either in the execution, or as a detail) then hopefully we still get bug reports.

The most straightforward solution I see is to modify PinningDependenciesData

I think the simplest is to just throw in a log that mentions it, but I can see the value in having it in the details. We currently handle some parse errors just as a silent debug statement:

if rr.Msg != nil {
dl.Debug(&checker.LogMessage{
Path: rr.Location.Path,
Type: rr.Location.Type,
Offset: rr.Location.Offset,
EndOffset: rr.Location.EndOffset,
Text: *rr.Msg,
Snippet: rr.Location.Snippet,
})
continue

A type assertion can then be used to identify such cases and extract the data from the error

These days, it's usually done with errors.As

Of course the other option is to fix the root cause of #3316, instead of skipping over it. But that may be a more complicated fix. Any thoughts @laurentsimon @raghavkaul ?

I'm fine with logging only for now, until we have better parsing code

checker/raw_result.go Outdated Show resolved Hide resolved
checks/evaluation/pinned_dependencies_test.go Outdated Show resolved Hide resolved
checks/raw/pinned_dependencies_test.go Show resolved Hide resolved
checks/raw/pinned_dependencies_test.go Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Will store all errors handled during analysis, which may lead to incomplete results.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Dockerfile pinning is not affected.
Everything in a 'broken' Dockerfile RUN block is ignored
Everything in a 'broken' shell script is ignored
testdata/script-invalid.sh modified to demonstrate the above

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
checker/raw_result defines types used to describe analysis results.

ElementError is meant to describe potential flaws in the analysis
and is therefore a sort of analysis result itself.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
@spencerschrock
Copy link
Member

Related discussion here: #1327

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
@pnacht pnacht force-pushed the pinned-deps-continue-on-error branch from 6b6dba7 to 74f9892 Compare November 6, 2023 21:59
@pnacht pnacht requested a review from a team as a code owner November 6, 2023 21:59
Copy link
Contributor Author

@pnacht pnacht left a comment

Choose a reason for hiding this comment

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

I've just pushed a few changes.

ElementError is now in checker/raw_result, where it should've been from the start. It's .Element field is no longer a string, but a Location, making it easier to register precisely where the error occurred.

Due to this change, we also have Improved logging of the errors, now showing the specific lines and following the styling in the other details (since we're using the same plumbing).

Lastly, PinningDependenciesData.Incomplete []error has been replaced with .ProcessingErrors []ElementError. The name is better and using a struct makes more sense in this case.

I'd thought of using the error interface in case other error types come up which aren't a good fit for ElementError, but using the interface only makes sense if we actually work with the errors as errors – using .Error() to display them. However, since our logging relies on passing LogMessage structs to the logger, the string returned by .Error() isn't a good fit.

Unless we want to create our own error interface (or, more broadly, a something-to-be-logged interface) that expects a LogMessage instead of a string?

type ScorecardError /* or */ LogMessenger interface {
  Message() checker.LogMessage
}

The errors are still displayed as Info, not Error as suggested in #1327. Don't know if that change (adding a new logger function) fits in this PR or if it should come in a follow-up.

checker/raw_result.go Outdated Show resolved Hide resolved
checks/evaluation/pinned_dependencies_test.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
checker/raw_result.go Outdated Show resolved Hide resolved
checks/raw/shell_download_validate_test.go Outdated Show resolved Hide resolved
errors/public.go Outdated Show resolved Hide resolved
@spencerschrock
Copy link
Member

Please fix the unit tests too

- Replace ElementError's `Element *finding.Location`
  with `Location finding.Location`
- Rename ErrorJobOSParsing to ErrJobOSParsing to satisfy linter
- Fix unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
@spencerschrock spencerschrock merged commit 6d35c86 into ossf:main Nov 8, 2023
38 checks passed
diogoteles08 pushed a commit to diogoteles08/scorecard that referenced this pull request Nov 13, 2023
* Continue on error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests for error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError to identify elements that errored

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Incomplete field to PinningDependenciesData

Will store all errors handled during analysis, which may lead to incomplete results.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register job steps that errored out

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps are caught

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add warnings to details about incomplete steps

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps generate warnings

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register shell files skipped due to parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests showing when parser errors affect analysis

Dockerfile pinning is not affected.
Everything in a 'broken' Dockerfile RUN block is ignored
Everything in a 'broken' shell script is ignored
testdata/script-invalid.sh modified to demonstrate the above

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Incomplete results logged as Info, not Warn

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Remove `Type` from logging of incomplete results

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Update tests after rebase

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Unwrap for ElementError, improve its docs

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError case to evaluation unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Move ElementError to checker/raw_result

checker/raw_result defines types used to describe analysis results.

ElementError is meant to describe potential flaws in the analysis
and is therefore a sort of analysis result itself.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use finding.Location for ElementError.Element

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use an ElementError for script parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Replace .Incomplete []error with .ProcessingErrors []ElementError

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Adopt from reviewer comments

- Replace ElementError's `Element *finding.Location`
  with `Location finding.Location`
- Rename ErrorJobOSParsing to ErrJobOSParsing to satisfy linter
- Fix unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

---------

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
ashearin pushed a commit to kgangerlm/scorecard-gitlab that referenced this pull request Nov 13, 2023
* Continue on error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests for error detecting OS

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError to identify elements that errored

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Incomplete field to PinningDependenciesData

Will store all errors handled during analysis, which may lead to incomplete results.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register job steps that errored out

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps are caught

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add warnings to details about incomplete steps

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests that incomplete steps generate warnings

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Register shell files skipped due to parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add tests showing when parser errors affect analysis

Dockerfile pinning is not affected.
Everything in a 'broken' Dockerfile RUN block is ignored
Everything in a 'broken' shell script is ignored
testdata/script-invalid.sh modified to demonstrate the above

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Incomplete results logged as Info, not Warn

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Remove `Type` from logging of incomplete results

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Update tests after rebase

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add Unwrap for ElementError, improve its docs

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Add ElementError case to evaluation unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Move ElementError to checker/raw_result

checker/raw_result defines types used to describe analysis results.

ElementError is meant to describe potential flaws in the analysis
and is therefore a sort of analysis result itself.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use finding.Location for ElementError.Element

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Use an ElementError for script parser errors

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Replace .Incomplete []error with .ProcessingErrors []ElementError

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

* Adopt from reviewer comments

- Replace ElementError's `Element *finding.Location`
  with `Location finding.Location`
- Rename ErrorJobOSParsing to ErrJobOSParsing to satisfy linter
- Fix unit test

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

---------

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>
Signed-off-by: Allen Shearin <allen.p.shearin@gmail.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: Runtime error on Pinned-Dependencies check causes a -1 on its score
4 participants