Checking certificate validity every 90 seconds is overkill. Every hour should be sufficient. However, simply adjusting the recommended time interval would have the negative side effect of unacceptably delaying the initial call to /acme/auto to provision the certificate.

Let's investigate if we can call /acme/auto after nginx has started successfully (perhaps via a script), and then reduce the frequency of polling that URL to something like an hour (again, making use of the script so that we don't have stuff all over).

Currently, if /acme/auto is requested and the certificate is present and more than 30 days away from expiring, then no action is taken.

This is a problem if the operator changes the hostname configuration (NJS_ACME_SERVER_NAMES variable) while the certificate is still in its validity period because the new hostname(s) in the configuration will not be supported by the existing certificate.

This request is to validate that the hostnames in the stored certificate match the hostnames in the njs_acme configuration. If they don't match, then we should kick off a new cert request.

Use Mocha to build tests for methods in the utils.ts file.

I followed exactly like the guide, but here is the result, https://mytest2.nilu.website/
what is wrong? I'm helpless

Is your feature request related to a problem? Please describe

njs-acme currently only supports the HTTP-01 challenge type, which requires that the host be accessible from the Internet on port 80. If you are operating a cluster of NGINX hosts, then you will need to use a shared storage layer for challenge tokens. While this will work just fine for some customers, for others it will not.

The DNS-01 challenge type is the only type that is out-of-band from the web server. It requires that a person or script add a TXT record to a domain for which a certificate is being requested for.

Describe the solution you'd like

Either manual or automated DNS record setup.

Describe alternatives you've considered

Problem

Log entries generated by this code are not easily discernible from other log entries. Using a log prefix requires the developer to include the prefix in each log event entry.

Proposed Solution

A utility method/class that wraps logging and adds a consistent log entry prefix.

From @ryepup:

nit/future: might be worth a tiny leveled logging wrapper to ensure we always have the acme-njs: [$MODULE] prefix and control verbosity.

import {Logger} from './logger'
const log = new Logger("utils")
...
log.info(`Generated a new account key and saved it to ${path}`)

Is your feature request related to a problem? Please describe

No.

Describe the solution you'd like

We are the authors of PKI.js and ASN.1js and were looking at projects that took a dependency on our libraries so I started looking at consumers like njs-acme.

We noticed that you import PKI.js but implement your own x509 parser. We created a higher-level library than PKI.js called peculiar/x509.

Your code would be simpler if you consumed our x509 library instead of PKI.js, it would also allow you to have more uniform code for your x509 parsing use cases.

Describe alternatives you've considered

Suggestion

Additional context

N/A

Is your feature request related to a problem? Please describe

Currently createCsr doesn't allow parameterized generation of Private/Public Key pair, where Public Key is used in CSR extensions. the interface looks like this:

export async function createCsr(params: {
  keySize?: number
  commonName: string
  altNames: string[]
  country?: string
  state?: string
  locality?: string
  organization?: string
  organizationUnit?: string
  emailAddress?: string
}): Promise<{ pkcs10Ber: ArrayBuffer; keys: Required<CryptoKeyPair> }> {
  // TODO:  allow to provide keys in addition to always generating one
  const { privateKey, publicKey } =
    (await generateKey()) as Required<CryptoKeyPair>
....
  addSubjectAttributes(pkcs10.subject.typesAndValues, params)
  await addExtensions(pkcs10, params, publicKey)
  await signCsr(pkcs10, privateKey)

So we need to provide a way to allow using existing Key pair and/or allow parameterized algo generation of the pair.

This would allow to generate keys with EC for example, as RSA is hard coded for now as the following:

export async function generateKey(): Promise<CryptoKey | CryptoKeyPair> {
  const keys = await crypto.subtle.generateKey({
  name: 'RSASSA-PKCS1-v1_5',
  hash: 'SHA-256',
  publicExponent: new Uint8Array([1, 0, 1]),
  modulusLength: 2048,
}, true, [
    'sign',
    'verify',
  ])
  return keys
}

Additional context

Currently people can't use their own public/public keys (e.g. password protected).

Describe the bug

NJS version 0.8.0 added native implementation for Array.from:
https://mailman.nginx.org/pipermail/nginx-announce/2023/A3O7JLCSI6DYGIYQA4XCXLJEUW2EGIHS.html

     *) Feature: added Array.from(), Array.prototype.toSorted(),
         Array.prototype.toSpliced(), Array.prototype.toReversed().

