Fixes for multiple hostnames (#11)

zsteinkamp · web-flow · commit 274755f50c2a · 2023-07-05T10:02:44.000-07:00
* Fixes for supporting SAN certs with multiple names

* typescript fixes

* Revert PROJECT_NAME in Makefile; Some small cleanups in Makefile, package.json, and examples/nginx.conf

* remove errant $domain var

* add nginxinc/ prefix to image name
diff --git a/Makefile b/Makefile
@@ -4,9 +4,9 @@ GREP               ?= $(shell command -v ggrep 2> /dev/null || command -v grep 2
 AWK                ?= $(shell command -v gawk 2> /dev/null || command -v awk 2> /dev/null)
 DOCKER             ?= docker
 PROJECT_NAME       ?= njs-acme
+DOCKER_IMAGE_NAME  ?= nginxinc/nginx-$(PROJECT_NAME)
 GITHUB_REPOSITORY  ?= nginxinc/$(PROJECT_NAME)
 SRC_REPO           := https://github.com/$(GITHUB_REPOSITORY)
-CURRENT_DIR 		= $(shell pwd)
 
 Q = $(if $(filter 1,$V),,@)
 M = $(shell printf "\033[34;1m▶\033[0m")
@@ -25,36 +25,35 @@ build: ## Run npm run build
 
 .PHONY: docker-build
 docker-build: ## Build docker image
-	$(DOCKER) buildx build $(DOCKER_BUILD_FLAGS) -t $(PROJECT_NAME) .
+	$(DOCKER) buildx build $(DOCKER_BUILD_FLAGS) -t $(DOCKER_IMAGE_NAME) .
 
 
 .PHONY: docker-copy
-docker-copy: CONTAINER_ID=$(shell $(DOCKER) create $(PROJECT_NAME))
+docker-copy: CONTAINER_NAME=njs_acme_dist_source
 docker-copy: docker-build ## Copy the acme.js file out of the container and save in dist/
-	echo ${CONTAINER_ID}
-	$(DOCKER) cp ${CONTAINER_ID}:/usr/lib/nginx/njs_modules/acme.js dist/acme.js
-	$(DOCKER) rm -v ${CONTAINER_ID}
+	mkdir -p dist
+	$(DOCKER) create --name $(CONTAINER_NAME) $(DOCKER_IMAGE_NAME)
+	$(DOCKER) cp $(CONTAINER_NAME):/usr/lib/nginx/njs_modules/acme.js dist/acme.js
+	$(DOCKER) rm -v $(CONTAINER_NAME)
 
 
 .PHONY: docker-nginx
 docker-nginx: docker-build ## Start nginx container
-	$(DOCKER) run --rm -it -p 8000:8000 \
-		-e "NJS_ACME_DIR=/etc/nginx/njs-acme" \
-		$(PROJECT_NAME)
+	$(DOCKER) run --rm -it -p 8000:8000 -p \
+		$(DOCKER_IMAGE_NAME)
 
 
 .PHONY: docker-njs
 docker-njs: docker-build ## Start nginx container and run `njs`
-	$(DOCKER) run --rm -it -p 8000:8000 \
-		-e "NJS_ACME_DIR=/etc/nginx/njs-acme" \
-		$(PROJECT_NAME) njs
+	$(DOCKER) run --rm -it \
+		$(DOCKER_IMAGE_NAME) njs
 
 
 .PHONY: docker-devup
-docker-devup: docker-build ## Start all docker compose services
+docker-devup: docker-build ## Start all docker compose services for development/testing
 	$(DOCKER) compose up -d
 
 
 .PHONY: docker-reload-nginx
-docker-reload-nginx: ## Reload nginx
+docker-reload-nginx: ## Reload nginx started from `docker compose`
 	$(DOCKER) compose up -d --force-recreate nginx && $(DOCKER) compose logs -f nginx
diff --git a/examples/nginx.conf b/examples/nginx.conf
@@ -14,21 +14,15 @@ http {
   js_fetch_trusted_certificate /etc/ssl/certs/ISRG_Root_X1.pem;
 
   js_import acme from acme.js;
-  # js_preload_object acme_account from acme_account_info.json;
-  # Define variable to hold private key value
-  # variable $dynamic_ssl_key;
-  # Define variable to hold ceritificate value
-  # variable $dynamic_ssl_cert;
 
-  # resolver 1.1.1.1 1.0.0.1 [2606:4700:4700::1111] [2606:4700:4700::1001] valid=300s; # Cloudflare
+  # One `resolver` directive must be defined.
   resolver 127.0.0.11 ipv6=off; # docker-compose
+  # resolver 1.1.1.1 1.0.0.1 [2606:4700:4700::1111] [2606:4700:4700::1001] valid=300s; # Cloudflare
+  # resolver 8.8.8.8 8.8.4.4; # Google
+  # resolver 172.16.0.23; # AWS EC2 Classic
+  # resolver 169.254.169.253; # AWS VPC
   resolver_timeout 5s;
 
-  map $ssl_server_name $domain {
-    default $ssl_server_name;
-    ~(([^\.]+)\.([^\.]+))$ $1;
-  }
-
   server {
 
     listen 0.0.0.0:8000; # testing with 8000 should be 80 in prod, pebble usees httpPort in integration-tests/pebble/config.json
@@ -50,13 +44,11 @@ http {
     js_set $dynamic_ssl_cert acme.js_cert;
     js_set $dynamic_ssl_key acme.js_key;
 
-    # /etc/letsencrypt/live/$domain/fullchain.pem;
-    # data:$variable can be specified instead of the file (
     ssl_certificate $dynamic_ssl_cert;
     ssl_certificate_key $dynamic_ssl_key;
 
     location = / {
-      return 200 "hello server_name:$server_name\nssl_server_name:$ssl_server_name\nssl_session_id:$ssl_session_id\n";
+      return 200 "hello server_name:$server_name\nssl_session_id:$ssl_session_id\n";
     }
 
     location ^~ /.well-known/acme-challenge/ {
@@ -76,17 +68,4 @@ http {
     location = /acme/new-acct {
       js_content acme.acmeNewAccount;
     }
-
   }
-
-  # server {
-  #   listen 4443 ssl http2;
-  #   listen [::]:4443 ssl http2;
-
-  #   server_name example.com www.example.com;
-  #   ssl_certificate /etc/letsencrypt/live/www.domain.com/fullchain.pem;
-  #   ssl_certificate_key /etc/letsencrypt/live/www.domain.com/privkey.pem;
-  #   ssl_session_cache shared:SSL:10m;
-  #   ssl_session_timeout 10m;
-  # }
-}
diff --git a/package.json b/package.json
@@ -1,5 +1,5 @@
 {
-  "name": "njs-acme-experemental",
+  "name": "njs-acme",
   "version": "1.0.0",
   "description": "## How do I use this template?",
   "main": "dist/acme.js",
@@ -9,15 +9,15 @@
   ],
   "repository": {
     "type": "git",
-    "url": "git+https://github.com/nginxinc/njs-acme-experemental.git"
+    "url": "git+https://github.com/nginxinc/njs-acme.git"
   },
   "keywords": [],
   "author": "",
   "license": "APACHE-2.0",
   "bugs": {
-    "url": "https://github.com/nginxinc/njs-acme-experemental/issues"
+    "url": "https://github.com/nginxinc/njs-acme/issues"
   },
-  "homepage": "https://github.com/nginxinc/njs-acme-experemental#readme",
+  "homepage": "https://github.com/nginxinc/njs-acme#readme",
   "engines": {
     "node": ">= 14.15"
   },
diff --git a/src/client.ts b/src/client.ts
@@ -1,5 +1,11 @@
 import { HttpClient } from './api'
-import { formatResponseError, getPemBodyAsB64u, retry } from './utils'
+import {
+  formatResponseError,
+  getPemBodyAsB64u,
+  readCsrDomainNames,
+  retry,
+  toPEM,
+} from './utils'
 import OGCrypto from 'crypto'
 
 export interface ClientExternalAccountBindingOptions {
@@ -795,7 +801,6 @@ export class AcmeClient {
     //}
 
     /* Return default certificate chain */
-    // FIXME: is it json() or text()
     return await resp.text()
   }
 
@@ -941,19 +946,45 @@ async function auto(
   /**
    * Parse domains from CSR
    */
-  // FIXME implement parsing CSR to get a list of domain...
   ngx.log(
     ngx.INFO,
     'njs-acme: [auto] Parsing domains from Certificate Signing Request'
   )
-  // const csrDomains = readCsrDomains(opts.csr);
-  // const domains = [csrDomains.commonName].concat(csrDomains.altNames);
-  // const uniqueDomains = Array.from(new Set(domains));
 
-  const uniqueDomains = ['proxy.nginx.com']
+  if (opts.csr === null) {
+    throw new Error('csr is required')
+  }
+
+  const csrDomains = readCsrDomainNames(toPEM(opts.csr, 'CERTIFICATE REQUEST'))
+
+  // Work around issue in x509.get_oid_value (called from readCsrDomainNames)
+  // where altNames comes back as an Array in an Array, e.g.:
+  //   [[ 'hostname1', 'hostname2' ]]
+  // We just want an Array:
+  //   [ 'hostname1', 'hostname2' ]
+  const uniqueDomains = [csrDomains.commonName]
+  // Hacky stuff to make Typescript happy
+  const origAltNames = csrDomains.altNames as unknown as string[][]
+  let altNames = csrDomains.altNames
+  if (altNames && altNames[0] && altNames[0][0]) {
+    altNames = origAltNames[0]
+  }
+
+  if (altNames) {
+    for (const altName of altNames) {
+      if (uniqueDomains.indexOf(altName) === -1) {
+        uniqueDomains.push(altName)
+      }
+    }
+  }
+
   ngx.log(
     ngx.INFO,
-    `njs-acme: [auto] Resolved ${uniqueDomains.length} unique domains from parsing the Certificate Signing Request`
+    `njs-acme: [auto] Resolved ${
+      uniqueDomains.length
+    } unique domains (${uniqueDomains.join(
+      ', '
+    )}) from parsing the Certificate Signing Request`
   )
 
   /**
diff --git a/src/index.ts b/src/index.ts
@@ -112,11 +112,8 @@ async function clientAutoMode(r: NginxHTTPRequest): Promise<void> {
 
     // Create a new CSR
     const params = {
-      altNames: [commonName],
+      altNames: serverNames.length > 1 ? serverNames.slice(1) : [],
       commonName: commonName,
-      // state: "WA",
-      // country: "US",
-      // organizationUnit: "NGINX",
       emailAddress: email,
     }
 
@@ -277,9 +274,10 @@ async function createCsrHandler(r: NginxHTTPRequest): Promise<void> {
  */
 function js_cert(r: NginxHTTPRequest): string {
   const prefix = acmeDir(r)
+  const serverNames = acmeServerNames(r)
   const { path, data } = read_cert_or_key(
     prefix,
-    r.variables.ssl_server_name?.toLowerCase() || '',
+    serverNames[0].toLowerCase(), // filename is the commonName (first serverName)
     CERTIFICATE_SUFFIX
   )
   // ngx.log(ngx.INFO, `njs-acme: Loaded cert for ${r.variables.ssl_server_name} from path: ${path}`);
@@ -304,9 +302,10 @@ function js_cert(r: NginxHTTPRequest): string {
  */
 function js_key(r: NginxHTTPRequest): string {
   const prefix = acmeDir(r)
+  const serverNames = acmeServerNames(r)
   const { path } = read_cert_or_key(
     prefix,
-    r.variables.ssl_server_name?.toLowerCase() || '',
+    serverNames[0].toLowerCase(), // filename is the commonName (first serverName)
     KEY_SUFFIX
   )
   // r.log(`njs-acme: loaded key for ${r.variables.ssl_server_name} from path: ${path}`);
diff --git a/src/utils.ts b/src/utils.ts
@@ -158,7 +158,7 @@ export type PemTag =
  * @returns The converted PEM string
  */
 export function toPEM(
-  buffer: ArrayBufferView | ArrayBuffer,
+  buffer: string | Buffer | ArrayBufferView | ArrayBuffer,
   tag: PemTag
 ): string {
   /**
@@ -749,6 +749,7 @@ export function readCsrDomainNames(csrPem: string | Buffer): {
     csrPem = csrPem.toString()
   }
   const csr = x509.parse_pem_cert(csrPem)
+
   return {
     commonName: x509.get_oid_value(csr, '2.5.4.3'),
     altNames: x509.get_oid_value(csr, '2.5.29.17'),
