diff --git a/backend/bdd/features/environment.py b/backend/bdd/features/environment.py index 9a2e9f90b..976998c72 100644 --- a/backend/bdd/features/environment.py +++ b/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() diff --git a/backend/bdd/features/pki/acme/access-control.feature b/backend/bdd/features/pki/acme/access-control.feature index 6615d00f8..50588be76 100644 --- a/backend/bdd/features/pki/acme/access-control.feature +++ b/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" 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 with jq "" as + When I send a raw ACME request to "" + """ + { + "protected": { + "alg": "RS256", + "nonce": "{nonce_value}", + "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} | diff --git a/backend/bdd/features/pki/acme/account.feature b/backend/bdd/features/pki/acme/account.feature index 589c5ab24..14e304c6c 100644 --- a/backend/bdd/features/pki/acme/account.feature +++ b/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" diff --git a/backend/bdd/features/pki/acme/dicrectory.feature b/backend/bdd/features/pki/acme/directory.feature similarity index 86% rename from backend/bdd/features/pki/acme/dicrectory.feature rename to backend/bdd/features/pki/acme/directory.feature index 664ff7457..53084a681 100644 --- a/backend/bdd/features/pki/acme/dicrectory.feature +++ b/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 + } } """ diff --git a/backend/bdd/features/steps/pki_acme.py b/backend/bdd/features/steps/pki_acme.py index 5043b0e7d..353ec942d 100644 --- a/backend/bdd/features/steps/pki_acme.py +++ b/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( diff --git a/backend/src/db/migrations/20251119025017_add-unique-constraint-for-pki-acme-account-public-key-and-profile-id.ts b/backend/src/db/migrations/20251119025017_add-unique-constraint-for-pki-acme-account-public-key-and-profile-id.ts new file mode 100644 index 000000000..5bc4601e3 --- /dev/null +++ b/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 { + 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 { + 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); + } + }); + } +} diff --git a/backend/src/ee/services/pki-acme/pki-acme-challenge-service.ts b/backend/src/ee/services/pki-acme/pki-acme-challenge-service.ts index 9148b0336..7a3747fed 100644 --- a/backend/src/ee/services/pki-acme/pki-acme-challenge-service.ts +++ b/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}` diff --git a/backend/src/ee/services/pki-acme/pki-acme-schemas.ts b/backend/src/ee/services/pki-acme/pki-acme-schemas.ts index 58ca7e833..23b86d172 100644 --- a/backend/src/ee/services/pki-acme/pki-acme-schemas.ts +++ b/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 diff --git a/backend/src/ee/services/pki-acme/pki-acme-service.ts b/backend/src/ee/services/pki-acme/pki-acme-service.ts index 43da08b1c..4f560ade7 100644 --- a/backend/src/ee/services/pki-acme/pki-acme-service.ts +++ b/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> => { 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,