Skip to content

Enable YAML configuration #325

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.

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

fredrikekre
Copy link
Contributor

@fredrikekre fredrikekre commented May 22, 2025

This patch allows configuring nginx-certbot with a config.yml file. In particular this allows to directly declare the certificates that should be requested by certbot with finer granularity compared to the automatic discovery based on the nginx config files.

Main motivations:

  • Currently, since automatic discovery is implemented on a per file basis, all domain names in a file are attached ot all certificates in that file. This means that for e.g.

    server {
        server_name      example.com *.example.com;
        ssl_certificate  /etc/letsencrypt/live/example-com/fullchain.pem;
        # [...]
    }
    server {
        server_name      a.example.com;
        ssl_certificate  /etc/letsencrypt/live/a-example-com/fullchain.pem;
        # [...]
    }

    both example-com and a-example-com will contain the domain names example.com, *.example.com, and a.example.com. With this patch it is possible to instead do

    certificates:
      - name: example-com
        domains: [example.com, *.example.com]
      - name: a-example-com
        domains: [a.example.com]
  • Currently the authenticator credentials can't be specified on a per certificate basis (see e.g. How to use two different cloudflare accounts #315). With this patch that is possible:

    certbot:
      authenticator: dns-cloudflare
    certificates:
      - name: example-com
        domains: [example.com]
        credentials: /etc/letsencrypt/example-com-cloudflare.ini
      - name: example-se
        domains: [example.se]
        credentials: /etc/letsencrypt/example-se-cloudflare.ini
  • Authenticator and key type can currently be specified on a per-certificate basis by naming them appropriately. This works okay, but it becomes a bit clunky to support more such per-certificate configurations (such as e.g. the elliptic curve or the authenticator credentials). This patch allows to directly specify everything for each certificate:

    certbot:
      authenticator: dns-cloudflare
      key-type: ecdsa
      certificates:
        - name: example-com-rsa
          domains: [example.com]
          key-type: rsa
        - name: example-com
          domains: [example.com]

Since there with this patch will be a YAML config file for certificates I think it make sense to also allow configuring nginx-certbot with the same file to specify e.g. DH param size and renewal interval. This is included in the example YAML file but not implemented just yet.

The automatic disabling/enabling of nginx config files with missing certificates works just like before.

Opening this as a draft because this needs some touchups and documentation.

I hope this doesn't go too far off the "spirit" of this image. I personally don't think so at least :) If you think this is something that would be merged I will add documentation and finish the PR.

@fredrikekre fredrikekre force-pushed the fe/yml branch 2 times, most recently from 78142f2 to 00405d8 Compare May 23, 2025 11:26
@fredrikekre fredrikekre marked this pull request as ready for review May 23, 2025 11:27
@fredrikekre
Copy link
Contributor Author

Enabled this also for local CA, added some docs, and fixed some bugs.

This patch allows configuring nginx-certbot with a config.yml file. In
particular this allows to directly declare the certificates that should
be requested by certbot with finer granularity compared to the automatic
discovery based on the nginx config files.

