Issue with DNS challenge in auto mode

[ackava]

Since DNS challenge contains two tokens, and callback is called one after another, propagating challenges to NS server such as AWS Route 53 fails as two consecutive DNS updates fails some times as they are asynchronous in nature within AWS DNS Cluster (Sometimes only one challenge is successfully saved in DNS). I am using one library in C# for different project which gives both tokens together and I can update AWS DNS in a single update and there has never been any issue.

The issue also persists with remove challenge, one of the challenge remains in AWS DNS.

    cert = await client.auto({
           csr,
           email: maintainerEmail,
           termsOfServiceAgreed: true,
           skipChallengeVerification: false,
           challengePriority: wildcard ? ["dns-01"] :  ["http-01"],
           async challengeCreateFn(authz, challenge, keyAuthorization) {
                if (challenge.type !== "http-01") {
                     await dns.saveChallenge(commonName, keyAuthorization);
                     return;
                 }
                 httpServer.challenges.set(challenge.token, keyAuthorization);
             },
             challengeRemoveFn(authz, challenge, keyAuthorization) {
                 httpServer.challenges.delete(challenge.token);
                 return Promise.resolve();
             },
         });

Is it possible to change callback to receive all tokens in an array? Something like following?

There will be challangesCreateFn and challengesRemoveFn (Plural), and call methods accordingly?

    cert = await client.auto({
           csr,
           email: maintainerEmail,
           termsOfServiceAgreed: true,
           skipChallengeVerification: false,
           challengePriority: wildcard ? ["dns-01"] :  ["http-01"],
           async challengesCreateFn(challenges) {
           },
           async challengesRemoveFn(challenges) {
           },
       });

[bchr02]

I am having a similar issue (intermittently) using the following code. Both challenges seem to get added and deleted but sometimes I get Error processing ACME client: Error: Incorrect TXT record.

// index.js
import cron from "node-cron";
import { renewCert } from "./modules/renewCert.js";
import { uploadFileToS3 } from "./modules/uploadFileToS3.js";
import fs from "fs/promises";

async function checkFileReady(filePath) {
  try {
    await fs.access(filePath);
    return true; // File exists
  } catch {
    return false; // File does not exist
  }
}

async function attemptUpload(filePath, attempts = 0) {
  const fileReady = await checkFileReady(filePath);
  if (fileReady) {
    console.log("File is ready for upload.");
    await uploadFileToS3("thedomain-certs", filePath);
    console.log("File upload successful.");
  } else if (attempts < 5) {
    console.log(`File not ready for upload, retrying... (${attempts + 1}/5)`);
    setTimeout(() => attemptUpload(filePath, attempts + 1), 5000);
  } else {
    console.error("Failed to upload file after 5 attempts.");
  }
}

async function performTasks(forced = false) {
  console.log("Running scheduled certificate renewal and upload.");
  const certRenewed = await renewCert(forced);
  if (certRenewed) {
    const filePath = "thedomain.pfx";
    attemptUpload(filePath); // Call the function to handle the upload and retries
  }
}

// Execute immediately on start
performTasks(true);

// Schedule to execute every 7 days
cron.schedule("0 0 */7 * *", () => performTasks(), {
  scheduled: true,
  timezone: "America/New_York",
});

console.log("Scheduler activated. Task will run at 00:00 every 7 days.");

// /modules/renewCert.js
import acme from "acme-client";
import { addTxtRecord, deleteTxtRecord } from "./dns_management.js";
import createPfx from "./createPFX.js";
import { getFileAgeInDays } from "./getFileAgeInDays.js";

/**
 * Renews the certificate for thedomain.com domain.
 * 
 * @param {boolean} [forced=false] - Indicates whether the certificate renewal should be forced, regardless of the file age.
 * @returns {Promise<boolean>} - A promise that resolves to `true` if the certificate renewal is successful, or `false` otherwise.
 */