Now we can remove:
https://github.com/nginxinc/njs-acme/blob/274755f50c2ae86a6fed4fd17b359e6f30d02faa/src/arrayFrom.js#L1

Your environment

• Version/release of this project or specific commit

• Target deployment platform

Additional context

Add any other context about the problem here.

Is your feature request related to a problem? Please describe

In order to simplify usage and report potential issues for the njs-acme project we need to publish versions and releases periodically when have changes worth releasing (new features, bug-fix, incompatible changes, etc.).

Describe the solution you'd like

We should follow the semantic version (reminder: <breaking change>.<backward-compatible>.<minor-or-bugfix>) and make releases

Follow simple steps:

• For a set of changes, we use tags and create a GH release where a "compiled" version of njs-acme as single a JS file can be found.
• Document which NJS version is tested and compatible against NGINX NJS in the readme.
• Maintain a changelog file
• Optionally, create a docker image with a semantic version tag of njs-acme release and publish (docker hub or GH registry TBD later)
• Optionally, publish to NPM

          > allow to use of specified nginx bin path when running integration tests as nginx-testing still has no support for NJS v0.8.0

This is already fixed (nginx-binaries provides nginx 1.24.x and 1.25.x with njs 0.8.0), so you can revert these unnecessary Dockerfails. ;)
Next time please open an issue; I would have resolved it sooner if I knew it was being used in this project.

Originally posted by @jirutka in #22 (comment)

add FOSSA GitHub actions so we can check reports

Describe the bug

When a certificate is renewed, the cached entries are not invalidated, so the old certificate is still used. And, in the case where a domain name is added to the server block, that old certificate is also used for the new domain name.

To reproduce

Steps to reproduce the behavior:

1. Have a certificate issued and cached
2. Add a second domain name to the server block (and $njs_acme_server_names)
3. Reload (not restart) nginx
4. Invoke the auto renew endpoint
5. Visit the newly added domain
6. The certificate used does not contain the new domain name

Expected behavior

The newly issued certificate should be used immediately for both domains.

Your environment

• I'm using this commit 9306982

NJS 0.8.0 introduced a new functionality called shared dict:
https://mailman.nginx.org/pipermail/nginx-announce/2023/A3O7JLCSI6DYGIYQA4XCXLJEUW2EGIHS.html

This would allow us to store certs in memory and avoid reading cert/key from disk on each SSL handshake using js_set like we do now in the example config:

    js_set $dynamic_ssl_cert acme.js_cert;
    js_set $dynamic_ssl_key acme.js_key;

We can persist cert/key on disk and store it in memory to improve performance:

Details about shared dict:

- shared dictionaries:
A shared dictionary keeps the key-value pairs shared between worker
processes. This allows to cache data in memory and share it between
workers.

: example.conf:
:   # Creates a 1Mb dictionary with string values,
:   # removes key-value pairs after 60 seconds of inactivity:
:   js_shared_dict_zone zone=foo:1M timeout=60s;
:
:   # Creates a 512Kb dictionary with string values,
:   # forcibly removes oldest key-value pairs when the zone is overflowed:
:   js_shared_dict_zone zone=bar:512K timeout=30s evict;
:
:   # Creates a 32Kb permanent dictionary with numeric values:
:   js_shared_dict_zone zone=num:32k type=number;
:
: example.js:
:    function get(r) {
:        r.return(200, ngx.shared.foo.get(r.args.key));
:    }
:
:    function set(r) {
:        r.return(200, ngx.shared.foo.set(r.args.key, r.args.value));
:    }
:
:    function delete(r) {
:        r.return(200, ngx.shared.bar.delete(r.args.key));
:    }
:
:    function increment(r) {
:        r.return(200, ngx.shared.num.incr(r.args.key, 2));
:    }

