Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,7 @@ require (
golang.org/x/crypto v0.37.0
)

require github.com/x448/float16 v0.8.4 // indirect
require (
github.com/x448/float16 v0.8.4 // indirect
go.mozilla.org/pkcs7 v0.9.0 // indirect
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See top-level Finding 1. notation-core-go/go.mod already requires github.com/notaryproject/tspclient-go v1.0.0 directly, whose (currently internal) cms + oid packages already implement RFC 5652 SignedData parse + verify on stdlib with correct OIDs. Three options to reuse that work without adding go.mozilla.org/pkcs7: (a) copy the code into notation-core-go/internal/cms as a temporary measure (same-org duplication, refactor later), (b) lift tspclient-go/internal/cms to a non-internal package and import directly, or (c) extract to a new shared notaryproject/cms-go module. Worth raising with @priteshbandi / @rgnote before merging.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing to the in-tree CMS library @shizhMSFT. Agreed that dropping go.mozilla.org/pkcs7 is the right call here.

I see two reasonable paths and would like guidance before pushing:

(a) Copy tspclient-go/internal/cms into notation-core-go/internal/cms. This would be fast to land but introduces duplication and the two copies could drift. tspclient-go/internal/cms has been stable for ~a year so that risk seems to be low.

(b) Lift tspclient-go/internal/cms to a public tspclient-go/cms package and have notation-core-go import it. This seems to be cleaner long-term but requires a new PR and a new tspclient-go release.

I'm happy to push (a) with a tracking issue, or switch to (b) if you'd prefer. Please let me know which direction you'd like to take.

)
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,7 @@ github.com/veraison/go-cose v1.3.0 h1:2/H5w8kdSpQJyVtIhx8gmwPJ2uSz1PkyWFx0idbd7r
github.com/veraison/go-cose v1.3.0/go.mod h1:df09OV91aHoQWLmy1KsDdYiagtXgyAwAl8vFeFn1gMc=
github.com/x448/float16 v0.8.4 h1:qLwI1I70+NjRFUR3zs1JPUCgaCXSh3SW62uAKT1mSBM=
github.com/x448/float16 v0.8.4/go.mod h1:14CWIYCyZA/cWjXOioeEpHeN/83MdbZDRQHoFcYsOfg=
go.mozilla.org/pkcs7 v0.9.0 h1:yM4/HS9dYv7ri2biPtxt8ikvB37a980dg69/pKmS+eI=
go.mozilla.org/pkcs7 v0.9.0/go.mod h1:SNgMg+EgDFwmvSmLRTNKC5fegJjB7v23qTQ0XLGUNHk=
golang.org/x/crypto v0.37.0 h1:kJNSjF/Xp7kU0iB2Z+9viTPMW4EqqsrywMXLJOOsXSE=
golang.org/x/crypto v0.37.0/go.mod h1:vg+k43peMZ0pUMhYmVAWysMK35e6ioLh3wB8ZCAfbVc=
229 changes: 229 additions & 0 deletions signature/pkcs7/envelope.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
// Copyright The Notary Project Authors.
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

// Package pkcs7 provides PKCS#7/CMS signature envelope for dm-verity signing.
// Supports both local keys and remote signers (Azure Key Vault, plugins) via
// the signerAdapter pattern.
package pkcs7

