From a668822e192495c4163ab0701d18b55d5566a5fb Mon Sep 17 00:00:00 2001 From: Fang-Pen Lin Date: Tue, 18 Nov 2025 09:28:10 -0800 Subject: [PATCH] Only check EAB when creating a new account --- backend/bdd/features/pki/acme/account.feature | 9 --- .../ee/services/pki-acme/pki-acme-service.ts | 63 +++++++++++-------- 2 files changed, 38 insertions(+), 34 deletions(-) diff --git a/backend/bdd/features/pki/acme/account.feature b/backend/bdd/features/pki/acme/account.feature index 04b1f29c6a..5214464a3c 100644 --- a/backend/bdd/features/pki/acme/account.feature +++ b/backend/bdd/features/pki/acme/account.feature @@ -15,15 +15,6 @@ Feature: Account And the value retrieved_account.uri should be equal to "{account_uri}" # Note: 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 - # ref: https://github.com/cert-manager/cert-manager/issues/7388#issuecomment-3535630925 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" 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 ccacf816e8..d9c28b4986 100644 --- a/backend/src/ee/services/pki-acme/pki-acme-service.ts +++ b/backend/src/ee/services/pki-acme/pki-acme-service.ts @@ -389,11 +389,48 @@ export const pkiAcmeServiceFactory = ({ payload: TCreateAcmeAccountPayload; }): Promise> => { const profile = await validateAcmeProfile(profileId); + const publicKeyThumbprint = await calculateJwkThumbprint(jwk, "sha256"); + + if (onlyReturnExisting) { + const existingAccount: TPkiAcmeAccounts | null = await acmeAccountDAL.findByProfileIdAndPublicKeyThumbprintAndAlg( + profileId, + alg, + publicKeyThumbprint + ); + if (!existingAccount) { + throw new AcmeAccountDoesNotExistError({ message: "ACME account not found" }); + } + // 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"` + } + }; + } + + // 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" }); } - const publicKeyThumbprint = await calculateJwkThumbprint(jwk, "sha256"); const certificateManagerKmsId = await getProjectKmsCertificateKeyId({ projectId: profile.projectId, projectDAL, @@ -444,30 +481,6 @@ 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"` - } - }; - } - const newAccount = await acmeAccountDAL.create({ profileId: profile.id, alg,