feat: export the PluginPrimitiveSigner to enable signing with PKCS#7 by dallasd1 · Pull Request #572 · notaryproject/notation-go

dallasd1 · 2026-03-12T23:45:10Z

Type PluginPrimitiveSigner is needed for generating PKCS#7 signatures by code living in the notation CLI repo. That new (CLI) logic bypasses the PluginSigner wrapper that JWS and COSE use. This change exports the type to make it available outside of the notation-go repo and additionally adds helper functions for external callers.

The default behavior in the signing path for JWS/COSE when calling generateSignature remains unchanged.

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

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

dallasd1 · 2026-03-17T20:24:38Z

+}
+
+// GetKeySpecFromPlugin retrieves the key specification from a plugin by calling DescribeKey.
+func GetKeySpecFromPlugin(ctx context.Context, p plugin.SignPlugin, keyID string, pluginConfig map[string]string) (signature.KeySpec, error) {


This is needed for the CLI PR that will pull the signing type to include in the PKCS#7 envelope

bketelsen

Some blockers - KeyID validation is dropped from new public method, no input validation for NewPluginPrimitiveSigner()

bketelsen · 2026-05-01T15:40:04Z

+}
+
+// GetKeySpecFromPlugin retrieves the key specification from a plugin by calling DescribeKey.
+func GetKeySpecFromPlugin(ctx context.Context, p plugin.SignPlugin, keyID string, pluginConfig map[string]string) (signature.KeySpec, error) {


the private getKeySpec validates the KeyID matching the requested keyID, but the public method doesn't. It should be validated like the existing private method does.

bketelsen · 2026-05-01T15:40:51Z

+
+	resp, err := p.DescribeKey(ctx, req)
+	if err != nil {
+		return signature.KeySpec{}, err


wrap these errors in some context with fmt.Errorf("failed to describe key %q: %w", keyID, err)) to match the rest of plugin.go

bketelsen · 2026-05-01T15:43:09Z

+// NewPluginPrimitiveSigner creates a new PluginPrimitiveSigner that delegates
+// signing to a plugin. This is used for dm-verity PKCS#7 signing where raw
+// signature bytes are needed instead of JWS/COSE envelopes.
+func NewPluginPrimitiveSigner(ctx context.Context, p plugin.SignPlugin, keyID string, keySpec signature.KeySpec, pluginConfig map[string]string) *PluginPrimitiveSigner {


NewPluginSigner validates inputs and returns an error but this version does neither. It would silently accept a nil plugin and/or empty keyID and store them, which would cause a panic later.

bketelsen · 2026-05-01T15:43:58Z

 	}
 	basicVerification(t, data, envelopeType, mockPlugin.certs[len(mockPlugin.certs)-1], &validMetadata)
 }
+


add test for nil plugin - see previous comment

and a test for empty keyID

bketelsen · 2026-05-01T15:45:02Z

+		t.Fatalf("GetKeySpecFromPlugin() = %v, want %v", got, defaultKeySpec)
+	}
+}
+


Only happy path testing here. Need Sign() failure and at least one non-RSA key tested.

shizhMSFT

Caution

AI-generated review. This review was produced by an AI agent (Claude Opus 4.7, shizh-reviewer skill) on behalf of @shizhMSFT. It is posted as COMMENT (not REQUEST_CHANGES) — please weigh findings on their merits and treat them as starting points, not gating concerns.

Thanks @dallasd1. notation-go is more flexed than notation-core-go, but exporting an internal type into the public API of the most-imported library in the Notary Project ecosystem still deserves careful thought. A few concerns and one design question.

Design question first

Do we actually need to export pluginPrimitiveSigner?

The stated reason is that the dm-verity flow lives in notaryproject/notation (the CLI) and needs a signature.Signer. Two alternatives that don't expand notation-go's public API:

