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 description:
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);
             });
         });