Skip to content

Commit

Permalink
Add additional header validation for payload (#178)
Browse files Browse the repository at this point in the history 
Addresses TODO's tracked in [Issue 80](#80)

- Note that new tests for NewSignerFromFiles were already added back [in September](2bcfd34)
- Validations around no top-level attributes being added are from [5.c.c](https://github.com/notaryproject/notaryproject/blob/main/specs/plugin-extensibility.md#signing-workflow-using-plugin-1) in the spec

Signed-off-by: Jonathan Donas <[email protected]>
  • Loading branch information
@jondonas
jondonas authored Dec 5, 2022
1 parent e9545a7 commit 0355e8e
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 43 deletions.
7 changes: 5 additions & 2 deletions notation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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.
Expand Down
51 changes: 45 additions & 6 deletions signer/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,14 @@ func (s *pluginSigner) Sign(ctx context.Context, desc ocispec.Descriptor, opts n

logger.Debugf("Using plugin %v with capabilities %v to sign artifact %v in signature media type %v", metadata.Name, metadata.Capabilities, desc.Digest, opts.SignatureMediaType)
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) {
logger := log.GetLogger(ctx)
logger.Debug("Generating signature by plugin")
config := s.mergeConfig(opts.PluginConfig)
Expand Down Expand Up @@ -94,6 +94,8 @@ func (s *pluginSigner) generateSignature(ctx context.Context, desc ocispec.Descr
keySpec: ks,
},
}

opts.SigningAgent = fmt.Sprintf("%s %s/%s", signingAgent, metadata.Name, metadata.Version)
return genericSigner.Sign(ctx, desc, opts)
}

Expand Down Expand Up @@ -141,17 +143,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, following unknown attributes were added to subject descriptor: %+q", unknownAttributes)
}

return resp.SignatureEnvelope, &envContent.SignerInfo, nil
}

Expand Down Expand Up @@ -197,6 +202,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 {
Expand Down
123 changes: 94 additions & 29 deletions signer/plugin_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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})
Expand All @@ -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"
Expand All @@ -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, following unknown attributes were added to subject descriptor: [\"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})
})
Expand All @@ -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)
})
}
}
Expand All @@ -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)
})
}
}
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
8 changes: 7 additions & 1 deletion signer/signer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,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,
Expand All @@ -90,7 +96,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
Expand Down
Loading

0 comments on commit 0355e8e

Please sign in to comment.