Move the dm-verity command into a signer/dmverity subpackage of notation-go and call the existing pluginPrimitiveSigner internally. The CLI then composes via a public function like signer.NewDMVeritySigner(plugin, keyID, opts) that returns []byte directly. This keeps pluginPrimitiveSigner private.
Add a single narrow public function signer.SignPrimitive(ctx, plugin, keyID, payload, pluginConfig) ([]byte, []*x509.Certificate, error) that internally constructs and calls pluginPrimitiveSigner. The caller never holds the type; we can refactor later without breaking compat.

Both reduce the API surface from "a struct + constructor + helper + KeySpec method" to one function. Per clig.dev reasoning applied to libraries: public API surface is a permanent contract; we should add the smallest amount that solves the use case. Right now this PR exports more than is strictly needed (see comments below). Worth at least answering "did we consider these?" before merge.

Concerns (if we proceed with the export)

GetKeySpecFromPlugin drops the keyID round-trip check. The existing PluginSigner.getKeySpec (signer/plugin.go:155–157) compares s.keyID to descKeyResp.KeyID and rejects mismatches. The new helper does not. A malicious or buggy plugin can return a different key's spec; the caller will then sign with the wrong spec/algorithm. This is the kind of TOCTOU-adjacent issue that has bitten supply-chain tooling before.
NewPluginPrimitiveSigner performs no input validation. NewPluginSigner (signer/plugin.go:66–78) rejects nil plugin and empty keyID. The new constructor accepts both and only fails later, deep inside Sign(). It also accepts an arbitrary signature.KeySpec value from the caller without sanity-checking it — the suggestion below adds local validation via proto.HashAlgorithmFromKeySpec. Note: this is purely a "did you pass me a valid (Type, Size) pair?" check; it does not cross-validate the keySpec against what the plugin would report — for that, the caller must use KeySpecFromPlugin first.
*PluginPrimitiveSigner returned from constructor. Constructors that return concrete pointers commit us to every exported field/method/struct-tag. Return signature.Signer (the interface the type already implements). Less to maintain; same usefulness.

Non-blocking design tension

context.Context baked into the public struct. The original code already does this internally, so exporting it doesn't introduce the anti-pattern — it propagates it. Sign(payload []byte) ignores its surroundings and uses s.ctx, so timeouts/cancellations from the calling goroutine cannot reach the plugin RPC. Resolving this properly (either passing ctx to Sign(ctx, payload) or hiding the ctx behind constructor scope) means changing signature.Signer, which is out of scope for this PR. Worth flagging as a tracking issue for the next signing-API revision.

Naming

