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.

Sign up for GitHub

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

Jump to bottom

🐛 Signed-Releases - Check releases for signatures even if they only have source code #2186

Closed
wants to merge 5 commits into from
Closed

🐛 Signed-Releases - Check releases for signatures even if they only have source code #2186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d5925fd
Change uninitialized check for Assets
raghavkaul Aug 22, 2022
a718de0
Update zero-value check with tarball/zipfile URL
raghavkaul Aug 22, 2022
98cd871
Add tests
raghavkaul Aug 22, 2022
f55fd47
Fix lint
raghavkaul Aug 22, 2022
c26dcd9
Add logging
raghavkaul Aug 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions checks/evaluation/signed_releases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ var (

const releaseLookBack = 5

//SignedReleases applies the score policy for the Signed-Releases check.
// SignedReleases applies the score policy for the Signed-Releases check.
//nolint
func SignedReleases(name string, dl checker.DetailLogger, r *checker.SignedReleasesData) checker.CheckResult {
if r == nil {
Expand All @@ -42,7 +42,10 @@ 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 {
raghavkaul marked this conversation as resolved.
Show resolved Hide resolved
dl.Warn(&checker.LogMessage{
Text: fmt.Sprintf("Couldn't find assets for release %s at url %s", release.TagName, release.URL),
})
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there anyway to sign or provide provenance for source code? @laurentsimon fyi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@laurentsimon laurentsimon Aug 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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:

  1. Skip these releases and return an -1
  2. Treat releases with source code only as signed and return a score of 10.
  3. 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

+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?

}

Expand Down Expand Up @@ -127,6 +130,6 @@ func SignedReleases(name string, dl checker.DetailLogger, r *checker.SignedRelea
}

score = int(math.Floor(float64(score) / float64(totalReleases)))
reason := fmt.Sprintf("%d out of %d artifacts are signed or have provenance", total, totalReleases)
reason := fmt.Sprintf("%d out of %d most recent artifacts are signed or have provenance", total, totalReleases)
return checker.CreateResultWithScore(name, reason, score)
}
49 changes: 49 additions & 0 deletions checks/signed_releases_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,6 +371,55 @@ func TestSignedRelease(t *testing.T) {
Error: errors.New("Error getting releases"),
},
},
{
name: "Releases with no assets",
releases: []clients.Release{
{
TagName: "v1.0.0",
URL: "http://foo.com/v1.0.0",
TargetCommitish: "master",
Assets: []clients.ReleaseAsset{},
TarballURL: "http://foo.com/asset.tar.gz",
ZipballURL: "http://foo.com/asset.zip",
},
},
expected: checker.CheckResult{
Score: 0,
},
},
{
name: "Some releases with no assets",
releases: []clients.Release{
{
TagName: "v1.0.0",
URL: "http://foo.com/v1.0.0",
TargetCommitish: "master",
Assets: []clients.ReleaseAsset{},
TarballURL: "http://foo.com/asset.tar.gz",
ZipballURL: "http://foo.com/asset.zip",
},
{
TagName: "v1.0.0",
URL: "http://foo.com/v1.0.0",
TargetCommitish: "master",
Assets: []clients.ReleaseAsset{
{
Name: "foo.sig",
URL: "http://foo.com/v2.0.0/foo.sig",
},
{
Name: "foo.txt",
URL: "http://foo.com/v1.0.0/foo.txt",
},
},
TarballURL: "http://foo.com",
ZipballURL: "http://foo.com/asset.zip",
},
},
expected: checker.CheckResult{
Score: 4,
},
},
}

for _, tt := range tests {
Expand Down
2 changes: 2 additions & 0 deletions clients/githubrepo/releases.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ func releasesFrom(data []*github.RepositoryRelease) []clients.Release {
TagName: r.GetTagName(),
URL: r.GetURL(),
TargetCommitish: r.GetTargetCommitish(),
ZipballURL: r.GetZipballURL(),
TarballURL: r.GetTarballURL(),
}
for _, a := range r.Assets {
release.Assets = append(release.Assets, clients.ReleaseAsset{
Expand Down
2 changes: 2 additions & 0 deletions clients/release.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ type Release struct {
TagName string
URL string
TargetCommitish string
ZipballURL string
TarballURL string
Assets []ReleaseAsset
}

Expand Down