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

Introduce SignedArtifactBundle #171

Merged
merged 2 commits into from
Dec 14, 2022
Merged

Introduce SignedArtifactBundle #171

merged 2 commits into from
Dec 14, 2022

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Nov 25, 2022

Summary

This commit adds a new struct named SignedArtifactBundle which represents a signed artifact bundle as produced when signing with cosign and specifying the --bundle option.

The motivation for change comes from trying to implement an example that verifies a blob using a bundle.
For example, first a blob is signed using the following command:

cosign sign-blob --bundle=artifact.bundle artifact.txt

The artifact.bundle file generated by the above command will look something like this (shortened to fit the commit message format):

{
  "base64Signature": "...",
  "cert": "...",
  "rekorBundle": {
    "SignedEntryTimestamp": "...",
    "Payload": {
      "body": "...",
      "integratedTime": 1669361833,
      "logIndex": 7810348,
      "logID": "..."
    }
  }
}

Currently, to create Bundle from this, one would have to parse the string as json, and then access the rekorBundle element, and then serialize it so that it can be passed to Bundle::new_verified.

With the changes in this commit it will be possible to call SignedArtifactBundle::new_verified and pass in the contents for the bundle file directly.

Refs: #117
Signed-off-by: Daniel Bevenius [email protected]

Release Note

NONE

Documentation

NONE

src/cosign/bundle.rs Outdated Show resolved Hide resolved
viccuad
viccuad previously approved these changes Dec 5, 2022
Copy link
Collaborator

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! looks good to me, modulo the serde nitpick.

viccuad
viccuad previously approved these changes Dec 5, 2022
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

I have nothing against adding this new struct, but I'm a bit reluctant to rename the current Bundle struct. This would break our public API, which is not a taboo, but should be pondered.

Why don't we create instead a new struct called SignedArtifactBundle? The bundle you're referencing is created only by the sign-artifact command of cosign.
I think this approach would be more clear about what this new struct is about and avoid the API break.

What do you think?

PS: regardless of that, the new struct should have some inline documentation

@viccuad
Copy link
Collaborator

viccuad commented Dec 6, 2022

Indeed, this is a breaking change for SignatureLayer{} if there's a name change. I am also happy with SignedArtifactBundle.

@danbev
Copy link
Contributor Author

danbev commented Dec 6, 2022

I have nothing against adding this new struct, but I'm a bit reluctant to rename the current Bundle struct. This would break our public API, which is not a taboo, but should be pondered.

Why don't we create instead a new struct called SignedArtifactBundle? The bundle you're referencing is created only by the sign-artifact command of cosign. I think this approach would be more clear about what this new struct is about and avoid the API break.

What do you think?

Sounds good, I'll take a look at this and also add some documentation 👍

@flavio
Copy link
Member

flavio commented Dec 6, 2022

Thanks @danbev! 🙏

This commit adds a new struct named SignedArtifactBundle which
represents a signed artifact bundle as produced when signing with cosign
and specifying the --bundle option.

The motivation for change comes from trying to implement an example
that verifies a blob using a bundle. For example, first a blob is
signed using the following command:

cosign sign-blob --bundle=artifact.bundle artifact.txt

The `artifact.bundle` file generated by the above command will look
something like this (shortened to fit the commit message format):

{
  "base64Signature": "...",
  "cert": "...",
  "rekorBundle": {
    "SignedEntryTimestamp": "...",
    "Payload": {
      "body": "...",
      "integratedTime": 1669361833,
      "logIndex": 7810348,
      "logID": "..."
    }
  }
}

Currently, to create Bundle from this, one would have to parse the
string as json, and then access the `rekorBundle` element, and then
serialize it so that it can be passed to `Bundle::new_verified`.

With the changes in this commit it will be possible to call
`SignedArtifactBundle::new_verified` and pass in the contents for the
bundle file directly.

Refs: sigstore#117

Signed-off-by: Daniel Bevenius <[email protected]>
@danbev danbev changed the title Rename Bundle to RekorBundle and modify Bundle Introduce SignedArtifactBundle Dec 8, 2022
Copy link
Member

@flavio flavio left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes. I just left some minor comments

src/cosign/bundle.rs Show resolved Hide resolved
src/cosign/bundle.rs Show resolved Hide resolved
Add inline documentation.

Signed-off-by: Daniel Bevenius <[email protected]>
@flavio flavio merged commit bd866a9 into sigstore:main Dec 14, 2022
@flavio
Copy link
Member

flavio commented Dec 14, 2022

@danbev thanks for the submission!

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.

3 participants