-
Notifications
You must be signed in to change notification settings - Fork 11
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
Clarify OSPS-BR-01 to better express the intent #104
base: main
Are you sure you want to change the base?
Clarify OSPS-BR-01 to better express the intent #104
Conversation
Signed-off-by: Ben Cotton <[email protected]>
Commentary on the "build and release" versus "release + build in certain cases": As I think about it, beyond the concern with "does 'release and can read secret data' sufficiently cover all of the things we're trying to guard against?" that I raised in my comment on #103, I am also worried about the cognitive load that a more narrowly-carved criterion might create. I'm sympathetic to the argument David made in today's meeting about having a solid justification for adding a criterion. In this particular case, though, I worry that trying to figure out exactly where the boundary is will distract maintainers from taking the action necessary to secure their workflows. As noted on the call by someone, most builds don't get released, but my hunch is that most build workflows could lead to a release, so there's not a lot of benefit in saying "if you have a build workflow that you know will never lead to a release, you can run whatever you want on it and we don't care". Or at least the benefit of not caring doesn't outweigh the added complexity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been struggling a bit with how to validate this criteria (as writ, it seems to require avoiding security bugs in writing release software -- I assume this is aspirational, but it doesn't provide a clear "you are done" threshold for either project authors or validation software).
Ensure that the project's build and release | ||
pipelines do not execute arbitrary code | ||
provided from external sources. | ||
pipelines do not execute untrusted code | ||
provided from external sources. Maintainers | ||
may establish trust in a variety of ways, | ||
including digital signature verification and | ||
inspection of external code. For clarity, | ||
this criterion does not prohibit the use of | ||
software in a package format that executes | ||
scripts (e.g. RPM, .deb) so long as the | ||
package itself is trusted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm struggling with how to enforce this. For example, does this prohibit using a tool built and maintained in the same repository which automates some of the release steps unless we can ensure that the tool cannot be subverted? As written, it suggests every Makefile and data processor needs a security review, which seems high for maturity level 1.
Similarly, it's not clear whether this criteria establishes a transitive duty of care to audit not just your own codebase, but those of pipeline dependencies (e.g., do I need to check on untrusted code execution from actions/setup-go
in my GitHub workflows)? Or can I say "I'm using this action from $ELSEWHERE, I assume it's secure" at level 1, and have additional security criteria at level 2 or 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not establish a transitive duty of care. As an example, a workflow that uses an Ubuntu image from Canonical wouldn't violate this because it's a trusted source. A workflow that uses some-random-rebuild/ubuntu-latest
would, unless the project has established trust in the some-random-rebuild
project. The idea is more along the lines of "we're getting the code we think we're getting" as opposed to "we have inspected every line of software all the way down and it is clean".
For the first part, I would say if it's in the same repository, it's part of the project and therefore not an "external source".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example, a workflow that uses an Ubuntu image from Canonical wouldn't violate this because it's a trusted source.
But it would violate this requirement as written. The term "trusted source" isn't present in this text.
I'll try to make a change, I think the point is that build only runs code in the repo or from external trusted sources.
objective: | | ||
Reduce the risk of code injection or other | ||
security vulnerabilities in the project's | ||
build and release processes by restricting | ||
the execution of external code. | ||
the execution of external code in workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what "in workflows" here is supposed to capture compared with the older version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is to scope it specifically to build and release workflows as opposed to, say, the user execution of the program. Since it's in the "Build and Release" category, I'd accept an argument that it is redundant.
Co-authored-by: David A. Wheeler <[email protected]> Signed-off-by: Ben Cotton <[email protected]>
Co-authored-by: David A. Wheeler <[email protected]> Signed-off-by: Ben Cotton <[email protected]>
Additionally, the scorecard check is more about workflows that run on PR, rather than on project build. I think both are important, but the reference to the scorecard check here is confusing. |
This is my own attempt at #63. It differs from @david-a-wheeler's #103 by:
It does include some edits from David's PR as well.