import (
"crypto"
"crypto/x509"
"encoding/asn1"
"fmt"
"io"

"github.com/notaryproject/notation-core-go/signature"
gopkcs7 "go.mozilla.org/pkcs7"
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jws and cose have compile-time assertions... this doesn't. var _ signature.Envelope = &envelope{}

// MediaTypeEnvelope is the PKCS#7 signature envelope media type.
const MediaTypeEnvelope = "application/pkcs7-signature"

func init() {
if err := signature.RegisterEnvelopeType(MediaTypeEnvelope, NewEnvelope, ParseEnvelope); err != nil {
panic(err)
}
}
Comment on lines +33 to +37
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See top-level "non-blocking suggestions". JWS/COSE register the same way, but PKCS#7 has a single consumer (dm-verity). Worth considering blank-import opt-in (database/sql-driver pattern).


type envelope struct {
raw []byte
p7 *gopkcs7.PKCS7
certs []*x509.Certificate
sigBytes []byte
}

// NewEnvelope creates a new PKCS#7 envelope.
// Note: Unlike JWS/COSE, PKCS#7 for dm-verity does NOT use base.Envelope wrapper
// because dm-verity signatures must not have signing-time (kernel requirement).
func NewEnvelope() signature.Envelope {
return &envelope{}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jws and cose return &base.Envelope{Envelope: &envelope{}} which has critical validations. returning a bare envelope bypasses all of these checks.

}
Comment on lines +49 to +51
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NewEnvelope() returns a zero-value *envelope, then Sign() mutates e.raw/p7/certs/sigBytes (lines 134–138) so subsequent Verify/Content calls return what was just signed. This conflates construction with signing-result. The JWS/COSE envelopes use base.Envelope precisely to separate these. The PR comment acknowledges skipping base.Envelope "because dm-verity signatures must not have signing-time" — but base.Envelope doesn't force signing-time on you; only the envelope-specific Sign method does. Please reconsider; sharing base.Envelope (with the signing-time bypass) gives consistent state semantics and free Payload() accessors.


// ParseEnvelope parses PKCS#7 DER bytes into an envelope.
func ParseEnvelope(envelopeBytes []byte) (signature.Envelope, error) {
p7, err := gopkcs7.Parse(envelopeBytes)
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: err.Error()}
}

var sigBytes []byte
if len(p7.Signers) > 0 {
sigBytes = p7.Signers[0].EncryptedDigest
}

return &envelope{
raw: envelopeBytes,
p7: p7,
certs: p7.Certificates,
sigBytes: sigBytes,
}, nil
}

// Sign implements signature.Envelope interface.
// Uses signerAdapter pattern to support both local and remote signers (AKV, plugins).
Comment on lines +73 to +74
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Sign implements signature.Envelope interface.
// Uses signerAdapter pattern to support both local and remote signers (AKV, plugins).
// Sign implements signature.Envelope interface.
//
// Constraints (kernel dm-verity, see kernel docs/admin-guide/device-mapper/verity.rst):
// - RSASSA-PKCS#1 v1.5 only (no PSS, no ECDSA)
// - SHA-256 only
// - No signed attributes (so the signature is over SHA-256(content) directly)
//
// The implementation MUST therefore reject any KeySpec for which the upstream
// signer would not produce a SHA-256 RSASSA-PKCS#1 v1.5 signature (today,
// only RSA-2048 — see proto.HashAlgorithmFromKeySpec), or it will declare
// digestAlgorithm = id-sha256 in the envelope while the actual signature
// covers a SHA-384/SHA-512 digest.

The current code declares SHA-256 in the envelope but accepts any signature the upstream signer produces. See top-level Finding 3 for the full chain. Two ways to fix the body: (a) restrict req.Signer keyspecs to RSA-2048 and reject anything else with UnsupportedSigningKeyError, or (b) introduce a signature.Signer extension that lets us request an explicit hash algorithm before signing.

func (e *envelope) Sign(req *signature.SignRequest) ([]byte, error) {
// Get key spec to determine algorithm
keySpec, err := req.Signer.KeySpec()
if err != nil {
return nil, &signature.InvalidSignRequestError{Msg: err.Error()}
}

// Sign the content using the underlying signer (works with AKV, local keys, etc.)
sig, certs, err := req.Signer.Sign(req.Payload.Content)
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: fmt.Sprintf("signing failed: %v", err)}
}

if len(certs) == 0 {
return nil, &signature.InvalidSignatureError{Msg: "no certificates returned from signer"}
}

// Create a crypto.Signer adapter that returns our pre-computed signature
adapter := &signerAdapter{
sig: sig,
certs: certs,
}

// Build PKCS#7 SignedData using mozilla library
sd, err := gopkcs7.NewSignedData(req.Payload.Content)
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: err.Error()}
}

