feat: added PKCS#7 envelope implementation #298
Conversation
Adds PKCS#7/CMS signature envelope implementation following the same pattern as existing JWS and COSE envelopes. Features: - Implements signature.Envelope interface (Sign, Verify, Content) - Uses go.mozilla.org/pkcs7 library for ASN.1 encoding - Uses signerAdapter pattern to support both local and remote signers - Works with Azure Key Vault and other plugins via signature.Signer - Produces detached signatures for dm-verity kernel verification - Supports RSA and ECDSA key types with SHA-256 - Registers media type application/pkcs7-signature The signerAdapter wraps pre-computed signatures from any signature.Signer to satisfy the crypto.Signer interface expected by the Mozilla library. This enables remote signers (like Azure Key Vault) that don't expose private keys to work with the library. Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
The base.Envelope wrapper validates that signing-time is present, but PKCS#7 signatures for dm-verity must not include authenticated attributes (including signing-time) per Linux kernel requirements. The kernel's PKCS#7 verifier in crypto/asymmetric_keys/public_key.c expects raw signature data without CMS authenticated attributes. Remove the base.Envelope wrapper from NewEnvelope() and ParseEnvelope() so the pkcs7 envelope implements signature.Envelope directly. Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
dallasd1
force-pushed
the
dadelan/pkcs7-envelope
branch
from
9dc2a7d to
7dc8aca
Compare
dallasd1
requested review from
NiazFK,
gokarnm,
priteshbandi,
rgnote,
shizhMSFT,
toddysm,
vaninrao10 and
yizha1
as code owners
|
This PR is stale because it has been opened for 45 days with no activity. Remove stale label or comment. Otherwise, it will be closed in 30 days. |
bketelsen
left a comment
bketelsen
left a comment
There was a problem hiding this comment.
PR looks good but there are a few blockers, critically Verify() doesn't actually do cryptographic validation and SHA256 is hardcoded for all key types.
| return nil, &signature.SignatureEnvelopeNotFoundError{} | ||
| } | ||
| // For detached signatures, the kernel does dm-verity verification | ||
| return e.Content() |
There was a problem hiding this comment.
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()
| } | ||
|
|
||
| // Set digest algorithm to SHA-256 (kernel dm-verity requirement) | ||
| sd.SetDigestAlgorithm(gopkcs7.OIDDigestAlgorithmSHA256) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // Parse the result to populate envelope fields | ||
| p7, _ := gopkcs7.Parse(encoded) |
There was a problem hiding this comment.
don't swallow an error on parsing... if caller sees success, subsequent calls to Verify() or Content() will be incorrect.
| // 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{} |
There was a problem hiding this comment.
jws and cose return &base.Envelope{Envelope: &envelope{}} which has critical validations. returning a bare envelope bypasses all of these checks.
|
|
||
| const testPayload = "test dm-verity root hash payload" | ||
|
|
||
| // newRSATestSigner creates an RSA-2048 test signer using testhelper certs. |
There was a problem hiding this comment.
need tests to validate other chains still work like RSA-3072 and ECDSA p-384
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| package pkcs7 |
There was a problem hiding this comment.
this test suite is missing many of the patterns present in jws and cose - fuzzing, negative tests, ECDSA, conformance. It is not complete without them.
| "github.com/notaryproject/notation-core-go/signature" | ||
| gopkcs7 "go.mozilla.org/pkcs7" | ||
| ) | ||
|
|
There was a problem hiding this comment.
jws and cose have compile-time assertions... this doesn't. var _ signature.Envelope = &envelope{}
There was a problem hiding this comment.
Important
Hybrid review. The dependency concern (Finding 1 on go.mozilla.org/pkcs7) reflects @shizhMSFT's own position and is the basis for this REQUEST_CHANGES. Everything else in this review was drafted by an AI agent (Claude Opus 4.7, shizh-reviewer skill) and is offered for the author's consideration only — please weigh those points on their merits, not as gating concerns.
Thanks for kicking this off, @dallasd1. notation-core-go is the cryptographic core SDK for the entire Notary Project ecosystem, so the bar on new direct dependencies is high.
Findings
Finding 1 below is the gating concern from @shizhMSFT. Findings 2–6 are AI-drafted observations to consider on their merits.
-
Do we need
go.mozilla.org/pkcs7at all?tspclient-goalready gives us most of the CMS surface in-house.notaryproject/tspclient-goshipsinternal/cms,internal/oid,internal/encoding/asn1/ber, andinternal/hashutil— a pure-stdlib RFC 5652 implementation that has been in production for RFC 3161 timestamping. Andnotation-core-go/go.modalready directly requiresgithub.com/notaryproject/tspclient-go v1.0.0(line 8 of this PR's go.mod), so reusing that code costs zero new modules; importinggo.mozilla.org/pkcs7adds one. Concrete contrast points:- Correct OIDs.
tspclient-go/internal/oid/oid.godefinesECDSAWithSHA256 = {1,2,840,10045,4,3,2}per RFC 5758 §3.2 (a signature-algorithm OID). This PR inheritsgopkcs7.OIDEncryptionAlgorithmECDSAP256 = {1,2,840,10045,3,1,7}which is thesecp256r1curve OID — putting it inSignerInfo.signatureAlgorithmviolates RFC 5652 §10.1.2 and produces a non-conformant CMS structure (Finding 5). - Real
Verify().tspclient-go/internal/cms/signed.go::verifySignatureperforms cert-chain validation, signature verification viacert.CheckSignature, and signed-attribute processing. This PR'sVerify()performs no cryptographic check at all (Finding 2). - No dead weight.
go.mozilla.org/pkcs7shipsdecrypt.go,encrypt.go, a BER parser, and DSA support — none of which dm-verity needs.
Concrete proposal — three options, in increasing order of permanence:
- (a) Copy + refactor later. Since both
notation-core-goandtspclient-golive undernotaryproject, copytspclient-go/internal/cms(+oid,ber,hashutil) intonotation-core-go/internal/cmsas a temporary measure, with a// TODOlinking to a tracking issue. Pros: zero coordination cost, lands in this PR. Cons: code duplication until we deduplicate; both copies will drift unless we explicitly track them. - (b) Lift
tspclient-go/internal/cmsto a non-internal package. E.g.github.com/notaryproject/tspclient-go/cms.notation-core-gothen imports it directly (it already requirestspclient-go v1.0.0). Pros: single source of truth; no new module. Cons: requires atspclient-gorelease with new public API surface, and any future change tocmsis now subject to thetspclient-goABI contract. - (c) New shared module
notaryproject/cms-go. Extract the CMS code into a dedicated repo + module that bothtspclient-goandnotation-core-goconsume. Pros: cleanest separation; mirrors howtspclient-goitself was factored out. Cons: new repo, new release cadence, more moving parts.
One caveat for any of these options: today
tspclient-go/internal/cmsonly implements parse + verify. It does not implement sign. So this is not a literal drop-in replacement for gopkcs7's signing path; it's "reuse the structs + BER + correct OIDs + verify, then add ~150 lines ofSignedData.Marshal+SignerInfo.Marshal." Still a substantially smaller delta than importinggo.mozilla.org/pkcs7, with correct OIDs from day one and a verifier that actually verifies./cc @priteshbandi @yizha1 — happy to file a tracking issue covering whichever path the maintainers prefer.
- Correct OIDs.
-
Verify()doesn't verify, and structurally cannot in this shape.envelope.go:188-194just returnsContent(). No cryptographic check. Worse, even fixing it is non-trivial: detached-PKCS#7 verification needs the original content, butsignature.Envelope.Verify()takes no argument — afterParseEnvelopethe envelope only holds the DER bytes. So unless we change the interface or stash the content in the envelope itself (which JWS/COSE don't have to do because they're attached), a freshly-parsed PKCS#7 envelope can never have a workingVerify()through this interface. This is the strongest signal that PKCS#7 doesn't fit thesignature.Envelopeabstraction (see design question). For now, registering a verifier that returns "verified" without verifying is worse than not registering it at all. -
Hardcoded SHA-256 + signature-passthrough adapter is cryptographically broken for any key that isn't RSA-2048. Walk through with RSA-3072:
proto.HashAlgorithmFromKeySpecreturns SHA-384, AKV'sSignDataAsync(RS384, payload)server-side hashes with SHA-384 and returnsRSA(SHA-384(payload)), this PR wraps that signature in a PKCS#7 SignerInfo whosedigestAlgorithmis hardcoded toid-sha256. Any verifier (kernel dm-verity or compliant CMS verifier) computesSHA-256(content)and the signature does not match. Same for RSA-4096 → SHA-512, EC P-384 → SHA-384, EC P-521 → SHA-512. ThesignerAdapter.Sign()ignoring thedigestparameter (a violation ofcrypto.Signer's contract) is what hides the mismatch. -
SignatureAlgorithmis mislabeled in the parsed envelope.extractAlgorithm(line 220-228) callskeySpec.SignatureAlgorithm(), which for anyKeyTypeRSAreturnsAlgorithmPS{256,384,512}(internal/algorithm/algorithm.go:83-90). But this PR produces RSASSA-PKCS#1 v1.5 signatures, not PSS. SoContent().SignerInfo.SignatureAlgorithmwill say "RSASSA-PSS" for a signature that is actually PKCS#1 v1.5 — a silent type-system lie. Fixing it requires addingRSASSA-PKCS1-v1_5-SHA-{256,384,512}tosignature.Algorithm(the enum currently has only PS/ES —signature/algorithm.go:29-34), which in turn requires a coordinated change tonotaryproject/specifications(signature-specification.md algorithm table) andnotation-goplugin proto validation. This is a cross-PR cascade — the feature stack does not work end-to-end without all three repos moving together. Worth filing a tracking issue. -
EC OIDs are wrong; should we just reject EC at the envelope layer? Tied to Finding 1:
getEncryptionOIDreturnsOIDEncryptionAlgorithmECDSAP256/384/521fromgo.mozilla.org/pkcs7, which are named-curve OIDs (prime256v1/secp384r1/secp521r1), not signature-algorithm OIDs. RFC 5652 §10.1.2 requiressignatureAlgorithminSignerInfoto identify the signature algorithm (e.g.,ecdsa-with-SHA2561.2.840.10045.4.3.2); putting a curve OID there fails any conformant CMS verifier. Given that gopkcs7 ships these incorrectly and the kernel dm-verity flow does not consume ECDSA anyway, should we just reject EC keys explicitly at the envelope boundary and remove the EC branches? -
Test coverage misses the critical path. Tests use a local
crypto.Signer. The PR's stated motivation forsignerAdapteris "to support remote signers (Azure Key Vault, plugins)" — but no test exercises that path. Please add a test that signs via a fake remote signer with a non-SHA-256 hash and asserts the resulting PKCS#7 either (a) fails to produce a SHA-256 envelope or (b) verifies againstSHA256(content)end-to-end with the leaf cert. The bug in Finding 3 will surface immediately.
Design question
Per the issue thread, dm-verity needs detached PKCS#7 with no signed attributes, hardcoded SHA-256, and RSA-PKCS#1 v1.5. That's a very narrow contract — barely a "signature envelope" in the JWS/COSE sense (no expiry, no signing-time, no envelope verify). And as Finding 2 shows, Verify() is structurally broken in this shape. Should this instead be a small pkcs7 helper package that exposes Sign(payload, signer) ([]byte, error) directly, without registering through signature.Envelope? Once we register MediaTypeEnvelope = "application/pkcs7-signature" in a release, removing or renaming it is a breaking change every future notation-core-go release must honor — for a single-consumer envelope. Are we OK with that ABI commitment?
Verdict: REQUEST_CHANGES from @shizhMSFT, gating specifically on Finding 1 (the go.mozilla.org/pkcs7 dependency in notation-core-go). Findings 2–6, the design question, the non-blocking suggestions, and all inline comments below are AI-drafted and offered for the author's consideration — not gating. Happy to discuss in the next community meeting.
Non-blocking suggestions
init()registration. Oncesignature.RegisterEnvelopeType("application/pkcs7-signature", ...)runs at import time, every downstream that importsnotation-core-go/signature(transitively, through any verifier) pulls ingo.mozilla.org/pkcs7. JWS/COSE do the same today, so this is an established pattern, not a new contract — but PKCS#7 has exactly one consumer (dm-verity). Should it be opt-in registration via blank import (_ "github.com/notaryproject/notation-core-go/signature/pkcs7"), the waydatabase/sqldrivers do?go.modhygiene. Line 15 listsgo.mozilla.org/pkcs7 v0.9.0 // indirect, butsignature/pkcs7/envelope.go:27imports it directly.go mod tidyshould move it to the directrequireblock.
| func init() { | ||
| if err := signature.RegisterEnvelopeType(MediaTypeEnvelope, NewEnvelope, ParseEnvelope); err != nil { | ||
| panic(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
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).
| func NewEnvelope() signature.Envelope { | ||
| return &envelope{} | ||
| } |
There was a problem hiding this comment.
NewEnvelope()returns a zero-value*envelope, thenSign()mutatese.raw/p7/certs/sigBytes(lines 134–138) so subsequentVerify/Contentcalls return what was just signed. This conflates construction with signing-result. The JWS/COSE envelopes usebase.Envelopeprecisely to separate these. The PR comment acknowledges skippingbase.Envelope"because dm-verity signatures must not have signing-time" — butbase.Envelopedoesn't force signing-time on you; only the envelope-specificSignmethod does. Please reconsider; sharingbase.Envelope(with the signing-time bypass) gives consistent state semantics and freePayload()accessors.
| // Sign implements signature.Envelope interface. | ||
| // Uses signerAdapter pattern to support both local and remote signers (AKV, plugins). |
There was a problem hiding this comment.
| // 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.
| } | ||
|
|
||
| // Set digest algorithm to SHA-256 (kernel dm-verity requirement) | ||
| sd.SetDigestAlgorithm(gopkcs7.OIDDigestAlgorithmSHA256) |
There was a problem hiding this comment.
See top-level Finding 3.
| sd.SetEncryptionAlgorithm(encryptionOID) | ||
|
|
||
| // Sign without authenticated attributes (kernel dm-verity requirement) | ||
| if err := sd.SignWithoutAttr(certs[0], adapter, gopkcs7.SignerInfoConfig{}); err != nil { |
There was a problem hiding this comment.
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.
| // 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 | ||
| } |
There was a problem hiding this comment.
| // 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.
| 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 |
There was a problem hiding this comment.
See top-level Finding 4.
keySpec.SignatureAlgorithm()forKeyTypeRSAreturnsAlgorithmPS{256,384,512}(internal/algorithm/algorithm.go:83-90) — RSASSA-PSS, not PKCS#1 v1.5. So the parsedEnvelopeContent.SignerInfo.SignatureAlgorithmreports the wrong algorithm. To fix this honestly we need a newsignature.Algorithmenum member (RSASSA-PKCS1-v1_5-SHA-{256,384,512}), which in turn requires coordinated changes innotaryproject/specificationsandnotation-goplugin proto validation. Please file a tracking issue covering all three repos.
| func newRSATestSigner() *testPrimitiveSigner { | ||
| tuple := testhelper.GetRSACertTuple(2048) | ||
| rootCert := testhelper.GetRSARootCertificate().Cert | ||
| return &testPrimitiveSigner{ | ||
| key: tuple.PrivateKey, | ||
| certs: []*x509.Certificate{tuple.Cert, rootCert}, | ||
| keySpec: signature.KeySpec{Type: signature.KeyTypeRSA, Size: 2048}, | ||
| } | ||
| } |
There was a problem hiding this comment.
Once the keyspec restriction lands per the comment above, please add explicit negative tests: RSA-3072, RSA-4096, EC P-256/P-384/P-521 should all return
UnsupportedSigningKeyError. Right now the test signs with RSA-2048 and never exercises the buggy paths.
| // TestSignError tests that Sign fails with a broken signer. | ||
| func TestSignError(t *testing.T) { | ||
| env := NewEnvelope() | ||
| req := &signature.SignRequest{ | ||
| Payload: signature.Payload{ | ||
| ContentType: MediaTypeEnvelope, | ||
| Content: []byte(testPayload), | ||
| }, | ||
| Signer: &failingSigner{}, | ||
| } | ||
| _, err := env.Sign(req) | ||
| if err == nil { | ||
| t.Fatal("Sign() with failing signer expected error, got nil") | ||
| } | ||
| } |
There was a problem hiding this comment.
The whole stated motivation for
signerAdapteris supporting remote signers. Please add a test that:
- Uses a fake
signature.SignerwhoseSign(content)returns a precomputedRSA-PKCS#1 v1.5(SHA-256(content))from a known cert chain.- Signs via the envelope.
- Parses the result and independently recomputes
SHA256(content)and verifies the signature withrsa.VerifyPKCS1v15.This is the contract dm-verity needs end-to-end. If it doesn't pass with RSA-3072 today, the bug surfaces immediately.
| 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 |
There was a problem hiding this comment.
See top-level Finding 1.
notation-core-go/go.modalready requiresgithub.com/notaryproject/tspclient-go v1.0.0directly, whose (currentlyinternal)cms+oidpackages already implement RFC 5652 SignedData parse + verify on stdlib with correct OIDs. Three options to reuse that work without addinggo.mozilla.org/pkcs7: (a) copy the code intonotation-core-go/internal/cmsas a temporary measure (same-org duplication, refactor later), (b) lifttspclient-go/internal/cmsto a non-internal package and import directly, or (c) extract to a new sharednotaryproject/cms-gomodule. Worth raising with @priteshbandi / @rgnote before merging.
4 participants
Add the PKCS#7 envelope implementation to support creating signatures for dm-verity. The kernel's dm-verity feature requires PKCS#7 signatures without authenticated attributes.
This PR leverages the open source mozilla pkcs7 package.
#298