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

Redondant config #1091

Open
azlux opened this issue Jan 19, 2024 · 5 comments
Open

Redondant config #1091

azlux opened this issue Jan 19, 2024 · 5 comments

Comments

@azlux
Copy link

azlux commented Jan 19, 2024

Hi,
I was looking for my vhost.d file and observe the /.well-known/acme-challenge/ part write on top of my config part.
I think the code making that is here :

else
# Else write the location configuration to a temp file ...
echo "$START_HEADER" > "${VHOST_DIR}/${domain}".new
cat /app/nginx_location.conf >> "${VHOST_DIR}/${domain}".new
echo "$END_HEADER" >> "${VHOST_DIR}/${domain}".new
# ... append the existing file content to the temp one ...
[[ -f "${VHOST_DIR}/${domain}" ]] && cat "${VHOST_DIR}/${domain}" >> "${VHOST_DIR}/${domain}".new
# ... and copy the temp file to the old one (if the destination file is bind mounted, you can't change
# its inode from within the container, so mv won't work and cp has to be used), then remove the temp file.
cp -f "${VHOST_DIR}/${domain}".new "${VHOST_DIR}/${domain}" && rm -f "${VHOST_DIR}/${domain}".new
return 1

But the nginx-proxy already generate this config : https://github.com/nginx-proxy/nginx-proxy/blob/main/nginx.tmpl#L605-L611

So I think this configuration is on double (you can see it with a nginx -T).
Do have I make the good observation or did I miss something ?

Best regards,
Azlux

@Montana
Copy link

Montana commented Feb 10, 2024

What are the actual results of nginx -T?

@azlux
Copy link
Author

azlux commented Feb 13, 2024

Something like that :

# cloud.azlux.fr/
upstream cloud.azlux.fr {
    # Container: nextcloud
    #     networks:
    #         infra_backend (reachable)
    #         infra_backend_av (unreachable)
    #         infra_database_postgres (unreachable)
    #         infra_database_redis (unreachable)
    #         infra_mail (unreachable)
    #     IP address: 172.20.0.16
    #     exposed ports: 80/tcp
    #     default port: 80
    #     using port: 80
    server 172.20.0.16:80;
}
server {
    server_name cloud.azlux.fr;
    access_log /var/log/nginx/access.log vhost;
    listen 80 ;
    listen [::]:80 ;
    # Do not HTTPS redirect Let's Encrypt ACME challenge
    location ^~ /.well-known/acme-challenge/ {
        auth_basic off;
        auth_request off;
        allow all;
        root /usr/share/nginx/html;
        try_files $uri =404;
        break;
    }
    location / {
        return 301 https://$host$request_uri;
    }
}
server {
    server_name cloud.azlux.fr;
    access_log /var/log/nginx/access.log vhost;
    http2 on;
    listen 443 ssl ;
    listen [::]:443 ssl ;
    ssl_session_timeout 5m;
    ssl_session_cache shared:SSL:50m;
    ssl_session_tickets off;
    ssl_certificate /etc/nginx/certs/cloud.azlux.fr.crt;
    ssl_certificate_key /etc/nginx/certs/cloud.azlux.fr.key;
    ssl_dhparam /etc/nginx/certs/cloud.azlux.fr.dhparam.pem;
    ssl_stapling on;
    ssl_stapling_verify on;
    ssl_trusted_certificate /etc/nginx/certs/cloud.azlux.fr.chain.pem;
    set $sts_header "";
    if ($https) {
        set $sts_header "max-age=31536000";
    }
    add_header Strict-Transport-Security $sts_header always;
    include /etc/nginx/vhost.d/cloud.azlux.fr;
    location / {
        proxy_pass http://cloud.azlux.fr;
        set $upstream_keepalive false;
    }
}

[...]
# configuration file /etc/nginx/vhost.d/default:
## Start of configuration add by letsencrypt container
location ^~ /.well-known/acme-challenge/ {
    auth_basic off;
    auth_request off;
    allow all;
    root /usr/share/nginx/html;
    try_files $uri =404;
    break;
}
## End of configuration add by letsencrypt container

# configuration file /etc/nginx/vhost.d/cloud.azlux.fr:
## Start of configuration add by letsencrypt container
location ^~ /.well-known/acme-challenge/ {
    auth_basic off;
    auth_request off;
    allow all;
    root /usr/share/nginx/html;
    try_files $uri =404;
    break;
}
## End of configuration add by letsencrypt container
client_max_body_size 1024m;

@buchdag
Copy link
Member

buchdag commented Feb 13, 2024

@azlux the issue here is we have no reliable way (or more accurately I haven't found one yet, despite trying) to tell that someone is using acme-companion with an nginx-proxy template that already include the .well-known/acme-challenge config, so the companion has to dynamically insert it just in case.

I agree that it's not ideal and that the redundant config does not look good, but since it should not have much of an impact, getting rid of it has been a low priority.

@azlux
Copy link
Author

azlux commented Feb 15, 2024

@buchdag because you don't consider nginx-proxy and acme-companion as a stack ? Do you really need to check the acme .well-know location existancec since both member of the stack is running for a working SSL nginx.

@buchdag
Copy link
Member

buchdag commented Feb 15, 2024

Wether anyone consider it a stack or not isn't really the question here, the issue comes from the fact that people might unexpectedly combine new version of acme-companion with older versions of nginx-proxy (or the nginx-proxy template when running in three containers configuration). If .well-known automatic configuration is removed from acme-companion, this combination will result in failed certificate creation.

Again I know this isn't ideal, I know the versions of nginx-proxy incompatible with the removal of the automatic .well-known configuration are really old (0.7.0 and below, so ~5 years old), I'll come to this eventually but right now this, to the best of my knowledge, does not cause any issue beside the full configuration looking bad and redundant, so it's a low priority (my current priority is this feature).

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

No branches or pull requests

3 participants