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.

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

feat: standalone certs with LETSENCRYPT_HOST #1128

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pini-gh
Copy link
Contributor

@pini-gh pini-gh commented Jun 10, 2024

This commit enables the creation of standalone certificates using the environment variable LETSENCRYPT_HOST without using VIRTUAL_HOST.

@buchdag would you be open to such an idea?

This commit enables the creation of standalone certificates using the
environment variable `LETSENCRYPT_HOST` without using `VIRTUAL_HOST`.
@buchdag

This comment was marked as outdated.

@pini-gh
Copy link
Contributor Author

pini-gh commented Jun 10, 2024

The intent is to create standalone certificates with LETSENCRYPT_HOST for services with no HTTP exposure (no VIRTUAL_HOST variable). For instance an SMTP server:

version: '3.5'

services:
  smtp:
    image: mysmtp:latest
    restart: unless-stopped
    environment:
      LETSENCRYPT_HOST: smtp.example.org
      LETSENCRYPT_RESTART_CONTAINER: "true"

Without this change we have to use the file letsencrypt_user_data for that because there is no nginx-proxy configuration generated for this container. But since our certificate can be tied to a container we could just use LETSENCRYPT_HOST which is easier.

@buchdag
Copy link
Member

buchdag commented Jun 10, 2024

@pini-gh while I do get the idea and the easier / more user friendly approach, I'm not at ease with the shift in project scope that change would introduce by removing the clear separation between the original scope of the project (obtaining ACME SSL certificate for containers proxied by nginx-proxy) and a secondary feature added by request (obtaining SSL certificate not tied to specific containers or containers proxied by nginx-proxy, ie standalone certificate).

@pini-gh
Copy link
Contributor Author

pini-gh commented Jun 10, 2024

I fail to see it as a shift because the feature exists already with letsencrypt_user_data. This change just makes it easier and more consistent.

For a web app you often need to glue a few containers together into a docker-compose file. Some of them with no HTTP may need certificates as well (SMTP, IMAP, LDAP, ...). When looking at the related docker-compose file it's weird that one container can have its certificate with LETENCRYPT_HOST and the other cannot just because it uses a protocol other than HTTP. Moreover you have to tinker into another docker-compose file (acme-companion's one) to obtain this certificate.

@buchdag
Copy link
Member

buchdag commented Jun 12, 2024

If we set aside the project scope discussion which is kinda debatable, I'm not opposed to this in theory as it make sense and would be a nice usability improvement.

What concern me with the concrete implementation is the fact that now every certificate we request trigger this function:

function add_standalone_configuration {
    local domain="${1:?}"
    if grep -q "server_name ${domain};" /etc/nginx/conf.d/*.conf; then
        # If the domain is already present in nginx's conf, use the location configuration.
        if parse_true "${ACME_HTTP_CHALLENGE_LOCATION:=false}"; then
            add_location_configuration "$domain"
        fi
    else
        # Else use the standalone configuration.
        cat > "/etc/nginx/conf.d/standalone-cert-$domain.conf" << EOF
server {
    server_name $domain;
    listen 80;
    access_log /var/log/nginx/access.log vhost;
    location ^~ /.well-known/acme-challenge/ {
        auth_basic off;
        auth_request off;
        allow all;
        root /usr/share/nginx/html;
        try_files \$uri =404;
        break;
    }
}
EOF
    fi
}

Doesn't that mean that everyone who runs acme-companion without sharing the /etc/nginx/conf.d folder between nginx-proxy and acme-companion (ie the normal two containers setup), would have its logs filled with the following for every domain (because in this setup /etc/nginx/conf.d does not exist inside acme-companion container) ?

grep: /etc/nginx/conf.d/*.conf: No such file or directory
bash: /etc/nginx/conf.d/standalone-cert-test.domain.tld.conf: No such file or directory

Additionally when I originally wrote this code I was okay with "if we grep the domain in the rendered nginx conf generate a location block, otherwise generate a server block" being used for standalone certificates, but I'm not confortable with the "main use case" certificate (ie certificates tied to a container proxied through nginx-proxy) triggering this. I'm concerned that we could mistakenly go into the else branch of if grep because somehow this function was called before the nginx config was fully rendered, generate a server block that we shouldn't, and break the nginx config.

I think it might be possible to identify which LETSENCRYPT_HOST are tied to containers with VIRTUAL_HOST / VIRTUAL_HOST_MULTIPORTS with a modification to letsencrypt_service_data.tmpl.

@pini-gh
Copy link
Contributor Author

pini-gh commented Jun 12, 2024

Your concerns about the implementation are valid. The current status of this PR is POC and it needs some work.

@buchdag
Copy link
Member

buchdag commented Jun 14, 2024

would you be open to such an idea?

That's a yes then, let me know if you need help to refine this.

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