Repository: caddyserver/caddy (language: go)

Issue:
Removing on_demand rate_limit causes a configuration error
This is related to #5368 

To fix the vulnerability warning, I added the on_demand ask and rate_limit options to our JSON config. Since that is fixed, I am trying to remove them from the Caddy config by updating the config using the API endpoint. This causes an error though.

Previously, I would update the Caddy config with a `POST /load` that included the following config generated by Ruby. The relevant snippet that is causing the issue:

```ruby
        tls: {
          automation: {
            policies: @tls_policies,
            on_demand: {
              ask: Rails.application.routes.url_helpers.api_v1_caddy_ask_url,
              rate_limit: {
                interval: "2m",
                burst: 5
              }
            }
          },
```

Now we're sending over the following with `POST /load`

```ruby
        tls: {
          automation: {
            policies: @tls_policies
            }
          },
```

I've also tried setting it to an empty hash or null to clear out the on_demand param but same result with those.

```
on_demand: {} // null
```

Caddy's internal updating of the config seems to be tripping on this removal as the POST request fails and we get this error in the Caddy logs from certmagic.

```
invalid configuration: maxEvents = 0 and window != 0 would not allow any events
goroutine 116 [running]:
net/http.(*conn).serve.func1()
net/http/server.go:1854 +0xbf
panic({0x1af0120, 0x234d3f0})
runtime/panic.go:890 +0x263
github.com/caddyserver/certmagic.(*RingBufferRateLimiter).SetMaxEvents(0xc0002dc780, 0x0)
github.com/caddyserver/certmagic@v0.17.2/ratelimiter.go:154 +0x218
github.com/caddyserver/caddy/v2/modules/caddytls.(*TLS).Provision(0xc0007d91e0, {{0x236a758, 0xc0004e41e0}, 0xc0005d7bf0, 0xc0004e4140, {0x0, 0x0, 0x0}, {0xc000646e60, 0x2, ...}})
```

This seems like a bug where Caddy's retaining that rate limit option internally when it should have been removed?


Discussion / hints:
The `/load` endpoint claims to replace the existing config, but internally it is just calling `POST /config/` which merges the new config with the old.

https://github.com/caddyserver/caddy/blob/master/caddy.go#L127

I bet that's why removing keys doesn't work?
I tried `DELETE /config/apps/tls/automation/on_demand` to remove that section of the config and that produces the same result:

```
2023/03/17 15:35:25.016	[34mINFO[0m	admin.api	received request	{"method": "DELETE", "host": "localhost:2019", "uri": "/config/apps/tls/automation/on_demand", "remote_ip": "127.0.0.1", "remote_port": "33496", "headers": {"Accept":["*/*"],"User-Agent":["curl/7.81.0"]}}
2023/03/17 15:35:25.017	[34mINFO[0m	admin	admin endpoint started	{"address": "localhost:2019", "enforce_origin": false, "origins": ["//[::1]:2019", "//127.0.0.1:2019", "//localhost:2019"]}
{"level":"info","ts":1679067325.0174391,"msg":"http: panic serving 127.0.0.1:33496: invalid configuration: maxEvents = 0 and window != 0 would not allow any events\ngoroutine 177 [running]:\nnet/http.(*conn).serve.func1()\n\tnet/http/server.go:1854 +0xbf\npanic({0x1af0120, 0x234d3f0})\n\truntime/panic.go:890 +0x263\ngithub.com/caddyserver/certmagic.(*RingBufferRateLimiter).SetMaxEvents(0xc0002dc780, 0x0)\n\tgithub.com/caddyserver/certmagic@v0.17.2/ratelimiter.go:154 +0x218\ngithub.com/caddyserver/caddy/v2/modules/caddytls.(*TLS).Provision(0xc00068de10, {{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0xc0006372a0, 0x2, ...}})\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/modules/caddytls/tls.go:184 +0xb39\ngithub.com/caddyserver/caddy/v2.Context.LoadModuleByID({{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0xc0006372a0, 0x2, 0x2}}, ...)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/context.go:347 +0x43c\ngithub.com/caddyserver/caddy/v2.Context.App({{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0xc0006b9710, 0x1, 0x1}}, ...)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/context.go:418 +0x10e\ngithub.com/caddyserver/caddy/v2/modules/caddyhttp.(*App).Provision(0xc0008366e0, {{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0xc0006b9710, 0x1, ...}})\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/modules/caddyhttp/app.go:159 +0x65\ngithub.com/caddyserver/caddy/v2.Context.LoadModuleByID({{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0xc0006b9710, 0x1, 0x1}}, ...)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/context.go:347 +0x43c\ngithub.com/caddyserver/caddy/v2.Context.App({{0x236a758, 0xc0008f6910}, 0xc0005accc0, 0xc0008f6640, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}}, ...)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/context.go:418 +0x10e\ngithub.com/caddyserver/caddy/v2.run.func3(...)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/caddy.go:477\ngithub.com/caddyserver/caddy/v2.run(0xc0005e67e4?, 0x1)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/caddy.go:482 +0x53c\ngithub.com/caddyserver/caddy/v2.unsyncedDecodeAndRun({0xc000034a80, 0x9e2, 0xa80}, 0x1)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/caddy.go:337 +0x145\ngithub.com/caddyserver/caddy/v2.changeConfig({0xc000860040, 0x6}, {0xc000860047, 0x25}, {0x0, 0x0, 0x0}, {0x0, 0x0}, 0x0)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/caddy.go:228 +0x79a\ngithub.com/caddyserver/caddy/v2.handleConfig({0x2365370?, 0xc0004643c0?}, 0xc0005a2100)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/admin.go:980 +0x746\ngithub.com/caddyserver/caddy/v2.AdminHandlerFunc.ServeHTTP(0x8?, {0x2365370?, 0xc0004643c0?}, 0x77b2e0?)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/admin.go:1243 +0x2f\ngithub.com/caddyserver/caddy/v2.(*AdminConfig).newAdminHandler.func2.1({0x2365370, 0xc0004643c0}, 0xc0005a2100)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/admin.go:235 +0x87\nnet/http.HandlerFunc.ServeHTTP(0xc000449540?, {0x2365370?, 0xc0004643c0?}, 0xc0004bb770?)\n\tnet/http/server.go:2122 +0x2f\ngithub.com/caddyserver/caddy/v2.instrumentHandlerCounter.func1({0x2368d00?, 0xc00077e000}, 0xc0005a2100)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/metrics.go:46 +0xbc\nnet/http.HandlerFunc.ServeHTTP(0xc0005332f0?, {0x2368d00?, 0xc00077e000?}, 0x6fdc65?)\n\tnet/http/server.go:2122 +0x2f\nnet/http.(*ServeMux).ServeHTTP(0xc000449540?, {0x2368d00, 0xc00077e000}, 0xc0005a2100)\n\tnet/http/server.go:2500 +0x149\ngithub.com/caddyserver/caddy/v2.adminHandler.serveHTTP({0xc000449540, 0x0, 0x1, {0xc0005332f0, 0x3, 0x3}, 0x0}, {0x2368d00, 0xc00077e000}, 0xc0005a2100)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/admin.go:807 +0x532\ngithub.com/caddyserver/caddy/v2.adminHandler.ServeHTTP({0xc000449540, 0x0, 0x1, {0xc0005332f0, 0x3, 0x3}, 0x0}, {0x2368d00, 0xc00077e000}, 0xc0005a2100)\n\tgithub.com/caddyserver/caddy/v2@v2.6.3/admin.go:759 +0x7fe\nnet/http.serverHandler.ServeHTTP({0xc0005ac2d0?}, {0x2368d00, 0xc00077e000}, 0xc0005a2100)\n\tnet/http/server.go:2936 +0x316\nnet/http.(*conn).serve(0xc00083e000, {0x236a800, 0xc000589f50})\n\tnet/http/server.go:1995 +0x612\ncreated by net/http.(*Server).Serve\n\tnet/http/server.go:3089 +0x5ed"}
```
@excid3 What does your resulting config look like? I'm having a hard time following with just snippets and such. We'll probably need the full config to diagnose and reproduce. Ideally, what's the starting config, then what's your exact API request (and the resulting config if possible, just as a double-check)?
Sure thing! I meant to post it. 

To reproduce:

1. I added a config with the `rate_limit`

```bash
curl --max-time 5 -s --fail-with-body -X POST -H 'Content-Type: application/json' localhost:2019/load --data '{"apps":{"http":{"servers":{"srv0":{"listen":[":443",":80"],"routes":[{"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"vars","root":"/home/deploy/nameless-pond-2385/current/public"},{"handler":"encode","encodings":{"gzip":{},"zstd":{}}}]},{"handle":[{"handler":"reverse_proxy","upstreams":[{"dial":"127.0.0.1:9010"}],"trusted_proxies":["173.245.48.0/20","103.21.244.0/22","103.22.200.0/22","103.31.4.0/22","141.101.64.0/18","108.162.192.0/18","190.93.240.0/20","188.114.96.0/20","197.234.240.0/22","198.41.128.0/17","162.158.0.0/15","104.16.0.0/13","104.24.0.0/14","172.64.0.0/13","131.0.72.0/22","2400:cb00::/32","2606:4700::/32","2803:f800::/32","2405:b500::/32","2405:8100::/32","2a06:98c0::/29","2c0f:f248::/32"]}],"match":[{"not":[{"file":{"try_files":["{http.request.uri.path}","index.html"]}}]}]},{"handle":[{"handler":"file_server"}]}]}],"match":[{"host":["yvgpy.hatchboxdev.com","192.168.64.4"]}],"terminal":true},{"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"vars","root":"/home/deploy/solitary-snow-1183/current/public"},{"handler":"encode","encodings":{"gzip":{},"zstd":{}}}]},{"handle":[{"handler":"reverse_proxy","upstreams":[{"dial":"127.0.0.1:9000"}],"trusted_proxies":["173.245.48.0/20","103.21.244.0/22","103.22.200.0/22","103.31.4.0/22","141.101.64.0/18","108.162.192.0/18","190.93.240.0/20","188.114.96.0/20","197.234.240.0/22","198.41.128.0/17","162.158.0.0/15","104.16.0.0/13","104.24.0.0/14","172.64.0.0/13","131.0.72.0/22","2400:cb00::/32","2606:4700::/32","2803:f800::/32","2405:b500::/32","2405:8100::/32","2a06:98c0::/29","2c0f:f248::/32"]}],"match":[{"not":[{"file":{"try_files":["{http.request.uri.path}","index.html"]}}]}]},{"handle":[{"handler":"file_server"}]}]}],"match":[{"host":["6nv01.hatchboxdev.com","192.168.64.4"]}],"terminal":true}],"errors":{"routes":[{"match":[{"host":["yvgpy.hatchboxdev.com","6nv01.hatchboxdev.com"]}],"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"headers","response":{"add":{"Cache-Control":["no-cache"],"expires":["0"]}}}]},{"handle":[{"handler":"vars","root":"/home/deploy/.hatchbox/templates"}]},{"handle":[{"handler":"rewrite","uri":"/error.html"}]},{"handle":[{"handler":"file_server"}]}]}],"terminal":true}]},"tls_connection_policies":[{}]}}},"tls":{"automation":{"policies":[{"subjects":["yvgpy.hatchboxdev.com"],"on_demand":true},{"subjects":["6nv01.hatchboxdev.com"],"on_demand":true}],"on_demand":{"ask":"http://app.lvh.me:3000/api/v1/caddy/ask","rate_limit":{"interval":"2m","burst":5}}},"certificates":{"load_files":[]}}},"logging":{"logs":{"default":{"encoder":{"format":"console"},"level":"debug"}}}}'
```

2. Updating it to remove the `on_demand` key under tls automation (Fails)

