feat: typed credentials for rsa binding (Phase 2)#2418
Merged
jakobmoellerdev merged 23 commits intoMay 7, 2026
Merged
Conversation
✅ Deploy Preview for ocm-website ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR migrates the RSA signing/verification handler from untyped credential maps to strongly-typed RSACredentials and RSAIdentity structures. New credential and identity specs in ChangesRSA Typed Credentials and Identities Migration
Sequence DiagramsequenceDiagram
participant Caller
participant Handler as RSA Handler
participant Creds as RSACredentials<br/>(typed)
participant Identity as RSAIdentity<br/>(typed)
participant Runtime as Runtime Scheme
participant KeyMgmt as Key Management
Caller->>Handler: Sign(document, credentials map)
Handler->>Creds: Load typed RSACredentials<br/>from map
Handler->>Identity: Build RSAIdentity via<br/>baseIdentity(algorithm)
Handler->>Identity: Set Signature field
Handler->>Runtime: Convert RSAIdentity<br/>to runtime map via<br/>rsaIdentityToMap
Handler->>KeyMgmt: PrivateKeyFromCredentials<br/>(typed creds)
KeyMgmt-->>Handler: *rsa.PrivateKey
Handler->>Handler: Sign document
Handler-->>Caller: signature + identity
Caller->>Handler: Verify(signature, identity map,<br/>credentials map)
Handler->>Creds: Load typed RSACredentials<br/>from map
Handler->>KeyMgmt: PublicKeyFromCredentials<br/>(typed creds)
KeyMgmt-->>Handler: *rsapem.RSAPublicKeyPEM
Handler->>Handler: Verify signature against<br/>public key
Handler-->>Caller: valid/invalid result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
bindings/go/rsa/signing/handler/handler.go (1)
28-29: ⚡ Quick winPrefer
identityv1attribute-key constants to avoid key drift.Now that this file imports
identityv1, reusingidentityv1.IdentityAttributeAlgorithm/identityv1.IdentityAttributeSignaturewould remove duplicate literals and keep identity key definitions centralized.♻️ Proposed refactor
-const ( - IdentityAttributeAlgorithm = "algorithm" - IdentityAttributeSignature = "signature" -) +// Use identityv1.IdentityAttributeAlgorithm and +// identityv1.IdentityAttributeSignature directly. @@ - id[IdentityAttributeSignature] = name + id[identityv1.IdentityAttributeSignature] = name @@ - id[IdentityAttributeSignature] = signature.Name + id[identityv1.IdentityAttributeSignature] = signature.Name @@ - id := runtime.Identity{IdentityAttributeAlgorithm: string(algorithm)} + id := runtime.Identity{identityv1.IdentityAttributeAlgorithm: string(algorithm)}Also applies to: 225-270, 405-409
🤖 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 28 - 29, This file currently uses duplicate literal keys for identity attribute names; replace those string literals with the centralized constants from the imported package by using identityv1.IdentityAttributeAlgorithm and identityv1.IdentityAttributeSignature wherever the algorithm/signature attribute keys are referenced (e.g., in the functions and variables around the blocks currently using literal keys at the top import area and the regions noted around lines 225-270 and 405-409). Update any map lookups, assignments, and comparisons that use the literal names to use these constants so the code references identityv1.IdentityAttributeAlgorithm and identityv1.IdentityAttributeSignature instead of hard-coded strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindings/go/rsa/spec/credentials/v1/rsa_credentials_test.go`:
- Around line 13-18: Replace PEM-shaped fixture values and raw key-name literals
in the test's props map and other test cases with neutral placeholder strings
and the existing CredentialKey* constants: use CredentialKeyPublicKeyPEM,
CredentialKeyPublicKeyPEMFile, CredentialKeyPrivateKeyPEM, and
CredentialKeyPrivateKeyPEMFile as the map keys instead of raw literals, and set
their values to non-secret placeholders like "public-pem-placeholder",
"public-pem-file-placeholder", "private-pem-placeholder",
"private-pem-file-placeholder"; update all other occurrences in the same test
that currently use PEM-like strings or the hardcoded key names (the other props
usages) to follow the same pattern.
---
Nitpick comments:
In `@bindings/go/rsa/signing/handler/handler.go`:
- Around line 28-29: This file currently uses duplicate literal keys for
identity attribute names; replace those string literals with the centralized
constants from the imported package by using
identityv1.IdentityAttributeAlgorithm and identityv1.IdentityAttributeSignature
wherever the algorithm/signature attribute keys are referenced (e.g., in the
functions and variables around the blocks currently using literal keys at the
top import area and the regions noted around lines 225-270 and 405-409). Update
any map lookups, assignments, and comparisons that use the literal names to use
these constants so the code references identityv1.IdentityAttributeAlgorithm and
identityv1.IdentityAttributeSignature instead of hard-coded strings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c81f1d6b-b1ff-4b0b-b8c5-7a10899fc06d
📒 Files selected for processing (11)
bindings/go/rsa/signing/handler/handler.gobindings/go/rsa/signing/handler/internal/credentials/credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials_test.gobindings/go/rsa/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/rsa/spec/identity/v1/register.gobindings/go/rsa/spec/identity/v1/type.gobindings/go/rsa/spec/identity/v1/type_test.gobindings/go/rsa/spec/identity/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/identity/v1/zz_generated.ocm_type.go
8e08b13 to
036df9a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bindings/go/rsa/signing/handler/handler.go (1)
405-409: ⚡ Quick winUse the canonical
RSA/v1identity type here.
V1Alpha1Typeis the deprecated alias. EmittingVersionedTypefrombaseIdentity()keeps new requests on the canonical spec while preserving backward compatibility through the alias registration.♻️ Proposed fix
func baseIdentity(algorithm v1alpha1.SignatureAlgorithm) runtime.Identity { id := runtime.Identity{IdentityAttributeAlgorithm: string(algorithm)} - id.SetType(identityv1.V1Alpha1Type) + id.SetType(identityv1.VersionedType) return id }🤖 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 405 - 409, The baseIdentity function currently sets the deprecated alias identityv1.V1Alpha1Type; change it to emit the canonical RSA/v1 identity by setting the identity type to identityv1.VersionedType in baseIdentity(algorithm v1alpha1.SignatureAlgorithm) so new requests use the canonical spec while alias registration (V1Alpha1Type) elsewhere preserves backward compatibility.
🤖 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 405-409: The baseIdentity function currently sets the deprecated
alias identityv1.V1Alpha1Type; change it to emit the canonical RSA/v1 identity
by setting the identity type to identityv1.VersionedType in
baseIdentity(algorithm v1alpha1.SignatureAlgorithm) so new requests use the
canonical spec while alias registration (V1Alpha1Type) elsewhere preserves
backward compatibility.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 06227b58-273f-4341-a310-72f3d6bcdc68
📒 Files selected for processing (15)
bindings/go/rsa/signing/handler/handler.gobindings/go/rsa/signing/handler/internal/credentials/credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials_test.gobindings/go/rsa/spec/credentials/v1/schemas/RSACredentials.schema.jsonbindings/go/rsa/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/rsa/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/rsa/spec/identity/v1/register.gobindings/go/rsa/spec/identity/v1/schemas/RSAIdentity.schema.jsonbindings/go/rsa/spec/identity/v1/type.gobindings/go/rsa/spec/identity/v1/type_test.gobindings/go/rsa/spec/identity/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/rsa/spec/identity/v1/zz_generated.ocm_type.go
✅ Files skipped from review due to trivial changes (6)
- bindings/go/rsa/spec/credentials/v1/zz_generated.ocm_jsonschema.go
- bindings/go/rsa/spec/identity/v1/zz_generated.ocm_type.go
- bindings/go/rsa/spec/credentials/v1/schemas/RSACredentials.schema.json
- bindings/go/rsa/spec/credentials/v1/zz_generated.deepcopy.go
- bindings/go/rsa/spec/credentials/v1/zz_generated.ocm_type.go
- bindings/go/rsa/spec/identity/v1/zz_generated.deepcopy.go
🚧 Files skipped from review as they are similar to previous changes (3)
- bindings/go/rsa/spec/identity/v1/register.go
- bindings/go/rsa/signing/handler/internal/credentials/credentials.go
- bindings/go/rsa/spec/credentials/v1/rsa_credentials.go
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
a616525 to
98c3607
Compare
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
9 tasks
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
ba46bf2 to
5b3e727
Compare
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/go/rsa/signing/handler/handler_test.go (1)
57-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove the shared
rootPEMstate from parallel subtests.
rootPEMis captured once for the wholealgsubtest, but several cases mutate/read it while each case runs witht.Parallel(). That makes these cases race with each other and can verify against the wrong anchor path or a temp file owned by another subtest.Minimal safe fix
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - t.Parallel() sig := tt.build(t) err := h.Verify(t.Context(), sig, nil, tt.creds(t)) if tt.wantErr == "" { require.NoError(t, err) returnAlso applies to: 508-519
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/go/rsa/signing/handler/handler_test.go` around lines 57 - 65, The shared variable rootPEM is causing data races between parallel subtests (the outer alg subtest and inner t.Parallel() cases); remove the shared state by making rootPEM local to each parallel case — e.g. move rootPEM creation into the tc.build or tc.creds functions (or return it from build) so each test case gets its own independent rootPEM, and update references in the test cases and any helper functions to use the per-case value instead of the package-scoped rootPEM.
🧹 Nitpick comments (1)
bindings/go/rsa/signing/handler/handler.go (1)
422-431: 💤 Low valueConsider conditional assignment in
rsaIdentityToMaponly if identity-graph matching requires absent keys rather than empty values.The function unconditionally populates
IdentityAttributeAlgorithmandIdentityAttributeSignatureeven when the corresponding fields are empty strings. The typedRSAIdentityusesomitemptyJSON tags, creating a semantic mismatch: the typed representation omits empty fields, while the map always includes them. This could theoretically cause identity matching to fail if consumers expect the absence of a key rather than an empty string value.However, the test suite explicitly validates that both keys are always present in the resulting map (e.g.,
require.Equal(t, alg, id[identityv1.IdentityAttributeAlgorithm])). Applying the proposed conditional-assignment refactor would require updating all identity assertions. Only pursue this if you confirm that identity-graph matching actually fails with the current unconditional behavior, or if the code path documents that empty values must be omitted to match the typed representation's semantics.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bindings/go/rsa/signing/handler/handler.go` around lines 422 - 431, The rsaIdentityToMap function currently always sets identityv1.IdentityAttributeAlgorithm and identityv1.IdentityAttributeSignature to potentially-empty strings, which can differ from the typed RSAIdentity omitempty semantics; change rsaIdentityToMap (in handler.go) to only set m[identityv1.IdentityAttributeAlgorithm] = id.Algorithm and m[identityv1.IdentityAttributeSignature] = id.Signature when id.Algorithm and id.Signature are non-empty (e.g., check id.Algorithm != "" and id.Signature != "") so absent fields are truly omitted from the returned runtime.Identity map; if you make this change, run/update tests that assert keys exist (they reference rsaIdentityToMap outputs) to reflect the new omission behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@bindings/go/rsa/signing/handler/internal/credentials/credentials.go`:
- Around line 51-56: CertificateChainFromCredentials currently swallows
loadBytes errors and returns (nil, nil); change it so any error from loadBytes
is propagated: call loadBytes(creds.PublicKeyPEM, creds.PublicKeyPEMFile), if
err != nil return nil, err, and only if err == nil and len(b) == 0 return nil,
nil; then continue to call rsapem.ParseCertificateChain(b) and return its
result. This ensures failures reading PublicKeyPEMFile (or other IO errors) are
not treated as "no chain configured" — update the function
CertificateChainFromCredentials and keep references to loadBytes,
creds.PublicKeyPEM, and creds.PublicKeyPEMFile.
In `@bindings/go/rsa/spec/credentials/v1/schemas/RSACredentials.schema.json`:
- Around line 34-37: The schema currently only requires "type" so an empty
RSACredentials object is considered valid; update the RSACredentials schema by
adding an "anyOf" constraint that requires at least one key-material property
(e.g., require one of "privateKeyPem", "publicKeyPem", "privateKeyJwk",
"publicKeyJwk" — or whatever key names your template uses) so validation fails
when no usable key is present; keep "additionalProperties": false and ensure the
new "anyOf" entries reference the exact property names used in the schema (e.g.,
the RSACredentials schema object where "required": ["type"] is defined).
---
Outside diff comments:
In `@bindings/go/rsa/signing/handler/handler_test.go`:
- Around line 57-65: The shared variable rootPEM is causing data races between
parallel subtests (the outer alg subtest and inner t.Parallel() cases); remove
the shared state by making rootPEM local to each parallel case — e.g. move
rootPEM creation into the tc.build or tc.creds functions (or return it from
build) so each test case gets its own independent rootPEM, and update references
in the test cases and any helper functions to use the per-case value instead of
the package-scoped rootPEM.
---
Nitpick comments:
In `@bindings/go/rsa/signing/handler/handler.go`:
- Around line 422-431: The rsaIdentityToMap function currently always sets
identityv1.IdentityAttributeAlgorithm and identityv1.IdentityAttributeSignature
to potentially-empty strings, which can differ from the typed RSAIdentity
omitempty semantics; change rsaIdentityToMap (in handler.go) to only set
m[identityv1.IdentityAttributeAlgorithm] = id.Algorithm and
m[identityv1.IdentityAttributeSignature] = id.Signature when id.Algorithm and
id.Signature are non-empty (e.g., check id.Algorithm != "" and id.Signature !=
"") so absent fields are truly omitted from the returned runtime.Identity map;
if you make this change, run/update tests that assert keys exist (they reference
rsaIdentityToMap outputs) to reflect the new omission behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a6fdfd0-8d21-4d05-bf29-23eeabb4a2e1
📒 Files selected for processing (18)
bindings/go/rsa/signing/handler/handler.gobindings/go/rsa/signing/handler/handler_test.gobindings/go/rsa/signing/handler/internal/credentials/credentials.gobindings/go/rsa/signing/handler/internal/credentials/credentials_test.gobindings/go/rsa/spec/credentials/v1/rsa_credentials.gobindings/go/rsa/spec/credentials/v1/rsa_credentials_test.gobindings/go/rsa/spec/credentials/v1/schemas/RSACredentials.schema.jsonbindings/go/rsa/spec/credentials/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/credentials/v1/zz_generated.ocm_jsonschema.gobindings/go/rsa/spec/credentials/v1/zz_generated.ocm_type.gobindings/go/rsa/spec/identity/v1/register.gobindings/go/rsa/spec/identity/v1/schemas/RSAIdentity.schema.jsonbindings/go/rsa/spec/identity/v1/type.gobindings/go/rsa/spec/identity/v1/type_test.gobindings/go/rsa/spec/identity/v1/zz_generated.deepcopy.gobindings/go/rsa/spec/identity/v1/zz_generated.ocm_jsonschema.gobindings/go/rsa/spec/identity/v1/zz_generated.ocm_type.godocs/adr/0018_typed_credentials.md
🚧 Files skipped from review as they are similar to previous changes (1)
- bindings/go/rsa/spec/identity/v1/zz_generated.ocm_jsonschema.go
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
On-behalf-of: SAP <matthias.bruns@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
Draft
4 tasks
4 tasks
jakobmoellerdev
approved these changes
May 7, 2026
jakobmoellerdev
left a comment
Member
There was a problem hiding this comment.
this is lgtm to me. only think im wondering is if the keys should change but there I need some more context. apart from this, great change, very focused!
frewilhelm
approved these changes
May 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
Phase 2 typed signing bindings ADR
Which issue(s) this PR fixes
Contributes: open-component-model/ocm-project#988
You can see the whole picture in #2148
Testing
How to test the changes
Unit & integration tests only.
I ran
task init/go.workwith the updatedcredentialsbinding to see if the update breaks ocm - it does not :)Verification
task testandtask test/integrationif applicable)