From e48db415d697b9137f2aead7884934ed1b68b051 Mon Sep 17 00:00:00 2001 From: Jonathan Donas Date: Mon, 31 Oct 2022 16:31:27 -0700 Subject: [PATCH] Add additional header validation for payload Signed-off-by: Jonathan Donas --- notation.go | 7 ++- signer/plugin.go | 50 ++++++++++++++--- signer/plugin_test.go | 123 ++++++++++++++++++++++++++++++++---------- signer/signer.go | 8 ++- signer/signer_test.go | 31 +++++++++-- 5 files changed, 176 insertions(+), 43 deletions(-) diff --git a/notation.go b/notation.go index 04bac5c3..0b3dc348 100644 --- a/notation.go +++ b/notation.go @@ -26,7 +26,7 @@ var errDoneVerification = errors.New("done verification") // SignOptions contains parameters for Signer.Sign. type SignOptions struct { - // Reference of the artifact that needs to be signed. + // ArtifactReference sets the reference of the artifact that needs to be signed. ArtifactReference string // SignatureMediaType is the envelope type of the signature. @@ -38,8 +38,11 @@ type SignOptions struct { // represents no expiry duration. ExpiryDuration time.Duration - // Sets or overrides the plugin configuration. + // PluginConfig sets or overrides the plugin configuration. PluginConfig map[string]string + + // SigningAgent sets the signing agent name + SigningAgent string } // Signer is a generic interface for signing an artifact. diff --git a/signer/plugin.go b/signer/plugin.go index bc37d8a7..391caddf 100644 --- a/signer/plugin.go +++ b/signer/plugin.go @@ -54,14 +54,14 @@ func (s *pluginSigner) Sign(ctx context.Context, desc ocispec.Descriptor, opts n return nil, nil, err } if metadata.HasCapability(proto.CapabilitySignatureGenerator) { - return s.generateSignature(ctx, desc, opts) + return s.generateSignature(ctx, desc, opts, metadata) } else if metadata.HasCapability(proto.CapabilityEnvelopeGenerator) { return s.generateSignatureEnvelope(ctx, desc, opts) } return nil, nil, fmt.Errorf("plugin does not have signing capabilities") } -func (s *pluginSigner) generateSignature(ctx context.Context, desc ocispec.Descriptor, opts notation.SignOptions) ([]byte, *signature.SignerInfo, error) { +func (s *pluginSigner) generateSignature(ctx context.Context, desc ocispec.Descriptor, opts notation.SignOptions, metadata *proto.GetMetadataResponse) ([]byte, *signature.SignerInfo, error) { config := s.mergeConfig(opts.PluginConfig) // Get key info. key, err := s.describeKey(ctx, config) @@ -88,6 +88,7 @@ func (s *pluginSigner) generateSignature(ctx context.Context, desc ocispec.Descr }, } + opts.SigningAgent = fmt.Sprintf("%s %s/%s", signingAgent, metadata.Name, metadata.Version) return genericSigner.Sign(ctx, desc, opts) } @@ -132,17 +133,20 @@ func (s *pluginSigner) generateSignatureEnvelope(ctx context.Context, desc ocisp return nil, nil, err } + content := envContent.Payload.Content var signedPayload envelope.Payload - if err = json.Unmarshal(envContent.Payload.Content, &signedPayload); err != nil { + if err = json.Unmarshal(content, &signedPayload); err != nil { return nil, nil, fmt.Errorf("signed envelope payload can't be unmarshalled: %w", err) } - // TODO: Verify plugin did not add any additional top level payload - // attributes. https://github.com/notaryproject/notation-go/issues/80 - if !isDescriptorSubset(desc, signedPayload.TargetArtifact) { + if !isPayloadDescriptorValid(desc, signedPayload.TargetArtifact) { return nil, nil, fmt.Errorf("during signing descriptor subject has changed from %+v to %+v", desc, signedPayload.TargetArtifact) } + if unknownAttributes := areUnknownAttributesAdded(content); len(unknownAttributes) != 0 { + return nil, nil, fmt.Errorf("during signing descriptor subject added unknown attributes %+q", unknownAttributes) + } + return resp.SignatureEnvelope, &envContent.SignerInfo, nil } @@ -188,6 +192,40 @@ func isDescriptorSubset(original, newDesc ocispec.Descriptor) bool { return true } +func isPayloadDescriptorValid(originalDesc, newDesc ocispec.Descriptor) bool { + return content.Equal(originalDesc, newDesc) && + isDescriptorSubset(originalDesc, newDesc) +} + +func areUnknownAttributesAdded(content []byte) []string { + 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, "urls") + delete(descriptor, "annotations") + delete(descriptor, "data") + delete(descriptor, "platform") + delete(descriptor, "artifactType") + delete(targetArtifactMap, "targetArtifact") + + unknownAttributes := append(getKeySet(descriptor), getKeySet(targetArtifactMap)...) + return unknownAttributes +} + +func getKeySet(inputMap map[string]interface{}) []string { + keySet := make([]string, 0, len(inputMap)) + for k, _ := range inputMap { + keySet = append(keySet, k) + } + return keySet +} + func parseCertChain(certChain [][]byte) ([]*x509.Certificate, error) { certs := make([]*x509.Certificate, len(certChain)) for i, cert := range certChain { diff --git a/signer/plugin_test.go b/signer/plugin_test.go index cb390943..27f76f41 100644 --- a/signer/plugin_test.go +++ b/signer/plugin_test.go @@ -46,35 +46,36 @@ func init() { } type mockPlugin struct { - failEnvelope bool - wantEnvelope bool - invalidSig bool - invalidCertChain bool - key crypto.PrivateKey - certs []*x509.Certificate - keySpec signature.KeySpec + failEnvelope bool + wantEnvelope bool + invalidSig bool + invalidCertChain bool + invalidDescriptor bool + key crypto.PrivateKey + certs []*x509.Certificate + keySpec signature.KeySpec } -func newMockPlugin(failEnvelope, wantEnvelope, invalidSig, invalidCertChain bool, key crypto.PrivateKey, certs []*x509.Certificate, keySpec signature.KeySpec) *mockPlugin { +func newMockPlugin(key crypto.PrivateKey, certs []*x509.Certificate, keySpec signature.KeySpec) *mockPlugin { return &mockPlugin{ - failEnvelope: failEnvelope, - wantEnvelope: wantEnvelope, - invalidSig: invalidSig, - invalidCertChain: invalidCertChain, - key: key, - certs: certs, - keySpec: keySpec, + key: key, + certs: certs, + keySpec: keySpec, } } func (p *mockPlugin) GetMetadata(ctx context.Context, req *proto.GetMetadataRequest) (*proto.GetMetadataResponse, error) { if p.wantEnvelope { return &proto.GetMetadataResponse{ + Name: "testPlugin", + Version: "1.0", SupportedContractVersions: []string{proto.ContractVersion}, Capabilities: []proto.Capability{proto.CapabilityEnvelopeGenerator}, }, nil } return &proto.GetMetadataResponse{ + Name: "testPlugin", + Version: "1.0", SupportedContractVersions: []string{proto.ContractVersion}, Capabilities: []proto.Capability{proto.CapabilitySignatureGenerator}, }, nil @@ -121,13 +122,57 @@ func (p *mockPlugin) GenerateSignature(ctx context.Context, req *proto.GenerateS // GenerateEnvelope generates the Envelope with signature based on the request. func (p *mockPlugin) GenerateEnvelope(ctx context.Context, req *proto.GenerateEnvelopeRequest) (*proto.GenerateEnvelopeResponse, error) { + internalPluginSigner := pluginSigner{ + plugin: newMockPlugin(p.key, p.certs, p.keySpec), + } + if p.failEnvelope { return nil, errors.New("failed GenerateEnvelope") } - if p.wantEnvelope { - internalPluginSigner := pluginSigner{ - plugin: newMockPlugin(false, false, false, false, p.key, p.certs, p.keySpec), + if p.invalidDescriptor { + var payload map[string]interface{} + if err := json.Unmarshal(req.Payload, &payload); err != nil { + return nil, err + } + payload["additional_field"] = "some_string" + + updatedPayload, err := json.Marshal(payload) + if err != nil { + return nil, err + } + + primitivePluginSigner := &pluginPrimitiveSigner{ + ctx: ctx, + plugin: internalPluginSigner.plugin, + keyID: internalPluginSigner.keyID, + pluginConfig: req.PluginConfig, + keySpec: p.keySpec, + } + + signReq := &signature.SignRequest{ + Payload: signature.Payload{ + ContentType: envelope.MediaTypePayloadV1, + Content: updatedPayload, + }, + Signer: primitivePluginSigner, + SigningTime: time.Now(), + ExtendedSignedAttributes: nil, + SigningScheme: signature.SigningSchemeX509, + SigningAgent: "testing agent", } + + sigEnv, err := signature.NewEnvelope(req.SignatureEnvelopeType) + if err != nil { + return nil, err + } + + sig, err := sigEnv.Sign(signReq) + return &proto.GenerateEnvelopeResponse{ + SignatureEnvelope: sig, + SignatureEnvelopeType: req.SignatureEnvelopeType, + }, err + } + if p.wantEnvelope { var payload envelope.Payload if err := json.Unmarshal(req.Payload, &payload); err != nil { return nil, err @@ -155,7 +200,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{Type: signature.KeyTypeRSA, Size: 2048}), + plugin: newMockPlugin(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) @@ -166,7 +211,7 @@ func TestSigner_Sign_DescribeKeyIDMismatch(t *testing.T) { for _, envelopeType := range signature.RegisteredEnvelopeTypes() { t.Run(fmt.Sprintf("envelopeType=%v", envelopeType), func(t *testing.T) { signer := pluginSigner{ - plugin: newMockPlugin(false, false, false, false, nil, nil, signature.KeySpec{}), + plugin: newMockPlugin(nil, nil, signature.KeySpec{}), keyID: "1", } testSignerError(t, signer, fmt.Sprintf("keyID in describeKey response %q does not match request %q", respKeyId, signer.keyID), notation.SignOptions{SignatureMediaType: envelopeType}) @@ -179,7 +224,7 @@ func TestSigner_Sign_ExpiryInValid(t *testing.T) { t.Run(fmt.Sprintf("envelopeType=%v", envelopeType), func(t *testing.T) { ks, _ := signature.ExtractKeySpec(keyCertPairCollections[0].certs[0]) signer := pluginSigner{ - plugin: newMockPlugin(false, false, false, false, keyCertPairCollections[0].key, keyCertPairCollections[0].certs, ks), + plugin: newMockPlugin(keyCertPairCollections[0].key, keyCertPairCollections[0].certs, ks), } _, _, err := signer.Sign(context.Background(), ocispec.Descriptor{}, notation.SignOptions{ExpiryDuration: -24 * time.Hour, SignatureMediaType: envelopeType}) wantEr := "expiry cannot be equal or before the signing time" @@ -193,19 +238,37 @@ func TestSigner_Sign_ExpiryInValid(t *testing.T) { func TestSigner_Sign_InvalidCertChain(t *testing.T) { for _, envelopeType := range signature.RegisteredEnvelopeTypes() { t.Run(fmt.Sprintf("envelopeType=%v", envelopeType), func(t *testing.T) { + mockPlugin := newMockPlugin(defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec) + mockPlugin.invalidCertChain = true signer := pluginSigner{ - plugin: newMockPlugin(false, false, false, true, defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec), + plugin: mockPlugin, } testSignerError(t, signer, "x509: malformed certificate", notation.SignOptions{SignatureMediaType: envelopeType}) }) } } +func TestSigner_Sign_InvalidDescriptor(t *testing.T) { + for _, envelopeType := range signature.RegisteredEnvelopeTypes() { + t.Run(fmt.Sprintf("envelopeType=%v", envelopeType), func(t *testing.T) { + mockPlugin := newMockPlugin(defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec) + mockPlugin.wantEnvelope = true + mockPlugin.invalidDescriptor = true + signer := pluginSigner{ + plugin: mockPlugin, + } + testSignerError(t, signer, "during signing descriptor subject added unknown attributes [\"additional_field\"]", notation.SignOptions{SignatureMediaType: envelopeType}) + }) + } +} + func TestPluginSigner_Sign_SignatureVerifyError(t *testing.T) { for _, envelopeType := range signature.RegisteredEnvelopeTypes() { t.Run(fmt.Sprintf("envelopeType=%v", envelopeType), func(t *testing.T) { + mockPlugin := newMockPlugin(defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec) + mockPlugin.invalidSig = true signer := pluginSigner{ - plugin: newMockPlugin(false, false, true, false, defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec), + plugin: mockPlugin, } testSignerError(t, signer, "signature is invalid", notation.SignOptions{SignatureMediaType: envelopeType}) }) @@ -218,9 +281,9 @@ func TestPluginSigner_Sign_Valid(t *testing.T) { t.Run(fmt.Sprintf("external plugin,envelopeType=%v_keySpec=%v", envelopeType, keyCert.keySpecName), func(t *testing.T) { keySpec, _ := proto.DecodeKeySpec(proto.KeySpec(keyCert.keySpecName)) pluginSigner := pluginSigner{ - plugin: newMockPlugin(false, false, false, false, keyCert.key, keyCert.certs, keySpec), + plugin: newMockPlugin(keyCert.key, keyCert.certs, keySpec), } - basicSignTest(t, &pluginSigner, envelopeType) + basicSignTest(t, &pluginSigner, envelopeType, &validMetadata) }) } } @@ -246,10 +309,12 @@ func TestPluginSigner_SignEnvelope_Valid(t *testing.T) { for _, keyCert := range keyCertPairCollections { t.Run(fmt.Sprintf("envelopeType=%v, keySpec: %v", envelopeType, keyCert.keySpecName), func(t *testing.T) { keySpec, _ := proto.DecodeKeySpec(proto.KeySpec(keyCert.keySpecName)) + mockPlugin := newMockPlugin(keyCert.key, keyCert.certs, keySpec) + mockPlugin.wantEnvelope = true pluginSigner := pluginSigner{ - plugin: newMockPlugin(false, true, false, false, keyCert.key, keyCert.certs, keySpec), + plugin: mockPlugin, } - basicSignTest(t, &pluginSigner, envelopeType) + basicSignTest(t, &pluginSigner, envelopeType, &validMetadata) }) } } @@ -263,7 +328,7 @@ func testSignerError(t *testing.T, signer pluginSigner, wantEr string, opts nota } } -func basicSignTest(t *testing.T, pluginSigner *pluginSigner, envelopeType string) { +func basicSignTest(t *testing.T, pluginSigner *pluginSigner, envelopeType string, metadata *proto.GetMetadataResponse) { validSignOpts.SignatureMediaType = envelopeType data, signerInfo, err := pluginSigner.Sign(context.Background(), validSignDescriptor, validSignOpts) if err != nil { @@ -309,5 +374,5 @@ func basicSignTest(t *testing.T, pluginSigner *pluginSigner, envelopeType string if !reflect.DeepEqual(mockPlugin.certs, signerInfo.CertificateChain) { t.Fatalf(" Signer.Sign() cert chain changed") } - basicVerification(t, data, envelopeType, mockPlugin.certs[len(mockPlugin.certs)-1]) + basicVerification(t, data, envelopeType, mockPlugin.certs[len(mockPlugin.certs)-1], metadata) } diff --git a/signer/signer.go b/signer/signer.go index 83f7da62..d4cf5bb5 100644 --- a/signer/signer.go +++ b/signer/signer.go @@ -79,6 +79,12 @@ func (s *genericSigner) Sign(ctx context.Context, desc ocispec.Descriptor, opts return nil, nil, fmt.Errorf("envelope payload can't be marshalled: %w", err) } + var signingAgentId string + if opts.SigningAgent != "" { + signingAgentId = opts.SigningAgent + } else { + signingAgentId = signingAgent + } signReq := &signature.SignRequest{ Payload: signature.Payload{ ContentType: envelope.MediaTypePayloadV1, @@ -87,7 +93,7 @@ func (s *genericSigner) Sign(ctx context.Context, desc ocispec.Descriptor, opts Signer: s.Signer, SigningTime: time.Now(), SigningScheme: signature.SigningSchemeX509, - SigningAgent: signingAgent, // TODO: include external signing plugin's name and version. https://github.com/notaryproject/notation-go/issues/80 + SigningAgent: signingAgentId, } // Add expiry only if ExpiryDuration is not zero diff --git a/signer/signer_test.go b/signer/signer_test.go index a28a78c5..cfc8b930 100644 --- a/signer/signer_test.go +++ b/signer/signer_test.go @@ -13,6 +13,7 @@ import ( "fmt" "os" "path/filepath" + "regexp" "testing" "time" @@ -140,7 +141,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], nil) } func TestNewFromFiles(t *testing.T) { @@ -194,7 +195,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], &validMetadata) }) } } @@ -220,7 +221,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], nil) }) } } @@ -276,7 +277,7 @@ func generateSigningContent(tsa *timestamptest.TSA) (ocispec.Descriptor, notatio return desc, sOpts } -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 *proto.GetMetadataResponse) { // basic verification sigEnv, err := signature.ParseEnvelope(envelopeType, sig) if err != nil { @@ -296,6 +297,26 @@ 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, signingAgentId string, metadata *proto.GetMetadataResponse) { + signingAgentRegex := regexp.MustCompile("^(?P.*) (?P.*)/(?P.*)$") + match := signingAgentRegex.FindStringSubmatch(signingAgentId) + + results := map[string]string{} + for i, name := range match { + results[signingAgentRegex.SubexpNames()[i]] = name + } + + if metadata == nil { + if signingAgentId != signingAgent { + t.Fatalf("Expected signingAgent of %s but signature contained %s instead", signingAgent, signingAgentId) + } + } else if results["agent"] != signingAgent || results["name"] != metadata.Name || results["version"] != metadata.Version { + t.Fatalf("Expected signingAgent of %s %s/%s but signature contained %s instead", signingAgent, metadata.Name, metadata.Version, signingAgentId) + } } func validateSignWithCerts(t *testing.T, envelopeType string, key crypto.PrivateKey, certs []*x509.Certificate) { @@ -313,5 +334,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], nil) }