GetKeySpecFromPlugin → KeySpecFromPlugin (Go style: no Get prefix on getters; cf. https://go.dev/wiki/CodeReviewComments#getters)
NewPluginPrimitiveSigner is fine, but consider NewSignaturePrimitiveSigner or just folding into NewPluginSigner as a mode flag. "PluginPrimitive" reads as two adjectives stacked; users will get it wrong.

Verdict: COMMENT (AI review only — not gating).

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

-// pluginPrimitiveSigner implements signature.Signer
-type pluginPrimitiveSigner struct {
+// PluginPrimitiveSigner implements signature.Signer
+type PluginPrimitiveSigner struct {


Now that this is PluginPrimitiveSigner, every field is almost part of the public API by reflection / unsafe use. Keep them unexported (they already are), but please also confirm that signer.PluginPrimitiveSigner{} (zero value) cannot be misused — e.g., add a runtime guard in Sign and KeySpec that returns an error if s.plugin == nil. Otherwise external callers using &PluginPrimitiveSigner{} literals will hit nil-deref panics rather than clean errors.

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

+// NewPluginPrimitiveSigner creates a new PluginPrimitiveSigner that delegates
+// signing to a plugin. This is used for dm-verity PKCS#7 signing where raw
+// signature bytes are needed instead of JWS/COSE envelopes.
+func NewPluginPrimitiveSigner(ctx context.Context, p plugin.SignPlugin, keyID string, keySpec signature.KeySpec, pluginConfig map[string]string) *PluginPrimitiveSigner {
+	return &PluginPrimitiveSigner{
+		ctx:          ctx,
+		plugin:       p,
+		keyID:        keyID,
+		keySpec:      keySpec,
+		pluginConfig: pluginConfig,
+	}
+}


Suggested change

// NewPluginPrimitiveSigner creates a new PluginPrimitiveSigner that delegates

// signing to a plugin. This is used for dm-verity PKCS#7 signing where raw

// signature bytes are needed instead of JWS/COSE envelopes.

func NewPluginPrimitiveSigner(ctx context.Context, p plugin.SignPlugin, keyID string, keySpec signature.KeySpec, pluginConfig map[string]string) *PluginPrimitiveSigner {

return &PluginPrimitiveSigner{

ctx: ctx,

plugin: p,

keyID: keyID,

keySpec: keySpec,

pluginConfig: pluginConfig,

}

}

// NewPluginPrimitiveSigner creates a new PluginPrimitiveSigner that delegates

// signing to a plugin. This is intended for callers that need raw signature

// bytes (e.g., dm-verity PKCS#7) rather than a JWS/COSE envelope.

//

// The keySpec must match what the plugin reports via DescribeKey. Use

// KeySpecFromPlugin to obtain it, or construct via NewPluginSigner if you

// just need standard envelope signing.

func NewPluginPrimitiveSigner(ctx context.Context, p plugin.SignPlugin, keyID string, keySpec signature.KeySpec, pluginConfig map[string]string) (signature.Signer, error) {

if p == nil {

return nil, errors.New("nil plugin")

}

if keyID == "" {

return nil, errors.New("keyID not specified")

}

if _, err := proto.HashAlgorithmFromKeySpec(keySpec); err != nil {

return nil, fmt.Errorf("invalid keySpec: %w", err)

}

return &PluginPrimitiveSigner{

ctx: ctx,

plugin: p,

keyID: keyID,

keySpec: keySpec,

pluginConfig: pluginConfig,

}, nil

}

Three changes: (1) validate inputs the same way NewPluginSigner does, (2) return signature.Signer (interface) rather than the concrete type so we can refactor later, (3) return an error so callers must handle setup failure. Same pattern as NewPluginSigner for consistency.

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

+// GetKeySpecFromPlugin retrieves the key specification from a plugin by calling DescribeKey.
+func GetKeySpecFromPlugin(ctx context.Context, p plugin.SignPlugin, keyID string, pluginConfig map[string]string) (signature.KeySpec, error) {
+	req := &plugin.DescribeKeyRequest{
+		ContractVersion: plugin.ContractVersion,
+		KeyID:           keyID,
+		PluginConfig:    pluginConfig,
+	}
+
+	resp, err := p.DescribeKey(ctx, req)
+	if err != nil {
+		return signature.KeySpec{}, err
+	}
+
+	return proto.DecodeKeySpec(resp.KeySpec)
+}


Suggested change

// GetKeySpecFromPlugin retrieves the key specification from a plugin by calling DescribeKey.

func GetKeySpecFromPlugin(ctx context.Context, p plugin.SignPlugin, keyID string, pluginConfig map[string]string) (signature.KeySpec, error) {

req := &plugin.DescribeKeyRequest{

ContractVersion: plugin.ContractVersion,

KeyID: keyID,

PluginConfig: pluginConfig,

}

resp, err := p.DescribeKey(ctx, req)

if err != nil {

return signature.KeySpec{}, err

}

return proto.DecodeKeySpec(resp.KeySpec)

}

// KeySpecFromPlugin retrieves and validates the key specification from a plugin

// by calling DescribeKey. It enforces that the plugin's response references the

// same keyID that was requested; this guards against a misbehaving plugin

// returning a spec for a different key.

func KeySpecFromPlugin(ctx context.Context, p plugin.SignPlugin, keyID string, pluginConfig map[string]string) (signature.KeySpec, error) {

if p == nil {

return signature.KeySpec{}, errors.New("nil plugin")

}

if keyID == "" {

return signature.KeySpec{}, errors.New("keyID not specified")

}

resp, err := p.DescribeKey(ctx, &plugin.DescribeKeyRequest{

ContractVersion: plugin.ContractVersion,

KeyID: keyID,

PluginConfig: pluginConfig,

})

if err != nil {

return signature.KeySpec{}, err

}

if resp.KeyID != keyID {

return signature.KeySpec{}, fmt.Errorf("keyID in describeKey response %q does not match request %q", resp.KeyID, keyID)

}

return proto.DecodeKeySpec(resp.KeySpec)

}

Two changes:

Drop the Get prefix per Go style.

Add the same keyID round-trip validation that PluginSigner.getKeySpec performs (lines 155–157). Without it, a plugin can respond with a different key's spec and we'd silently sign with the wrong algorithm. This is the same defense-in-depth check PluginPrimitiveSigner.Sign() already does at lines 362–364 after signing — please mirror it before signing.

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

+func TestGetKeySpecFromPlugin_Error(t *testing.T) {
+	ctx := context.Background()
+	mp := &mockPlugin{}
+	_, err := GetKeySpecFromPlugin(ctx, mp, "testKeyID", nil)
+	if err == nil {


&mockPlugin{} returns proto.DescribeKeyResponse{KeySpec: ks} where ks, _ := proto.EncodeKeySpec(p.keySpec) — for a zero-valued keySpec this returns {}. The test passes today because proto.DecodeKeySpec("") errors, but the comment "expected error for empty keySpec" is misleading: we're really testing decode-side error handling, not the helper's input contract. Once the validation suggested above is added, please also add tests for: nil plugin, empty keyID, mismatched keyID round-trip, and a plugin whose DescribeKey itself errors. The Error test name should match the failure mode (TestKeySpecFromPlugin_KeyIDMismatch, etc.).

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

+func TestNewPluginPrimitiveSigner(t *testing.T) {
+	ctx := context.Background()
+	mp := newMockPlugin(defaultKeyCert.key, defaultKeyCert.certs, defaultKeySpec)
+
+	s := NewPluginPrimitiveSigner(ctx, mp, "testKeyID", defaultKeySpec, nil)
+
+	// verify KeySpec
+	ks, err := s.KeySpec()
+	if err != nil {
+		t.Fatalf("KeySpec() error: %v", err)
+	}
+	if ks != defaultKeySpec {
+		t.Fatalf("KeySpec() = %v, want %v", ks, defaultKeySpec)
+	}
+


Once NewPluginPrimitiveSigner returns (signature.Signer, error) per the suggestion above, please also assert it returns an error for (nil, "key", spec, nil) and (plugin, "", spec, nil). Mirroring NewPluginSigner's test coverage.

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

 		}

-		primitivePluginSigner := &pluginPrimitiveSigner{
+		primitivePluginSigner := &PluginPrimitiveSigner{


nit: the local variable was primitivePluginSigner and is now &PluginPrimitiveSigner{...} — fine, but consider renaming the variable to pluginPrimitiveSigner (lowercase first letter for a local var) so the only thing that changed is the type, not the variable's spelling. Helps git blame.

dallasd1 added 2 commits March 12, 2026 16:31

signer: export PluginPrimitiveSigner for dm-verity signing

ec13c70

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

test: add unit tests for exported PluginPrimitiveSigner and GetKeySpe…

ac45f3e

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

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

dallasd1 mentioned this pull request Mar 12, 2026

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

Open

dallasd1 commented Mar 17, 2026

View reviewed changes

dallasd1 changed the title ~~Export the PluginPrimitiveSigner to enable signing with PKCS#7~~ feat: export the PluginPrimitiveSigner to enable signing with PKCS#7 Mar 18, 2026

bketelsen suggested changes May 1, 2026

View reviewed changes

shizhMSFT reviewed May 2, 2026

View reviewed changes

Conversation

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

shizhMSFT 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!

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dallasd1 commented Mar 12, 2026 •

edited

Loading