Expected behavior

The proposed change in acme.js_cert and acme.js_key:

• if shared_dict has no cert/key read from disk for the first time and stored in memory for the next time
• when issuing a new cert/renew cert/key pair we would persist on disk and update the shared dict with a new value

Your environment

min required version NJS 0.8.0

Additional context

We currently read files (cert and key) from disk on each SSL handshake in the reference implementation. In NginxPlus is already possible to use the KV feature to store cert/key in memory as of today. Shared Dict is a sunset of KV functionality for the Nginx OSS version.

Describe the bug

When renewing a cert, we generate a new key + CSR at the beginning of the process. The new .key immediately replaces the existing .key file. If there is a problem in communication with the ACME provider, then the system is left in a state with a new .key and and old .crt, which does not work. If the server is restarted in this state, then HTTPS does not work at all.

To reproduce

Steps to reproduce the behavior:

1. Add an invalid hostname (i.e. does not resolve) to the njs_acme_server_names variable.
2. Restart/reload nginx
3. Observe njs-acme trying to request a new cert and receiving a failure notif from the ACME provider.
4. Observe that the .crt file is old, but the .key file is new, and that nginx no longer properly handles https requests due to the mismatch. (ls -l /etc/nginx/njs-acme/)

To fix the problem, the cert, key, and csr files need to be removed. njs-acme will then re-request the certs and all is well. For example, in the Docker container that this project builds, this needs to be run:

