Skip to content

feat: boolean variable ACME_HTTP_CHALLENGE_LOCATION #2468

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

Merged
merged 1 commit into from
May 30, 2024

Conversation

pini-gh
Copy link
Contributor

@pini-gh pini-gh commented May 29, 2024

Enable / disable ACME HTTP Challenge blocks generation by nginx-proxy.

Default: true.

This feature is currently needed because acme-companion may generate the HTTP Challenge configuration while it was done already by nginx-proxy (see #2465#issuecomment-2136361373).

Also sometimes a hardcoded ACME challenge location is not wanted because the challenge validation is not done with acme-companion / Let's Encrypt, and with a challenge location setup differently.

@buchdag
Copy link
Member

buchdag commented May 30, 2024

@pini-gh the variable should be set to false by default, and the condition on line 746 and 756 (on the HTTP redirect server) removed. The idea is to disable the new behaviour introduced in #2446 by default, and it alone.

The priority for now is to avoid breaking existing setups (even if they insist on using latest) without forcing people to use a new environment variable, we'll revisit this soon when the ability to disable automatic location config will be added to acme-companion and we can release a version of both that are compatible with each other out of the box.

@buchdag buchdag added kind/bug Issue reporting a bug type/fix PR for a bug fix and removed kind/bug Issue reporting a bug labels May 30, 2024
@@ -750,6 +753,7 @@ server {
try_files $uri =404;
break;
}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- end }}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are broken use cases with both defaults:

true
acme_companion configures an extraneous HTTP Challenge location block which is not valid.

false
Use cases where certificates are not generated with acme_companion loose HTTP Challenge configuration and may fail to renew their certificates.

Why not fixing acme_companion instead?

EDIT: The easiest way would be to keep the default as true and update acme_companion so it wouldn't generate the HTTP Challenge configuration by default (but for standalone certificates).

EDIT1: moving this comment with was on the wrong thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about having 3 possible values for ACME_HTTP_CHALLENGE?

  • legacy: previous incomplete behavior
  • true: generate ACME HTTP Challenge location blocks when HTTPS_METHOD is in [ redirect, noredirect ] (current default behavior)
  • false: do not generate ACME HTTP Challenge location blocks

And we set the new default to legacy.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The legacy / true / false solution with legacy being the default (until we change acme-companion default behaviour, which I plan on doing soon) seems perfect to me, if it's not too much additional work for you.

Why not fixing acme_companion instead?

Because I'm a bit short in time this week 😬 but that's planned, yes.

(by the way regarding acme-companion and it's location tinkering, I tried to automatically skip location configuration when not required a few years ago, I could not come up with anything else than very hacky and fragile solutions that did not even work in the end. Manually enabling or disabling location configuration as you suggest seem to be the only realistic way).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor request: could we use ACME_HTTP_CHALLENGE_LOCATION instead of ACME_HTTP_CHALLENGE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@buchdag buchdag added the type/feat PR for a new feature label May 30, 2024
@pini-gh pini-gh force-pushed the pini-feat-disable-http-challenge branch from d8495d1 to b5b1320 Compare May 30, 2024 21:13
Values:
* `legacy` (default): generate location blocks for ACME HTP Challenge
  excepted when `HTTPS_METHOD=noredirect` or there is no certificate for
  the domain
* `true`: generate location blocks for ACME HTP Challenge in all cases
* `false`: do not generate location blocks for ACME HTP Challenge

This feature is currently needed because acme-companion may generate
the HTTP Challenge configuration while it was done already by nginx-proxy
(see nginx-proxy#2465#issuecomment-2136361373).

Also sometimes a hardcoded ACME challenge location is not wanted because
the challenge validation is not done with acme-companion / Let's Encrypt,
and with a challenge location setup differently.
@pini-gh pini-gh force-pushed the pini-feat-disable-http-challenge branch from b5b1320 to 854427c Compare May 30, 2024 21:33
@buchdag buchdag merged commit 9cf736f into nginx-proxy:main May 30, 2024
@pini-gh pini-gh deleted the pini-feat-disable-http-challenge branch May 30, 2024 22:19
@Montana
Copy link

Montana commented May 30, 2024

Hi @pini-gh,

Just an idea here for the location block:

Perhaps set ACME_ENABLED to false to include the ACME challenge location block. Then set the Production Environment: ACME_ENABLED to true to exclude the ACME challenge location block.

So you would have a default.conf that would check for an env var assigned ACME_ENABLED this would determine to either include the ACME challenge block or not. This is a default.conf I made:

server {
    server_name localhost.pwn.college;
    access_log /var/log/nginx/access.log vhost;
    http2 on;
    listen 80 default_server;

    {{ if eq (or (index $globals.Env "ACME_ENABLED") "false") "false" }}
]    location /.well-known/acme-challenge/ {
        auth_basic off;
        allow all;
        root /usr/share/nginx/html;
        try_files $uri =404;
        break;
    }
    {{ end }}

    listen 443 ssl default_server;
    ssl_certificate /etc/nginx/certs/default.crt;
    ssl_certificate_key /etc/nginx/certs/default.key;
    if ($https) {
        return 500;
    }

    include /etc/nginx/vhost.d/default;

    location / {
        proxy_pass http://localhost.pwn.college;
        set $upstream_keepalive false;
        include /etc/nginx/vhost.d/default_location;
    }
}

Setting these environment variables, I'm obviously talking about Docker Compose:

environment:
      - ACME_ENABLED=false # set to true in production

This is only an idea, maybe it helps or gets something stirring. Have a wonderful Thursday.

@pini-gh
Copy link
Contributor Author

pini-gh commented May 31, 2024

Perhaps set ACME_ENABLED to false to include the ACME challenge location block. Then set the Production Environment: ACME_ENABLED to true to exclude the ACME challenge location block.

So you would have a default.conf that would check for an env var assigned ACME_ENABLED this would determine to either include the ACME challenge block or not.

All the uses cases are enabled by the current PR already:

  • legacy behavior
  • generate ACME HTTP Challenge location blocks
  • do not generate ACME HTTP Challenge location blocks

SchoNie added a commit to SchoNie/nginx-proxy that referenced this pull request May 31, 2024
buchdag pushed a commit that referenced this pull request May 31, 2024
@buchdag buchdag changed the title feat: boolean variable ACME_HTTP_CHALLENGE feat: boolean variable ACME_HTTP_CHALLENGE_LOCATION Jun 6, 2024
f403 pushed a commit to f403/nginx-proxy that referenced this pull request Jul 25, 2024
Values:
* `legacy` (default): generate location blocks for ACME HTP Challenge
  excepted when `HTTPS_METHOD=noredirect` or there is no certificate for
  the domain
* `true`: generate location blocks for ACME HTP Challenge in all cases
* `false`: do not generate location blocks for ACME HTP Challenge

This feature is currently needed because acme-companion may generate
the HTTP Challenge configuration while it was done already by nginx-proxy
(see nginx-proxy#2465#issuecomment-2136361373).

Also sometimes a hardcoded ACME challenge location is not wanted because
the challenge validation is not done with acme-companion / Let's Encrypt,
and with a challenge location setup differently.
f403 pushed a commit to f403/nginx-proxy that referenced this pull request Jul 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feat PR for a new feature type/fix PR for a bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nginx-proxy fails with nginx: [emerg] duplicate location "/.well-known/acme-challenge/" in /etc/nginx/vhost.d/default:2
3 participants