Skip to content

chore: add basic constraints deduction for sign certificate#5313

Merged
carlosmonastyrski merged 4 commits intomainfrom
misc/add-basic-constraints-deduction
Jan 29, 2026
Merged

chore: add basic constraints deduction for sign certificate#5313
carlosmonastyrski merged 4 commits intomainfrom
misc/add-basic-constraints-deduction

Conversation

@sheensantoscapadngan
Copy link
Member

@sheensantoscapadngan sheensantoscapadngan commented Jan 29, 2026

Context

This PR ensures that when certKeySign is requested then isCA basic constraints is treated as true. This is needed because some clients like cert-manager do not pass in basic constraints extension in the CSR

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

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 29, 2026

Greptile Overview

Greptile Summary

Enhanced certificate signing to automatically deduce CA status when KEY_CERT_SIGN key usage is present in the CSR, per RFC 5280. This handles cases where clients don't explicitly send basicConstraints.

  • Added early deduction logic that computes resolvedBasicConstraints by checking if KEY_CERT_SIGN is in key usages or if isCA: true is explicitly set
  • Applied resolvedBasicConstraints consistently in the approval flow path
  • Found one critical inconsistency: line 1446 still uses original basicConstraints instead of resolvedBasicConstraints in the direct signing path, which means the deduced CA status won't be stored in certificate requests that bypass approval

Confidence Score: 3/5

  • Contains a logic bug that causes inconsistent behavior between approval and direct signing paths
  • The deduction logic itself is sound and includes proper security checks (line 1372 validates policy allows CA issuance), but line 1446 uses the wrong variable, causing the deduced basicConstraints to not be stored for certificates that bypass approval. This creates inconsistent behavior and could lead to CA certificates not being properly tracked in the database.
  • backend/src/services/certificate-v3/certificate-v3-service.ts requires fixing the inconsistency at line 1446

Important Files Changed

Filename Overview
backend/src/services/certificate-v3/certificate-v3-service.ts Added logic to deduce CA status from KEY_CERT_SIGN key usage, but inconsistent use of resolved vs original basicConstraints in direct signing path

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.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 29, 2026

Additional Comments (1)

backend/src/services/certificate-v3/certificate-v3-service.ts
inconsistent use of original basicConstraints instead of resolvedBasicConstraints. The approval path (line 1289) correctly uses resolvedBasicConstraints, but this direct signing path still uses the original basicConstraints parameter, which may not reflect the deduced CA status from KEY_CERT_SIGN key usage

          basicConstraints: resolvedBasicConstraints,

@maidul98
Copy link
Collaborator

maidul98 commented Jan 29, 2026

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 merged commit b9e5f35 into main Jan 29, 2026
10 checks passed
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