Skip to content

feat: add code-signing to PKI#5682

Open
carlosmonastyrski wants to merge 3 commits intomainfrom
feat/PKI-149
Open

feat: add code-signing to PKI#5682
carlosmonastyrski wants to merge 3 commits intomainfrom
feat/PKI-149

Conversation

@carlosmonastyrski
Copy link
Contributor

Context

Adds code signing support to the cert-manager project type, allowing users to create signers backed by certificates with the codeSigning EKU and an approval policy. Signers can be enabled/disabled, and all signing operations require an active approval grant (supporting manual, time-window, and count-limited approval modes).

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)
  • Updated CLAUDE.md files (if needed)
  • Read the contributing guide

@linear
Copy link

linear bot commented Mar 13, 2026

@maidul98
Copy link
Collaborator

maidul98 commented Mar 13, 2026

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

Status Scan Engine 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.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 13, 2026

Greptile Summary

This PR introduces a complete code-signing subsystem for the cert-manager project type, including a Signers entity backed by a codeSigning EKU certificate, an approval-grant flow (manual, time-window, and n-signings modes), signing operation audit records, and corresponding frontend pages. The design is well-structured and integrates cleanly with the existing approval-policy factory pattern.

Key issues found:

  • Race condition in maxSignings enforcement (signer-service.ts:sign): Under PostgreSQL's default READ COMMITTED isolation, two concurrent requests sharing the same n-signings=1 (or Manual mode) grant can both read usedCount = 0 before either transaction commits, allowing more signings than the grant permits. A SELECT ... FOR UPDATE lock on the grant row within the transaction is needed to make the count check atomic.
  • Actor name lost on user deletion (signing-operation-dal.ts): The actorName column in signing_operations is always NULL at write time; names are resolved via joins at read time. If a user or identity is deleted, all their historical signing records permanently lose their actor name, breaking audit log immutability. The name should be captured at signing time.
  • Generic permission action names (project-permission.ts): The new ProjectPermissionCodeSigningActions enum uses generic names (Read, Create, Edit, Delete) alongside the well-named Sign action — consider more descriptive subject-specific names.
  • User-controlled clientMetadata.ip: The IP field in signing request metadata is entirely self-reported by the client and unverified, which could mislead security investigations.
  • No documentation in /docs: The code signing feature has no corresponding documentation in the /docs folder. How will customers discover this feature?

Confidence Score: 2/5

  • Not safe to merge — a concurrency race condition can allow a single-use approval grant to be used more than once under concurrent signing requests.
  • The TOCTOU race in sign() is a real security bypass: an n-signings=1 or Manual grant can be consumed multiple times if two requests arrive simultaneously, circumventing the core governance guarantee of the approval flow. The actor-name issue also undermines audit immutability. These should be addressed before merge.
  • backend/src/services/signer/signer-service.ts (race condition in sign), backend/src/services/signer/signing-operation-dal.ts (actorName not captured at write time)

Important Files Changed

Filename Overview
backend/src/services/signer/signer-service.ts Core signing service — contains a TOCTOU race condition in maxSignings enforcement and does not capture actorName at write time, which breaks audit log immutability on user deletion.
backend/src/db/migrations/20260309141236_code-signing-tables.ts Idempotent migration using hasTable/hasColumn guards; creates Signers and SigningOperations tables and extends approval_request_grants with granteeMachineIdentityId. Down migration is correctly ordered.
backend/src/services/signer/signing-operation-dal.ts Signing operation DAL with multi-table joins to resolve actor names at query time; actorName is never persisted at write time meaning it's lost on user/identity deletion.
backend/src/services/approval-policy/code-signing/code-signing-policy-factory.ts Code signing approval policy factory implementing canAccess, validateConstraints, and postApprovalRoutine; canAccess parameter named userId is used for both user and identity grant lookups which is slightly misleading but functionally correct.
backend/src/server/routes/v1/signer-router.ts Well-structured router with audit logging for all endpoints; data size validation in body schema matches the 128-byte service limit; clientMetadata.ip is user-controlled.
backend/src/ee/services/permission/project-permission.ts Adds ProjectPermissionCodeSigningActions enum and CodeSigners subject; uses generic Read/Create/Edit/Delete action names alongside the specific Sign action.
backend/src/services/approval-policy/approval-policy-service.ts Extends createRequest to support machine identity requesters by conditionally setting requesterUserId vs machineIdentityId; machineIdentityId comes from authenticated token not body.
backend/src/services/certificate/certificate-dal.ts Adds extendedKeyUsage filter using parameterized whereRaw for safe PostgreSQL array containment check; properly applied to both count and list queries.

Comments Outside Diff (2)

  1. backend/src/services/signer/signer-service.ts, line 613-664 (link)

    TOCTOU race condition in maxSignings enforcement

    Under PostgreSQL's default READ COMMITTED isolation level, two concurrent signing requests sharing the same grant (e.g., maxSignings: 1) can both bypass the limit:

    1. Request A reads usedCount = 0 — proceeds
    2. Request B reads usedCount = 0 (before A's transaction commits) — also proceeds
    3. Both create a signing operation, resulting in two successful signings against a one-time grant

    The countByGrantId check and the subsequent signingOperationDAL.create are not atomic at this isolation level.

    Fix: Add SELECT ... FOR UPDATE on the grant row inside the transaction to pessimistically lock it before checking the count:

    // After finding matchingGrant, lock it before the count check:
    const [lockedGrant] = await (tx)(TableName.ApprovalRequestGrants)
      .where({ id: matchingGrant.id })
      .forUpdate();
    
    if (!lockedGrant || lockedGrant.status !== ApprovalRequestGrantStatus.Active) {
      throw new ForbiddenRequestError({ message: "Grant is no longer active." });
    }
    
    // Now safe to count and sign
    const usedCount = await signingOperationDAL.countByGrantId(matchingGrant.id, tx);
    if (attrs.maxSignings && usedCount >= attrs.maxSignings) {
      await approvalRequestGrantsDAL.updateById(matchingGrant.id, { status: ApprovalRequestGrantStatus.Expired }, tx);
      throw new ForbiddenRequestError({ message: "Signing grant has been exhausted." });
    }

    This ensures only one transaction at a time can proceed past the count check for a given grant, eliminating the race window for NSignings and Manual (which sets maxSignings: 1) approval modes.

  2. backend/src/ee/services/permission/project-permission.ts, line 395-401 (link)

    Generic permission action names

    Per the project's convention for new permission subjects, actions like Read, Create, Edit, and Delete are generic. Consider more descriptive, subject-specific action names that make it clearer what they govern. For example:

    Sign is already well-named and specific. Making the others subject-specific (e.g., ReadSigner) improves clarity when these permissions appear in logs, role-definition UI, and policy configs.

    Rule Used: When using parameters in API calls, ensure they ar... (source)

    Learnt From
    Infisical/infisical#3643

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Last reviewed commit: 7bf1275

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.

2 participants