Skip to content

feat: add PKCS#1 v1.5 padding to support PKCS#7 signing#291

Open
dallasd1 wants to merge 4 commits into
Azure:mainfrom
dallasd1:dadelan/pkcs1-signing-scheme
Open

feat: add PKCS#1 v1.5 padding to support PKCS#7 signing#291
dallasd1 wants to merge 4 commits into
Azure:mainfrom
dallasd1:dadelan/pkcs1-signing-scheme

Conversation

@dallasd1
Copy link
Copy Markdown

@dallasd1 dallasd1 commented Mar 16, 2026

Add RSA PKCS#1 v1.5 signature algorithm support alongside the existing PSS algorithms. This is required for dm-verity kernel signature verification which expects PKCS#1 v1.5 signature padding. The signing scheme is now selected using the signing_scheme plugin config key. AKV already supports PKCS#1 v1.5 so this change adds the option to select it from the plugin.

Add RSA PKCS#1 v1.5 signature algorithm support (RS256, RS384, RS512)
alongside the existing PSS algorithms. This is required for dm-verity
kernel signature verification which expects PKCS#1 v1.5 signatures.

Changes:
- Add PKCS1 key spec variants to Protocol/KeySpec.cs
- Extend KeySpecExtension to map PKCS1 specs to Azure Key Vault algorithms
- Update GenerateSignature to select the correct signing scheme
- Add unit tests for new key specs and algorithm mappings

Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 16, 2026

Codecov Report

❌ Patch coverage is 92.64706% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.97%. Comparing base (3396292) to head (7a1de8f).

Files with missing lines Patch % Lines
....Plugin.AzureKeyVault/KeyVault/KeySpecExtension.cs 92.30% 1 Missing and 1 partial ⚠️
Notation.Plugin.AzureKeyVault/Protocol/KeySpec.cs 92.30% 1 Missing and 1 partial ⚠️
....Plugin.AzureKeyVault/Command/GenerateSignature.cs 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #291      +/-   ##
==========================================
- Coverage   93.03%   92.97%   -0.06%     
==========================================
  Files          17       17              
  Lines         617      683      +66     
  Branches       78       89      +11     
==========================================
+ Hits          574      635      +61     
- Misses         31       33       +2     
- Partials       12       15       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Improve CI patch coverage for tests to get over 80%
Add tests for all RSA key sizes (2048/3072/4096) and EC key sizes (256/384/521) through the PKCS1 code paths.
Add tests for empty string and rsassa-pss scheme routing.
Add direct tests for ToKeyVaultSignatureAlgorithmPKCS1 and ToSigningAlgorithmPKCS1.
Fix trailing whitespace in KeySpec.cs.

Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
Copy link
Copy Markdown
Member

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

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. The plugin layer is the right place for AKV-specific signature scheme selection, but a few things are worth reconsidering before merge — the most important being that this PR alone does not appear to work end-to-end without coordinated changes in notaryproject/specifications, notation-core-go, and notation-go.

Concerns

  1. signingAlgorithm = "RSASSA-PKCS1-v1_5-SHA-*" is not in the Notary signature spec, so notation will reject this AKV response. AKV's GenerateSignature response (per Protocol/KeySpec.cs) will set signingAlgorithm to one of "RSASSA-PKCS1-v1_5-SHA-{256,384,512}". But:

    • notation-core-go's signature.Algorithm enum (signature/algorithm.go:29-34) defines only PS{256,384,512} and ES{256,384,512}. There is no PKCS#1 v1.5 member.
    • notation-go's proto.DecodeSigningAlgorithm will not recognize the new value.
    • Per notaryproject/specifications plugin-extensibility.md §generate-signature, response.signingAlgorithm MUST be one of the spec-listed algorithms — and PKCS#1 v1.5 isn't in the table.
    • End result: the standard envelope flow will reject AKV's response.

    So this PR alone, even with all other concerns addressed, appears non-functional end-to-end. It depends on coordinated PRs to:

    1. notaryproject/specifications — add RSASSA-PKCS1-v1_5-SHA-{256,384,512} to the algorithm-selection table,
    2. notation-core-go — extend signature.Algorithm and proto.DecodeSigningAlgorithm,
    3. notation-go — update PluginSigner.generateSignature validation.

    Worth opening a tracking issue covering all three repos and linking from this PR. /cc @yizha1 @priteshbandi

  2. ToKeyVaultSignatureAlgorithmPKCS1 over-promises for EC keys. The function name promises PKCS#1 v1.5 but for KeyType.EC it returns ES256/ES384/ES512 (ECDSA). Technically correct (EC has no padding variant) but the naming makes a CLI that requests signing_scheme=rsassa-pkcs1-v1_5 against an EC key silently get ECDSA, which the kernel cannot consume for dm-verity. Consider either (a) rejecting EC keys explicitly when the scheme is rsassa-pkcs1-v1_5, or (b) renaming to ToDmVeritySignatureAlgorithm and documenting the EC fallback as deliberate.

  3. Public ABI growth in a NuGet package. Notation.Plugin.Protocol.SigningScheme, SigningAlgorithms.RSASSA_PKCS1_V1_5_SHA_*, KeySpec.ToSigningAlgorithmPKCS1(), KeySpec.ToSigningAlgorithm(string?), KeySpecExtension.ToKeyVaultSignatureAlgorithmPKCS1(), KeySpecExtension.ToKeyVaultSignatureAlgorithm(string?), GenerateSignature.SigningSchemeConfigKey — that's seven new public symbols for what could be one. Worth reducing by (a) keeping ToSigningAlgorithmPKCS1 / ToKeyVaultSignatureAlgorithmPKCS1 internal (they're only used as fallbacks of the scheme-aware variants), and (b) only exposing ToSigningAlgorithm(string? scheme) / ToKeyVaultSignatureAlgorithm(string? scheme) plus the SigningScheme constants.