// Set digest algorithm to SHA-256 (kernel dm-verity requirement)
sd.SetDigestAlgorithm(gopkcs7.OIDDigestAlgorithmSHA256)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notary spec has algo/hash pairings:

  • RSA-3072 → SHA-384; RSA-4096 → SHA-512
  • ECDSA P-384 → SHA-384; ECDSA P-521 → SHA-512

but this hardcodes SHA-256 for all. Tests only cover RSA-2048 so this bug is never exposed.

Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See top-level Finding 3.


// Set encryption algorithm based on key type
encryptionOID, err := getEncryptionOID(keySpec)
if err != nil {
return nil, &signature.UnsupportedSigningKeyError{Msg: err.Error()}
}
sd.SetEncryptionAlgorithm(encryptionOID)

// Sign without authenticated attributes (kernel dm-verity requirement)
if err := sd.SignWithoutAttr(certs[0], adapter, gopkcs7.SignerInfoConfig{}); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per the upstream comment: "This function is needed to sign old Android APKs, something you probably shouldn't do unless you're maintaining backward compatibility for old applications." Please add a comment explaining why we use it here (kernel dm-verity requires no SignedAttributes) so future readers don't think we hit this by accident.

return nil, &signature.InvalidSignatureError{Msg: err.Error()}
}

// Add intermediate/CA certificates to the chain
for i := 1; i < len(certs); i++ {
sd.AddCertificate(certs[i])
}

// Create detached signature (content not embedded)
sd.Detach()

encoded, err := sd.Finish()
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: err.Error()}
}

// Parse the result to populate envelope fields
p7, _ := gopkcs7.Parse(encoded)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't swallow an error on parsing... if caller sees success, subsequent calls to Verify() or Content() will be incorrect.

e.raw = encoded
e.p7 = p7
e.certs = certs
if p7 != nil && len(p7.Signers) > 0 {
e.sigBytes = p7.Signers[0].EncryptedDigest
}
Comment on lines +132 to +139
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Parse the result to populate envelope fields
p7, _ := gopkcs7.Parse(encoded)
e.raw = encoded
e.p7 = p7
e.certs = certs
if p7 != nil && len(p7.Signers) > 0 {
e.sigBytes = p7.Signers[0].EncryptedDigest
}
p7, err := gopkcs7.Parse(encoded)
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: fmt.Sprintf("self-parse failed after Sign: %v", err)}
}
e.raw = encoded
e.p7 = p7
e.certs = certs
if len(p7.Signers) > 0 {
e.sigBytes = p7.Signers[0].EncryptedDigest
}

"Always check errors before processing any return values." We just produced the bytes ourselves; if Parse fails on our own output, something is very wrong and we should fail fast.


return encoded, nil
}

// getEncryptionOID returns the encryption algorithm OID for the key spec.
func getEncryptionOID(keySpec signature.KeySpec) (asn1.ObjectIdentifier, error) {
switch keySpec.Type {
case signature.KeyTypeRSA:
return gopkcs7.OIDEncryptionAlgorithmRSA, nil
case signature.KeyTypeEC:
switch keySpec.Size {
case 256:
return gopkcs7.OIDEncryptionAlgorithmECDSAP256, nil
case 384:
return gopkcs7.OIDEncryptionAlgorithmECDSAP384, nil
case 521:
return gopkcs7.OIDEncryptionAlgorithmECDSAP521, nil
default:
return nil, fmt.Errorf("unsupported EC key size: %d", keySpec.Size)
}
default:
return nil, fmt.Errorf("unsupported key type: %v", keySpec.Type)
}
}
Comment on lines +144 to +163
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// getEncryptionOID returns the encryption algorithm OID for the key spec.
func getEncryptionOID(keySpec signature.KeySpec) (asn1.ObjectIdentifier, error) {
switch keySpec.Type {
case signature.KeyTypeRSA:
return gopkcs7.OIDEncryptionAlgorithmRSA, nil
case signature.KeyTypeEC:
switch keySpec.Size {
case 256:
return gopkcs7.OIDEncryptionAlgorithmECDSAP256, nil
case 384:
return gopkcs7.OIDEncryptionAlgorithmECDSAP384, nil
case 521:
return gopkcs7.OIDEncryptionAlgorithmECDSAP521, nil
default:
return nil, fmt.Errorf("unsupported EC key size: %d", keySpec.Size)
}
default:
return nil, fmt.Errorf("unsupported key type: %v", keySpec.Type)
}
}
// getEncryptionOID returns the encryption algorithm OID for the key spec.
// Only RSA is supported because the Linux kernel's dm-verity feature
// only verifies RSASSA-PKCS#1 v1.5 signatures.
func getEncryptionOID(keySpec signature.KeySpec) (asn1.ObjectIdentifier, error) {
if keySpec.Type != signature.KeyTypeRSA || keySpec.Size != 2048 {
return nil, fmt.Errorf("dm-verity PKCS#7 only supports RSA-2048; got %v-%d", keySpec.Type, keySpec.Size)
}
return gopkcs7.OIDEncryptionAlgorithmRSA, nil
}

