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

spec: add spec for sign/verify an arbitrary file using Notation #765

Closed
wants to merge 11 commits into from

Conversation

Two-Hearts
Copy link
Contributor

This PR adds specs for sign and verify on arbitrary file.


# Use flag "--file" to enable signing a file
# Use flag "--signature" to specify path where the generated signature is stored
notation sign --file --signature <signature_path> <target_file_path>
Copy link
Contributor

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?

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 22, 2023

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?

This command will succeed.

And should we use --blob instead of --file?

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 a blob is in their scenario. All they need to know is they are signing/verifying a file. Notation will take care of the rest.

Copy link
Contributor

@priteshbandi priteshbandi Aug 22, 2023

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

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 22, 2023

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 of one 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 a blob, 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 the content of a file in the flag's description of the CLI. I updated the PR to reflect this change.

-h, --help help for verify
--insecure-registry use HTTP protocol while connecting to registries. Should be used only for testing
--max-signatures int maximum number of signatures to evaluate or examine (default 100)
--oci-layout [Experimental] verify the artifact stored as OCI image layout
-p, --password string password for registry operations (default to $NOTATION_PASSWORD if not specified)
--plugin-config stringArray {key}={value} pairs that are passed as it is to a plugin, if the verification is associated with a verification plugin, refer plugin documentation to set appropriate values
--scope string [Experimental] set trust policy scope for artifact verification, required and can only be used when flag "--oci-layout" is set
--scope string [Experimental] set trust policy scope for artifact verification, required when flag "--oci-layout" is set, can only be used when "--oci-layout" or "--file" is set
Copy link
Contributor

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?

Copy link
Contributor Author

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 a file 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.

Copy link
Contributor

@priteshbandi priteshbandi Aug 22, 2023

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 *.

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 22, 2023

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.

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 22, 2023

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

Copy link
Contributor

@priteshbandi priteshbandi Aug 22, 2023

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.

{
  "targetArtifact": {
    "mediaType": "sbom/example", 
    "digest": "sha256:9834876dcfb05cb167a5c24953eba58c4ac89b1adf57f28f2f9d09af107ee8f0",
    "size": 32654 <-- 
  }
}
  • What would be the media type ?
  • Should we give control to user on which hashing algo to use in digest calculation?
  • size, is this the size of actual content that we are trying to sign?

Signing process look like this?
  1. Notation Reads the content that needs be signed (can be read from file or stdin or flag value)
  2. Notation creates descriptor from the content, which needs following operation
  3. Calculate hash/digest and size of content.
  4. Calculate hash/digest of descriptor. This part will be either done by plugin or by notation depending upon plugin type.
  5. Sign the hash/digest of descriptor. This would be done by plugin
  6. return detached signature to user.

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
  1. Notation reads the detached signature
  2. Verify the signature is valid (authenticity, integrity, expiry, etc checks)
  3. Read the descriptor from signature.
  4. Notation Reads the content to be verified
  5. Calculate hash/digest of content and its size
  6. Verify that ash/digest of content and its size matches with the descriptor stored in signature.

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 23, 2023

Choose a reason for hiding this comment

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

@priteshbandi Answering them below:

This will also require updates to descriptor which is being signing.

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 on sign and verification workflow.

{
  "targetArtifact": {
    "mediaType": "sbom/example", 
    "digest": "sha256:9834876dcfb05cb167a5c24953eba58c4ac89b1adf57f28f2f9d09af107ee8f0",
    "size": 32654 <-- 
  }
}
  • What would be the media type ?

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

  • Should we give control to user on which hashing algo to use in digest calculation?

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).

  • size, is this the size of actual content that we are trying to sign?

Yes, since the actual content is signed, the size corresponds to the size of the actual content.

Signing process look like this?
  1. Notation Reads the content that needs be signed (can be read from file or stdin or flag value)
  2. Notation creates descriptor from the content, which needs following operation
  3. Calculate hash/digest and size of content.
  4. Calculate hash/digest of descriptor. This part will be either done by plugin or by notation depending upon plugin type.
  5. Sign the hash/digest of descriptor. This would be done by plugin
  6. return detached signature to user.

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.

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.