Design questions / non-blocking

  • Cross-vendor coordination of signing_scheme. Plugin config is the established mechanism for vendor-specific configuration (this plugin already uses ca_certs, self_signed, credential_type without spec entries) — so there's no protocol violation. But other notation plugins (HashiCorp Vault, AWS KMS, GCP KMS) will need the same scheme switch for dm-verity. Should we coordinate with notaryproject/specifications to register signing_scheme as a standard plugin config key before each vendor invents their own?
  • Key-policy validation. AKV has keys whose policy disallows PKCS#1 v1.5 (managed-HSM keys frequently restrict to PSS). When the CLI requests rsassa-pkcs1-v1_5 against such a key, the AKV REST call will fail mid-stream with a confusing error. Pre-flighting via GetKeyProperties would surface a clear error early — but adds a round-trip on every sign. Worth doing only if the in-stream error message turns out to be opaque in practice. Defer until we see real complaints.

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

Comment thread Notation.Plugin.AzureKeyVault/Command/GenerateSignature.cs
Comment thread Notation.Plugin.AzureKeyVault/Command/GenerateSignature.cs Outdated
Comment thread Notation.Plugin.AzureKeyVault/KeyVault/KeySpecExtension.cs Outdated
Comment thread Notation.Plugin.AzureKeyVault/KeyVault/KeySpecExtension.cs
Comment thread Notation.Plugin.AzureKeyVault/Protocol/KeySpec.cs Outdated
Comment thread Notation.Plugin.AzureKeyVault/Protocol/KeySpec.cs Outdated
Comment thread Notation.Plugin.AzureKeyVault/Protocol/KeySpec.cs
Comment thread Notation.Plugin.AzureKeyVault.Tests/KeyVault/KeySpecExtensionTests.cs Outdated
Comment thread Notation.Plugin.AzureKeyVault.Tests/KeyVault/KeySpecExtensionTests.cs Outdated
dallasd1 added 2 commits May 19, 2026 11:50
- expand SigningSchemeConfigKey XML doc to clarify supported values
- reject rsassa-pkcs1-v1_5 + EC key in RunAsync (new theory test)
- mark PKCS1 helpers internal to hide EC fallback
- collapse null / "" switch arms to 'null or ""'
- rename ToSignatureAlgorithm_WithScheme_* tests to
  ToKeyVaultSignatureAlgorithm_WithScheme_*
- assert offending scheme value in ArgumentException

Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
- hoist signing_scheme validation upstream in RunAsync so the failure
  reports the plugin's VALIDATION_ERROR code
- add RSA happy-path and EC-rejection PKCS1 RunAsync tests
- add uppercase-scheme row for case-insensitivity
- split mixed RSA+EC PKCS1 helper tests
- rename EC variant to ReturnsECDSAForSymmetry
- mirror PKCS1 helper docs across KeySpec and KeySpecExtension
- note scheme-router switch must stay in lock-step
- rename SigningScheme to SigningSchemes

Signed-off-by: Dallas Delaney <dadelan@microsoft.com>
@dallasd1
Copy link
Copy Markdown
Author

Thanks for the feedback @shizhMSFT! The comments have been addressed listed have been addressed in the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants