fix(rsa): fix PEM signature chain validation and trust anchor handling by jakobmoellerdev · Pull Request #2137 · open-component-model/open-component-model

jakobmoellerdev · 2026-04-01T20:40:24Z

Three bugs in the PEM signature verification path are fixed:

Root certificates were not validated chain[1:] was placed unconditionally into the intermediates pool, so the root CA in the embedded chain was never checked against trusted roots. A self-signed root CA embedded in the signature could satisfy its own chain, letting a signer assert their own trust anchor.

Fix: split certificate classification by source. The embedded (signer- controlled) chain now rejects any self-signed certificate outright via classifyEmbeddedChain(). Root trust must come exclusively from the verifier's credential store or system roots.
Credential chain only supplied one certificate The old code called PublicKeyFromCredentials() / a single-cert parser, so only the first CERTIFICATE block in the credential PEM was used. A multi-cert credential file [interm, root] silently dropped the root.

Fix: call CertificateChainFromCredentials() for the full chain. Credential certificates are classified at the call site in Verify():
- non-self-signed → intermediates pool (path building only)
- self-signed at the last position → isolated root anchor (system roots are then bypassed entirely; a self-signed cert elsewhere is rejected)
- no self-signed cert → system roots remain the only trust anchors
Issuer field was matched against the wrong certificate The Issuer field on the signature was compared against the credential anchor's Subject instead of the X.509 Issuer of the leaf certificate (the Subject of the CA that directly signed the leaf).

Fix: verifyIssuerForLeafCert() now compares signed.Signature.Issuer against leaf.Issuer, which is the intermediate's Subject in a three- tier chain — not the root's Subject.

What this PR does / why we need it

Fixes various PEM encoding signature related edge cases.

Which issue(s) this PR fixes

This fixes a lot of issues I had during verification and testing of flows such as "sign with certificate, verify with Certificate Authority".

prepares part of open-component-model/ocm-project#1000

Testing

Regression tests added:

pem_signature_embedded_root_rejected
A self-signed root CA in the embedded chain is rejected.

pem_signature_intermediate_not_trusted_as_root
An intermediate CA in the embedded chain cannot be elevated to a root.

pem_signature_issuer_matches_leaf_issuer_field
Issuer set to the intermediate's Subject (leaf's X.509 Issuer) passes.

pem_signature_issuer_mismatch_with_leaf_issuer_field
Issuer set to the root's Subject (not leaf's Issuer) is rejected.

pem_signature_issuer_set_to_anchor_subject_fails
Issuer set to the credential anchor's Subject is rejected when the
leaf was signed by an intermediate.

pem_signature_leaf_only_cred_chain_interm_and_root_ok
Credential PEM [interm, root]: interm goes to intermediates pool,
root becomes the isolated anchor.

pem_signature_leaf_in_sig_chain_in_creds_consistent_chain_ok
Leaf-only signature with [interm, root] credentials succeeds when
the chain is consistent.

pem_signature_cred_chain_interm_only_no_root_fails
Intermediate-only credentials leave no root to terminate the chain.

pem_signature_cred_chain_self_signed_not_last_fails
Self-signed certificate at position 0 in credentials is rejected.

pem_signature_leaf_only_signature_only_root_in_credentials_fails
Root-only credentials cannot build the path when the intermediate is
absent from both the signature and credentials.

Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_correct_succeeds
Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_wrong_cert_fails_despite_system_roots
A self-signed credential anchor isolates the root pool; system roots
are not consulted even when the handler was created with useSystemRoots.

Test_RSA_Credentials_Override_SystemRoots/non_self_signed_credential_cert_is_intermediate_not_anchor
Non-self-signed credential certs are treated as intermediates only;
they never become an anchor regardless of position.

How to test the changes

Please observe the added test cases, they are well commented

coderabbitai · 2026-04-01T20:40:32Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 66422828-a817-4fad-ac36-fe6a72b37654

📥 Commits

Reviewing files that changed from the base of the PR and between 010311c and e499321.

📒 Files selected for processing (2)

bindings/go/rsa/signing/handler/handler.go
bindings/go/rsa/signing/handler/handler_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

bindings/go/rsa/signing/handler/handler_test.go

📝 Walkthrough

Walkthrough

Refactors PEM verification in the RSA signing handler: extracts PEM logic into verifyPEMSignature, centralizes PEM parsing and chain validation, adds certificate-chain classification helpers, changes chain verification to respect an optional credential anchor-only root pool, and updates issuer-check target to the X.509 leaf certificate issuer. Tests expanded accordingly.

Changes

Cohort / File(s)	Summary
Handler implementation `bindings/go/rsa/signing/handler/handler.go`	Extracted PEM verification into `verifyPEMSignature(...)`; added `isSelfSigned`, `classifyCredentialChain`, `classifyEmbeddedChain`; reworked `verifyChainWithOptionalAnchor` to use credential anchor-only roots and to reject embedded self-signed certs; replaced issuer-check helper with `verifyIssuerForLeafCert`.
Tests and test helpers `bindings/go/rsa/signing/handler/handler_test.go`	Expanded tests with PEM/RSA scenarios: new helper `buildLeafOnlyPEM`, table-driven cases for issuer matching/mismatch, tests rejecting embedded self-signed certs and malformed credential ordering, and new `Test_RSA_Credentials_Override_SystemRoots`. Adjusted chain construction and minor test robustness fixes.

Sequence Diagram(s)

sequenceDiagram
    participant Client as Client (Signed PEM)
    participant Handler as Handler.Verify
    participant PEM as verifyPEMSignature
    participant Cred as classifyCredentialChain
    participant Chain as verifyChainWithOptionalAnchor
    participant RSA as verifyRSA
    participant Roots as h.roots

    Client->>Handler: send signed message (MediaTypePEM)
    Handler->>PEM: delegate PEM verification
    PEM->>Cred: load & classify credential certs
    Cred-->>PEM: intermediates + optional anchor
    PEM->>Chain: merge embedded intermediates + credential intermediates, pass anchor and Roots
    Chain->>Roots: if anchor!=nil use anchor-only pool else use Roots
    Chain-->>PEM: chain verification result
    PEM->>Handler: verifyIssuerForLeafCert(leaf issuer vs SignatureInfo.Issuer)
    PEM->>RSA: verify RSA signature with parsed public key
    RSA-->>Handler: verification outcome
    Handler-->>Client: return verification result

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

"I hopped through certs with a nibble and twitch,
PEM leaves and anchors all snug in my ditch.
I checked every issuer, no root out of place,
Verified signatures with whiskered grace.
🐇✨ — a rabbit who loves a tidy chain."

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately summarizes the main change: fixing PEM signature chain validation and trust anchor handling bugs in RSA verification.
Description check	✅ Passed	The description is directly related to the changeset, detailing the three bugs fixed, their root causes, solutions, and regression tests added.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

🧹 Nitpick comments (1)

bindings/go/rsa/signing/handler/handler.go (1)

185-219: Add validation for the algorithm extracted from PEM headers.

The algFromPEM string from GetSignatureFromPem is cast directly to v1alpha1.SignatureAlgorithm without validation at line 219. While verifyRSA handles invalid values by returning ErrInvalidAlgorithm, early validation would provide a clearer error message indicating the actual problem (e.g., empty or unrecognized algorithm in PEM header).

Proposed validation before calling verifyRSA

+	algo := v1alpha1.SignatureAlgorithm(algFromPEM)
+	if algo != v1alpha1.AlgorithmRSASSAPSS && algo != v1alpha1.AlgorithmRSASSAPKCS1V15 {
+		return fmt.Errorf("unsupported or missing algorithm %q in PEM signature", algFromPEM)
+	}
+
-	return verifyRSA(v1alpha1.SignatureAlgorithm(algFromPEM), rsaPub, hash, dig, sig)
+	return verifyRSA(algo, rsaPub, hash, dig, sig)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@bindings/go/rsa/signing/handler/handler.go` around lines 185 - 219, The code
casts algFromPEM (returned by GetSignatureFromPem) directly to
v1alpha1.SignatureAlgorithm before calling verifyRSA, which can hide missing or
unrecognized algorithm values; add explicit validation after obtaining
algFromPEM: check it is non-empty and matches one of the supported algorithm
constants/enums in v1alpha1 (or use a helper like v1alpha1.IsValid/parse
function if available), and return a clear, descriptive error (e.g., "invalid or
missing signature algorithm in PEM header: %q") before calling verifyRSA;
reference algFromPEM, v1alpha1.SignatureAlgorithm, GetSignatureFromPem, and
verifyRSA to locate where to insert this validation.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@bindings/go/rsa/signing/handler/handler.go`:
- Around line 185-219: The code casts algFromPEM (returned by
GetSignatureFromPem) directly to v1alpha1.SignatureAlgorithm before calling
verifyRSA, which can hide missing or unrecognized algorithm values; add explicit
validation after obtaining algFromPEM: check it is non-empty and matches one of
the supported algorithm constants/enums in v1alpha1 (or use a helper like
v1alpha1.IsValid/parse function if available), and return a clear, descriptive
error (e.g., "invalid or missing signature algorithm in PEM header: %q") before
calling verifyRSA; reference algFromPEM, v1alpha1.SignatureAlgorithm,
GetSignatureFromPem, and verifyRSA to locate where to insert this validation.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 460968b6-84b8-49d6-91e8-8fbe816b4591

📥 Commits

Reviewing files that changed from the base of the PR and between b726faa and 010311c.

📒 Files selected for processing (2)

bindings/go/rsa/signing/handler/handler.go
bindings/go/rsa/signing/handler/handler_test.go

🚧 Files skipped from review as they are similar to previous changes (1)

bindings/go/rsa/signing/handler/handler_test.go

… handling Three bugs in the PEM signature verification path are fixed: 1. Root certificates were not validated chain[1:] was placed unconditionally into the intermediates pool, so the root CA in the embedded chain was never checked against trusted roots. A self-signed root CA embedded in the signature could satisfy its own chain, letting a signer assert their own trust anchor. Fix: split certificate classification by source. The embedded (signer- controlled) chain now rejects any self-signed certificate outright via classifyEmbeddedChain(). Root trust must come exclusively from the verifier's credential store or system roots. 2. Credential chain only supplied one certificate The old code called PublicKeyFromCredentials() / a single-cert parser, so only the first CERTIFICATE block in the credential PEM was used. A multi-cert credential file [interm, root] silently dropped the root. Fix: call CertificateChainFromCredentials() for the full chain. Credential certificates are classified at the call site in Verify(): - non-self-signed → intermediates pool (path building only) - self-signed at the last position → isolated root anchor (system roots are then bypassed entirely; a self-signed cert elsewhere is rejected) - no self-signed cert → system roots remain the only trust anchors 3. Issuer field was matched against the wrong certificate The Issuer field on the signature was compared against the credential anchor's Subject instead of the X.509 Issuer of the leaf certificate (the Subject of the CA that directly signed the leaf). Fix: verifyIssuerForLeafCert() now compares signed.Signature.Issuer against leaf.Issuer, which is the intermediate's Subject in a three- tier chain — not the root's Subject. Regression tests added: pem_signature_embedded_root_rejected A self-signed root CA in the embedded chain is rejected. pem_signature_intermediate_not_trusted_as_root An intermediate CA in the embedded chain cannot be elevated to a root. pem_signature_issuer_matches_leaf_issuer_field Issuer set to the intermediate's Subject (leaf's X.509 Issuer) passes. pem_signature_issuer_mismatch_with_leaf_issuer_field Issuer set to the root's Subject (not leaf's Issuer) is rejected. pem_signature_issuer_set_to_anchor_subject_fails Issuer set to the credential anchor's Subject is rejected when the leaf was signed by an intermediate. pem_signature_leaf_only_cred_chain_interm_and_root_ok Credential PEM [interm, root]: interm goes to intermediates pool, root becomes the isolated anchor. pem_signature_leaf_in_sig_chain_in_creds_consistent_chain_ok Leaf-only signature with [interm, root] credentials succeeds when the chain is consistent. pem_signature_cred_chain_interm_only_no_root_fails Intermediate-only credentials leave no root to terminate the chain. pem_signature_cred_chain_self_signed_not_last_fails Self-signed certificate at position 0 in credentials is rejected. pem_signature_leaf_only_signature_only_root_in_credentials_fails Root-only credentials cannot build the path when the intermediate is absent from both the signature and credentials. Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_correct_succeeds Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_wrong_cert_fails_despite_system_roots A self-signed credential anchor isolates the root pool; system roots are not consulted even when the handler was created with useSystemRoots. Test_RSA_Credentials_Override_SystemRoots/non_self_signed_credential_cert_is_intermediate_not_anchor Non-self-signed credential certs are treated as intermediates only; they never become an anchor regardless of position. Signed-off-by: Jakob Möller <contact@jakob-moeller.com>

open-component-model#2137) Three bugs in the PEM signature verification path are fixed: 1. Root certificates were not validated chain[1:] was placed unconditionally into the intermediates pool, so the root CA in the embedded chain was never checked against trusted roots. A self-signed root CA embedded in the signature could satisfy its own chain, letting a signer assert their own trust anchor. Fix: split certificate classification by source. The embedded (signer- controlled) chain now rejects any self-signed certificate outright via classifyEmbeddedChain(). Root trust must come exclusively from the verifier's credential store or system roots. 2. Credential chain only supplied one certificate The old code called PublicKeyFromCredentials() / a single-cert parser, so only the first CERTIFICATE block in the credential PEM was used. A multi-cert credential file [interm, root] silently dropped the root. Fix: call CertificateChainFromCredentials() for the full chain. Credential certificates are classified at the call site in Verify(): - non-self-signed → intermediates pool (path building only) - self-signed at the last position → isolated root anchor (system roots are then bypassed entirely; a self-signed cert elsewhere is rejected) - no self-signed cert → system roots remain the only trust anchors 3. Issuer field was matched against the wrong certificate The Issuer field on the signature was compared against the credential anchor's Subject instead of the X.509 Issuer of the leaf certificate (the Subject of the CA that directly signed the leaf). Fix: verifyIssuerForLeafCert() now compares signed.Signature.Issuer against leaf.Issuer, which is the intermediate's Subject in a three- tier chain — not the root's Subject.  #### What this PR does / why we need it Fixes various PEM encoding signature related edge cases. #### Which issue(s) this PR fixes  This fixes a lot of issues I had during verification and testing of flows such as "sign with certificate, verify with Certificate Authority". prepares part of open-component-model/ocm-project#1000 #### Testing Regression tests added: pem_signature_embedded_root_rejected A self-signed root CA in the embedded chain is rejected. pem_signature_intermediate_not_trusted_as_root An intermediate CA in the embedded chain cannot be elevated to a root. pem_signature_issuer_matches_leaf_issuer_field Issuer set to the intermediate's Subject (leaf's X.509 Issuer) passes. pem_signature_issuer_mismatch_with_leaf_issuer_field Issuer set to the root's Subject (not leaf's Issuer) is rejected. pem_signature_issuer_set_to_anchor_subject_fails Issuer set to the credential anchor's Subject is rejected when the leaf was signed by an intermediate. pem_signature_leaf_only_cred_chain_interm_and_root_ok Credential PEM [interm, root]: interm goes to intermediates pool, root becomes the isolated anchor. pem_signature_leaf_in_sig_chain_in_creds_consistent_chain_ok Leaf-only signature with [interm, root] credentials succeeds when the chain is consistent. pem_signature_cred_chain_interm_only_no_root_fails Intermediate-only credentials leave no root to terminate the chain. pem_signature_cred_chain_self_signed_not_last_fails Self-signed certificate at position 0 in credentials is rejected. pem_signature_leaf_only_signature_only_root_in_credentials_fails Root-only credentials cannot build the path when the intermediate is absent from both the signature and credentials. Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_correct_succeeds Test_RSA_Credentials_Override_SystemRoots/self_signed_anchor_wrong_cert_fails_despite_system_roots A self-signed credential anchor isolates the root pool; system roots are not consulted even when the handler was created with useSystemRoots. Test_RSA_Credentials_Override_SystemRoots/non_self_signed_credential_cert_is_intermediate_not_anchor Non-self-signed credential certs are treated as intermediates only; they never become an anchor regardless of position. ##### How to test the changes Please observe the added test cases, they are well commented Signed-off-by: Jakob Möller <contact@jakob-moeller.com> Co-authored-by: Matthias Bruns <github@matthiasbruns.com> Signed-off-by: Gerald Morrison (SAP) <gerald.morrison@sap.com>

github-actions Bot added kind/bugfix Bug size/l Large labels Apr 1, 2026

jakobmoellerdev force-pushed the sig-fixes branch 2 times, most recently from 4f9b723 to b726faa Compare April 1, 2026 20:50

jakobmoellerdev marked this pull request as ready for review April 1, 2026 20:53

jakobmoellerdev requested a review from a team as a code owner April 1, 2026 20:53

frewilhelm reviewed Apr 2, 2026

View reviewed changes

Comment thread bindings/go/rsa/signing/handler/handler_test.go Outdated

jakobmoellerdev force-pushed the sig-fixes branch from b726faa to 010311c Compare April 2, 2026 06:41

coderabbitai Bot reviewed Apr 2, 2026

View reviewed changes

matthiasbruns reviewed Apr 2, 2026

View reviewed changes

Comment thread bindings/go/rsa/signing/handler/handler_test.go

jakobmoellerdev force-pushed the sig-fixes branch from 010311c to 72487aa Compare April 2, 2026 07:10

Merge branch 'main' into sig-fixes

e499321

jakobmoellerdev requested review from frewilhelm and matthiasbruns April 2, 2026 10:53

Merge branch 'main' into sig-fixes

c0b0163

matthiasbruns approved these changes Apr 2, 2026

View reviewed changes

frewilhelm approved these changes Apr 2, 2026

View reviewed changes

jakobmoellerdev merged commit 89b6658 into open-component-model:main Apr 2, 2026
16 checks passed

coderabbitai Bot mentioned this pull request Apr 3, 2026

feat: add RFC 3161 TSA timestamping for OCM signatures #2154

Closed

1 task

coderabbitai Bot mentioned this pull request May 1, 2026

feat: typed credentials for rsa binding (Phase 2) #2418

Merged

3 tasks

coderabbitai Bot mentioned this pull request May 20, 2026

feat!: update rsa, blob and signing to use runtime.Typed #2580

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rsa): fix PEM signature chain validation and trust anchor handling#2137

fix(rsa): fix PEM signature chain validation and trust anchor handling#2137
jakobmoellerdev merged 3 commits into
open-component-model:mainfrom
jakobmoellerdev:sig-fixes

jakobmoellerdev commented Apr 1, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Apr 1, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

jakobmoellerdev commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does / why we need it

Which issue(s) this PR fixes

Testing

How to test the changes

Uh oh!

coderabbitai Bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

❌ Failed checks (1 warning)

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jakobmoellerdev commented Apr 1, 2026 •

edited

Loading

coderabbitai Bot commented Apr 1, 2026 •

edited

Loading