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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subject names for attestations #3070

Merged
merged 4 commits into from
Sep 26, 2022
Merged

Conversation

jedevc
Copy link
Member

@jedevc jedevc commented Aug 30, 2022

Follow up to #2935.

From conversation with @tonistiigi:

thinking about this bit more, I think we should put the repo name here. We should at least have something that says that the digest points to an image (when we add attestation to local output then it points to a file and not an image)

SGTM 馃憤 - this makes sense from the buildkit side, since pushed images always have a name.

@jedevc jedevc requested a review from tonistiigi August 30, 2022 10:34
}
for _, name := range names {
statements[i].Subject = append(statements[i].Subject, intoto.Subject{
Name: name,
Copy link
Member

Choose a reason for hiding this comment

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

Should this be converted to https://github.com/package-url/purl-spec ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe. I'm not entirely sure, in-toto doesn't require it, so it's up to us. I do think we want to make sure that the platform is present in the name though (to ensure uniqueness), since then we'll have a 1-to-1 relationship between digests and names.

So maybe PURL would be the right choice here, with the version field containing the image tag, and the image repository location and architecture/os data in the qualifiers? Not sure whether we should use the sha256: in the name though, since that would be duplicated.

Copy link
Member Author

Choose a reason for hiding this comment

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

Have cherry-picked your PURL util package (with a fixup to use reference for the distribution reference package, like we have in the other parts of the codebase).

@jedevc jedevc force-pushed the attestation-subject-name branch 2 times, most recently from f4726f9 to cacd12d Compare September 1, 2022 11:32
@@ -4,8 +4,8 @@ import (
"strings"

"github.com/containerd/containerd/platforms"
distreference "github.com/docker/distribution/reference"
"github.com/opencontainers/go-digest"
"github.com/docker/distribution/reference"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that makes sense, I think we should make that change everywhere then at some point, instead of just here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Happy to do that in a follow-up, if this is a blocker 馃憤

tonistiigi and others added 3 commits September 23, 2022 21:54
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
Use convention of importing docker/distribution/reference as reference.

Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Justin Chadwell <me@jedevc.com>
@tonistiigi tonistiigi force-pushed the attestation-subject-name branch 2 times, most recently from 2684b24 to 406f036 Compare September 24, 2022 05:33
Signed-off-by: Justin Chadwell <me@jedevc.com>
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
@jedevc
Copy link
Member Author

jedevc commented Sep 26, 2022

@tonistiigi LGTM - good catch on OSVersion 馃帀

@tonistiigi tonistiigi merged commit 47e953b into moby:master Sep 26, 2022
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.

None yet

2 participants