Skip to content

Commit

Permalink
Add additional header validation for payload
Browse files Browse the repository at this point in the history 
Signed-off-by: Jonathan Donas <[email protected]>
  • Loading branch information
@jondonas
jondonas committed Dec 2, 2022
1 parent bc022cc commit e48db41
Show file tree
Hide file tree
Showing 5 changed files with 176 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
50 changes: 44 additions & 6 deletions signer/plugin.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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)
}

Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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 {
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 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})
})
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 @@ -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,
Expand All @@ -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
Expand Down
Loading

0 comments on commit e48db41

Please sign in to comment.