Merge pull request #4895 from Infisical/PKI-53-workaround-for-cert-manager

[PKI-53] Workaround for cert manager ACME bug

backend/bdd/features/environment.py

@@ -3,6 +3,7 @@ import os
 
 import pathlib
 import typing
+from copy import deepcopy
 
 import httpx
 from behave.runner import Context
@@ -185,28 +186,33 @@ def bootstrap_infisical(context: Context):
 
 
 def before_all(context: Context):
+    base_vars = {
+        "BASE_URL": BASE_URL,
+        "PEBBLE_URL": PEBBLE_URL,
+    }
     if BOOTSTRAP_INFISICAL:
         details = bootstrap_infisical(context)
-        context.vars = {
-            "BASE_URL": BASE_URL,
-            "PEBBLE_URL": PEBBLE_URL,
+        vars = base_vars | {
             "PROJECT_ID": details["project"]["id"],
             "CERT_CA_ID": details["ca"]["id"],
             "CERT_TEMPLATE_ID": details["cert_template"]["id"],
             "AUTH_TOKEN": details["auth_token"],
         }
     else:
-        context.vars = {
-            "BASE_URL": BASE_URL,
-            "PEBBLE_URL": PEBBLE_URL,
+        vars = base_vars | {
             "PROJECT_ID": PROJECT_ID,
             "CERT_CA_ID": CERT_CA_ID,
             "CERT_TEMPLATE_ID": CERT_TEMPLATE_ID,
             "AUTH_TOKEN": AUTH_TOKEN,
         }
+    context._initial_vars = vars
     context.http_client = httpx.Client(base_url=BASE_URL)
 
 
+def before_scenario(context: Context, scenario: typing.Any):
+    context.vars = deepcopy(context._initial_vars)
+
+
 def after_scenario(context: Context, scenario: typing.Any):
     if hasattr(context, "web_server"):
         context.web_server.shutdown_and_server_close()

backend/bdd/features/pki/acme/access-control.feature

@@ -221,7 +221,6 @@ Feature: Access Control
       | order   | .authorizations[0].uri                    | auth_uri      | {auth_uri}                                                                          |                 |
       | order   | .authorizations[0].body.challenges[0].url | challenge_uri | {challenge_uri}                                                                     | {}              |
 
-
   Scenario Outline: URL mismatch
     Given I have an ACME cert profile as "acme_profile"
     When I have an ACME client connecting to "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/directory"
@@ -271,3 +270,52 @@ Feature: Access Control
       | order   | .authorizations[0].uri                    | auth_uri      | {auth_uri}                                                                         | https://example.com/acmes/auths/FOOBAR                                                   | URL mismatch in the protected header |
       | order   | .authorizations[0].body.challenges[0].url | challenge_uri | {challenge_uri}                                                                    | BAD                                                                                      | Invalid URL in the protected header  |
       | order   | .authorizations[0].body.challenges[0].url | challenge_uri | {challenge_uri}                                                                    | https://example.com/acmes/challenges/FOOBAR                                              | URL mismatch in the protected header |
+
+  Scenario Outline: Send KID and JWK in the same time
+    Given I have an ACME cert profile as "acme_profile"
+    When I have an ACME client connecting to "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/directory"
+    Then I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
+    And I memorize acme_account.uri with jq "capture("/(?<id>[^/]+)$") | .id" as account_id
+    When I create certificate signing request as csr
+    Then I add names to certificate signing request csr
+      """
+      {
+        "COMMON_NAME": "localhost"
+      }
+      """
+    Then I create a RSA private key pair as cert_key
+    And I sign the certificate signing request csr with private key cert_key and output it as csr_pem in PEM format
+    And I submit the certificate signing request PEM csr_pem certificate order to the ACME server as order
+    And I peak and memorize the next nonce as nonce_value
+    And I memorize <src_var> with jq "<jq>" as <dest_var>
+    When I send a raw ACME request to "<url>"
+      """
+      {
+        "protected": {
+          "alg": "RS256",
+          "nonce": "{nonce_value}",
+          "url": "<url>",
+          "kid": "{acme_account.uri}",
+          "jwk": {
+            "n": "mmEWxUv2lUYDZe_M2FXJ_WDXgHoEG7PVvg-dfz1STzyMwx0qvM66KMenXSyVA0r-_Ssb6p8VexSWGOFKskM4ryKUihn2KNH5e8nXZBqzqYeKQ8vqaCdaWzTxFI1dg0xhk0CWptkZHxpRpLalztFJ1Pq7L2qvQOM2YT7wPYbwQhpaSiVNXAb1W4FwAPyC04v1mHehvST-esaDT7j_5-eU5cCcmyi4_g5nBawcinOjj5o3VCg4X8UjK--AjhAyYHx1nRMr-7xk4x-0VIpQ_OODjLB3WzN8s1YEb0Jx5Bv1JyeCw35zahqs3fAFyRje-p5ENk9NCxfz5x9ZGkszkkNt0Q",
+            "e": "AQAB",
+            "kty": "RSA"
+          }
+        },
+        "payload": {}
+      }
+      """
+    Then the value response.status_code should be equal to 400
+    And the value response with jq ".status" should be equal to 400
+    And the value response with jq ".type" should be equal to "urn:ietf:params:acme:error:malformed"
+    And the value response with jq ".detail" should be equal to "Both JWK and KID are provided in the protected header"
+
+    Examples: Endpoints
+      | src_var | jq                                        | dest_var      | url                                                                                |
+      | order   | .                                         | not_used      | {BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/accounts/{account_id}/orders |
+      | order   | .                                         | not_used      | {BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/new-order                    |
+      | order   | .                                         | not_used      | {order.uri}                                                                        |
+      | order   | .                                         | not_used      | {order.uri}/finalize                                                               |
+      | order   | .                                         | not_used      | {order.uri}/certificate                                                            |
+      | order   | .authorizations[0].uri                    | auth_uri      | {auth_uri}                                                                         |
+      | order   | .authorizations[0].body.challenges[0].url | challenge_uri | {challenge_uri}                                                                    |

backend/bdd/features/pki/acme/account.feature

@@ -6,13 +6,32 @@ Feature: Account
     Then I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
     And the value acme_account.uri with jq "." should match pattern {BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/accounts/(.+)
 
+  Scenario: Create a new account with the same key pair twice
+    Given I have an ACME cert profile as "acme_profile"
+    When I have an ACME client connecting to "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/directory"
+    Then I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
+    And I memorize acme_account.uri as kid
+    And I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account2
+    And the value error.__class__.__name__ should be equal to "ConflictError"
+    And the value error.location should be equal to "{kid}"
+
   Scenario: Find an existing account
     Given I have an ACME cert profile as "acme_profile"
     When I have an ACME client connecting to "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/directory"
     Then I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
     And I memorize acme_account.uri as account_uri
-    And I find the existing ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
-    And the value acme_account.uri should be equal to "{account_uri}"
+    And I find the existing ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as retrieved_account
+    And the value retrieved_account.uri should be equal to "{account_uri}"
+
+  # Note: This is a very special case for cert-manager.
+  Scenario: Create a new account with EAB then retrieve it without EAB
+    Given I have an ACME cert profile as "acme_profile"
+    When I have an ACME client connecting to "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/directory"
+    Then I register a new ACME account with email fangpen@infisical.com and EAB key id "{acme_profile.eab_kid}" with secret "{acme_profile.eab_secret}" as acme_account
+    And I memorize acme_account.uri as account_uri
+    And I find the existing ACME account without EAB as retrieved_account
+    And the value error with should be absent
+    And the value retrieved_account.uri should be equal to "{account_uri}"
 
   Scenario: Create a new account without EAB
     Given I have an ACME cert profile as "acme_profile"

backend/bdd/features/pki/acme/directory.feature

@@ -9,6 +9,9 @@ Feature: Directory
       {
         "newNonce": "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/new-nonce",
         "newAccount": "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/new-account",
-        "newOrder": "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/new-order"
+        "newOrder": "{BASE_URL}/api/v1/pki/acme/profiles/{acme_profile.id}/new-order",
+        "meta": {
+          "externalAccountRequired": true
+        }
       }
       """

backend/bdd/features/steps/pki_acme.py

@@ -387,6 +387,9 @@ def register_account_with_eab(
 ):
     acme_client = context.acme_client
     account_public_key = acme_client.net.key.public_key()
+    if not only_return_existing:
+        # clear the account in case if we want to register twice
+        acme_client.net.account = None
     if hasattr(context, "alt_eab_url"):
         eab_directory = messages.Directory.from_json(
             {"newAccount": context.alt_eab_url}
@@ -406,8 +409,14 @@ def register_account_with_eab(
         only_return_existing=only_return_existing,
     )
     try:
-        context.vars[account_var] = acme_client.new_account(registration)
+        if not only_return_existing:
+            context.vars[account_var] = acme_client.new_account(registration)
+        else:
+            context.vars[account_var] = acme_client.query_registration(
+                acme_client.net.account
+            )
     except Exception as exp:
+        logger.error(f"Failed to register: {exp}", exc_info=True)
         context.vars["error"] = exp
 
 
@@ -434,6 +443,17 @@ def step_impl(context: Context, email: str, kid: str, secret: str, account_var:
     )
 
 
+@then("I find the existing ACME account without EAB as {account_var}")
+def step_impl(context: Context, account_var: str):
+    acme_client = context.acme_client
+    # registration = messages.RegistrationResource.from_json(dict(uri=""))
+    registration = acme_client.net.account
+    try:
+        context.vars[account_var] = acme_client.query_registration(registration)
+    except Exception as exp:
+        context.vars["error"] = exp
+
+
 @then("I register a new ACME account with email {email} without EAB")
 def step_impl(context: Context, email: str):
     acme_client = context.acme_client
@@ -600,6 +620,19 @@ def step_impl(context: Context, var_path: str, jq_query: str):
     )
 
 
+@then("the value {var_path} with should be absent")
+def step_impl(context: Context, var_path: str):
+    try:
+        value = eval_var(context, var_path)
+    except Exception as exp:
+        if isinstance(exp, KeyError):
+            return
+        raise
+    assert False, (
+        f"value at {var_path!r} should be absent, but we got this instead: {value!r}"
+    )
+
+
 @then('the value {var_path} with jq "{jq_query}" should be equal to {expected}')
 def step_impl(context: Context, var_path: str, jq_query: str, expected: str):
     value, result = apply_value_with_jq(

backend/src/db/migrations/20251119025017_add-unique-constraint-for-pki-acme-account-public-key-and-profile-id.ts

@@ -0,0 +1,32 @@
+import { Knex } from "knex";
+
+import { dropConstraintIfExists } from "@app/db/migrations/utils/dropConstraintIfExists";
+import { TableName } from "@app/db/schemas";
+
+const CONSTRAINT_NAME = "unique_pki_acme_account_public_key_and_profile_id";
+
+export async function up(knex: Knex): Promise<void> {
+  if (await knex.schema.hasTable(TableName.PkiAcmeAccount)) {
+    const hasProfileId = await knex.schema.hasColumn(TableName.PkiAcmeAccount, "profileId");
+    const hasPublicKeyThumbprint = await knex.schema.hasColumn(TableName.PkiAcmeAccount, "publicKeyThumbprint");
+
+    if (hasProfileId && hasPublicKeyThumbprint) {
+      await knex.schema.alterTable(TableName.PkiAcmeAccount, (table) => {
+        table.unique(["profileId", "publicKeyThumbprint"], { indexName: CONSTRAINT_NAME });
+      });
+    }
+  }
+}
+
+export async function down(knex: Knex): Promise<void> {
+  if (await knex.schema.hasTable(TableName.PkiAcmeAccount)) {
+    const hasProfileId = await knex.schema.hasColumn(TableName.PkiAcmeAccount, "profileId");
+    const hasPublicKeyThumbprint = await knex.schema.hasColumn(TableName.PkiAcmeAccount, "publicKeyThumbprint");
+
+    await knex.schema.alterTable(TableName.PkiAcmeAccount, async () => {
+      if (hasProfileId && hasPublicKeyThumbprint) {
+        await dropConstraintIfExists(TableName.PkiAcmeAccount, CONSTRAINT_NAME, knex);
+      }
+    });
+  }
+}

backend/src/ee/services/pki-acme/pki-acme-challenge-service.ts

@@ -74,7 +74,12 @@ export const pkiAcmeChallengeServiceFactory = ({
         // Notice: well, we are in a transaction, ideally we should not hold transaction and perform
         //         a long running operation for long time. But assuming we are not performing a tons of
         //         challenge validation at the same time, it should be fine.
-        const challengeResponse = await fetch(challengeUrl, { signal: AbortSignal.timeout(timeoutMs) });
+        const challengeResponse = await fetch(challengeUrl, {
+          // In case if we override the host in the development mode, still provide the original host in the header
+          // to help the upstream server to validate the request
+          headers: { Host: host },
+          signal: AbortSignal.timeout(timeoutMs)
+        });
         if (challengeResponse.status !== 200) {
           throw new AcmeIncorrectResponseError({
             message: `ACME challenge response is not 200: ${challengeResponse.status}`

backend/src/ee/services/pki-acme/pki-acme-schemas.ts

@@ -58,7 +58,15 @@ export const GetAcmeDirectoryResponseSchema = z.object({
   newNonce: z.string(),
   newAccount: z.string(),
   newOrder: z.string(),
-  revokeCert: z.string().optional()
+  revokeCert: z.string().optional(),
+  meta: z
+    .object({
+      termsOfService: z.string().optional(),
+      website: z.string().optional(),
+      caaIdentities: z.array(z.string()).optional(),
+      externalAccountRequired: z.boolean().optional()
+    })
+    .optional()
 });
 
 // New Account payload schema

backend/src/ee/services/pki-acme/pki-acme-service.ts

@@ -206,6 +206,9 @@ export const pkiAcmeServiceFactory = ({
     const { protectedHeader: rawProtectedHeader, payload: rawPayload } = result;
     try {
       const protectedHeader = ProtectedHeaderSchema.parse(rawProtectedHeader);
+      if (protectedHeader.jwk && protectedHeader.kid) {
+        throw new AcmeMalformedError({ message: "Both JWK and KID are provided in the protected header" });
+      }
       const parsedUrl = (() => {
         try {
           return new URL(protectedHeader.url);
@@ -288,6 +291,7 @@ export const pkiAcmeServiceFactory = ({
       url,
       rawJwsPayload,
       getJWK: async (protectedHeader) => {
+        // get jwk instead of kid
         if (!protectedHeader.kid) {
           throw new AcmeMalformedError({ message: "KID is required in the protected header" });
         }
@@ -353,7 +357,10 @@ export const pkiAcmeServiceFactory = ({
     return {
       newNonce: buildUrl(profile.id, "/new-nonce"),
       newAccount: buildUrl(profile.id, "/new-account"),
-      newOrder: buildUrl(profile.id, "/new-order")
+      newOrder: buildUrl(profile.id, "/new-order"),
+      meta: {
+        externalAccountRequired: true
+      }
     };
   };
 
@@ -386,11 +393,61 @@ export const pkiAcmeServiceFactory = ({
     payload: TCreateAcmeAccountPayload;
   }): Promise<TAcmeResponse<TCreateAcmeAccountResponse>> => {
     const profile = await validateAcmeProfile(profileId);
+    const publicKeyThumbprint = await calculateJwkThumbprint(jwk, "sha256");
+
+    const existingAccount: TPkiAcmeAccounts | null = await acmeAccountDAL.findByProfileIdAndPublicKeyThumbprintAndAlg(
+      profileId,
+      alg,
+      publicKeyThumbprint
+    );
+    if (onlyReturnExisting) {
+      if (!existingAccount) {
+        throw new AcmeAccountDoesNotExistError({ message: "ACME account not found" });
+      }
+      return {
+        status: 200,
+        body: {
+          status: "valid",
+          contact: existingAccount.emails,
+          orders: buildUrl(profile.id, `/accounts/${existingAccount.id}/orders`)
+        },
+        headers: {
+          Location: buildUrl(profile.id, `/accounts/${existingAccount.id}`),
+          Link: `<${buildUrl(profile.id, "/directory")}>;rel="index"`
+        }
+      };
+    }
+
+    // Note: We only check EAB for the new account request. This is a very special case for cert-manager.
+    // There's a bug in their ACME client implementation, they don't take the account KID value they have
+    // and relying on a '{"onlyReturnExisting": true}' new-account request to find out their KID value.
+    // But the problem is, that new-account request doesn't come with EAB. And while the get existing account operation
+    // fails, they just discard the error and proceed to request a new order. Since no KID provided, their ACME
+    // client will send JWK instead. As a result, we are seeing KID not provide in header error for the new-order
+    // endpoint.
+    //
+    // To solve the problem, we lose the check for EAB a bit for the onlyReturnExisting new account request.
+    // It should be fine as we've already checked EAB when they created the account.
+    // And the private key ownership indicating they are the same user.
+    // ref: https://github.com/cert-manager/cert-manager/issues/7388#issuecomment-3535630925
     if (!externalAccountBinding) {
       throw new AcmeExternalAccountRequiredError({ message: "External account binding is required" });
     }
+    if (existingAccount) {
+      return {
+        status: 200,
+        body: {
+          status: "valid",
+          contact: existingAccount.emails,
+          orders: buildUrl(profile.id, `/accounts/${existingAccount.id}/orders`)
+        },
+        headers: {
+          Location: buildUrl(profile.id, `/accounts/${existingAccount.id}`),
+          Link: `<${buildUrl(profile.id, "/directory")}>;rel="index"`
+        }
+      };
+    }
 
-    const publicKeyThumbprint = await calculateJwkThumbprint(jwk, "sha256");
     const certificateManagerKmsId = await getProjectKmsCertificateKeyId({
       projectId: profile.projectId,
       projectDAL,
@@ -441,30 +498,7 @@ export const pkiAcmeServiceFactory = ({
       });
     }
 
-    const existingAccount: TPkiAcmeAccounts | null = await acmeAccountDAL.findByProfileIdAndPublicKeyThumbprintAndAlg(
-      profileId,
-      alg,
-      publicKeyThumbprint
-    );
-    if (onlyReturnExisting && !existingAccount) {
-      throw new AcmeAccountDoesNotExistError({ message: "ACME account not found" });
-    }
-    if (existingAccount) {
-      // With the same public key, we found an existing account, just return it
-      return {
-        status: 200,
-        body: {
-          status: "valid",
-          contact: existingAccount.emails,
-          orders: buildUrl(profile.id, `/accounts/${existingAccount.id}/orders`)
-        },
-        headers: {
-          Location: buildUrl(profile.id, `/accounts/${existingAccount.id}`),
-          Link: `<${buildUrl(profile.id, "/directory")}>;rel="index"`
-        }
-      };
-    }
-
+    // TODO: handle unique constraint violation error, should be very very rare
     const newAccount = await acmeAccountDAL.create({
       profileId: profile.id,
       alg,