feat: add NewPluginPrimitiveSigner and KeySpecFromPlugin for raw-signature use cases by dallasd1 · Pull Request #572 · notaryproject/notation-go

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

Add two helper functions to the signer package so external callers like the notation CLI can produce raw plugin-backed signatures and certificate chains without going through the JWS/COSE envelope path.

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

signer.NewPluginPrimitiveSigner returns a signature.Signer that calls plugin.GenerateSignature and returns raw signature bytes + certificate chain
signer.KeySpecFromPlugin calls plugin.DescribeKey and returns the decoded keySpec, enforcing that the plugin's response references the same keyID that was requested

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

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

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.

KeySpecFromPlugin now validates the input and KeyIDs

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

Errors are now wrapped 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.

NewPluginPrimitiveSigner now validates input and returns error. This update also returns the signature.Signer type, so there's no need to export pluginPrimitiveSigner

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

Tests for nil plugin and empty KeyID added

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.

More failure tests and ECDSA tests added

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.

I've changed the return type to signature.Signer and unexported pluginPrimitiveSigner

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.

All three suggestions have been implemented

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.

GetKeySpecFromPlugin renamed to KeySpecFromPlugin. Validation checks have also been added

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

Extra tests for nil plugin, empty KeyID, mismatched KeyID, DescribeKey have all been added

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.

Error handling has been updated

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.

pluginPrimitiveSigner has been unexported

Copilot

Pull request overview

This PR exports a previously-internal “primitive” plugin-backed signer so external callers (e.g., notation CLI) can generate raw signatures + certificate chains for formats like PKCS#7, while keeping the existing JWS/COSE signing flow unchanged.

Changes:

Export PluginPrimitiveSigner (formerly unexported) and wire it into the existing plugin signing path.
Add helper APIs NewPluginPrimitiveSigner() and KeySpecFromPlugin() for external consumers.
Extend plugin signer unit tests to cover the new exported signer and helper behaviors.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`signer/plugin.go`	Exports the primitive signer type and adds helper constructor / key-spec retrieval function for external PKCS#7-style signing flows.
`signer/plugin_test.go`	Updates mocks and adds new tests for `NewPluginPrimitiveSigner` and `KeySpecFromPlugin`.

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

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

…nature.Signer. Clean up test logs Signed-off-by: Dallas Delaney <dadelan@microsoft.com>

dallasd1 · 2026-05-08T00:22:11Z

Thanks for the review @bketelsen! Your feedback should be addressed concerning the validation checks and test coverage.

dallasd1 · 2026-05-08T00:27:29Z

Thanks for the feedback @shizhMSFT! The comments have been addressed along with unexporting PluginPrimitiveSigner in favor of returning signature.Signer

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

Comment thread signer/plugin.go Outdated

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

Copilot AI review requested due to automatic review settings May 7, 2026 21:18

Copilot started reviewing on behalf of dallasd1 May 7, 2026 21:19 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

Comment thread signer/plugin.go Outdated

Comment thread signer/plugin.go Outdated

Comment thread signer/plugin.go Outdated

Comment thread signer/plugin_test.go

dallasd1 changed the title ~~feat: export the PluginPrimitiveSigner to enable signing with PKCS#7~~ feat: add NewPluginPrimitiveSigner and KeySpecFromPlugin for raw-signature use cases May 7, 2026

dallasd1 added 2 commits May 7, 2026 17:06

feat: expand test coverage and update API names and return types

1f82a42

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

feat: Unexport pluginPrimitiveSigner now that constructor returns sig…

bbc31b2

…nature.Signer. Clean up test logs Signed-off-by: Dallas Delaney <dadelan@microsoft.com>

dallasd1 force-pushed the dadelan/export-primitive-signer branch from 2634ac5 to bbc31b2 Compare May 8, 2026 00:06

Conversation

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

Uh oh!

Uh oh!

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!

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!

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!

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!

dallasd1 commented May 8, 2026

Uh oh!

dallasd1 commented May 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dallasd1 commented Mar 12, 2026 •

edited

Loading