🐛 Signed-Releases - Check releases for signatures even if they only have source code #2186
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2186 +/- ##
==========================================
- Coverage 42.28% 42.25% -0.03%
==========================================
Files 95 95
Lines 7871 7876 +5
==========================================
Hits 3328 3328
- Misses 4283 4288 +5
Partials 260 260 |
|
Integration tests success for |
| @@ -42,7 +42,7 @@ func SignedReleases(name string, dl checker.DetailLogger, r *checker.SignedRelea | |||
| total := 0 | |||
| score := 0 | |||
| for _, release := range r.Releases { | |||
| if len(release.Assets) == 0 { | |||
| if release.ZipballURL == "" && release.TarballURL == "" && len(release.Assets) == 0 { | |||
| continue | |||
There was a problem hiding this comment.
Is there anyway to sign or provide provenance for source code? @laurentsimon fyi
There was a problem hiding this comment.
I believe you can PGP-sign a tarball or zipfile (we already check for the .asc file extension, including .tar.gz.asc). However we could expand the set of signatureExtensions and provenanceExtensions if there are any tarball or zip-specific formats that we don't already have.
There was a problem hiding this comment.
I suppose it's possible to sign too. The 2 default assets are generated by GitHub and cannot be updated by maintainers. So signing them does not improve security by much, unless we distrust GH. This is very different from the threat model on other assets that can be altered by maintainers.
There was a problem hiding this comment.
Any update? I'm not entirely sold on the benefit of this change. If there are no assets in the GH releases, it'd be fine to return a -1. Forcing signature that adds little to no security guarantees feels like asking users to do something that's not necessary and a burden.
Happy to chat more.
There was a problem hiding this comment.
The benefit of signing releases is that someone can be sure an artifact comes from the maintainer of a repo. It feels like a better approach would be to have a "not enough data" or "inconclusive" result (as per #1874), because there are no real artifacts in recent releases. I'm not even sure what benefit it provides to sign an artifact that's already being released through GitHub. If it were a package, Package integrity would be better captured by the Packaging check instead of this check.
In short, the way forward would be to wrap this kind of finding into #1874.
There was a problem hiding this comment.
The benefit of signing releases is that someone can be sure an artifact comes from the maintainer of a repo.
But you need to be a maintainer to release. Are you saying that the benefit is that someone takes over a GH workflow to cut a release? Even then, the code is still from the repository, ie from a maintainer.
I'm not even sure what benefit it provides to sign an artifact that's already being released through GitHub.
In transit, these artifacts can be tampered with. I agree it's not perfect, and finding the public keys to verify is something nobody does in practice. I actually proposed getting rid of signature in #1776 as well.
If it were a package, Package integrity would be better captured by the Packaging check instead of this check.
Agreed, but no ecosystem really supports this. In golang, you don't even need signatures because code is fetched directly from repo.
There was a problem hiding this comment.
This PR still fixes a bug in our code - instead of looking at the latest 5 releases with a release artifact, look for the latest 5 releases irrespective of whether it has a release artifact or not. The only question that remains is, how we should treat those. IMO, we have 3 options:
- Skip these releases and return an
-1 - Treat releases with source code only as signed and return a score of 10.
- Expect a signature even if it is source code only.
(1) should be the easiest/most acceptable fix. We can follow up separately to decide b/w (2) and (3). What do folks thinks?
There was a problem hiding this comment.
+1 on fixing the bug in the code. 1 seems fine for now.
I think we should discuss 2 and 3 more broadly in the context of what guarantees / goals this check should provide, and how it fits in the improvement of supply-chain. Today this check is not actionable due to lack of ecosystem support, and it's not "ecosystem-aware".
Wdut?
|
Integration tests success for |
raghavkaul
requested review from
justaugustus,
laurentsimon and
naveensrinivasan
as code owners
|
Integration tests success for |
|
Integration tests success for |
* GitHub releases can be created without assets * Source code tarballs/zips are attached to releases automatically and aren't considered assets * If we skip these as invalid releases, we won't realize that they aren't signed
naveensrinivasan
force-pushed
the
bugfix-2169-signedreleases
branch
from
03ad9f0
to
c26dcd9
Compare
|
Stale pull request message |
What kind of change does this PR introduce?
Fix issue where we skipped recent releases when looking for signature/provenance.
What is the current behavior?
Previously we checked for the presence of any assets. However, source code zips/tarballs aren't considered assets; they are created by default at the GitHub tag.
What is the new behavior (if this is a feature change)?**
Source code zips/tarballs
Which issue(s) this PR fixes
Fixes #2169.
Does this PR introduce a user-facing change?