-
-
Notifications
You must be signed in to change notification settings - Fork 179
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
base: master
Are you sure you want to change the base?
Conversation
78142f2
to
00405d8
Compare
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.
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 :) |
Hah, yea you are probably right there :)
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.
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? |
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.
both
example-com
anda-example-com
will contain the domain namesexample.com
,*.example.com
, anda.example.com
. With this patch it is possible to instead doCurrently 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:
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:
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.