diff --git a/notation.go b/notation.go index 18665f1b..312a6551 100644 --- a/notation.go +++ b/notation.go @@ -35,6 +35,17 @@ func (d Descriptor) Equal(t Descriptor) bool { return d.MediaType == t.MediaType && d.Digest == t.Digest && d.Size == t.Size } +// DescriptorAnnotationsPartialEqual checks that newDesc hasn't replaced or overridden existing annotations. +func (d Descriptor) DescriptorAnnotationsPartialEqual(t Descriptor) bool { + // Plugins may append additional annotations but not replace/override existing. + for k, v := range d.Annotations { + if v2, ok := t.Annotations[k]; !ok || v != v2 { + return false + } + } + return true +} + // Payload describes the content that gets signed. type Payload struct { TargetArtifact Descriptor `json:"targetArtifact"` diff --git a/signature/plugin.go b/signature/plugin.go index f0dca93d..16d68932 100644 --- a/signature/plugin.go +++ b/signature/plugin.go @@ -56,7 +56,7 @@ func (s *pluginSigner) Sign(ctx context.Context, desc notation.Descriptor, opts ) } if metadata.HasCapability(plugin.CapabilitySignatureGenerator) { - return s.generateSignature(ctx, desc, opts) + return s.generateSignature(ctx, desc, opts, metadata) } else if metadata.HasCapability(plugin.CapabilityEnvelopeGenerator) { return s.generateSignatureEnvelope(ctx, desc, opts) } @@ -95,7 +95,7 @@ func (s *pluginSigner) describeKey(ctx context.Context, config map[string]string return resp, nil } -func (s *pluginSigner) generateSignature(ctx context.Context, desc notation.Descriptor, opts notation.SignOptions) ([]byte, error) { +func (s *pluginSigner) generateSignature(ctx context.Context, desc notation.Descriptor, opts notation.SignOptions, metadata *plugin.Metadata) ([]byte, error) { config := s.mergeConfig(opts.PluginConfig) // Get key info. key, err := s.describeKey(ctx, config) @@ -123,6 +123,7 @@ func (s *pluginSigner) generateSignature(ctx context.Context, desc notation.Desc } extProvider.prepareSigning(config, ks) } + signingAgentId := fmt.Sprintf("%s;%s/%s", notation.SigningAgent, metadata.Name, metadata.Version) signReq := &signature.SignRequest{ Payload: signature.Payload{ ContentType: notation.MediaTypePayloadV1, @@ -132,7 +133,7 @@ func (s *pluginSigner) generateSignature(ctx context.Context, desc notation.Desc SigningTime: time.Now(), ExtendedSignedAttributes: nil, SigningScheme: signature.SigningSchemeX509, - SigningAgent: notation.SigningAgent, // TODO: include external signing plugin's name and version. https://github.com/notaryproject/notation-go/issues/80 + SigningAgent: signingAgentId, } if !opts.Expiry.IsZero() { @@ -220,32 +221,40 @@ func (s *pluginSigner) generateSignatureEnvelope(ctx context.Context, desc notat return nil, err } + content := envContent.Payload.Content var signedPayload notation.Payload - if err = json.Unmarshal(envContent.Payload.Content, &signedPayload); err != nil { + if err = json.Unmarshal(content, &signedPayload); err != nil { return nil, fmt.Errorf("signed envelope payload can't be unmarshaled: %w", err) } - // TODO: Verify plugin didnot add any additional top level payload attributes. https://github.com/notaryproject/notation-go/issues/80 - if !descriptorPartialEqual(desc, signedPayload.TargetArtifact) { + if !isPayloadDescriptorValid(content, desc, signedPayload.TargetArtifact) { return nil, errors.New("descriptor subject has changed") } return resp.SignatureEnvelope, nil } -// descriptorPartialEqual checks if the both descriptors point to the same resource -// and that newDesc hasn't replaced or overridden existing annotations. -func descriptorPartialEqual(original, newDesc notation.Descriptor) bool { - if !original.Equal(newDesc) { - return false - } - // Plugins may append additional annotations but not replace/override existing. - for k, v := range original.Annotations { - if v2, ok := newDesc.Annotations[k]; !ok || v != v2 { - return false - } - } - return true +func isPayloadDescriptorValid(signedContent []byte, originalDesc, newDesc notation.Descriptor) bool { + return originalDesc.Equal(newDesc) && + originalDesc.DescriptorAnnotationsPartialEqual(newDesc) && + !isUnknownAttributeAdded(signedContent) +} + +// TODO: unit test this method: https://github.com/notaryproject/notation-go/issues/194 +// isUnknownAttributeAdded checks that the unmarshalled json content does not have any unexpected keys added +func isUnknownAttributeAdded(content []byte) bool { + var targetArtifactMap map[string]interface{} + // Ignoring error because we already successfully unmarshalled before this point + _ = json.Unmarshal(content, &targetArtifactMap) + descriptor := targetArtifactMap["targetArtifact"].(map[string]interface{}) + + // Explicitly remove expected keys to check if any are left over + delete(descriptor, "mediaType") + delete(descriptor, "digest") + delete(descriptor, "size") + delete(descriptor, "annotations") + + return len(targetArtifactMap) != 1 || len(descriptor) != 0 } func parseCertChain(certChain [][]byte) ([]*x509.Certificate, error) { diff --git a/signature/plugin_test.go b/signature/plugin_test.go index dbbfe588..600be52e 100644 --- a/signature/plugin_test.go +++ b/signature/plugin_test.go @@ -479,7 +479,7 @@ func TestSigner_Sign_SignatureVerifyError(t *testing.T) { } } -func basicSignTest(t *testing.T, pluginSigner *pluginSigner) { +func basicSignTest(t *testing.T, pluginSigner *pluginSigner, metadata *plugin.Metadata) { data, err := pluginSigner.Sign(context.Background(), validSignDescriptor, validSignOpts) if err != nil { t.Fatalf("Signer.Sign() error = %v, wantErr nil", err) @@ -541,7 +541,7 @@ func basicSignTest(t *testing.T, pluginSigner *pluginSigner) { if !reflect.DeepEqual(certChain, signerInfo.CertificateChain) { t.Fatalf(" Signer.Sign() cert chain changed") } - basicVerification(t, data, pluginSigner.envelopeMediaType, certChain[len(certChain)-1]) + basicVerification(t, data, pluginSigner.envelopeMediaType, certChain[len(certChain)-1], metadata) } func TestSigner_Sign_Valid(t *testing.T) { @@ -552,7 +552,7 @@ func TestSigner_Sign_Valid(t *testing.T) { sigProvider: newTestBuiltInProvider(keyCert), envelopeMediaType: envelopeType, } - basicSignTest(t, &pluginSigner) + basicSignTest(t, &pluginSigner, builtInPluginMetaData) }) keyID := "Key" t.Run(fmt.Sprintf("external plugin,envelopeType=%v_keySpec=%v", envelopeType, keyCert.keySpecName), func(t *testing.T) { @@ -561,7 +561,7 @@ func TestSigner_Sign_Valid(t *testing.T) { envelopeMediaType: envelopeType, keyID: keyID, } - basicSignTest(t, &pluginSigner) + basicSignTest(t, &pluginSigner, &validMetadata) }) } } @@ -825,7 +825,7 @@ func TestPluginSigner_SignEnvelope_Valid(t *testing.T) { sigProvider: newMockEnvelopeProvider(keyCert.key, keyCert.certs, ""), envelopeMediaType: envelopeType, } - basicSignTest(t, &signer) + basicSignTest(t, &signer, &validMetadata) }) } } diff --git a/signature/signer.go b/signature/signer.go index 9fa2ce7e..2cebdcae 100644 --- a/signature/signer.go +++ b/signature/signer.go @@ -11,7 +11,6 @@ import ( ) // NewSignerFromFiles creates a signer from key, certificate files -// TODO: Add tests for this method. https://github.com/notaryproject/notation-go/issues/80 func NewSignerFromFiles(keyPath, certPath, envelopeMediaType string) (notation.Signer, error) { if keyPath == "" { return nil, errors.New("key path not specified") diff --git a/signature/signer_test.go b/signature/signer_test.go index 81fb916e..ed4e9455 100644 --- a/signature/signer_test.go +++ b/signature/signer_test.go @@ -13,6 +13,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "testing" "time" @@ -128,7 +129,7 @@ func testSignerFromFile(t *testing.T, keyCert *keyCertPair, envelopeType, dir st t.Fatalf("Sign() failed: %v", err) } // basic verification - basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1]) + basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1], builtInPluginMetaData) } func TestNewSignerFromFiles(t *testing.T) { @@ -181,7 +182,7 @@ func TestSignWithTimestamp(t *testing.T) { } // basic verification - basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1]) + basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1], builtInPluginMetaData) }) } } @@ -206,7 +207,7 @@ func TestSignWithoutExpiry(t *testing.T) { } // basic verification - basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1]) + basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1], builtInPluginMetaData) }) } } @@ -255,7 +256,7 @@ func TestExternalSigner_Sign(t *testing.T) { t.Fatalf("Sign() error = %v", err) } // basic verification - basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1]) + basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1], &validMetadata) } } } @@ -275,7 +276,7 @@ func TestExternalSigner_SignEnvelope(t *testing.T) { t.Fatalf("Sign() error = %v", err) } // basic verification - basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1]) + basicVerification(t, sig, envelopeType, keyCert.certs[len(keyCert.certs)-1], &validMetadata) }) } } @@ -315,7 +316,7 @@ func generateKeyCertPair() (crypto.PrivateKey, []*x509.Certificate, error) { return pk, []*x509.Certificate{certTuple.Cert, rsaRoot.Cert}, nil } -func basicVerification(t *testing.T, sig []byte, envelopeType string, trust *x509.Certificate) { +func basicVerification(t *testing.T, sig []byte, envelopeType string, trust *x509.Certificate, metadata *plugin.Metadata) { // basic verification sigEnv, err := signature.ParseEnvelope(envelopeType, sig) if err != nil { @@ -335,6 +336,22 @@ func basicVerification(t *testing.T, sig []byte, envelopeType string, trust *x50 if err != nil || !trustedCert.Equal(trust) { t.Fatalf("VerifyAuthenticity failed. error = %v", err) } + + verifySigningAgent(t, envContent.SignerInfo.UnsignedAttributes.SigningAgent, metadata) +} + +func verifySigningAgent(t *testing.T, signingAgent string, metadata *plugin.Metadata) { + signingAgentRegex := regexp.MustCompile("^(?P.*);(?P.*)/(?P.*)$") + match := signingAgentRegex.FindStringSubmatch(signingAgent) + + results := map[string]string{} + for i, name := range match { + results[signingAgentRegex.SubexpNames()[i]] = name + } + + if results["agent"] != notation.SigningAgent || results["name"] != metadata.Name || results["version"] != metadata.Version { + t.Fatalf("Expected signingAgent of %s;%s/%s but signature contained %s instead", notation.SigningAgent, metadata.Name, metadata.Version, signingAgent) + } } func validateSignWithCerts(t *testing.T, envelopeType string, key crypto.PrivateKey, certs []*x509.Certificate) { @@ -351,5 +368,5 @@ func validateSignWithCerts(t *testing.T, envelopeType string, key crypto.Private } // basic verification - basicVerification(t, sig, envelopeType, certs[len(certs)-1]) + basicVerification(t, sig, envelopeType, certs[len(certs)-1], builtInPluginMetaData) }