Skip to content

Conversation

@carlosmonastyrski
Copy link
Contributor

Context

This PR contains some fixes and improvements on PKI Certificate Authorities managment:

  • Now root CAs can be created without a certificate attached to them and instead issue a certificate for them after creation
  • Add new endpoint to issue root CA certificates and simplified the intermediate CAs certificate issuance so everything is centralized in a single endpoint
  • Add new audit log for this new CA certificate issuance endpoint
  • Update frontend to now use the centralized endpoint as well
  • Small fix on self-signed certificate profiles throwing an error when fetching those with the GET profile endpoint.

Screenshots

Steps to verify the change

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Read the contributing guide

@maidul98
Copy link
Collaborator

maidul98 commented Dec 11, 2025

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@carlosmonastyrski carlosmonastyrski changed the title feat: fixes and improvements on PKI CAs fix: fixes and improvements on PKI CAs Dec 11, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 11, 2025

Greptile Overview

Greptile Summary

This PR refactors PKI certificate authority management to support deferred certificate generation. Root CAs can now be created without certificates and have certificates generated later through a new centralized endpoint.

Key Changes

  • New centralized endpoint: POST /api/v1/cert-manager/ca/internal/:caId/certificate handles both root and intermediate CA certificate generation
  • Deferred certificate generation: Root CAs can be created in PENDING_CERTIFICATE status and activated later
  • Simplified intermediate CA flow: Frontend now uses the unified endpoint instead of separate sign + import operations
  • Bug fix: Certificate profile DAL properly returns undefined for certificateAuthority when caId is null

Issues Found

  • Critical logic bug (line 206-208 in service): notAfterDate calculation ignores the provided notAfter parameter and always sets it to 10 years from now
  • Security concern (line 1214-1226 in service): Missing cross-project permission validation allows using parent CAs from unauthorized projects when generating intermediate certificates
  • Null safety issues (lines 1293, 1303 in service): Force-unwrapping actorOrgId that may be undefined

Confidence Score: 2/5

  • This PR has critical logic bugs and security vulnerabilities that must be fixed before merging
  • Score reflects a critical date calculation bug that breaks certificate validity, a security vulnerability allowing cross-project CA access, and multiple null safety issues. The refactoring is well-structured but the implementation has serious issues.
  • Pay close attention to backend/src/services/certificate-authority/internal/internal-certificate-authority-service.ts - contains critical bugs and security issues that must be resolved

Important Files Changed

File Analysis

Filename Score Overview
backend/src/services/certificate-authority/internal/internal-certificate-authority-service.ts 3/5 Added generateRootCaCertificate and generateIntermediateCaCertificate functions, modified createCa to support deferred certificate generation. Logic bug in notAfterDate calculation, missing cross-project permission validation.
backend/src/server/routes/v1/certificate-authority-routers/internal-certificate-authority-router.ts 4/5 Added new POST /:caId/certificate endpoint to centralize CA certificate generation for both root and intermediate CAs with audit logging.
backend/src/services/certificate-profile/certificate-profile-dal.ts 5/5 Fixed bug where certificateAuthority was always returned even when caId was null, now properly returns undefined for self-signed profiles.
frontend/src/pages/cert-manager/CertAuthDetailsByIDPage/components/CaGenerateRootCertModal.tsx 4/5 New modal component for generating root CA certificates with date validation and max path length configuration. Missing notBefore field in UI.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

13 files reviewed, 6 comments

Edit Code Review Agent Settings | Greptile

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.

4 participants