Main motivations:

 - Currently, since automatic discovery is implemented on a per file
   basis, all domain names in a file are attached ot all certificates in
   that file. This means that for e.g.
   ```nginx
   server {
       server_name      example.com *.example.com;
       ssl_certificate  /etc/letsencrypt/live/example-com/fullchain.pem;
       # [...]
   }
   server {
       server_name      a.example.com;
       ssl_certificate  /etc/letsencrypt/live/a-example-com/fullchain.pem;
       # [...]
   }
   ```
   both `example-com` and `a-example-com` will contain the domain names
   `example.com`, `*.example.com`, and `a.example.com`.
   With this patch it is possible to instead do
   ```yaml
   certificates:
     - name: example-com
       domains: [example.com, *.example.com]
     - name: a-example-com
       domains: [a.example.com]
   ```

 - Currently the authenticator credentials can't be specified on a per
   certificate basis (see e.g. JonasAlfredsson#315). With this patch that is possible:
   ```yaml
   certbot:
     authenticator: dns-cloudflare
   certificates:
     - name: example-com
       domains: [example.com]
       credentials: /etc/letsencrypt/example-com-cloudflare.ini
     - name: example-se
       domains: [example.se]
       credentials: /etc/letsencrypt/example-se-cloudflare.ini
   ```

 - Authenticator and key type can currently be specified on a
   per-certificate basis by naming them appropriately. This works okay,
   but it becomes a bit clunky to support more such per-certificate
   configurations (such as e.g. the elliptic curve or the authenticator
   credentials). This patch allows to directly specify everything for
   each certificate:
   ```yaml
   certbot:
     authenticator: dns-cloudflare
     key-type: ecdsa
   certificates:
     - name: example-com-rsa
       domains: [example.com]
       key-type: rsa
     - name: example-com
       domains: [example.com]
   ```

The file examples/config.yml is documented with all the various options
that are enabled.
@JonasAlfredsson
Copy link
Owner

Hi fredrikekre,

Once again you provide quality code with a lot of comments and documentation going along with it, gold star!

However, I am going to ask you to just hold up a second before continuing and have a conversation with me before spending more time on this. I am not saying this feature won't get merged, but we should probably evaluate if Bash is the correct language for this :)

I am going to rewind the tape a bit to when I started coding this project, and I was looking for a much simpler "get certificates and reload Nginx" solution than what the other options at that time offered (in my opinion).

What I knew I didn't want was the need to mount the Docker socket into a container, or need an entire web application/GUI to do click-ops to configure the service. I also found it impractical to have to create duplicate configs for Certbot since basically all the information needed was in the Nginx config, and not needing to keep two configurations in sync is one less source of problems.

At that point I found staticfloat's image, which was just a very simple bash script which ran locally in the container and did what I wanted, except that it couldn't create dhparam files. However, the script was so simple at that time so it was very easy to just expand it a little bit to look for another line in the Nginx config files. And when I was at it, it I thought it should be able to handle more than one server name. It took a little bit of work, but it was still simple.

Then there came ideas like that it should be possible to specify DNS authenticator, other types of encryption, and let you define the same certificate file in multiple Nginx configuration files. IMO this started to push Bash as the language of choice to the limit, and while it all works there are definitely stuff written that would be soooo much simpler in another language like Python (which is already installed in the container because of Certbot).

For that reason I have been toying with the idea of rewriting the current code in Python instead, to have more control on variables and perform string manipulation in a much easier way. As it is right now I feel like the Bash scripts are already complex enough that it is difficult to understand how it all works, and adding a custom YAML parser on top of that does not help things ^^ (the YAML stuff would also be simpler in Python).

I do see benefits of exposing an advanced YAML config for controlling each specific certificate, but it does introduce this "double config" I tried to avoid in the beginning. But at this point in time this project has also grown to such a size that there are people requesting control of the certificates to such a specific detail that it is hard to do it in another way than a more structured, and separate, configuration file.

While I have not yet made up my mind on the best way forward, a solution might be to start some kind of "docker-nginx-certbot-2" repo and starting fresh in Python with this feature being included from the start instead of having to force it into the already cluttered Bash code.

I am looking forward to hearing your thought regarding this, since by this point you are probably as familiar with this codebase as I am :)

@fredrikekre
Copy link
Contributor Author

Hah, yea you are probably right there :)

I do see benefits of exposing an advanced YAML config for controlling each specific certificate, but it does introduce this "double config" I tried to avoid in the beginning. But at this point in time this project has also grown to such a size that there are people requesting control of the certificates to such a specific detail that it is hard to do it in another way than a more structured, and separate, configuration file.

Yea, already during testing this I have been annoyed myself with keeping the yaml file and the nginx files in sync. It is indeed very elegant to keep everything in a single file. Perhaps some kind of YAML frontmatter within the nginx files would be easier, for example.

While I have not yet made up my mind on the best way forward, a solution might be to start some kind of "docker-nginx-certbot-2" repo and starting fresh in Python with this feature being included from the start instead of having to force it into the already cluttered Bash code.

Sure, that might be easier. I don't mind Bash, but, as you say, Python is already available so probably easier to use that instead. Would you consider eventually merging such a Python rework as version 6 of this image once it has been tested a bit in a separate repository/fork?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants