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

feat: add support for signing blob #379

Merged
merged 15 commits into from
Mar 14, 2024

Conversation

priteshbandi
Copy link
Contributor

@priteshbandi priteshbandi commented Jan 26, 2024

This PR adds support for signing blobs.

  • This PR is ready for review**

@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2024

Codecov Report

Attention: Patch coverage is 82.85714% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 75.33%. Comparing base (9ff1891) to head (2044e2b).
Report is 5 commits behind head on main.

Files Patch % Lines
signer/plugin.go 82.22% 5 Missing and 3 partials ⚠️
signer/signer.go 75.00% 3 Missing and 3 partials ⚠️
notation.go 88.88% 2 Missing and 2 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #379      +/-   ##
==========================================
+ Coverage   73.43%   75.33%   +1.89%     
==========================================
  Files          24       24              
  Lines        2507     2019     -488     
==========================================
- Hits         1841     1521     -320     
+ Misses        526      353     -173     
- Partials      140      145       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shizhMSFT shizhMSFT changed the title Add support for signing blob feat: add support for signing blob Jan 26, 2024
notation.go Show resolved Hide resolved
notation.go Outdated Show resolved Hide resolved
notation.go Outdated Show resolved Hide resolved
notation.go Outdated Show resolved Hide resolved
notation.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

Blob signing introduced by notaryproject/specifications#283 is slightly different from the OCI artifact signing. In summary, blob signing has the same signing / verification core as OCI artifact signing where the digest algorithm in the notary-project specific descriptor is restricted to the signing / verification key.

It is natural to share the same Signer interface with both OCI artifact and blob signing and add a new option IsBlob in SignerSignOptions (and VerifierVerifyOptions for Verifier) since the signing core is the notary-project specific descriptor. However, this solution is vulnerable for those existing implementations (if any) don't support blob signing / verification. Precisely, the IsBlob option may be dropped by those implementations unintentionally where a blob may be verified as an artifact although IsBlob is set to true, which produces unexpected results. Therefore, it is better to split it to two different interfaces as this PR does. In notation-go/v2, maybe we can combine the signing cores into a single one.

To elaborate a bit, we need the following new stuffs for signing

type BlobSigner interface {
    SignBlob(ctx context.Context, desc ocispec.Descriptor, opts SignerSignOptions) ([]byte, *signature.SignerInfo, error)
    BlobDigestAlgorithm(ctx context.Context) (digest.Algorithm, error)
}

type SignBlobOptions struct {
    SignerSignOptions

    ContentMediaType string
    UserMetadata map[string]string
}

func SignBlob(ctx context.Context, signer BlobSigner, reader io.Reader, signOpts SignBlobOptions) ([]byte, error)

The BlobSigner.SignBlob does not take a reader since notation-go, as a library, it should provide flexibility where the digest of a huge blob is generated by other component efficiently. The BlobSigner.SignBlob implementation can later check if the digest algorithm matches the signing key or not and reject if not.

The SignBlob is more like a utility function of BlobSigner.SignBlob where it takes a reader to compute the digest using the algorithm specified by BlobSigner.BlobDigestAlgorithm() backed by the signing key.

/cc @Two-Hearts

@priteshbandi
Copy link
Contributor Author

priteshbandi commented Mar 3, 2024

As discussed over slack with Shiwei, I will be updating notation blob signing functions to following:

type SignBlobOptions struct {
	SignerSignOptions
	ContentMediaType string
	UserMetadata map[string]string
}

type BlobDescriptorGenerator func(digest.Algorithm) (ocispec.Descriptor, error)

type BlobSigner interface {
	SignBlob(ctx context.Context, genDesc BlobDescriptorGenerator, opts SignerSignOptions) ([]byte, *signature.SignerInfo, error)
}

Reasoning: This approach reduces the number of calls to the plugin's DescribeKey to just one, compared to the two calls required in the previous approach. Additionally, in earlier approach we did explore caching the DescribeKey response based on keyId, but it limits the seamless migration of signing keys behind the KeyId.

@priteshbandi priteshbandi force-pushed the BlobSigning branch 5 times, most recently from f58ed61 to 0a4c92c Compare March 3, 2024 05:49
@priteshbandi priteshbandi marked this pull request as ready for review March 3, 2024 05:53
@priteshbandi priteshbandi force-pushed the BlobSigning branch 3 times, most recently from 6532bba to ad012dc Compare March 3, 2024 07:05
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
@priteshbandi priteshbandi requested a review from shizhMSFT March 12, 2024 02:16
signer/plugin.go Outdated Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
signer/signer.go Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
Signed-off-by: Pritesh Bandi <[email protected]>
rgnote
rgnote previously approved these changes Mar 12, 2024
Copy link
Contributor

@rgnote rgnote left a comment

Choose a reason for hiding this comment

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

LGTM

notation.go Outdated Show resolved Hide resolved
signer/plugin.go Show resolved Hide resolved
signer/signer.go Outdated Show resolved Hide resolved
Co-authored-by: Shiwei Zhang <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Signed-off-by: Pritesh Bandi <[email protected]>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@rgnote rgnote left a comment

Choose a reason for hiding this comment

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

LGTM

@priteshbandi priteshbandi merged commit ec42378 into notaryproject:main Mar 14, 2024
7 checks passed
@JeyJeyGao JeyJeyGao mentioned this pull request May 29, 2024
6 tasks
JeyJeyGao added a commit to JeyJeyGao/notation-go that referenced this pull request May 31, 2024
JeyJeyGao added a commit to JeyJeyGao/notation-go that referenced this pull request May 31, 2024
JeyJeyGao added a commit to JeyJeyGao/notation-go that referenced this pull request May 31, 2024
This reverts commit ec42378.

Signed-off-by: Junjie Gao [email protected]
Signed-off-by: Junjie Gao <[email protected]>
JeyJeyGao added a commit that referenced this pull request May 31, 2024
Revert:
- reverted #379 
- reverted test cases: `TestSignOptsMissingSignatureMediaType` and
`TestSignOptsUnknownMediaType`, introduced in #405

Test:
- added test for `log` package to pass 80% coverage target

This reverts commit ec42378.


Resolves part of #412 
Signed-off-by: Junjie Gao <[email protected]>

---------

Signed-off-by: Junjie Gao [email protected]
Signed-off-by: Junjie Gao <[email protected]>
@Two-Hearts Two-Hearts mentioned this pull request Jul 23, 2024
6 tasks
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.

6 participants