Two issues with the current code: (1) the EC branches are misleading because the OIDs returned (OIDEncryptionAlgorithmECDSAP256/384/521 from gopkcs7) are named-curve OIDs, not signature-algorithm OIDs — putting them in SignerInfo.digestEncryptionAlgorithm produces a non-conformant CMS structure per RFC 5652 §10.1.2; (2) RSA-3072/4096 are accepted but produce a hash mismatch with the hardcoded SHA-256 digest algorithm. Restricting to RSA-2048 fixes both.


// signerAdapter wraps a pre-computed signature to satisfy crypto.Signer interface.
// This enables remote signers (Azure Key Vault, plugins) to work with the
// mozilla pkcs7 library which expects crypto.Signer.
type signerAdapter struct {
sig []byte // pre-computed signature from actual signer
certs []*x509.Certificate // certificate chain
}
Comment on lines +165 to +171
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// signerAdapter wraps a pre-computed signature to satisfy crypto.Signer interface.
// This enables remote signers (Azure Key Vault, plugins) to work with the
// mozilla pkcs7 library which expects crypto.Signer.
type signerAdapter struct {
sig []byte // pre-computed signature from actual signer
certs []*x509.Certificate // certificate chain
}
// signerAdapter satisfies crypto.Signer by replaying a signature that was
// pre-computed by the upstream signature.Signer. It is required because
// gopkcs7.SignedData.SignWithoutAttr expects a crypto.Signer, but our remote
// signers (AKV plugin, KMS plugins) cannot expose a long-lived private key.
//
// SECURITY INVARIANT: the caller MUST guarantee that `sig` was produced over
// the SAME hash algorithm that gopkcs7 will pass to Sign() (i.e., the digest
// algorithm set via SetDigestAlgorithm). This adapter intentionally ignores
// the `digest` and `opts` arguments because the signature is already finalized;
// any mismatch between the envelope's declared digest algorithm and the
// signer's actual hash will produce a PKCS#7 that no verifier can validate.
type signerAdapter struct {
sig []byte // pre-computed RSA-PKCS#1 v1.5 signature over SHA-256(content)
certs []*x509.Certificate // certificate chain, leaf at index 0
}

The current implementation silently violates crypto.Signer's documented contract ("Sign signs digest with the private key, possibly using entropy from rand"). The digest parameter is a []byte that the caller computed, and a conformant crypto.Signer must sign that digest. Returning a precomputed signature unrelated to the passed digest is a foot-gun for any future maintainer who plumbs this adapter through a different code path. Please at minimum document the invariant explicitly. Better: pass the SHA-256 digest down to the upstream signer and have it sign that, so the adapter becomes a true crypto.Signer.


// Public returns the public key from the leaf certificate.
func (a *signerAdapter) Public() crypto.PublicKey {
if len(a.certs) > 0 {
return a.certs[0].PublicKey
}
return nil
}