docker compose exec nginx -- rm /etc/nginx/njs-acme/*.{crt,key,csr}

Expected behavior

A failure in renewing a cert should emit an error in the logs, but not disturb the existing .crt/.key files and shared dict entries (if applicable).

Your environment

Running in docker compose up in the njs-acme repo.

Is your feature request related to a problem? Please describe

No.

Describe the solution you'd like

We are the authors of PKI.js and ASN.1js and were looking at projects that took a dependency on our libraries so I started looking at consumers like njs-acme.

We noticed that you import ASN.js but implement your own ASN.1 reader.

Though we didn't see any issues in your ASN.1 reader we also didn't look very closely. In general, parsers are a pain to get right and since you take the dependency and use it in some places you could make a relatively simple change to reduce to complexity of your code by using our reader in all places vs maintaining two implementations as a dependency.

Describe alternatives you've considered

This is just a suggestion.

Additional context

N/A

At the moment example config doesn't have any mentioning that users should limit access to /acme/auto to some IPs OR CIDRS (or use other means). With the current setup, everybody on the internet could trigger a renewal. We should just mention something like this somewhere in the README if possible.

Originally posted by @tippexs in #12 (comment)

Problem Statement

The configuration around $njs_acme_challenge_dir is delicate and inconsistent with configuring other aspects of njs-acme. It must be an NGINX config var and cannot be an environment variable, since it's used in both the JS code and as the value for the root directive in the relevant location {...} block:

set $njs_acme_challenge_dir /etc/acme/challenge;
...
location ^~ /.well-known/acme-challenge/ {
  default_type "text/plain";
  root $njs_acme_challenge_dir;
}

Describe the solution you'd like

By changing this location block to use js_content, we can hide the complexity of this path from the end-user and eliminate a possible footgun.

location ^~ /.well-known/acme-challenge/ {
  js_content acme.challengeResponse;
}

Is your feature request related to a problem? Please describe

As variables set using js_var are global, it cannot be used to set njs_acme_server_names when multiple server blocks are used. Only one certificate would be generated, and on all but one server block the wrong certificate will be selected by acme.js_cert and acme.js_key.

What we need is a good way to handle multiple server definitions.

Describe the solution you'd like

The documentation and maybe examples need to be updated to address this problem. I don't know enough of nginx internals to know what the best solution would be.

Something I can think of is this:
Use $ssl_server_name for serving certificates, and $host for the auto mode. Like this:

server {
    listen 8000;
    server_name _default;

    set $njs_acme_server_names $host;
    js_var $njs_acme_account_email '[email protected]';

   # locations etc
}

server {
    listen 8443 ssl;
    server_name proxy.nginx.com;

    js_var $njs_acme_server_names $ssl_server_name;
    js_var $njs_acme_account_email '[email protected]';

   # locations etc
} 

That would mean though that js_periodic cannot be used.

This is still a suboptimal solution though, as it means approaches cannot be mixed in a single nginx install. It would fall apart when support for wildcard certificates is added.

So, maybe what's really needed is an alternative way of passing variables to njs functions. Set cannot be used, because the TLS stuff comes before set is handled.

Describe the bug

A clear and concise description of what the bug is.

• GitHub Actions are failing.
• add integration tests:
  • issue a new cert with the Pebble container
  • test certificate loading

From @ivanitskiy:

here is an idea for you. let's have a callback on the client so users/developers can build their own validators (say they want to have allow list).
from: https://github.com/auto-ssl/lua-resty-auto-ssl

auto_ssl:set("allow_domain", function(domain, auto_ssl, ssl_options, renewal)
  return ngx.re.match(domain, "^(example.com|example.net)$", "ijo")
end)

we can provide a default callback but allow people to make their own validations.

Problem Statement

For users of njs-acme that want to develop their own flows, importing the utility functions requires less-than-ideal import syntax.

Proposed Solution

Re-export the library and client utility methods in index.ts.

From @ivanitskiy:

let's re-export all utils, and api/client so somebody can use them to implement their own request handler.

Describe the bug

Trying to build using Docker 20.10.25 on Ubuntu 20.04 fails.

To reproduce

$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 20.04.6 LTS
Release:        20.04
Codename:       focal

$ docker version
Client:
 Version:           20.10.25
 API version:       1.41
 Go version:        go1.18.1
 Git commit:        20.10.25-0ubuntu1~20.04.1
 Built:             Fri Jul 14 22:00:45 2023
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server:
 Engine:
  Version:          20.10.25
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.18.1
  Git commit:       20.10.25-0ubuntu1~20.04.1
  Built:            Thu Jun 29 21:55:06 2023
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.7.2
  GitCommit:        
 runc:
  Version:          1.1.7-0ubuntu1~20.04.1
  GitCommit:        
 docker-init:
  Version:          0.19.0
  GitCommit:        

$ make docker-build
docker buildx build  -t nginx/nginx-njs-acme .
unknown shorthand flag: 't' in -t
See 'docker --help'.

Usage:  docker [OPTIONS] COMMAND

A self-sufficient runtime for containers

Options:
      --config string      Location of client config files (default "/home/ubuntu/.docker")
  -c, --context string     Name of the context to use to connect to the daemon (overrides DOCKER_HOST
                           env var and default context set with "docker context use")
  -D, --debug              Enable debug mode
  -H, --host list          Daemon socket(s) to connect to
  -l, --log-level string   Set the logging level ("debug"|"info"|"warn"|"error"|"fatal") (default "info")
      --tls                Use TLS; implied by --tlsverify
      --tlscacert string   Trust certs signed only by this CA (default "/home/ubuntu/.docker/ca.pem")
      --tlscert string     Path to TLS certificate file (default "/home/ubuntu/.docker/cert.pem")
      --tlskey string      Path to TLS key file (default "/home/ubuntu/.docker/key.pem")
      --tlsverify          Use TLS and verify the remote
  -v, --version            Print version information and quit

Management Commands:
  builder     Manage builds
  config      Manage Docker configs
  container   Manage containers
  context     Manage contexts
  image       Manage images
  manifest    Manage Docker image manifests and manifest lists
  network     Manage networks
  node        Manage Swarm nodes
  plugin      Manage plugins
  secret      Manage Docker secrets
  service     Manage services
  stack       Manage Docker stacks
  swarm       Manage Swarm
  system      Manage Docker
  trust       Manage trust on Docker images
  volume      Manage volumes

Commands:
  attach      Attach local standard input, output, and error streams to a running container
  build       Build an image from a Dockerfile
  commit      Create a new image from a container's changes
  cp          Copy files/folders between a container and the local filesystem
  create      Create a new container
  diff        Inspect changes to files or directories on a container's filesystem
  events      Get real time events from the server
  exec        Run a command in a running container
  export      Export a container's filesystem as a tar archive
  history     Show the history of an image
  images      List images
  import      Import the contents from a tarball to create a filesystem image
  info        Display system-wide information
  inspect     Return low-level information on Docker objects
  kill        Kill one or more running containers
  load        Load an image from a tar archive or STDIN
  login       Log in to a Docker registry
  logout      Log out from a Docker registry
  logs        Fetch the logs of a container
  pause       Pause all processes within one or more containers
  port        List port mappings or a specific mapping for the container
  ps          List containers
  pull        Pull an image or a repository from a registry
  push        Push an image or a repository to a registry
  rename      Rename a container
  restart     Restart one or more containers
  rm          Remove one or more containers
  rmi         Remove one or more images
  run         Run a command in a new container
  save        Save one or more images to a tar archive (streamed to STDOUT by default)
  search      Search the Docker Hub for images
  start       Start one or more stopped containers
  stats       Display a live stream of container(s) resource usage statistics
  stop        Stop one or more running containers
  tag         Create a tag TARGET_IMAGE that refers to SOURCE_IMAGE
  top         Display the running processes of a container
  unpause     Unpause all processes within one or more containers
  update      Update configuration of one or more containers
  version     Show the Docker version information
  wait        Block until one or more containers stop, then print their exit codes

Run 'docker COMMAND --help' for more information on a command.

To get more help with docker, check out our guides at https://docs.docker.com/go/guides/

make: *** [Makefile:28: docker-build] Error 125

Expected behavior

The README should detail supported docker versions and/or this bug should be fixed

Your environment

$ docker version
Client:
 Version:           20.10.25
 API version:       1.41
 Go version:        go1.18.1
 Git commit:        20.10.25-0ubuntu1~20.04.1
 Built:             Fri Jul 14 22:00:45 2023
 OS/Arch:           linux/amd64
 Context:           default
 Experimental:      true

Server:
 Engine:
  Version:          20.10.25
  API version:      1.41 (minimum version 1.12)
  Go version:       go1.18.1
  Git commit:       20.10.25-0ubuntu1~20.04.1
  Built:            Thu Jun 29 21:55:06 2023
  OS/Arch:          linux/amd64
  Experimental:     false
 containerd:
  Version:          1.7.2
  GitCommit:        
 runc:
  Version:          1.1.7-0ubuntu1~20.04.1
  GitCommit:        
 docker-init:
  Version:          0.19.0
  GitCommit:        

Currently, we will split the value of NJS_ACME_SERVER_NAMES on space/comma and blindly use the result to request certs.

Since the HTTP-01 challenge type only supports full hostnames and not wildcard names, we should:

• document clearly that wildcards are not allowed
• explicitly test the values we get from that variable for validity as hostname characters (excluding *)
• display a clear and helpful error message in logs and in an early response to the /acme/auto endpoint if invalid values are detected.

Is your feature request related to a problem? Please describe

No, it is not related to a problem

Describe the solution you'd like

One of the features that Nginx supports is the use of a OpenSSL engine
which enables you to (turtles all-the-way-down) configure the use of a PKCS#11 library.

This may be possible today, but if it is I have not figured it out yet, it would be ideal to put both the ACME account key and the TLS server key on a PKCS#11 implementation such as SoftHSM, TPM2P11, or a HSM product.

Many organizations, including banks and governments, will require that the TLS key is in a hardware device since this is supported when not using njs-acme it would be nice if this capability was preserved.

Describe alternatives you've considered

The only alternative I can think of, unless I am missing this how to do this, is to use a different ACME client.

Additional context

N/A

Is your feature request related to a problem? Please describe

When running nginx in a cluster with shared storage for njs-acme, you need to be able to invalidate the cache across the cluster. Otherwise only a single node will use the updated certificate.

Describe the solution you'd like

A function exported from acme.js that clears the cache. Can be either based on the request, or the whole cache. This would make it possible for the node that renews the certificate to call an enpoint at the other nodes to clear the cache.

To be able to do this, it would probably be cleanest to export a function that returns structured information (like clientAutoModeInternal), or add the ability for registering a hook that is called on certificate renewal. One can than implement calling the other nodes in some custom njs code.

Is your feature request related to a problem? Please describe

ACME supports tls-alpn chalnages.

Describe the solution you'd like

Here is a potential solution:

use $ssl_preread_alpn_protocols to detect ALPN protocol and proxy traffic to the appropriate server.

stream {
  # set tls_port vari base on ALPN protocol
  map $ssl_preread_alpn_protocols $tls_port {
    ~\bacme-tls/1\b 9443;
    ~\bh2\b 10443;
    ~\bhttp/1.1\b 10443;
    default 11443;
  }

  # listen on :443 port and then proxy to the appropriate server based on ALPN protocol
  server {
    listen :443;
    ssl_preread on;
    proxy_pass 127.0.0.1:$tls_port;
  }

Then we can use js_set to read challenges from the FS similarly to how we currently read HTTP-01 chanallnages by doing this:

location ~ "^/\.well-known/acme-challenge/[-_A-Za-z0-9]{22,128}$" {
      js_content acme.challengeResponse;
    }

Here is an approximate nginx config:

stream {
  map $ssl_preread_alpn_protocols $tls_port {
    ~\bacme-tls/1\b 9443;
    ~\bh2\b 10443;
    ~\bhttp/1.1\b 10443;
    default 11443;
  }

  server {
    listen localhost:8443;
    ssl_preread on;
    proxy_pass 127.0.0.1:$tls_port;
  }

  server {
    js_set $challenge_crt acme.js_ch_cert;
    js_set $challenge_key acme.js_ch_key;

    listen localhost:9443 ssl;
    ssl_certificate     data:$challenge_crt;
    ssl_certificate_key data:$challenge_key;
    ssl_protocols TLSv1.2 TLSv1.3;

    return "this is a challenge server\n";
  }

  server {
    js_set $production_crt acme.js_ch_cert;
    js_set $production_key acme.js_ch_key;

    listen localhost:11443 ssl;
    ssl_certificate     data:$production_crt;
    ssl_certificate_key data:$production_key;
    ssl_protocols TLSv1.2 TLSv1.3;

    return "this is a production stream server\n";
  }
}



http {
  server {
  js_set $dynamic_ssl_cert acme.js_cert;
  js_set $dynamic_ssl_key acme.js_key;

    listen localhost:10443 ssl;
    ssl_certificate     data:$dynamic_ssl_cert;
    ssl_certificate_key data:$dynamic_ssl_key;
    ssl_protocols TLSv1.2 TLSv1.3;

    location / {
      return 200 "production server response\n";
    }
  }
}

we need to consider adding a new function similar to clientAutoMode or updating it so both tls-alpm and HTTP challenges are supported. this requires some experiments and dining in.