Repository: amplify-education/serverless-domain-manager (ts)

Issue:
Check if cert is in date before using it
<!--- Please keep this note for the community --->

### Community Note

* Please vote on this issue by adding a 👍 [reaction](https://blog.github.com/2016-03-10-add-reactions-to-pull-requests-issues-and-comments/) to the original issue to help the community and maintainers prioritize this request
* Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
* If you are interested in working on this issue or have submitted a pull request, please leave a comment

<!--- Thank you for keeping this note for the community --->

## Bug Report

**Error Description**
The situation:
- AWS ACM has multiple certs matching the same domain, e.g: `*.foo.bar`.
- The first cert that is returned from [the `listCertificates` call](https://github.com/amplify-education/serverless-domain-manager/blob/680661a/src/index.ts#L371) is expired, but still has `"Status": "ISSUED",` because it was valid but just hasn't been renewed.
- The cert to use is specified with `certificateName` (not by ARN):
    ```yaml
      customDomains:
        - http:
            domainName: 'blah.foo.bar'
            certificateName:  '*.foo.bar'
    ```


**Command Run**
`sls create_domain`

**Console Output**
```sh
  Error: Unable to create domain blah.foo.bar
```
...which comes [from here](https://github.com/amplify-education/serverless-domain-manager/blob/680661a/src/index.ts#L205).

**Domain Manager Configuration**
(see fragment above)

**Versions**
- Domain Manager version(s): `5.1.0`
- Node/npm version: Node `v14.17.0`/npm `6.14.13`
- Serverless Version: Framework Core: 2.44.0
- Lambda Code: Javascript

**Possible Solution**
The current code [calls `listCertificates`](https://github.com/amplify-education/serverless-domain-manager/blob/680661a/src/index.ts#L371) which only returns the ARN and cert name, no other details about the cert. Where the [cert is found](https://github.com/amplify-education/serverless-domain-manager/blob/680661a/src/index.ts#L385) we could add a call to the `describeCertificate` ACM API to get all the details for the cert and compare `Date.now()` to the values in the response:
```
        "NotBefore": 1610323200.0,
        "NotAfter": 1644451199.0,
```
...to confirm the cert is *in date*. If it's not, keep searching (and repeat the date check) because there can be multiple certs with the same `DomainName`.

The response from the underlying AWS API call to create the domain isn't very helpful
> `An error occurred (BadRequestException) when calling the CreateDomainName operation: The provided certificate does not exist.`

...so even if it was shown to the user, it wouldn't help. The proposed solution is better because the plugin will just work, even when expired certs are present.

Pull request:
Fix for gh-450: check AWS certificate expiry date before trying to use it
<!--- Information about referencing Github Issues: https://help.github.com/articles/basic-writing-and-formatting-syntax/#referencing-issues-and-pull-requests --->
Fixes #450 

**Description of Issue Fixed**
Previously the code didn't allow for
* multiple certificates with the same name
* expired certs

**Changes proposed in this pull request**:

Enumerate all certificates that match the specified name and find the first on that is in-date.

<!--- Please remember to allow edits from maintainers: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork --->


Patch:
diff --git a/.gitignore b/.gitignore
index 68e21df..a64d70e 100644
--- a/.gitignore
+++ b/.gitignore
@@ -8,3 +8,4 @@ npm-debug.log
 **/.serverless
 dist/
 .nyc_output/
+*.tgz
diff --git a/package.json b/package.json
index 625c1cd..c02868a 100644
--- a/package.json
+++ b/package.json
@@ -30,6 +30,7 @@
     "integration-basic": "nyc mocha -r ts-node/register --project tsconfig.json --parallel test/integration-tests/basic.test.ts",
     "integration-deploy": "nyc mocha -r ts-node/register --project tsconfig.json --parallel test/integration-tests/deploy.test.ts",
     "test": "nyc mocha -r ts-node/register --project tsconfig.json test/unit-tests/index.test.ts && nyc report --reporter=text-summary",
+    "test:debug": "NODE_OPTIONS='--inspect-brk' mocha -j 1 -r ts-node/register --project tsconfig.json test/unit-tests/index.test.ts",
     "integration-test": "npm run integration-basic && npm run integration-deploy && nyc report --reporter=text-summary",
     "lint": "tslint --project . && tslint --project tsconfig.json",
     "build": "tsc --project ."
diff --git a/src/aws/acm-wrapper.ts b/src/aws/acm-wrapper.ts
index 9789c27..aa98738 100644
--- a/src/aws/acm-wrapper.ts
+++ b/src/aws/acm-wrapper.ts
@@ -1,6 +1,6 @@
 import {ACM} from "aws-sdk";
 import Globals from "../globals";
-import {getAWSPagedResults} from "../utils";
+import {getAWSPagedResults, throttledCall} from "../utils";
 import DomainConfig = require("../domain-config");
 
 const certStatuses = ["PENDING_VALIDATION", "ISSUED", "INACTIVE"];
@@ -43,10 +43,26 @@ class ACMWrapper {
             let nameLength = 0;
             // Checks if a certificate name is given
             if (certificateName != null) {
-                const foundCertificate = certificates
-                    .find((certificate) => (certificate.DomainName === certificateName));
-                if (foundCertificate != null) {
-                    certificateArn = foundCertificate.CertificateArn;
+                // note: we only check DomainName, but a future enhancement could
+                // be to also check SubjectAlternativeNames
+                const matches = certificates
+                    .filter((certificate) => (certificate.DomainName === certificateName));
+                for (const curr of matches) {
+                  const currArn = curr.CertificateArn;
+                  const details = await throttledCall(
+                      this.acm,
+                      "describeCertificate",
+                      {CertificateArn: currArn},
+                  );
+                  const currNotAfter = details.Certificate.NotAfter;
+                  if (Date.now() < currNotAfter) {
+                    Globals.logInfo(`Selecting cert with ARN=${
+                      currArn} with future expiry (${currNotAfter.toISOString()})`);
+                    certificateArn = currArn;
+                    break;
+                  }
+                  Globals.logInfo(`Ignoring cert with ARN=${
+                    currArn} that is expired (${currNotAfter.toISOString()})`);
                 }
             } else {
                 certificateName = domain.givenDomainName;
@@ -66,10 +82,10 @@ class ACMWrapper {
                 });
             }
         } catch (err) {
-            throw Error(`Could not list certificates in Certificate Manager.\n${err.message}`);
+            throw Error(`Could not search certificates in Certificate Manager.\n${err.message}`);
         }
         if (certificateArn == null) {
-            throw Error(`Could not find the certificate '${certificateName}'.`);
+            throw Error(`Could not find an in-date certificate for '${certificateName}'.`);
         }
         return certificateArn;
     }


Test patch:
diff --git a/test/unit-tests/index.test.ts b/test/unit-tests/index.test.ts
index 325c909..4383d20 100644
--- a/test/unit-tests/index.test.ts
+++ b/test/unit-tests/index.test.ts
@@ -20,6 +20,10 @@ const certTestData = {
             CertificateArn: "test_arn",
             DomainName: "test_domain",
         },
+        {
+            CertificateArn: "expired_cert_name",
+            DomainName: "cert_name",
+        },
         {
             CertificateArn: "test_given_cert_name",
             DomainName: "cert_name",
@@ -584,6 +588,30 @@ describe("Custom Domain Plugin", () => {
 
         it("Get a given certificate name", async () => {
             AWS.mock("ACM", "listCertificates", certTestData);
+            AWS.mock("ACM", "describeCertificate", (params, callback) => {
+              const fifteenDaysAsMs = 15 * 24 * 60 * 60 * 1000;
+              const fifteenDaysInFuture = new Date(Date.now() + fifteenDaysAsMs);
+              const fifteenDaysInPast = new Date(Date.now() - fifteenDaysAsMs);
+                if (params.CertificateArn === "expired_cert_name") {
+                  callback(null, {
+                    Certificate: {
+                      CertificateArn: "doesnt_matter_wont_be_used",
+                      NotAfter: fifteenDaysInPast,
+                    }
+                  });
+                  return;
+                }
+                if (params.CertificateArn === "test_given_cert_name") {
+                  callback(null, {
+                    Certificate: {
+                      CertificateArn: params.CertificateArn,
+                      NotAfter: fifteenDaysInFuture,
+                    }
+                  });
+                  return;
+                }
+                throw new Error("Programmer error: didn't add new test mock data");
+            });
 
             const plugin = constructPlugin({certificateName: "cert_name"});
             plugin.initializeVariables();
@@ -1544,7 +1572,7 @@ describe("Custom Domain Plugin", () => {
             return acm.getCertArn(domain).then(() => {
                 throw new Error("Test has failed. getCertArn did not catch errors.");
             }).catch((err) => {
-                const expectedErrorMessage = "Could not find the certificate 'does_not_exist'.";
+                const expectedErrorMessage = "Could not find an in-date certificate for 'does_not_exist'.";
                 expect(err.message).to.equal(expectedErrorMessage);
             });
         });