For verification
  1. Notation reads the detached signature
  2. Verify the signature is valid (authenticity, integrity, expiry, etc checks)
  3. Read the descriptor from signature.
  4. Notation Reads the content to be verified
  5. Calculate hash/digest of content and its size
  6. Verify that ash/digest of content and its size matches with the descriptor stored in signature.

Yes, it reuses what we have in the current Notation workflow. The changes for sign and verify are actually small in our code base.

Copy link
Contributor

Choose a reason for hiding this comment

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

  • What would be the media type ?

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
Good point. We should allow users to pick their own media type.

IMO, for future extensibility and robustness, we should control the media type.

  • Should we give control to user on which hashing algo to use in digest calculation?

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).

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.

Signing process look like this?
  1. Notation Reads the content that needs be signed (can be read from file or stdin or flag value)
  2. Notation creates descriptor from the content, which needs following operation
  3. Calculate hash/digest and size of content.
  4. Calculate hash/digest of descriptor. This part will be either done by plugin or by notation depending upon plugin type.
  5. Sign the hash/digest of descriptor. This would be done by plugin
  6. return detached signature to user.

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.

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.
True, we could have directly signed pre-calculated digest(without rehashing). Lets discuss this in meeting

Copy link
Contributor

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?

# The global trust policy is used by default
notation verify --file --signature ./mySignature1.sig --signature ./mySignature2.sig ./myFile.txt

export NOTATION_EXPERIMENTAL=1
Copy link
Contributor

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?

Copy link
Contributor Author

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 the notation v1.0.0 verify command, and it is marked as experimental at the moment.

Copy link
Contributor

@priteshbandi priteshbandi Aug 22, 2023

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).

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

@Two-Hearts Two-Hearts Aug 23, 2023

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.

Signed-off-by: Patrick Zheng <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2023

Codecov Report

Merging #765 (0395b6a) into main (1dc6505) will not change coverage.
Report is 1 commits behind head on main.
The diff coverage is n/a.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@           Coverage Diff           @@
##             main     #765   +/-   ##
=======================================
  Coverage   63.98%   63.98%           
=======================================
  Files          40       40           
  Lines        2252     2252           
=======================================
  Hits         1441     1441           
  Misses        689      689           
  Partials      122      122           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: Patrick Zheng <[email protected]>
@shizhMSFT shizhMSFT changed the title spec: added spec for sign/verify an arbitrary file using Notation spec: add spec for sign/verify an arbitrary file using Notation Aug 22, 2023
@@ -19,6 +20,15 @@ Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(
Successfully signed <registry>/<repository>@<digest>
```

### Sign an arbitrary file stored in file system
The file content, i.e. the file blob, is signed.
Copy link
Contributor

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.

@@ -153,6 +165,30 @@ Warning: Always sign the artifact using digest(`@sha256:...`) rather than a tag(
Successfully signed localhost:5000/net-monitor@sha256:b94d27b9934d3e08a52e52d7da7dabfac484efe37a5380ee9088f7ace2efcde9
```

### Sign an arbitrary file located in file system
Notation supports signing a file located in user's file system. The file's content (blob) gets signed and the generated signature is stored in the same file system.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Notation supports signing a file located in user's file system. The file's content (blob) gets signed and the generated signature is stored in the same file system.
Notation supports signing files located in file systems.

Copy link
Contributor

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.

Signed-off-by: Patrick Zheng <[email protected]>
Signed-off-by: Patrick Zheng <[email protected]>
@Two-Hearts
Copy link
Contributor Author

Closing this PR as the spec work needs more discussions: #767. We need to have spec changes in the specifications repo, then CLI spec changes in Notation.

@Two-Hearts Two-Hearts closed this Aug 23, 2023
@priteshbandi
Copy link
Contributor

We area also missing a usecase where user might want to pass recalculated digest to notation. Reason can be privacy or compliance need.

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.

4 participants