Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
spec: add spec for sign/verify an arbitrary file using Notation #765
spec: add spec for sign/verify an arbitrary file using Notation #765
Changes from 9 commits
855f9fe
477f0bb
4c377a9
1d00480
6da89d6
a3c09cc
88fbb41
3520ec4
4d6e434
0c5c86a
0395b6a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to reword this sentence or remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File systems can be local (e.g.
ntfs
,ext4
,apfs
) or remote (cifs
) or anything. Since the signed content and the generated signatures are files, they can be on any file systems and can be on different file systems.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What will happen if I form command like
notation sign --file <target_file_path> --signature <signature_path>
, will it succeed or fail?And should we use
--blob
instead of--file
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This command will succeed.
It was actually called
--blob
, but on a second thought I changed it to--file
. The reason is that a user who wants to sign/verify a file might looking for a--file
flag instead of--blob
. In fact, they don't have to understand what ablob
is in their scenario. All they need to know is they are signing/verifying a file. Notation will take care of the rest.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signing a file might gave wrong impression that we are actually singing the file content along with its attributes but in reality we would only signing file content. Also, if we add support for passing data as stdin we will have to create a new flag, same goes for passing data as parameter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi Correct me if I'm wrong, you would like a single flag to sign arbitrary things, by things I mean it can be a file, it can be data from stdin, etc.
But I think if we want to support data from stdin, then we do need a new flag for it instead of sharing the same flag with
file
. Here was the proposal from @shizhMSFT when we brought in--oci-layout
: #601 (comment). Basically, we are following the practice ofone type per flag
here. This design aligns with what we already have.Given above, the name
--blob
becomes a bit general. In the future, if we want to support sign/verify a descriptor, then we would introduce a--descriptor
flag. If the digest of this descriptor belongs to ablob
, then there's ambiguity, user might confuse on which flag they should use,--blob
or--descriptor
.Coming back to the name
--file
, I think we can explicitly say that it sign/verify thecontent
of a file in the flag's description of the CLI. I updated the PR to reflect this change.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How will scope work for verifying arbitrary data? I am assuming there would be some trust-policy spec changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi This is an existing flag. It was initially introduced for verifying artifact as
oci-layout
. In verifying afile
scenario, it's only needed when the file and the signature are in user's file system. In this case, the user has two choices, they are mentioned in this PR (verify.md) as well, please take a look there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes but
registryScope
doesn't mean anything if user is verifying a blob/file because there is no registry.Also as per https://github.com/notaryproject/specifications/blob/main/specs/trust-store-trust-policy.md#registry-scopes-constraints
List of one or more fully qualified repository URIs. The repository URI MUST NOT contain the asterisk character *.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi Agree. Yes, we should have a spec change in the https://github.com/notaryproject/specifications repo as well, especially for the
trust policy
part.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi I will add the related spec changes into the specification's repo. Here's the issue created by @yizha1 on the spec changes: notaryproject/specifications#275
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will also require updates to descriptor which is being signing.
digest
calculation?size
, is this the size of actual content that we are trying to sign?Signing process look like this?
Here we are calculating digest twice instead of once (ideal). Should we worry about this? Thinking more we will probably need this to support user defined metadata.
For verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi Answering them below:
To be more precise, this is the update to the signature
payload
.And yes, we should have the corresponding changes in the specs of the specifications repo to introduce non-OCI-signature-specifications which includes
signature payload
,signature storage
,trust policy
, and any change onsign and verification workflow
.Good point. We should allow users to pick their own media type. We could do this by:
notation sign --file myFile:sbom/example
i.e., using ':' to delimit the file path and its media type. We should have a default if user doesn't pass their own media type, we should define this default in the specifications repo though. What do you think?
/cc: @shizhMSFT
Currently, I'm using opencontainers/go-digest to compute the digest. The algorithm is SHA256, which is the primary storage digest (https://pkg.go.dev/github.com/opencontainers/go-digest#Canonical).
Yes, since the actual content is signed, the size corresponds to the size of the actual content.
I don't think we should worry about this? Because even for the current Notation, we create the OCI descriptor out of the target OCI artifact, then sign/verify the descriptor.
Yes, it reuses what we have in the current Notation workflow. The changes for sign and verify are actually small in our code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, for future extensibility and robustness, we should control the media type.
IMO, we can decide for sane default but allow user to control hashing algo. Depending upon usecase and compliance need user might want to use different hashing algos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi Since this PR is closed, could you move the discussions to #767?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we want this to be experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because
--scope
is an existing flag in thenotation v1.0.0 verify command
, and it is marked asexperimental
at the moment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a flag marked as experimental doesn't mean that this new functionality needs to be experimental. We can still have signing local OCI artifact as experimental and still support signing arbitrary data as new feature(not experimental).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi Yes, you are right, I'm not marking this new feature as experimental. Only the flag
--scope
is experimental here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So does that mean initially we will only support wildcard trust policy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@priteshbandi I think we should also support user specified trust policy. And we need to make the corresponding changes in the spec of the specifications repo.