Add additional header validation for payload

Signed-off-by: Jonathan Donas <[email protected]>

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"`

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

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

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

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<agent>.*);(?P<name>.*)/(?P<version>.*)$")
+	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)
 }