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

WIP adopting manifests and signing classes #260

Closed
wants to merge 1 commit into from

Conversation

susperius
Copy link
Contributor

@susperius susperius commented Jul 26, 2024

WIP do not merge

This is my first draft of adopting the new signing and manifest primitives for actual signature creation and serialization to disk. For now the idea is to use BytesSigner to sign the contents of the manifest payload and provide verification material. That can then be stored on disk together with the signed payload as a tuple of (SigstoreBundle, Manifest).

@mihaimaruseac PLMK what you think about this approach

Signed-off-by: Martin Sablotny <msablotny@nvidia.com>
@susperius susperius requested review from a team as code owners July 26, 2024 17:20
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

Sorry, just saw this.

I was working on this area too, as said via chat. I'll push my PR shortly, once I clean it. But I think we can go directly to in-toto now, we don't need to replicate the manifest once more

"""BytesSigner is the abstract base class for signing bytes."""

@abc.abstractmethod
def sign(self, data: bytes) -> bytes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the same as signing.Signer. I'm curious why we'd duplicate the class hierarchy instead of using it?

"""


class BytesVerifier(metaclass=abc.ABCMeta):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, w.r.t signing.Verifier

"""Generic verification material"""


class SigstoreVerificationMaterial(VerificationMaterial):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can this be just a subclass of `signing.Signature, which will just wrap the bundle?

Comment on lines +39 to +42
descriptors = []
for d in manifest.resource_descriptors:
descriptors.append(d)
return cls(descriptors)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm unsure about this. We're just building a dictionary out of items fed from another dictionary. And we also lose the ordering guarantee.

Why not make the payload directly be in-toto here?

@mihaimaruseac
Copy link
Collaborator

If you want to see the entire work to convert manifests to in-toto, #267 (I split it into a series of PRs with various in-toto payloads to ease reviews - every PR should just look at the last commit).

@susperius
Copy link
Contributor Author

Sorry, just saw this.

I was working on this area too, as said via chat. I'll push my PR shortly, once I clean it. But I think we can go directly to in-toto now, we don't need to replicate the manifest once more

ok, I'll leave it at that and stop trying to contribute for now.

@susperius susperius closed this Jul 27, 2024
@mihaimaruseac
Copy link
Collaborator

That wasn't my intention :(

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.

2 participants