fix: x509 controller#338
Conversation
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds X.509 certificate support (controller, service, types, swagger), enhances DID key handling with curve normalization and validation, introduces curve/key-type utilities and constants, and removes the auto-generated TSOA routes file. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant X509Controller
participant x509Service
participant KMS
participant X509Service
Client->>X509Controller: POST /x509 (create options)
X509Controller->>x509Service: createCertificate(options)
rect rgb(230, 240, 255)
Note over x509Service,KMS: Ensure/derive authority key material
x509Service->>KMS: createKey / importKey (authority)
KMS-->>x509Service: authorityPublicJwk / keyId
end
alt subjectPublicKey provided
x509Service->>KMS: importKey(subjectPublicKeyJwk)
KMS-->>x509Service: subjectKeyId
end
rect rgb(220, 255, 230)
Note over x509Service,X509Service: Issue certificate using public JWK
x509Service->>X509Service: issueCertificate(issuer, subject, publicKey)
X509Service-->>x509Service: certificateDER
end
x509Service-->>X509Controller: base64EncodedCertificate
X509Controller-->>Client: 200 OK (certificate)
sequenceDiagram
participant Client
participant X509Controller
participant x509Service
participant X509Decoder
participant KMS
Client->>X509Controller: POST /x509/import (cert, privateKey)
X509Controller->>x509Service: ImportX509Certificates(options)
rect rgb(240, 230, 230)
Note over x509Service,X509Decoder: Decode certificate and extract public key
x509Service->>X509Decoder: decodeCertificate(base64)
X509Decoder-->>x509Service: X509Certificate object (public key)
end
rect rgb(230, 240, 255)
Note over x509Service,KMS: Transform & import private key (JWK)
x509Service->>KMS: transformPrivateKeyToPrivateJwk(privateKey)
KMS-->>x509Service: privateJwk
x509Service->>KMS: importKey(privateJwk)
KMS-->>x509Service: keyId / KeyExists error
end
alt import and validation succeed
x509Service-->>X509Controller: Success response
X509Controller-->>Client: 200 OK
else failure
x509Service-->>X509Controller: throw
X509Controller-->>Client: 4xx/5xx error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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 |
Signed-off-by: Krishna Waske <krishna.waske@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/controllers/did/DidController.ts (2)
75-75: Remove debug console.log statement.This appears to be debug code that should be removed before merging.
🔎 Proposed fix
- console.log("dsajsa", askar.version())
164-166: Logic error: condition is always false.The condition
!Network.Bcovrin_Testnet && !Network.Indicio_Demonet && !Network.Indicio_Testnetwill always befalsebecause enum values are truthy strings. This validation never triggers.You likely meant to check if the provided network doesn't match any of the valid networks:
🔎 Proposed fix
- if (!Network.Bcovrin_Testnet && !Network.Indicio_Demonet && !Network.Indicio_Testnet) { - throw new BadRequestError(`Invalid network for 'indy' method: ${createDidOptions.network}`) - } + const validNetworks = [Network.Bcovrin_Testnet, Network.Indicio_Demonet, Network.Indicio_Testnet, Network.Indicio_Mainnet] + if (!validNetworks.includes(createDidOptions.network as Network)) { + throw new BadRequestError(`Invalid network for 'indy' method: ${createDidOptions.network}`) + }
🧹 Nitpick comments (5)
src/enums/enum.ts (1)
101-109:declare enummay not emit runtime values.Using
export declare enumcreates an ambient declaration that won't generate JavaScript code at runtime. If this enum is intended for runtime use (e.g., in comparisons or mappings), useexport enuminstead.Also note the inconsistent casing:
Bls12381G2 = 'bls12381g2'uses lowercase value while others likeEd25519 = 'Ed25519'preserve case.🔎 Proposed fix
-export declare enum KeyAlgorithmCurve { +export enum KeyAlgorithmCurve { Ed25519 = 'Ed25519', X25519 = 'X25519', P256 = 'P-256', P384 = 'P-384', P521 = 'P-521', secp256k1 = 'secp256k1', - Bls12381G2 = 'bls12381g2', + Bls12381G2 = 'Bls12381G2', }src/utils/helpers.ts (1)
97-102: Silent fallback to P-256 may hide configuration errors.When an unrecognized curve is provided, this silently defaults to
EC/P-256. Consider logging a warning or throwing an error to surface misconfiguration early.🔎 Proposed fix
} else { + console.warn(`Unknown curve "${key}", defaulting to P-256`) keyTypeInfo = { kty: 'EC', crv: 'P-256', } }src/controllers/x509/x509.Controller.ts (1)
28-38: Method name should use camelCase.
ImportX509Certificatesshould beimportX509Certificatesto follow TypeScript/JavaScript naming conventions for methods.🔎 Proposed fix
@Post('/import') - public async ImportX509Certificates( + public async importX509Certificates( @Request() request: Req, @Body() importX509Options: X509ImportCertificateOptionsDto, ) { try { - return await x509ServiceT.ImportX509Certificates(request, importX509Options) + return await x509ServiceT.importX509Certificates(request, importX509Options)src/controllers/x509/x509.types.ts (1)
231-239: Typo in type name: "Assymetric" should be "Asymmetric".🔎 Proposed fix
-export type KmsCreateKeyTypeAssymetric = +export type KmsCreateKeyTypeAsymmetric = | { kty: 'OKP' crv: 'Ed25519' | 'X25519' } | { kty: 'EC' crv: 'P-256' | 'P-384' | 'P-521' | 'secp256k1' }src/controllers/x509/x509.service.ts (1)
213-216: Logging potentially undefined private key.
parsedCertificate.privateKeyis typically undefined on a parsed X509Certificate (certificates don't contain private keys). This log statement will always showundefined.🔎 Proposed fix
if (error instanceof Kms.KeyManagementKeyExistsError) { - console.error( - `key already exists while importing certificate ${JSON.stringify(parsedCertificate.privateKey)}`, - parsedCertificate.privateKey, - ) + agent.config.logger.warn('Key already exists while importing certificate') } else {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/controllers/did/DidController.tssrc/controllers/x509/x509.Controller.tssrc/controllers/x509/x509.service.tssrc/controllers/x509/x509.types.tssrc/enums/enum.tssrc/routes/routes.tssrc/routes/swagger.jsonsrc/utils/constant.tssrc/utils/helpers.ts
💤 Files with no reviewable changes (1)
- src/routes/routes.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/utils/constant.ts (1)
src/controllers/x509/x509.types.ts (1)
Curve(223-223)
src/controllers/x509/x509.Controller.ts (3)
src/controllers/x509/x509.types.ts (1)
X509CreateCertificateOptionsDto(176-208)src/controllers/x509/x509.service.ts (1)
x509ServiceT(265-265)src/controllers/types.ts (1)
X509ImportCertificateOptionsDto(457-471)
src/controllers/x509/x509.service.ts (5)
src/controllers/types.ts (2)
BasicX509CreateCertificateConfig(450-455)X509ImportCertificateOptionsDto(457-471)src/utils/helpers.ts (3)
getCertificateValidityForSystem(33-51)getTypeFromCurve(84-105)generateSecretKey(15-31)src/controllers/x509/x509.types.ts (1)
X509CreateCertificateOptionsDto(176-208)src/controllers/x509/crypto-util.ts (1)
pemToRawEd25519PrivateKey(23-38)src/utils/logger.ts (1)
error(120-122)
src/utils/helpers.ts (2)
src/controllers/x509/x509.types.ts (4)
Curve(223-223)OkpType(213-216)EcType(218-221)OkpCurve(210-210)src/utils/constant.ts (2)
curveToKty(14-21)keyAlgorithmToCurve(6-13)
src/controllers/did/DidController.ts (3)
src/utils/constant.ts (1)
keyAlgorithmToCurve(6-13)src/controllers/x509/x509.types.ts (1)
supportedKeyTypesDID(241-266)src/utils/helpers.ts (1)
getTypeFromCurve(84-105)
🪛 Biome (2.1.2)
src/controllers/x509/x509.service.ts
[error] 50-50: Incorrect suppression: missing reason. Example of suppression: // biome-ignore lint: false positive
A reason is mandatory: try to explain why the suppression is needed.
Example of suppression: // biome-ignore lint: reason
(suppressions/parse)
🪛 Checkov (3.2.334)
src/routes/swagger.json
[medium] 102-111: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (4)
src/controllers/x509/x509.Controller.ts (1)
11-78: LGTM on controller structure.The controller properly delegates to the service layer and consistently wraps operations with error handling via
ErrorHandlingService.handle(error).src/controllers/x509/x509.types.ts (1)
241-266: Well-structured DID method key type constraints.The
supportedKeyTypesDIDconstant provides clear validation rules per DID method. This is a good design for enforcing key type compatibility.src/routes/swagger.json (1)
1681-1891: X509 endpoints are well-defined.The new X509 API endpoints are properly documented with request/response schemas, security requirements, and appropriate HTTP methods.
src/utils/constant.ts (1)
6-13: Incomplete keyAlgorithmToCurve mapping: P-521 is missing.The
keyAlgorithmToCurvemapping does not include a P-521 entry, even thoughP-521is defined in theCurvetype and exists in thecurveToKtymapping. If a P-521KeyAlgorithm(e.g.,KeyAlgorithm.EcSecp521r1) is passed togetTypeFromCurve, thenormalizeToCurvefunction will returnundefined, causinggetTypeFromCurveto silently fall back to the defaultP-256.
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)
src/controllers/did/DidController.ts (1)
75-75: Remove debug console.log statement.
console.log("dsajsa", askar.version())appears to be debug code that should be removed.🔎 Proposed fix
- console.log("dsajsa", askar.version())
♻️ Duplicate comments (10)
src/utils/helpers.ts (1)
103-103: Remove debug console.log statement.This debug logging should be removed before merging.
src/controllers/did/DidController.ts (2)
19-19: Remove unused import.The
getimport from'http'is not used.
415-415: Remove debug console.log statements.Multiple debug statements should be removed before merging.
Also applies to: 444-444, 451-451, 481-482
src/controllers/x509/x509.service.ts (5)
50-51: Add reason to biome-ignore suppression.Per static analysis, the suppression requires a reason.
148-149: Remove debug console.log statements.Debug statements should be removed before merging.
Also applies to: 161-161, 207-207, 209-209
177-177: Typo: "issuerCertficicate" should be "issuerCertificate".This typo affects API response consistency.
Also applies to: 223-223
267-292:createKeyignoreskeyTypeparameter.The function accepts
keyType: KeyAlgorithmbut hardcodescrv: 'P-256'andkty: 'EC'.
271-271: Security: Logging cryptographic seed.Line 271 logs the seed used for key generation. This is sensitive cryptographic material that should never be logged.
🔎 Proposed fix
- agent.config.logger.debug(`createKey: got seed ${seed}`) + agent.config.logger.debug('createKey: seed generated successfully')src/routes/swagger.json (2)
364-364:keyTypeproperty lacks type specification.The schema is empty
{}, allowing any value.
1738-1739: Typo in response property name.
issuerCertficicateshould beissuerCertificate.
🧹 Nitpick comments (4)
src/utils/helpers.ts (1)
97-101: Silent fallback to P-256 may mask invalid input.When
normalizeToCurvefails (returnsundefined), the function silently defaults to{ kty: 'EC', crv: 'P-256' }. This could mask configuration errors where an unsupported curve is passed, making debugging difficult.Consider throwing an error or logging a warning for unsupported curves:
🔎 Proposed fix
} else { - keyTypeInfo = { - kty: 'EC', - crv: 'P-256', - } + throw new Error(`Unsupported curve or key algorithm: ${key}`) }src/controllers/x509/x509.Controller.ts (1)
28-38: Inconsistent method naming convention.
ImportX509Certificatesuses PascalCase while other methods use camelCase (createX509Certificate,addTrustedCertificate). Consider renaming for consistency:@Post('/import') - public async ImportX509Certificates( + public async importX509Certificates(src/controllers/did/DidController.ts (1)
405-411: Fragile string comparison for p521 check.The
'p521' as KeyAlgorithmcast is a workaround. If the KeyAlgorithm enum doesn't include p521, this check may not catch all cases. Consider validating againstkeyAlgorithmToCurvemapping instead:- if (didOptions.keyType === 'p521' as KeyAlgorithm) { - throw new BadRequestError('didOptions.keyType for type p521 is not supported') - } - const normalizedCurve = keyAlgorithmToCurve[didOptions.keyType as KeyAlgorithm] - if (!(normalizedCurve && supportedKeyTypesDID[DidMethod.Key]?.some(kt => kt.crv === normalizedCurve))) { + const normalizedCurve = keyAlgorithmToCurve[didOptions.keyType as KeyAlgorithm] + if (!normalizedCurve) { + throw new BadRequestError(`Unsupported keyType: ${didOptions.keyType}`) + } + if (!supportedKeyTypesDID[DidMethod.Key]?.some(kt => kt.crv === normalizedCurve)) { throw new BadRequestError(`Invalid keyType: ${didOptions.keyType}`) }src/controllers/x509/x509.types.ts (1)
231-239: Typo in type name: "Assymetric" should be "Asymmetric".
KmsCreateKeyTypeAssymetrichas a spelling error. Consider renaming:-export type KmsCreateKeyTypeAssymetric = +export type KmsCreateKeyTypeAsymmetric =
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/controllers/did/DidController.tssrc/controllers/x509/x509.Controller.tssrc/controllers/x509/x509.service.tssrc/controllers/x509/x509.types.tssrc/enums/enum.tssrc/routes/routes.tssrc/routes/swagger.jsonsrc/utils/constant.tssrc/utils/helpers.ts
💤 Files with no reviewable changes (1)
- src/routes/routes.ts
🧰 Additional context used
🧬 Code graph analysis (5)
src/utils/constant.ts (1)
src/controllers/x509/x509.types.ts (1)
Curve(223-223)
src/utils/helpers.ts (2)
src/controllers/x509/x509.types.ts (4)
Curve(223-223)OkpType(213-216)EcType(218-221)OkpCurve(210-210)src/utils/constant.ts (2)
curveToKty(14-21)keyAlgorithmToCurve(6-13)
src/controllers/x509/x509.Controller.ts (3)
src/controllers/x509/x509.types.ts (1)
X509CreateCertificateOptionsDto(176-208)src/controllers/x509/x509.service.ts (1)
x509ServiceT(265-265)src/controllers/types.ts (1)
X509ImportCertificateOptionsDto(457-471)
src/controllers/did/DidController.ts (4)
src/errors/errors.ts (1)
BadRequestError(102-102)src/utils/constant.ts (1)
keyAlgorithmToCurve(6-13)src/controllers/x509/x509.types.ts (1)
supportedKeyTypesDID(241-266)src/utils/helpers.ts (1)
getTypeFromCurve(84-105)
src/controllers/x509/x509.service.ts (5)
src/controllers/types.ts (2)
BasicX509CreateCertificateConfig(450-455)X509ImportCertificateOptionsDto(457-471)src/utils/helpers.ts (3)
getCertificateValidityForSystem(33-51)getTypeFromCurve(84-105)generateSecretKey(15-31)src/controllers/x509/x509.types.ts (1)
X509CreateCertificateOptionsDto(176-208)src/controllers/x509/crypto-util.ts (1)
pemToRawEd25519PrivateKey(23-38)src/utils/logger.ts (1)
error(120-122)
🪛 Biome (2.1.2)
src/controllers/x509/x509.service.ts
[error] 50-50: Incorrect suppression: missing reason. Example of suppression: // biome-ignore lint: false positive
A reason is mandatory: try to explain why the suppression is needed.
Example of suppression: // biome-ignore lint: reason
(suppressions/parse)
🪛 Checkov (3.2.334)
src/routes/swagger.json
[medium] 102-111: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🔇 Additional comments (4)
src/controllers/x509/x509.Controller.ts (1)
15-78: Controller implementation looks good.The X509Controller correctly implements CRUD operations with proper error handling delegation to
ErrorHandlingService. Security decorators and route configuration are appropriate.src/controllers/x509/x509.types.ts (1)
209-266: Well-structured type definitions.The new curve and key type definitions provide clear, type-safe representations of cryptographic parameters. The
supportedKeyTypesDIDmapping effectively constrains key types per DID method.src/routes/swagger.json (1)
1681-1891: X509 endpoint definitions look complete.The new
/x509endpoints are properly documented with appropriate request/response schemas, security requirements, and operation IDs.src/utils/constant.ts (1)
6-13: P-521 incurveToKtybut missing fromkeyAlgorithmToCurvemapping.The
curveToKtyobject includes'P-521': 'EC', butkeyAlgorithmToCurvelacks a corresponding mapping. While askar'sKeyAlgorithmenum doesn't currently appear to include a P-521 variant (the code only references Ed25519, X25519, EcSecp256r1, EcSecp384r1, EcSecp256k1, and Bls12381G2), this inconsistency means if P-521 support is added toKeyAlgorithmin the future—or if P-521 is passed through unmapped paths—getTypeFromCurvewould fail to normalize it and fall back to the default P-256. Consider adding the mapping proactively:export const keyAlgorithmToCurve: Partial<Record<KeyAlgorithm, Curve>> = { [KeyAlgorithm.Ed25519]: 'Ed25519', [KeyAlgorithm.X25519]: 'X25519', [KeyAlgorithm.EcSecp256r1]: 'P-256', [KeyAlgorithm.EcSecp384r1]: 'P-384', [KeyAlgorithm.EcSecp256k1]: 'secp256k1', + // [KeyAlgorithm.EcSecp521r1]: 'P-521', // Add when P-521 support is added to KeyAlgorithm }
Signed-off-by: sujitaw <sujit.sutar@ayanworks.com>
Signed-off-by: sujitaw <sujit.sutar@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>
3 participants
What:
ktyandcrvas dynamicSummary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.