// Sign returns the pre-computed signature.
// The digest parameter is ignored since signing already happened via req.Signer.Sign().
func (a *signerAdapter) Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error) {
return a.sig, nil
}

// Verify implements signature.Envelope interface.
func (e *envelope) Verify() (*signature.EnvelopeContent, error) {
if e.raw == nil {
return nil, &signature.SignatureEnvelopeNotFoundError{}
}
// For detached signatures, the kernel does dm-verity verification
return e.Content()
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method calls Content() which only parses structure... no cert chain validation or expiration check. cose and jws implementations each validate, but this doesn't. should call e.p7.Verify()

}
Comment on lines +188 to +194
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
func (e *envelope) Verify() (*signature.EnvelopeContent, error) {
if e.raw == nil {
return nil, &signature.SignatureEnvelopeNotFoundError{}
}
// For detached signatures, the kernel does dm-verity verification
return e.Content()
}
// Verify implements signature.Envelope interface by performing detached-PKCS#7
// signature verification: it recomputes SHA-256(content) and verifies the
// signature against the leaf certificate's public key.
//
// NOTE: dm-verity verification on the kernel side is independent and out of
// scope. This method establishes the cryptographic validity of the signature
// for any in-process consumer (tests, pre-submission checks, audit tooling).
func (e *envelope) Verify() (*signature.EnvelopeContent, error) {
if e.raw == nil {
return nil, &signature.SignatureEnvelopeNotFoundError{}
}
if e.p7 == nil {
return nil, &signature.InvalidSignatureError{Msg: "envelope not parsed"}
}
if err := e.p7.Verify(); err != nil {
return nil, &signature.SignatureIntegrityError{Err: err}
}
return e.Content()
}

A signature.Envelope.Verify() that doesn't verify is a contract violation that downstream code (including notation-go's verifier dispatch) cannot detect. The Mozilla pkcs7 library exposes (*PKCS7).Verify() — please use it. If the envelope is detached, the parser already needs the content to verify; we should require callers to provide it via something equivalent to the existing signature.SignRequest/Envelope.Content() flow. If we genuinely cannot do this verification (because dm-verity is out-of-process), then PKCS#7 should not implement signature.Envelope at all; it should be a separate, smaller interface.


// Content implements signature.Envelope interface.
func (e *envelope) Content() (*signature.EnvelopeContent, error) {
if e.raw == nil {
return nil, &signature.SignatureEnvelopeNotFoundError{}
}

alg, err := extractAlgorithm(e.certs)
if err != nil {
return nil, &signature.InvalidSignatureError{Msg: err.Error()}
}

return &signature.EnvelopeContent{
SignerInfo: signature.SignerInfo{
SignatureAlgorithm: alg,
CertificateChain: e.certs,
Signature: e.sigBytes,
},
Payload: signature.Payload{
ContentType: MediaTypeEnvelope,
},
}, nil
}

// extractAlgorithm derives the signature algorithm from the leaf certificate.
func extractAlgorithm(certs []*x509.Certificate) (signature.Algorithm, error) {
if len(certs) == 0 {
return 0, fmt.Errorf("no certificates available to determine algorithm")
}
keySpec, err := signature.ExtractKeySpec(certs[0])
if err != nil {
return 0, err
}
return keySpec.SignatureAlgorithm(), nil
Comment on lines +220 to +228
Copy link
Copy Markdown
Contributor

@shizhMSFT shizhMSFT May 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See top-level Finding 4. keySpec.SignatureAlgorithm() for KeyTypeRSA returns AlgorithmPS{256,384,512} (internal/algorithm/algorithm.go:83-90) — RSASSA-PSS, not PKCS#1 v1.5. So the parsed EnvelopeContent.SignerInfo.SignatureAlgorithm reports the wrong algorithm. To fix this honestly we need a new signature.Algorithm enum member (RSASSA-PKCS1-v1_5-SHA-{256,384,512}), which in turn requires coordinated changes in notaryproject/specifications and notation-go plugin proto validation. Please file a tracking issue covering all three repos.

}
Loading