```bash
curl --max-time 5 -s --fail-with-body -X POST -H 'Content-Type: application/json' localhost:2019/load --data '{"apps":{"http":{"servers":{"srv0":{"listen":[":443",":80"],"routes":[{"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"vars","root":"/home/deploy/nameless-pond-2385/current/public"},{"handler":"encode","encodings":{"gzip":{},"zstd":{}}}]},{"handle":[{"handler":"reverse_proxy","upstreams":[{"dial":"127.0.0.1:9010"}],"trusted_proxies":["173.245.48.0/20","103.21.244.0/22","103.22.200.0/22","103.31.4.0/22","141.101.64.0/18","108.162.192.0/18","190.93.240.0/20","188.114.96.0/20","197.234.240.0/22","198.41.128.0/17","162.158.0.0/15","104.16.0.0/13","104.24.0.0/14","172.64.0.0/13","131.0.72.0/22","2400:cb00::/32","2606:4700::/32","2803:f800::/32","2405:b500::/32","2405:8100::/32","2a06:98c0::/29","2c0f:f248::/32"]}],"match":[{"not":[{"file":{"try_files":["{http.request.uri.path}","index.html"]}}]}]},{"handle":[{"handler":"file_server"}]}]}],"match":[{"host":["yvgpy.hatchboxdev.com","192.168.64.4"]}],"terminal":true},{"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"vars","root":"/home/deploy/solitary-snow-1183/current/public"},{"handler":"encode","encodings":{"gzip":{},"zstd":{}}}]},{"handle":[{"handler":"reverse_proxy","upstreams":[{"dial":"127.0.0.1:9000"}],"trusted_proxies":["173.245.48.0/20","103.21.244.0/22","103.22.200.0/22","103.31.4.0/22","141.101.64.0/18","108.162.192.0/18","190.93.240.0/20","188.114.96.0/20","197.234.240.0/22","198.41.128.0/17","162.158.0.0/15","104.16.0.0/13","104.24.0.0/14","172.64.0.0/13","131.0.72.0/22","2400:cb00::/32","2606:4700::/32","2803:f800::/32","2405:b500::/32","2405:8100::/32","2a06:98c0::/29","2c0f:f248::/32"]}],"match":[{"not":[{"file":{"try_files":["{http.request.uri.path}","index.html"]}}]}]},{"handle":[{"handler":"file_server"}]}]}],"match":[{"host":["6nv01.hatchboxdev.com","192.168.64.4"]}],"terminal":true}],"errors":{"routes":[{"match":[{"host":["yvgpy.hatchboxdev.com","6nv01.hatchboxdev.com"]}],"handle":[{"handler":"subroute","routes":[{"handle":[{"handler":"headers","response":{"add":{"Cache-Control":["no-cache"],"expires":["0"]}}}]},{"handle":[{"handler":"vars","root":"/home/deploy/.hatchbox/templates"}]},{"handle":[{"handler":"rewrite","uri":"/error.html"}]},{"handle":[{"handler":"file_server"}]}]}],"terminal":true}]},"tls_connection_policies":[{}]}}},"tls":{"automation":{"policies":[{"subjects":["yvgpy.hatchboxdev.com"],"on_demand":true},{"subjects":["6nv01.hatchboxdev.com"],"on_demand":true}]},"certificates":{"load_files":[]}}},"logging":{"logs":{"default":{"encoder":{"format":"console"},"level":"debug"}}}}'
```

This does not seem to update the config and produces that "invalid config" error in the logs.

3. DELETE /config/apps/tls/automation/on_demand also fails with the same error.

Fix patch:
diff --git a/modules/caddytls/automation.go b/modules/caddytls/automation.go
index fffc0a36967..1cfb28c3e12 100644
--- a/modules/caddytls/automation.go
+++ b/modules/caddytls/automation.go
@@ -19,6 +19,7 @@ import (
 	"errors"
 	"fmt"
 	"net/http"
+	"strings"
 	"time"
 
 	"github.com/caddyserver/caddy/v2"
@@ -224,8 +225,10 @@ func (ap *AutomationPolicy) Provision(tlsApp *TLS) error {
 	// on-demand TLS
 	var ond *certmagic.OnDemandConfig
 	if ap.OnDemand {
-		// ask endpoint is now required after a number of negligence cases causing abuse
-		if !ap.onlyInternalIssuer() && (tlsApp.Automation == nil || tlsApp.Automation.OnDemand == nil || tlsApp.Automation.OnDemand.Ask == "") {
+		// ask endpoint is now required after a number of negligence cases causing abuse;
+		// but is still allowed for explicit subjects (non-wildcard, non-unbounded),
+		// and for the internal issuer since it doesn't cause ACME issuer pressure
+		if ap.isWildcardOrDefault() && !ap.onlyInternalIssuer() && (tlsApp.Automation == nil || tlsApp.Automation.OnDemand == nil || tlsApp.Automation.OnDemand.Ask == "") {
 			return fmt.Errorf("on-demand TLS cannot be enabled without an 'ask' endpoint to prevent abuse; please refer to documentation for details")
 		}
 		ond = &certmagic.OnDemandConfig{
@@ -294,6 +297,22 @@ func (ap *AutomationPolicy) onlyInternalIssuer() bool {
 	return ok
 }
 
+// isWildcardOrDefault determines if the subjects include any wildcard domains,
+// or is the "default" policy (i.e. no subjects) which is unbounded.
+func (ap *AutomationPolicy) isWildcardOrDefault() bool {
+	isWildcardOrDefault := false
+	if len(ap.Subjects) == 0 {
+		isWildcardOrDefault = true
+	}
+	for _, sub := range ap.Subjects {
+		if strings.HasPrefix(sub, "*") {
+			isWildcardOrDefault = true
+			break
+		}
+	}
+	return isWildcardOrDefault
+}
+
 // DefaultIssuers returns empty Issuers (not provisioned) to be used as defaults.
 // This function is experimental and has no compatibility promises.
 func DefaultIssuers() []certmagic.Issuer {
diff --git a/modules/caddytls/tls.go b/modules/caddytls/tls.go
index 92004b82308..486a58cb2b8 100644
--- a/modules/caddytls/tls.go
+++ b/modules/caddytls/tls.go
@@ -22,7 +22,6 @@ import (
 	"log"
 	"net/http"
 	"runtime/debug"
-	"strings"
 	"sync"
 	"time"
 
@@ -182,8 +181,8 @@ func (t *TLS) Provision(ctx caddy.Context) error {
 		onDemandRateLimiter.SetWindow(time.Duration(t.Automation.OnDemand.RateLimit.Interval))
 	} else {
 		// remove any existing rate limiter
-		onDemandRateLimiter.SetMaxEvents(0)
 		onDemandRateLimiter.SetWindow(0)
+		onDemandRateLimiter.SetMaxEvents(0)
 	}
 
 	// run replacer on ask URL (for environment variables) -- return errors to prevent surprises (#5036)
@@ -260,17 +259,7 @@ func (t *TLS) Start() error {
 	if t.Automation.OnDemand == nil ||
 		(t.Automation.OnDemand.Ask == "" && t.Automation.OnDemand.RateLimit == nil) {
 		for _, ap := range t.Automation.Policies {
-			isWildcardOrDefault := false
-			if len(ap.Subjects) == 0 {
-				isWildcardOrDefault = true
-			}
-			for _, sub := range ap.Subjects {
-				if strings.HasPrefix(sub, "*") {
-					isWildcardOrDefault = true
-					break
-				}
-			}
-			if ap.OnDemand && isWildcardOrDefault {
+			if ap.OnDemand && ap.isWildcardOrDefault() {
 				t.logger.Warn("YOUR SERVER MAY BE VULNERABLE TO ABUSE: on-demand TLS is enabled, but no protections are in place",
 					zap.String("docs", "https://caddyserver.com/docs/automatic-https#on-demand-tls"))
 				break


Test patch: