Skip to content

Conversation

@shitrerohit
Copy link

@shitrerohit shitrerohit commented Jan 27, 2026

What?

Return key id when import x5c certificate
Attach keyId when we store the sd-jwt credential with P-256 signer option

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced certificate handling for credential issuance flows to properly track and return key identifiers alongside certificate data.
  • Refactor

    • Streamlined certificate parsing logic to consolidate certificate usage and improve key identification in credential flows.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

  • 🔍 Trigger a full review
📝 Walkthrough

Walkthrough

The changes refactor X.509 certificate import and processing logic to extract and return key identifiers alongside certificates. Type imports are optimized, and certificate/key handling flows now track and return keyId metadata from imported certificates.

Changes

Cohort / File(s) Summary
Import Statement Optimization
src/controllers/x509/crypto-util.ts
Converted KeyObject to type-only import and added separate value import for createPrivateKey to clarify runtime vs. type-only dependencies.
Certificate and Key Identifier Handling
src/controllers/x509/x509.service.ts, src/utils/oid4vc-agent.ts
Modified ImportX509Certificates and related flows to declare key variables outside try blocks, extract keyId during certificate import, and update return signatures to include keyId alongside issuerCertificate. Introduced conditional certificate parsing in SdJwtDc and MsoMdoc flows with single parsed certificate usage instead of mapping multiple entries. Removed legacy holder default logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 Certificates now carry their IDs with pride,
KeyIds extracted and bundled inside,
Type imports refined, the logic runs clean,
X.509 dancing between layers unseen! 🔐

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% 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
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title describes fixing SD-JWT issuance issues, which aligns with the main changes: returning keyId when importing certificates and attaching keyId to SD-JWT credentials.

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

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/oid4vc-issuance-changes

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.

@shitrerohit shitrerohit requested review from GHkrishna and RinkalBhojani and removed request for GHkrishna January 27, 2026 07:48
@shitrerohit
Copy link
Author

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Jan 27, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@src/utils/oid4vc-agent.ts`:
- Around line 189-197: The code indexes issuerx509certificate[0] without
checking for an empty array which can pass undefined to
X509Service.parseCertificate; update the branch where
issuerDidVerificationMethod is falsy to first verify issuerx509certificate is an
array with length > 0, otherwise throw a clear Error mentioning
credentialConfigurationId, and only then call X509Service.parseCertificate and
set parsedCertificate.publicJwk.keyId (same change should be applied to the
similar block around the other occurrence referenced at line ~219); reference
symbols: issuerx509certificate, issuerDidVerificationMethod,
X509Service.parseCertificate, parsedCertificate, credential.signerOptions.keyId,
credentialConfigurationId.

GHkrishna and others added 22 commits January 31, 2026 15:30
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* fix: controller for oob, connection and QnA

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix: update to version 0.6.1

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix: remove unwanted var

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix: static types

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix: remove legacy connection invitation

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

---------

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
* fix: x509 controller

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix: x509 import fix

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>

* fix/added dynamic implementation for keyType of x509

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

* fix/sonarqube issue

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

* fix/sonarqube issue

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

* fix/sonar cube issue

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

* fix/code rabbit comments

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

* fix/pr comments

Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>

---------

Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Co-authored-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
@shitrerohit shitrerohit force-pushed the fix/oid4vc-issuance-changes branch from 80236e4 to f194bb3 Compare January 31, 2026 12:56
@shitrerohit shitrerohit changed the base branch from fix/agent-setup-and-controllers to feat/oidc-main-sync January 31, 2026 12:59
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
Signed-off-by: shitrerohit <rohit.shitre@ayanworks.com>
@sonarqubecloud
Copy link

Comment on lines +121 to +122
"@credo-ts/core": "0.6.1",
"@credo-ts/askar": "0.6.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can remove this

cNonceExpiresInSeconds: Number(process.env.OID4VCI_CNONCE_EXPIRY) || 3600,
dpopRequired: false,
credentialRequestToCredentialMapper: (...args) => getMixedCredentialRequestToCredentialMapper()(...args),
// openid4vc: new OpenId4VcModule({}),
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a TODO here, for the code to be un-commented in the future

@@ -1,4 +1,7 @@
// FIXME: We've made many changes in this file for building agent with OIDC modules, please check the types and proper implementation of the changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this comment if it's purpose is served?

Comment on lines +47 to +50
// public async deleteIssuer(agentReq: Req, issuerId: string) {
// const result = (agentReq.agent as Agent<RestAgentModules>).openid4vc.config.issuer.
// return result
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, why we have commented this??

throw ErrorHandlingService.handle(error)
}
}
// @Delete('{id}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it not necessary for us to delete issuer??

* Delete issuance session by session ID
*/
@Delete('/:issuanceSessionId')
@Delete('{issuanceSessionId}')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have we changed this?

Comment on lines +132 to +134
// if (credential instanceof W3cJwtVerifiableCredential || credential instanceof W3cJsonLdVerifiableCredential) {
// return await agentReq.agent.w3cCredentials.storeCredential({ credential })
// }
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we not need this code for now?

kms
.createKeyForSignatureAlgorithm({
algorithm: signatureAlgorithm!,
// FIXME: what should happen with already existing keys created in the secure environment?
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove if it has been tested??

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.

3 participants