From e6e07ce7a79206991cf4c1c76df5f36acfd33827 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 1 Dec 2022 21:28:35 +0800 Subject: [PATCH 1/5] added tag reference log for notation.Sign and notation.Verify Signed-off-by: Patrick Zheng --- notation.go | 44 +++++++++++++++++++++++++++++++++++++++++-- notation_test.go | 16 ++++++++++++++++ signer/plugin_test.go | 11 +++++++---- signer/signer_test.go | 2 ++ 4 files changed, 67 insertions(+), 6 deletions(-) diff --git a/notation.go b/notation.go index bb813554..93efd3b6 100644 --- a/notation.go +++ b/notation.go @@ -10,12 +10,15 @@ import ( "errors" "fmt" "reflect" + "strings" "time" "github.com/notaryproject/notation-core-go/signature" + "github.com/notaryproject/notation-go/log" "github.com/notaryproject/notation-go/registry" "github.com/notaryproject/notation-go/verifier/trustpolicy" ocispec "github.com/opencontainers/image-spec/specs-go/v1" + orasRegistry "oras.land/oras-go/v2/registry" ) const annotationX509ChainThumbprint = "io.cncf.notary.x509chain.thumbprint#S256" @@ -52,10 +55,23 @@ type Signer interface { // remote. // The descriptor of the sign content is returned upon sucessful signing. func Sign(ctx context.Context, signer Signer, repo registry.Repository, opts SignOptions) (ocispec.Descriptor, error) { - targetDesc, err := repo.Resolve(ctx, opts.ArtifactReference) + logger := log.GetLogger(ctx) + + artifactRef := opts.ArtifactReference + targetDesc, err := repo.Resolve(ctx, artifactRef) if err != nil { return ocispec.Descriptor{}, err } + if !isDigestReference(artifactRef) { + // artifactRef is not a digest reference + ref, err := orasRegistry.ParseReference(artifactRef) + if err != nil { + return ocispec.Descriptor{}, err + } + logger.Warnf("Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:%s`) because tags are mutable and a tag reference can point to a different artifact than the one signed", ref.ReferenceOrDefault()) + logger.Infof("Resolved artifact tag `%s` to digest `%s` before signing", ref.ReferenceOrDefault(), targetDesc.Digest.String()) + } + sig, signerInfo, err := signer.Sign(ctx, targetDesc, opts) if err != nil { return ocispec.Descriptor{}, err @@ -155,6 +171,8 @@ type RemoteVerifyOptions struct { // For more details on signature verification, see // https://github.com/notaryproject/notaryproject/blob/main/specs/trust-store-trust-policy.md#signature-verification func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, remoteOpts RemoteVerifyOptions) (ocispec.Descriptor, []*VerificationOutcome, error) { + logger := log.GetLogger(ctx) + // opts to be passed in verifier.Verify() opts := VerifyOptions{ ArtifactReference: remoteOpts.ArtifactReference, @@ -176,16 +194,28 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: fmt.Sprintf("verifyOptions.MaxSignatureAttempts expects a positive number, got %d", remoteOpts.MaxSignatureAttempts)} } - // get signature manifests + // get artifact descriptor artifactRef := remoteOpts.ArtifactReference artifactDescriptor, err := repo.Resolve(ctx, artifactRef) if err != nil { return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} } + if !isDigestReference(artifactRef) { + fmt.Println("here") + // artifactRef is not a digest reference + ref, err := orasRegistry.ParseReference(artifactRef) + if err != nil { + return ocispec.Descriptor{}, nil, err + } + logger.Infof("Resolved artifact tag `%s` to digest `%s` before verification", ref.ReferenceOrDefault(), artifactDescriptor.Digest.String()) + logger.Warn("The resolved digest may not point to the same signed artifact, since tags are mutable") + } var verificationOutcomes []*VerificationOutcome errExceededMaxVerificationLimit := ErrorVerificationFailed{Msg: fmt.Sprintf("total number of signatures associated with an artifact should be less than: %d", remoteOpts.MaxSignatureAttempts)} numOfSignatureProcessed := 0 + + // get signature manifests err = repo.ListSignatures(ctx, artifactDescriptor, func(signatureManifests []ocispec.Descriptor) error { // process signatures for _, sigManifestDesc := range signatureManifests { @@ -262,3 +292,13 @@ func generateAnnotations(signerInfo *signature.SignerInfo) (map[string]string, e annotationX509ChainThumbprint: string(val), }, nil } + +func isDigestReference(reference string) bool { + parts := strings.SplitN(reference, "/", 2) + if len(parts) == 1 { + return false + } + + _, _, found := strings.Cut(parts[1], "@") + return found +} diff --git a/notation_test.go b/notation_test.go index f3f31611..9ce1d286 100644 --- a/notation_test.go +++ b/notation_test.go @@ -30,6 +30,22 @@ func TestRegistryResolveError(t *testing.T) { } } +func TestVerifyTagReferenceFailed(t *testing.T) { + policyDocument := dummyPolicyDocument() + repo := mock.NewRepository() + verifier := dummyVerifier{&policyDocument, mock.PluginManager{}, false, *trustpolicy.LevelStrict} + + errorMessage := "invalid reference: invalid repository" + + // mock the repository + opts := RemoteVerifyOptions{ArtifactReference: "localhost/UPPERCASE/test", MaxSignatureAttempts: 50} + _, _, err := Verify(context.Background(), &verifier, repo, opts) + fmt.Println(err) + if err == nil || err.Error() != errorMessage { + t.Fatalf("VerifyTagReference expected: %q, got: %q", errorMessage, err.Error()) + } +} + func TestSkippedSignatureVerification(t *testing.T) { policyDocument := dummyPolicyDocument() repo := mock.NewRepository() diff --git a/signer/plugin_test.go b/signer/plugin_test.go index 72dc23ba..fc7eeb63 100644 --- a/signer/plugin_test.go +++ b/signer/plugin_test.go @@ -13,6 +13,8 @@ import ( "time" "github.com/notaryproject/notation-core-go/signature" + _ "github.com/notaryproject/notation-core-go/signature/cose" + _ "github.com/notaryproject/notation-core-go/signature/jws" "github.com/notaryproject/notation-go" "github.com/notaryproject/notation-go/internal/envelope" "github.com/notaryproject/notation-go/plugin" @@ -80,8 +82,9 @@ func (p *mockPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataRequ // DescribeKey returns the KeySpec of a key. func (p *mockPlugin) DescribeKey(ctx context.Context, req *proto.DescribeKeyRequest) (*proto.DescribeKeyResponse, error) { + ks, _ := proto.EncodeKeySpec(p.keySpec) return &proto.DescribeKeyResponse{ - KeySpec: proto.KeySpecRSA2048, + KeySpec: ks, }, nil } @@ -152,7 +155,7 @@ func TestNewFromPluginFailed(t *testing.T) { func TestSigner_Sign_EnvelopeNotSupported(t *testing.T) { signer := pluginSigner{ - plugin: newMockPlugin(false, false, false, false, nil, nil, signature.KeySpec{}), + plugin: newMockPlugin(false, false, false, false, nil, nil, signature.KeySpec{Type: signature.KeyTypeRSA, Size: 2048}), } opts := notation.SignOptions{SignatureMediaType: "unsupported"} testSignerError(t, signer, fmt.Sprintf("signature envelope format with media type %q is not supported", opts.SignatureMediaType), opts) @@ -204,7 +207,7 @@ func TestPluginSigner_Sign_SignatureVerifyError(t *testing.T) { signer := pluginSigner{ plugin: newMockPlugin(false, false, true, false, defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec), } - testSignerError(t, signer, "signature returned by generateSignature cannot be verified", notation.SignOptions{SignatureMediaType: envelopeType}) + testSignerError(t, signer, "signature is invalid", notation.SignOptions{SignatureMediaType: envelopeType}) }) } } @@ -233,7 +236,7 @@ func TestPluginSigner_SignEnvelope_RunFailed(t *testing.T) { signer := pluginSigner{ plugin: p, } - testSignerError(t, signer, "generate-envelope command failed: failed GenerateEnvelope", notation.SignOptions{SignatureMediaType: envelopeType}) + testSignerError(t, signer, "failed GenerateEnvelope", notation.SignOptions{SignatureMediaType: envelopeType}) }) } } diff --git a/signer/signer_test.go b/signer/signer_test.go index b26ae5d6..c276f27c 100644 --- a/signer/signer_test.go +++ b/signer/signer_test.go @@ -17,6 +17,8 @@ import ( "time" "github.com/notaryproject/notation-core-go/signature" + _ "github.com/notaryproject/notation-core-go/signature/cose" + _ "github.com/notaryproject/notation-core-go/signature/jws" "github.com/notaryproject/notation-core-go/testhelper" "github.com/notaryproject/notation-core-go/timestamp/timestamptest" "github.com/notaryproject/notation-go" From a906a64e46153b8d206af0715e17f3a80c2e8e77 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 1 Dec 2022 22:19:09 +0800 Subject: [PATCH 2/5] update Signed-off-by: Patrick Zheng --- notation.go | 1 - 1 file changed, 1 deletion(-) diff --git a/notation.go b/notation.go index 93efd3b6..c92a6b02 100644 --- a/notation.go +++ b/notation.go @@ -201,7 +201,6 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} } if !isDigestReference(artifactRef) { - fmt.Println("here") // artifactRef is not a digest reference ref, err := orasRegistry.ParseReference(artifactRef) if err != nil { From 4545a9f8c27b4dc0e63ba27c927f281afb5671dc Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 1 Dec 2022 22:23:51 +0800 Subject: [PATCH 3/5] update Signed-off-by: Patrick Zheng --- notation_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/notation_test.go b/notation_test.go index 9ce1d286..307d22e0 100644 --- a/notation_test.go +++ b/notation_test.go @@ -40,7 +40,6 @@ func TestVerifyTagReferenceFailed(t *testing.T) { // mock the repository opts := RemoteVerifyOptions{ArtifactReference: "localhost/UPPERCASE/test", MaxSignatureAttempts: 50} _, _, err := Verify(context.Background(), &verifier, repo, opts) - fmt.Println(err) if err == nil || err.Error() != errorMessage { t.Fatalf("VerifyTagReference expected: %q, got: %q", errorMessage, err.Error()) } From 776dffea612f65dd291d833ad7fc8257fd44c545 Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Thu, 1 Dec 2022 23:16:27 +0800 Subject: [PATCH 4/5] update per code review Signed-off-by: Patrick Zheng --- notation.go | 43 +++++++++++++++++++------------------------ notation_test.go | 21 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 26 deletions(-) diff --git a/notation.go b/notation.go index c92a6b02..fa868c12 100644 --- a/notation.go +++ b/notation.go @@ -10,7 +10,6 @@ import ( "errors" "fmt" "reflect" - "strings" "time" "github.com/notaryproject/notation-core-go/signature" @@ -62,14 +61,17 @@ func Sign(ctx context.Context, signer Signer, repo registry.Repository, opts Sig if err != nil { return ocispec.Descriptor{}, err } - if !isDigestReference(artifactRef) { + ref, err := orasRegistry.ParseReference(artifactRef) + if err != nil { + return ocispec.Descriptor{}, err + } + if ref.Reference == "" { + return ocispec.Descriptor{}, errors.New("reference is missing digest or tag") + } + if ref.ValidateReferenceAsDigest() != nil { // artifactRef is not a digest reference - ref, err := orasRegistry.ParseReference(artifactRef) - if err != nil { - return ocispec.Descriptor{}, err - } - logger.Warnf("Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:%s`) because tags are mutable and a tag reference can point to a different artifact than the one signed", ref.ReferenceOrDefault()) - logger.Infof("Resolved artifact tag `%s` to digest `%s` before signing", ref.ReferenceOrDefault(), targetDesc.Digest.String()) + logger.Warnf("Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:%s`) because tags are mutable and a tag reference can point to a different artifact than the one signed", ref.Reference) + logger.Infof("Resolved artifact tag `%s` to digest `%s` before signing", ref.Reference, targetDesc.Digest.String()) } sig, signerInfo, err := signer.Sign(ctx, targetDesc, opts) @@ -200,13 +202,16 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re if err != nil { return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} } - if !isDigestReference(artifactRef) { + ref, err := orasRegistry.ParseReference(artifactRef) + if err != nil { + return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} + } + if ref.Reference == "" { + return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: "reference is missing digest or tag"} + } + if ref.ValidateReferenceAsDigest() != nil { // artifactRef is not a digest reference - ref, err := orasRegistry.ParseReference(artifactRef) - if err != nil { - return ocispec.Descriptor{}, nil, err - } - logger.Infof("Resolved artifact tag `%s` to digest `%s` before verification", ref.ReferenceOrDefault(), artifactDescriptor.Digest.String()) + logger.Infof("Resolved artifact tag `%s` to digest `%s` before verification", ref.Reference, artifactDescriptor.Digest.String()) logger.Warn("The resolved digest may not point to the same signed artifact, since tags are mutable") } @@ -291,13 +296,3 @@ func generateAnnotations(signerInfo *signature.SignerInfo) (map[string]string, e annotationX509ChainThumbprint: string(val), }, nil } - -func isDigestReference(reference string) bool { - parts := strings.SplitN(reference, "/", 2) - if len(parts) == 1 { - return false - } - - _, _, found := strings.Cut(parts[1], "@") - return found -} diff --git a/notation_test.go b/notation_test.go index 307d22e0..52007c56 100644 --- a/notation_test.go +++ b/notation_test.go @@ -30,18 +30,35 @@ func TestRegistryResolveError(t *testing.T) { } } +func TestVerifyEmptyReference(t *testing.T) { + policyDocument := dummyPolicyDocument() + repo := mock.NewRepository() + verifier := dummyVerifier{&policyDocument, mock.PluginManager{}, false, *trustpolicy.LevelStrict} + + errorMessage := "reference is missing digest or tag" + expectedErr := ErrorSignatureRetrievalFailed{Msg: errorMessage} + + // mock the repository + opts := RemoteVerifyOptions{ArtifactReference: "localhost/test", MaxSignatureAttempts: 50} + _, _, err := Verify(context.Background(), &verifier, repo, opts) + if err == nil || !errors.Is(err, expectedErr) { + t.Fatalf("VerifyTagReference expected: %v got: %v", expectedErr, err) + } +} + func TestVerifyTagReferenceFailed(t *testing.T) { policyDocument := dummyPolicyDocument() repo := mock.NewRepository() verifier := dummyVerifier{&policyDocument, mock.PluginManager{}, false, *trustpolicy.LevelStrict} errorMessage := "invalid reference: invalid repository" + expectedErr := ErrorSignatureRetrievalFailed{Msg: errorMessage} // mock the repository opts := RemoteVerifyOptions{ArtifactReference: "localhost/UPPERCASE/test", MaxSignatureAttempts: 50} _, _, err := Verify(context.Background(), &verifier, repo, opts) - if err == nil || err.Error() != errorMessage { - t.Fatalf("VerifyTagReference expected: %q, got: %q", errorMessage, err.Error()) + if err == nil || !errors.Is(err, expectedErr) { + t.Fatalf("VerifyTagReference expected: %v got: %v", expectedErr, err) } } From ced606cfd63c1b9a95b22bc970f385e3371da0dc Mon Sep 17 00:00:00 2001 From: Patrick Zheng Date: Fri, 2 Dec 2022 00:04:25 +0800 Subject: [PATCH 5/5] update per code review Signed-off-by: Patrick Zheng --- notation.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/notation.go b/notation.go index fa868c12..5d533623 100644 --- a/notation.go +++ b/notation.go @@ -57,10 +57,6 @@ func Sign(ctx context.Context, signer Signer, repo registry.Repository, opts Sig logger := log.GetLogger(ctx) artifactRef := opts.ArtifactReference - targetDesc, err := repo.Resolve(ctx, artifactRef) - if err != nil { - return ocispec.Descriptor{}, err - } ref, err := orasRegistry.ParseReference(artifactRef) if err != nil { return ocispec.Descriptor{}, err @@ -68,6 +64,10 @@ func Sign(ctx context.Context, signer Signer, repo registry.Repository, opts Sig if ref.Reference == "" { return ocispec.Descriptor{}, errors.New("reference is missing digest or tag") } + targetDesc, err := repo.Resolve(ctx, artifactRef) + if err != nil { + return ocispec.Descriptor{}, err + } if ref.ValidateReferenceAsDigest() != nil { // artifactRef is not a digest reference logger.Warnf("Always sign the artifact using digest(`@sha256:...`) rather than a tag(`:%s`) because tags are mutable and a tag reference can point to a different artifact than the one signed", ref.Reference) @@ -198,10 +198,6 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re // get artifact descriptor artifactRef := remoteOpts.ArtifactReference - artifactDescriptor, err := repo.Resolve(ctx, artifactRef) - if err != nil { - return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} - } ref, err := orasRegistry.ParseReference(artifactRef) if err != nil { return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} @@ -209,6 +205,10 @@ func Verify(ctx context.Context, verifier Verifier, repo registry.Repository, re if ref.Reference == "" { return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: "reference is missing digest or tag"} } + artifactDescriptor, err := repo.Resolve(ctx, artifactRef) + if err != nil { + return ocispec.Descriptor{}, nil, ErrorSignatureRetrievalFailed{Msg: err.Error()} + } if ref.ValidateReferenceAsDigest() != nil { // artifactRef is not a digest reference logger.Infof("Resolved artifact tag `%s` to digest `%s` before verification", ref.Reference, artifactDescriptor.Digest.String())