feat: added PKCS#7 envelope implementation by dallasd1 · Pull Request #298 · notaryproject/notation-core-go

dallasd1 · 2026-03-12T23:08:12Z

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

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>

github-actions · 2026-04-27T02:17:28Z

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

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.

bketelsen · 2026-05-01T15:20:10Z

+		return nil, &signature.SignatureEnvelopeNotFoundError{}
+	}
+	// For detached signatures, the kernel does dm-verity verification
+	return e.Content()


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()

The initial proposal highlights that verification for this envelope happens in the host kernel via a trusted keyring, so Verify() doesn't check the layer signatures but informs the caller of the type of signatures included. In the current implementation, Verify() implements this by returning ErrDetachedNotVerifiable.

This envelope is an addition to the existing JWS/COSE digest signatures rather than a replacement, so users still run the existing JWS/COSE Verify() on the image manifest before pulling the image.

Alternatively, we could extend notation-go's verifier to not only pull the manifest signatures but also these new layer signatures. Then we could add a new API to verify this detached signature against the payload and validate the certificate chain. Since this would be non-trivial and the new dm-verity signing is gated behind the NOTATION_EXPERIMENTAL flag, it may be worth considering as a follow-up. Happy to create an issue for this if you'd like.

bketelsen · 2026-05-01T15:29:47Z

+// 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{}


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

This initial PR is scoped to a narrow profile of detached PKCS#7, no signed attributes, rsa-2048, and sha-256, validated end-to-end. The long-term plan would be to broaden this profile after the initial change lands. Broadening the profile would likely mean also coordinating changes to notation/specifications

Given the scope described above, this envelope intentionally does not wrap base.Envelope because it would require SigningTime and SigningScheme to be set. The PR as-is will only require RSA-2048 + SHA-256.

To ensure validation is still present, we use validateSignRequest() and verifySignerOutput(), which does similar validation checks (except for ValidateCodeSigningCertChain(), which can be added if you prefer)

shizhMSFT

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/pkcs7 at all? tspclient-go already gives us most of the CMS surface in-house. notaryproject/tspclient-go ships internal/cms, internal/oid, internal/encoding/asn1/ber, and internal/hashutil — a pure-stdlib RFC 5652 implementation that has been in production for RFC 3161 timestamping. And notation-core-go/go.mod already directly requires github.com/notaryproject/tspclient-go v1.0.0 (line 8 of this PR's go.mod), so reusing that code costs zero new modules; importing go.mozilla.org/pkcs7 adds one. Concrete contrast points:
- Correct OIDs. tspclient-go/internal/oid/oid.go defines ECDSAWithSHA256 = {1,2,840,10045,4,3,2} per RFC 5758 §3.2 (a signature-algorithm OID). This PR inherits gopkcs7.OIDEncryptionAlgorithmECDSAP256 = {1,2,840,10045,3,1,7} which is the secp256r1 curve OID — putting it in SignerInfo.signatureAlgorithm violates RFC 5652 §10.1.2 and produces a non-conformant CMS structure (Finding 5).
- Real Verify(). tspclient-go/internal/cms/signed.go::verifySignature performs cert-chain validation, signature verification via cert.CheckSignature, and signed-attribute processing. This PR's Verify() performs no cryptographic check at all (Finding 2).
- No dead weight. go.mozilla.org/pkcs7 ships decrypt.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-go and tspclient-go live under notaryproject, copy tspclient-go/internal/cms (+ oid, ber, hashutil) into notation-core-go/internal/cms as a temporary measure, with a // TODO linking 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/cms to a non-internal package. E.g. github.com/notaryproject/tspclient-go/cms. notation-core-go then imports it directly (it already requires tspclient-go v1.0.0). Pros: single source of truth; no new module. Cons: requires a tspclient-go release with new public API surface, and any future change to cms is now subject to the tspclient-go ABI contract.
- (c) New shared module notaryproject/cms-go. Extract the CMS code into a dedicated repo + module that both tspclient-go and notation-core-go consume. Pros: cleanest separation; mirrors how tspclient-go itself was factored out. Cons: new repo, new release cadence, more moving parts.
One caveat for any of these options: today tspclient-go/internal/cms only 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 of SignedData.Marshal + SignerInfo.Marshal." Still a substantially smaller delta than importing go.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.
Verify() doesn't verify, and structurally cannot in this shape. envelope.go:188-194 just returns Content(). No cryptographic check. Worse, even fixing it is non-trivial: detached-PKCS#7 verification needs the original content, but signature.Envelope.Verify() takes no argument — after ParseEnvelope the 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 working Verify() through this interface. This is the strongest signal that PKCS#7 doesn't fit the signature.Envelope abstraction (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.HashAlgorithmFromKeySpec returns SHA-384, AKV's SignDataAsync(RS384, payload) server-side hashes with SHA-384 and returns RSA(SHA-384(payload)), this PR wraps that signature in a PKCS#7 SignerInfo whose digestAlgorithm is hardcoded to id-sha256. Any verifier (kernel dm-verity or compliant CMS verifier) computes SHA-256(content) and the signature does not match. Same for RSA-4096 → SHA-512, EC P-384 → SHA-384, EC P-521 → SHA-512. The signerAdapter.Sign() ignoring the digest parameter (a violation of crypto.Signer's contract) is what hides the mismatch.
SignatureAlgorithm is mislabeled in the parsed envelope. extractAlgorithm (line 220-228) calls keySpec.SignatureAlgorithm(), which for any KeyTypeRSA returns AlgorithmPS{256,384,512} (internal/algorithm/algorithm.go:83-90). But this PR produces RSASSA-PKCS#1 v1.5 signatures, not PSS. So Content().SignerInfo.SignatureAlgorithm will say "RSASSA-PSS" for a signature that is actually PKCS#1 v1.5 — a silent type-system lie. Fixing it requires adding RSASSA-PKCS1-v1_5-SHA-{256,384,512} to signature.Algorithm (the enum currently has only PS/ES — signature/algorithm.go:29-34), which in turn requires a coordinated change to notaryproject/specifications (signature-specification.md algorithm table) and notation-go plugin 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: getEncryptionOID returns OIDEncryptionAlgorithmECDSAP256/384/521 from go.mozilla.org/pkcs7, which are named-curve OIDs (prime256v1/secp384r1/secp521r1), not signature-algorithm OIDs. RFC 5652 §10.1.2 requires signatureAlgorithm in SignerInfo to identify the signature algorithm (e.g., ecdsa-with-SHA256 1.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 for signerAdapter is "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 against SHA256(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. Once signature.RegisterEnvelopeType("application/pkcs7-signature", ...) runs at import time, every downstream that imports notation-core-go/signature (transitively, through any verifier) pulls in go.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 way database/sql drivers do?
go.mod hygiene. Line 15 lists go.mozilla.org/pkcs7 v0.9.0 // indirect, but signature/pkcs7/envelope.go:27 imports it directly. go mod tidy should move it to the direct require block.

shizhMSFT · 2026-05-02T09:05:32Z

+func NewEnvelope() signature.Envelope {
+	return &envelope{}
+}


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.

Sharing from comment above, this initial PR is scoped to a narrow profile of detached PKCS#7 and gated behind NOTATION_EXPERIMENTAL, but if base.Envelope is required at this stage I can draft up that change.

shizhMSFT · 2026-05-02T09:05:32Z

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


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.

Will add comment in a new commit bundled with the pending tspclient change

shizhMSFT · 2026-05-02T09:05:33Z

+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


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.

Will file the issue for the new enums linking the changes in each repo independently of this PR

shizhMSFT · 2026-05-02T09:05:33Z

-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


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.

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.

- Restrict to RSA-2048 + SHA-256 + RSASSA-PKCS#1 v1.5; reject other keys with UnsupportedSigningKeyError and remove the unreachable EC branches in getEncryptionOID. - Reject SignRequest fields the dm-verity profile cannot honor (SigningTime, Expiry, SigningScheme, SigningAgent, ExtendedSignedAttributes) and nil Signer with InvalidSignRequestError. - Verify upstream signer output is RSASSA-PKCS#1 v1.5 over SHA-256 of the payload before wrapping it. - Verify() returns exported sentinel ErrDetachedNotVerifiable; detached PKCS#7 cannot be verified through signature.Envelope.Verify. - Content() leaves SignerInfo.SignatureAlgorithm zero rather than mislabeling PKCS#1 v1.5 as PSS (enum lacks PKCS#1 v1.5 constants). - Surface the post-Sign Parse error instead of swallowing it. - Recover from gopkcs7 v0.9.0 panics on malformed BER. - signerAdapter panics on a second Sign call (single-use invariant). - Add compile-time signature.Envelope assertion; go mod tidy moves gopkcs7 to direct require. - Add negative tests, seeded fuzz target, and conformance test (OIDs, no signed attrs, detached, chain ordering, independent rsa.VerifyPKCS1v15). Signed-off-by: Dallas Delaney <dadelan@microsoft.com>

Copilot

Pull request overview

Adds a new signature/pkcs7 envelope implementation intended to produce dm-verity–compatible PKCS#7/CMS SignedData (detached, no authenticated/signed attributes), leveraging go.mozilla.org/pkcs7.

Changes:

Introduces a PKCS#7 envelope with Sign, ParseEnvelope, Content, and a dm-verity-specific Verify behavior.
Adds unit, conformance, and fuzz tests for the new PKCS#7 envelope implementation.
Adds the go.mozilla.org/pkcs7 dependency.

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
signature/pkcs7/envelope.go	Implements PKCS#7 envelope creation/parsing for dm-verity profile and registers the envelope type
signature/pkcs7/envelope_test.go	Adds unit tests for signing/parsing and request validation behaviors
signature/pkcs7/conformance_test.go	Validates produced PKCS#7 structure matches dm-verity expectations
signature/pkcs7/fuzz_test.go	Adds fuzz coverage for parsing and calling `Verify`/`Content`
go.mod	Adds `go.mozilla.org/pkcs7` dependency
go.sum	Records checksums for the new dependency

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- ParseEnvelope rejects non-1 signer count and empty EncryptedDigest - validateSignRequest rejects empty Payload.Content (matches base.Envelope) - verifySignerOutput guards against nil leaf certificate - signerAdapter.Sign returns error instead of panicking on second call - Content().Payload.ContentType is empty (PKCS#7 detached doesn't carry it) - add tests for new validation checks Signed-off-by: Dallas Delaney <dadelan@microsoft.com>

dallasd1 added 3 commits March 12, 2026 16:15

signature/pkcs7: fix Content() algorithm, add tests

7dc8aca

Signed-off-by: Dallas Delaney <dadelan@microsoft.com>

dallasd1 force-pushed the dadelan/pkcs7-envelope branch from 9dc2a7d to 7dc8aca Compare March 12, 2026 23:16

dallasd1 mentioned this pull request Mar 12, 2026

Support signing image layers with PKCS#7 envelope notaryproject/notation#1337

Open

dallasd1 marked this pull request as ready for review March 12, 2026 23:20

dallasd1 requested review from NiazFK, gokarnm, priteshbandi, rgnote, shizhMSFT, toddysm, vaninrao10 and yizha1 as code owners March 12, 2026 23:20

github-actions Bot added the Stale label Apr 27, 2026

yizha1 removed the Stale label Apr 27, 2026

bketelsen suggested changes May 1, 2026

View reviewed changes

shizhMSFT requested changes May 2, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings May 13, 2026 23:13

Copilot started reviewing on behalf of dallasd1 May 13, 2026 23:14 View session

Copilot AI reviewed May 13, 2026

View reviewed changes

dallasd1 force-pushed the dadelan/pkcs7-envelope branch from caf155a to 59d15e2 Compare May 14, 2026 00:27

Conversation

dallasd1 commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Apr 27, 2026

Uh oh!

bketelsen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shizhMSFT left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shizhMSFT May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shizhMSFT May 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dallasd1 commented Mar 12, 2026 •

edited

Loading

shizhMSFT left a comment •

edited

Loading

shizhMSFT May 2, 2026 •

edited

Loading

shizhMSFT May 2, 2026 •

edited

Loading