export const renewCert = async (forced = false) => {
  const age = await getFileAgeInDays("s3bucketname-certs", "thedomain.pfx");

  console.log(`File age: ${age} days.`);

  // When not forced and there is either an error because the file doesn't exist or the S3 is not accessible, or the file is not older than 30 days then return
  if (!forced && (age === -1 || age <= 30)) return;

  try {
    /* Init client */
    const client = new acme.Client({
      directoryUrl: process.env.NODE_ENV === "production" ? acme.directory.letsencrypt.production : acme.directory.letsencrypt.staging,
      accountKey: await acme.crypto.createPrivateKey(),
    });

    /* Create CSR */
    const [key, csr] = await acme.crypto.createCsr({
      commonName: "*.thedomain.com",
      altNames: ["*.thedomain.com", "thedomain.com"],
      organizationName: "thedomain, LLC",
      organizationalUnit: "thedomain",
    });

    /* Certificate */
    const cert = await client.auto({
      csr,
      email: "bill@thedomain.com",
      termsOfServiceAgreed: true,
      challengePriority: ["dns-01"],
      challengeCreateFn: async (authz, challenge, keyAuthorization) => {
        console.log(`Creating DNS record for ACME challenge: ${authz.identifier.value}`);
        await addTxtRecord({
          node: `_acme-challenge`,
          challenge: keyAuthorization,
        });
      },
      challengeRemoveFn: async (authz) => {
        console.log(`Removing DNS record for ACME challenge: ${authz.identifier.value}`);
        const deleteCount = await deleteTxtRecord({
          node: `_acme-challenge.${authz.identifier.value}`,
        });
        if (deleteCount > 0) {
          console.log(`${deleteCount} TXT record(s) successfully deleted.`);
        } else {
          console.log("No TXT records found for deletion.");
        }
      },
    });

    await createPfx(key, cert, process.env.CERT_PASSWORD, "thedomain - thedomain");
    console.log("PFX file saved successfully.");
    return true;
  } catch (error) {
    console.error("Error processing ACME client:", error);
    return false;
  }
};

Running scheduled certificate renewal and upload.
[cert-manager] [2024-05-10 14:26:42] Scheduler activated. Task will run at 00:00 every 7 days.
[cert-manager] [2024-05-10 14:26:42] File age: 1 days.
[cert-manager] [2024-05-10 14:26:44] Creating DNS record for ACME challenge: thedomain.com
[cert-manager] [2024-05-10 14:26:44] Creating DNS record for ACME challenge: thedomain.com
[cert-manager] [2024-05-10 14:26:44] TXT record successfully added.
[cert-manager] [2024-05-10 14:26:45] TXT record successfully added.
[cert-manager] [2024-05-10 14:26:45] Removing DNS record for ACME challenge: thedomain.com
[cert-manager] [2024-05-10 14:26:46] 2 TXT record(s) successfully deleted.
[cert-manager] [2024-05-10 14:26:50] Removing DNS record for ACME challenge: thedomain.com
[cert-manager] [2024-05-10 14:26:50] No TXT records found for deletion.
[cert-manager] [2024-05-10 14:26:50] Error processing ACME client: Error: Incorrect TXT record "2T2JvDms7I

[bchr02]

I think I might have figured out the issue. When calling challengeRemoveFn I would delete all DNS TXT records so this would remove both TXT records. I revised the function that challengeRemoveFn calls so that it passes the keyAuthorization value and only deletes the TXT record with the matching record. This so far seems to work reliably.

We should likely upadate the https://github.com/publishlab/node-acme-client/blob/master/examples/auto.js so the following line indicates to pass the recordValue

like so

// await dnsProvider.removeRecord(dnsRecord, 'TXT', recordValue);

[nmorsman]

Yep you are right @bchr02 - nuking all TXT records within _acme-challenge.domain.tld from challengeRemoveFn() will likely sabotage wildcard certificate issuance since it requires two TXT records, and the other challenge may still be pending when the records are removed. The correct way would be to only remove the single record matching keyAuthorization, as you have discovered. Thanks for the PR, making this more clear from the example.

@ackava Does this clear up the issue you were having as well? If not, please feel free to reopen.

[ackava]

@nmorsman No this does not fix the issue with AWS Route 53, because AWS does not allow saving multiple values for single ResourceRecordSet in different REST calls.

In order to set two values for same RRSet, both must be saved together.

await saveChallenge("zone", [ "c1"] );
await saveChallenge("zone", [ "c2"] );

// two separate save calls will only save last value ["c2"]

// correct way would be to save together
await saveChallenge("zone", [ "c1", "c2"] );

async saveChallenge(zone: string, value: string[]) {

        if (zone.startsWith("*.")) {
            zone = zone.substring(2);
        }

        const Name = `${zone}.le.${apexDomain}`;
        const Type = "TXT";
        const ResourceRecords = Value.map((x) => ({
            Value: `"${x}"`
        }));

        const c = new ChangeResourceRecordSetsCommand({
            HostedZoneId,
            ChangeBatch: {
                Comment: `Adding challenge ${zone}`,
                Changes: [
                    {
                        Action: "UPSERT",
                        ResourceRecordSet: {
                            Name,
                            Type,
                            TTL: 60,
                            ResourceRecords
                        }
                    }
                ]
            }
        });

        await client.send(c);
}