Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Search SubjectAlternativeNames when finding cert #529

Merged
merged 1 commit into from
Feb 3, 2023
Merged

Search SubjectAlternativeNames when finding cert #529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 29 additions & 36 deletions src/aws/acm-wrapper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,10 @@ class ACMWrapper {
"NextToken",
{CertificateStatuses: certStatuses},
);

// enhancement idea: weight the choice of cert so longer expiries
// and RenewalEligibility = ELIGIBLE is more preferable
if (certificateName != null) {
certificateArn = await this.getCertArnByCertName(certificates, certificateName);
certificateArn = this.getCertArnByCertName(certificates, certificateName);
} else {
certificateName = domain.givenDomainName;
certificateArn = this.getCertArnByDomainName(certificates, certificateName);
Expand All @@ -52,50 +53,42 @@ class ACMWrapper {
/**
* * Gets Certificate ARN that most closely matches Cert ARN and not expired
*/
private async getCertArnByCertName(certificates, certName): Promise<string> {
// note: we only check DomainName, but a future enhancement could
// be to also check SubjectAlternativeNames
const matches = certificates.filter((certificate) => (certificate.DomainName === certName));
for (const certificate of matches) {
const certificateArn = certificate.CertificateArn;
const details = await throttledCall(
this.acm,
"describeCertificate",
{CertificateArn: certificateArn},
);
const currNotAfter = details.Certificate.NotAfter;
if (Date.now() < currNotAfter) {
Globals.logInfo(
`Selecting cert with ARN=${certificateArn} with future expiry (${currNotAfter.toISOString()})`
);
return certificateArn;
}
Globals.logInfo(
`Ignoring cert with ARN=${certificateArn} that is expired (${currNotAfter.toISOString()})`
);
private getCertArnByCertName(certificates, certName): string {
const found = certificates.find((c) => c.DomainName === certName);
if (found) {
return found.CertificateArn;
}
return null;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the code around checking for expired certs because we don't need it with the cert status filter on line 6. Then I refactored what was left of the code.

I completely removed the call to describeCertificate because I realised all the details are already available from the summary list, so the extra API call is redundant.

}

/**
* * Gets Certificate ARN that most closely matches domain name
*/
private getCertArnByDomainName(certificates, domainName): Promise<string> {
private getCertArnByDomainName(certificates, domainName): string {
// The more specific name will be the longest
let nameLength = 0;
let certificateArn;
certificates.forEach((certificate) => {
let certificateListName = certificate.DomainName;
// Looks for wild card and takes it out when checking
if (certificateListName[0] === "*") {
certificateListName = certificateListName.substring(1);
}
// Looks to see if the name in the list is within the given domain
// Also checks if the name is more specific than previous ones
if (domainName.includes(certificateListName) && certificateListName.length > nameLength) {
nameLength = certificateListName.length;
certificateArn = certificate.CertificateArn;
for (const currCert of certificates) {
const allDomainsForCert = [
currCert.DomainName,
...(currCert.SubjectAlternativeNameSummaries || []),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is where we include alt names in the search.

];
for (const currCertDomain of allDomainsForCert) {
let certificateListName = currCertDomain;
// Looks for wild card and take it out when checking
if (certificateListName[0] === "*") {
certificateListName = certificateListName.substring(1);
}
// Looks to see if the name in the list is within the given domain
// Also checks if the name is more specific than previous ones
if (domainName.includes(certificateListName)
&& certificateListName.length > nameLength
) {
nameLength = certificateListName.length;
certificateArn = currCert.CertificateArn;
}
}
});
}
return certificateArn;
}
}
Expand Down
113 changes: 85 additions & 28 deletions test/unit-tests/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ const certTestData = {
CertificateArn: "test_arn",
DomainName: "test_domain",
},
{
CertificateArn: "expired_cert_name",
DomainName: "cert_name",
},
{
CertificateArn: "test_given_cert_name",
DomainName: "cert_name",
Expand Down Expand Up @@ -709,30 +705,6 @@ 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");
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't need this because it was related to the separate API call to check expiries.


const plugin = constructPlugin({certificateName: "cert_name"});
plugin.initializeVariables();
Expand All @@ -744,6 +716,91 @@ describe("Custom Domain Plugin", () => {
expect(result).to.equal("test_given_cert_name");
});

it("Get a given certificate by alt name with exact match", async () => {
AWS.mock("ACM", "listCertificates", {
CertificateSummaryList: [
{
CertificateArn: "test_nomatch",
DomainName: "dontmatch.com",
},
{
CertificateArn: "test_arn",
DomainName: "test.com",
SubjectAlternativeNameSummaries: [
"example.com",
],
},
],
});

const options = {
domainName: "example.com",
endpointType: "REGIONAL",
};
const plugin = constructPlugin(options);
plugin.initializeVariables();

const dc: DomainConfig = new DomainConfig(plugin.serverless.service.custom.customDomain);
const acm = new ACMWrapper(dc.endpointType);
const result = await acm.getCertArn(dc);

expect(result).to.equal("test_arn");
});

it("Get a given certificate by alt name with subdomain", async () => {
AWS.mock("ACM", "listCertificates", {
CertificateSummaryList: [
{
CertificateArn: "test_arn",
DomainName: "test.com",
SubjectAlternativeNameSummaries: [
"example.com",
],
},
],
});

const options = {
domainName: "sub.example.com",
endpointType: "REGIONAL",
};
const plugin = constructPlugin(options);
plugin.initializeVariables();

const dc: DomainConfig = new DomainConfig(plugin.serverless.service.custom.customDomain);
const acm = new ACMWrapper(dc.endpointType);
const result = await acm.getCertArn(dc);

expect(result).to.equal("test_arn");
});

it("Get a given certificate by alt name with wildcard", async () => {
AWS.mock("ACM", "listCertificates", {
CertificateSummaryList: [
{
CertificateArn: "test_arn",
DomainName: "test.com",
SubjectAlternativeNameSummaries: [
"*.example.com",
],
},
],
});

const options = {
domainName: "sub.example.com",
endpointType: "REGIONAL",
};
const plugin = constructPlugin(options);
plugin.initializeVariables();

const dc: DomainConfig = new DomainConfig(plugin.serverless.service.custom.customDomain);
const acm = new ACMWrapper(dc.endpointType);
const result = await acm.getCertArn(dc);

expect(result).to.equal("test_arn");
});

it("Create a domain name", async () => {
AWS.mock("APIGateway", "createDomainName", (params, callback) => {
callback(null, {distributionDomainName: "foo", securityPolicy: "TLS_1_2"});
Expand Down