Repository: caddyserver/caddy

Issue:
Weird bug with default server directive that has both HTTP and HTTPS?
With the following Caddyfile:
```
http:// {
        respond "DEFAULT"
}
```
Caddy will respond to a HTTP request on port 80 over direct IP as expected with "DEFAULT".
```
> GET / HTTP/1.1
> Host: 192.168.1.100
> User-Agent: curl/7.79.1
> Accept: */*
>
< HTTP/1.1 200 OK
< Server: Caddy
< Date: Tue, 07 Jun 2022 23:47:19 GMT
< Content-Length: 7
<
DEFAULT
```

However, if we try adding a https:// there like so:
```
http:// https:// {
        respond "DEFAULT"
}
```
suddenly the HTTP port starts redirecting to HTTPS.
```
> GET / HTTP/1.1
> Host: 192.168.1.100
> User-Agent: curl/7.79.1
> Accept: */*
>
< HTTP/1.1 308 Permanent Redirect
< Connection: close
< Location: https://192.168.1.100/
< Server: Caddy
< Date: Tue, 07 Jun 2022 23:46:06 GMT
< Content-Length: 0
<
```
This is most unexpected, given Caddy is normally supposed to support mixed HTTP and HTTPS site blocks, like this:
```
http://test.example.com test.example.com {
        respond "DOMAIN"
}
```
In the above example, you can normally access test.example.com over HTTP as expected (over HTTPS too ofc).

What's even more surprising, is that when we modify the second Caddyfile snippet to be this:
```
test.example.com {
        # must be https
        respond "TEST"
}

http:// https:// {
        respond "DEFAULT"
}
```
**Suddenly HTTP port responds with "DEFAULT" as expected again!**

Looking further at the default server example though, it seems that its HTTPS port does not work (no matter if tried by IP or dummy domain):
```
>curl -k -v https://192.168.1.100
*   Trying 192.168.1.100:443...
* Connected to 192.168.1.100 (192.168.1.100) port 443 (#0)
* schannel: disabled automatic use of client certificate
* schannel: using IP address, SNI is not supported by OS.
* schannel: ALPN, offering http/1.1
* schannel: next InitializeSecurityContext failed: SEC_E_ILLEGAL_MESSAGE (0x80090326) - This error usually occurs when a fatal SSL/TLS alert is received (e.g. handshake failed). More detail may be available in the Windows System event log.
* Closing connection 0
curl: (35) schannel: next InitializeSecurityContext failed: SEC_E_ILLEGAL_MESSAGE (0x80090326) - This error usually occurs when a fatal SSL/TLS alert is received (e.g. handshake failed). More detail may be available in the Windows System event log.
```
If I add `tls internal` into the "DEFAULT" block, nothing changes. If I add `tls <path>.pem <path>.pem` manually with a path to the certificate into the section, Caddy fails to start:
```
run: adapting config using caddyfile: server listening on [:80] is HTTP, but attempts to configure TLS connection policies
```

---

# **So to summarize, it seems there are two issues**:
1. Inconsistent behavior of the "default" (no-hostname) site block for both HTTP and HTTPS based on existence of **another** unrelated HTTPS site block (if "default" block is the only one, HTTP is redirected to HTTPS for no reason?).
2. You are supposed to be able to create a site block that can be served over HTTP and HTTPS both, but you seemingly cannot then use `tls` directive inside to set the "internal" or custom certificate paths, it only works with default encryption that specifies no custom options. Expected behavior would be that whatever is set in `tls` option would simply be ignored for HTTP connections...

It is worth to note that I was previously successfully using both Apache2 and nginx with such configuration that is basically the default if you enable SSL there, that is the same default server block is also served over HTTPS with a self-signed certificate, which works both over IP or any unrecognized domain, thus titled the "default".

### Workaround attempt

I tried this:
```
(default) {
        respond "DEFAULT"
}

http:// {
        import default
}

https:// {
        # I could not access this over HTTPS with neither TLS options or lack thereof
        #tls internal
        # adding the line below (instead of the above ofc) makes the Caddy server start with no error, but stop responding to ANY requests, both HTTP or HTTPS (???)
        #tls /etc/ssl/certs/ssl-cert-snakeoil.pem /etc/ssl/private/ssl-cert-snakeoil.key
        import default
}
```
Result: I still cannot load the server via HTTP, I still need to add a random HTTPS site block in order to be able to reach the default HTTP site.

---

Now of course I could simply accept that I will not get an unsecure default unnamed HTTPS endpoint with Caddy that I could by default with Apache2 and nginx. But I think my attempts still bring up some logic errors in Caddy config processing that shouldn't be behaving the way they behave regardless.

---

Caddy version: v2.5.1 h1:bAWwslD1jNeCzDa+jDCNwb8M3UJ2tPa8UZFFzPVmGKs=

Hints from issue comments:
@p0358: It seems that even an example of
```
:80 http://domain.example.com domain.example.com
```
confuses Caddy.

With a site block like above you can access the server via IP and port 80 as expected, but trying to access it via `http://domain.example.com` causes the unexpected redirect to HTTPS version to occur@francislavoie: This is working as intended, actually. The HTTP->HTTPS redirect routes get inserted _just before_ any HTTP catch-all routes, so that it's possible to keep the redirect and force certain domains to be served over HTTP.

See https://github.com/caddyserver/caddy/pull/4033 and https://github.com/caddyserver/caddy/issues/3212 which explain the problem.

To work around it in your situation, you'll want to use the global option `auto_https disable_redirects` which will remove the redirect routes and let HTTP requests actually reach your catch-all block.

As for the `tls` directive thing, I disagree. It's a configuration error to use `tls` inside of a site block configuring an HTTP server. Split them up.@p0358: > To work around it in your situation, you'll want to use the global option auto_https disable_redirects which will remove the redirect routes and let HTTP requests actually reach your catch-all block.

Honestly I think this might be the best solution in my case, thanks.

> As for the `tls` directive thing, I disagree. It's a configuration error to use tls inside of a site block configuring an HTTP server. Split them up.

I think this is inconsistent though -- because a single site block for HTTP and HTTPS is otherwise allowed and perfectly functional. It works fine with automatic HTTPS, but if you want to define custom TLS options, then you are required to split them up just to insert the `tls` option. I think that's just pretty inconvenient.

---

I still cannot wrap my head around on how to get catch-all HTTPS working, is it even supported? And speaking of which, I found another weird behavior. Suppose this config file:
```
{
    auto_https disable_redirects
}
(default) {
    respond "DEFAULT"
}
http:// {
    import default
}
https:// {
    tls internal
    import default
}
# ...
domain1.example.com:8443 {
    respond "UNRELATED BLOCK"
}
```
Let's ignore the last block that works independently and let's assume *.example.com resolves to our server's IP. The following happens:
1. I can access catch-all http://<serverIP> and http://domain1.example.com and http://random.example.com, all lead to "DEFAULT"
2. I cannot access catch-all https://<serverIP> or https://random.example.com
3. I can access only https://domain.example.com

^ Note that in point 3 I can access the catch-all only with a certificate that's in Caddy's memory, despite being there (generated by Let's Encrypt) for a completely unrelated block

Now if I do this:
```
{
    auto_https disable_redirects
}
(default) {
    respond "DEFAULT"
}
http:// {
    import default
}
https:// {
    # self-signed certs
    tls /var/lib/caddy/ssl-cert-snakeoil.pem /var/lib/caddy/ssl-cert-snakeoil.key
    import default
}
```

It works, I can curl the server on HTTPS with any domain or IP! But!!! Apparently now I cannot add any normal HTTPS site block on a normal. Let's add this:
```
domain1.example.com {
    respond "NON DEFAULT"
}
```
**If I open https://domain1.example.com, I get "NON DEFAULT" served, which means the block works. But I get certificate specified in https:// catch-all instead!!!** This is clearly a bug in certificate selection routines, since otherwise the block wouldn't be used at all if catch-all took precedence.

The automatic certificate in my case of `domain1.example.com` exists, and I can add a site block on this domain on another port and it works. It also works on the default port if I comment out the catch-all https block...
@francislavoie: > I think this is inconsistent though -- because a single site block for HTTP and HTTPS is otherwise allowed and perfectly functional. It works fine with automatic HTTPS, but if you want to define custom TLS options, then you are required to split them up just to insert the tls option. I think that's just pretty inconvenient.

I don't think it's inconsistent. It's the difference between implicit and explicit config for HTTPS. If you let Caddy enable automatic HTTPS (implicitly) then a single block is fine. Obviously you need to disable redirects if you want to do that though. But if you need to configure `tls` explicitly, then yes you need to split it up, because otherwise it's a logic error; you can't configure TLS for HTTP.

> I still cannot wrap my head around on how to get catch-all HTTPS working, is it even supported?

Yes it is supported, but you need to have a certificate to serve for the requested domain. If you don't put a domain in your config, Caddy won't be able to issue a certificate ahead of time, so it won't be able to complete a handshake.

If you use `tls internal`, then you are telling Caddy to use its internal CA, but you still didn't tell it which domains to manage a cert for. If you don't know the domains ahead of time, then you need to use On-Demand TLS, which enables a mode where Caddy will attempt to issue a certificate for the domain that was requested in the TLS handshake (the SNI field).

But keep in mind, enabling `on_demand` has risk, because it's unbounded by default; some attacker could force your server to issue an infinite amount of TLS certificates and fill up your disk storage with garbage, if your server is publicly accessible.

> Note that in point 3 I can access the catch-all only with a certificate that's in Caddy's memory, despite being there (generated by Let's Encrypt) for a completely unrelated block

Yeah, that's normal. Under the hood, Caddy's actual config language is JSON. If you run `caddy adapt --pretty` on your Caddyfile, you'll see what your config looks like. It'll probably be a bit clearer what's going on.

Keep in mind that this won't show the routes that automatic HTTPS adds in, because that happens at runtime just before starting the server, it's not included in the config.

How it works is Caddy has a `tls` app, which is what does all the certificate maintenance and caching, and there's the `http` app, which is what actually runs the server. The HTTP app asks the TLS app "hey do you have a cert for _this domain_?" and if the TLS app has one in cache, it'll use it to serve.

In JSON config, there's no concept of "sites" at all, that's just syntax sugar in the Caddyfile to try to make it easier to write configs. But you're kinda trying to abuse all the edgecases here, so of course you'll run up against all pitfalls/caveats of that abstraction.

> If I open [domain1.example.com](https://domain1.example.com/), I get "NON DEFAULT" served, which means the block works. But I get certificate specified in https:// catch-all instead!!!

In that case, you would need to use `auto_https ignore_loaded_certs` to tell Caddy "don't use certs from `tls` loaded into cache, and try to manage a cert anyways if there's an explicit domain listed in the config". But we didn't make it possible to specify both `disable_redirects` and `ignore_loaded_certs` at the same time... because that's _insanely_ edgecase-y. Do you _really_ need to do that? It seems absolutely crazy-bonkers to me. If you really, really need that, honestly you should probably just use JSON config directly, so you have full control over what's going on.@mholt: I think there might be improvements we could make here, either in docs or in implementation, but I need to reread this issue with fresher eyes.@whitestrake: > I don't think it's inconsistent. It's the difference between implicit and explicit config for HTTPS.

I think there is inconsistency. To bring this down to, ultimately, the simplest expression I can demonstrate with:

```
https:// {
  tls internal {
    on_demand
  }
  respond "HTTPS"
}

http:// {
  respond "HTTP"
}
```

And I make a request to `http://localhost`, I expect a 200 OK with "HTTP", period. Currently, I get a redirect to `https://localhost` and receive a 200 OK with "HTTPS". I'm not sure I can get more explicit than this, but the HTTP listener is just getting completely trodden over right now.

This changes as soon as I change the HTTPS site to `https://localhost`, in which case its overriding redirect becomes limited in scope to `http://localhost` requests and not e.g. `http://local.test`.

I haven't investigated, but I have a hunch this is part of the interaction of your logic, @francislavoie (which allows for another site to override a HTTP catch-all with its listener), and the fact that Caddy seems to interpret an explicit HTTPS listener as "HTTPS with a HTTP redirect", meaning that this HTTPS catch-all is having its catch-all redirect listener override the _actually configured_ HTTP listener.

Does that sound like it makes sense?

That said, from an ergonomics perspective, I think that:
- A manually configured catch-all HTTP site should take precedence over an auto-configured HTTP->S catch-all redirect
- I shouldn't have to use `auto_https disable_redirects` in the long run for this to work

And further (possibly strictly beyond the scope of this issue, but related because I believe it's interacting here):
- An explicit HTTPS listener shouldn't configure a HTTP redirect (only scheme-agnostic sites should)@francislavoie: > I shouldn't have to use `auto_https disable_redirects` in the long run for this to work

I disagree, this is exactly the situation where you should use it. I don't see how a redirect would ever be correct with your intent as described here, so you should turn off redirects.

I don't think more magic is warranted here. The logic is complex enough as it is. I don't think there's any particular situation that is "impossible" to configure anymore, which really is the goal. You have two knobs, the HTTP sites and `disable_redirects`, and armed with those, you can make it do whatever you need.@whitestrake: Yeah, sure, I agree that it functions as a workaround, but there's _no way_ that a HTTPS and a HTTP block, explicitly configured in the Caddyfile, should be interpreted as "please ignore my HTTP block entirely and redirect it to HTTPS" in the long run.

Whether there's an additional check that does this automatically or not, the intent behind this configuration is abundantly clear, and any logic that produces a result counter to that intent with that incredibly simple configuration is, in my opinion, not ideal.@mholt: (FYI, I'll chime in too when I'm back at the office tomorrow)@mholt: I think both sides have good, valid points:

- If redirects are not the intent, they can be disabled (Francis)
- Implicit rules should never override explicit configuration (Matthew)

The counterpoints are, respectively:

- Putting `http://` site in your config _is_ the intention to disable or override the implicit redirects (Matthew)
- The explicit configuration regarding redirects is `auto_https disable_redirects`, not `http://` (Francis)

I agree with @francislavoie that more magic is not needed, nor is it the answer.

I agree with @whitestrake that the current behavior is unintuitive; a site explicitly defined as `http://` should be served ~~as HTTP, not HTTPS~~ over HTTP, not overridden completely.

---

The reason this behavior was introduced was because its inverse was also confusing (#3212):

```
example.com {
	# ...
}

http:// {
	respond "HTTP"
}
```

Here, the author expects `http://example.com` to redirect to `https://example.com`, but wants all other traffic served over HTTP. In other words, they want the auto-redirects to come _BEFORE_ their explicit configuration _just for sites they've defined._ Of course, this does a disservice to those who want to serve HTTP in all cases, or who have catch-all cases like above.

The first-proposed solution was to do the redirect manually:

```
http://example.com {
	redir https://{hostport}{uri}
}
```

but this requires enumerating every domain that you have an HTTPS site for, a second time. And you have to know the domains.

In #3212, Francis also proposed a handler-based solution, where a matcher+directive could be used within the `http://` site to issue the redirect without having to enumerate all the other domains. I didn't love that solution though due to its internal complexity (and that the list of names could be very long) -- at the time we didn't have binary search implemented for the host matcher, but now we do. The OP also didn't love this solution though because it kind of requires knowing how Caddy's internals work, utilizing a special placeholder, etc.

So in the end we went with a solution that injects the implicit redirect logic _before_ the explicit configuration in some cases. Of course, this current implementation has a few problems:

- Sometimes that is not the user's intent. (They may want their explicit `http://` configuration to be effective.)
- Although `auto_https disable_redirects` can be used, this disables ALL automatic redirects, which may also be inconvenient. Sounds like they would only want to disable the redirects for domains they don't have HTTPS sites for (and the domains are unknown in some cases, like the current issue).
- Somewhat logically complex, including an edge case that had to be fixed in a later PR (#4033, then #4243)

Personally, I'd like to roll back the current fix for this, so that we don't override explicit configuration. And implement a new fix that can satisfy both valid use cases without being complicated.

As for what that is, I'm not sure yet, but I'm hoping to think on this or talk about it here.

---

One thing to keep in mind is to approach this JSON-first. The Caddyfile is only an adapter.

What if this was a handler-based approach like Francis once suggested, but less technical for users:

```json
{
    "handler": "auto_https_redirects"
}
```

or, perhaps a full Caddyfile to illustrate:

```
example.com {
    # ...
}

http:// {
    auto_https_redir
    respond "Staying HTTP"
}
```

What this would do is disable automatic HTTP->HTTPS redirects, because the user explicitly defined an `http://` site. (Technically, they'd still be inserted, but _after_ the user's explicit config, like we originally had.)

But instead of blindly injecting our redirect handler into the chain for the user, we create the handler and attach it to the server. Then the server, during its startup phase when it's building routes, will see whether that handler is present. If so, that's what does the redirects. If it's not there, we append it to the end like we originally did.

This allows explicit config to prevail, and lets the user choose where they want their config overridden.

Do you think something like that would work?@whitestrake: This summary post is excellent, @mholt. Just a few very quick clarifications of my own stance:

> a site explicitly defined as http:// should be served as HTTP, not HTTPS.

It’s arguably a step worse than just that; the explicit HTTP site wasn’t being served on HTTPS, it wasn’t being served _at all._ With the configuration I posted above, `respond "HTTP"` was _never_ served. That entire site block might as well have not existed at all.

> Putting http:// site in your config is the intention to disable or override the implicit redirects (Matthew)

To expound on this, naturally the existence of literal catch-all `http://` implies that it should be logically _possible_ for at least some request that can be made should be routed this way!

---

I would rank which HTTP listener configuration should 'win' in terms of priority by way of how specific and how explicitly defined they are:

1. Explicit HTTP configured site block with a hostname
2. HTTP redirect listener for a scheme-agnostic site with a hostname
3. Catch-all HTTP site block (no hostname)
4. HTTP redirect listener for explicit HTTPS-only sites with hostnames, or for HTTPS catch-alls

@francislavoie's PR to handle this case (where we have two catch-alls, one for each scheme) solves, in my view, the question of 4 (specifically the HTTPS catch-all) beating 3.@mholt: Great, I think so too! (I amended my comment to better reflect your stance.)

@p0358 I'm going to merge Francis' patch in #5051 and release it. When you have a chance would you give it a try?@ledlamp: > I don't think it's inconsistent. It's the difference between implicit and explicit config for HTTPS. If you let Caddy enable automatic HTTPS (implicitly) then a single block is fine. Obviously you need to disable redirects if you want to do that though. But if you need to configure `tls` explicitly, then yes you need to split it up, because otherwise it's a logic error; you can't configure TLS for HTTP.

Why not just simply ignore the tls settings for http so you don't have to make two copies of the configuration?

Whether a site has TLS or not can be explicitly defined by http:// or https:// in the site name, and sites can have multiple names with different protocols and even different ports. So I think it would make more sense if the `tls` directives simply had no effect on listeners that it is not applicable to, than to reject this configuration alltogether and require redundant configuration for what benefit? This is how I was expecting it to work.


Solution patch:
diff --git a/caddy.go b/caddy.go
index 9595c96c..584865bd 100644
--- a/caddy.go
+++ b/caddy.go
@@ -31,6 +31,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/caddyserver/caddy/v2/notify"
@@ -676,6 +677,10 @@ func Validate(cfg *Config) error {
 // Errors are logged along the way, and an appropriate exit
 // code is emitted.
 func exitProcess(ctx context.Context, logger *zap.Logger) {
+	// let the rest of the program know we're quitting
+	atomic.StoreInt32(exiting, 1)
+
+	// give the OS or service/process manager our 2 weeks' notice: we quit
 	if err := notify.Stopping(); err != nil {
 		Log().Error("unable to notify service manager of stopping state", zap.Error(err))
 	}
@@ -739,6 +744,12 @@ func exitProcess(ctx context.Context, logger *zap.Logger) {
 	}()
 }
 
+var exiting = new(int32) // accessed atomically
+
+// Exiting returns true if the process is exiting.
+// EXPERIMENTAL API: subject to change or removal.
+func Exiting() bool { return atomic.LoadInt32(exiting) == 1 }
+
 // Duration can be an integer or a string. An integer is
 // interpreted as nanoseconds. If a string, it is a Go
 // time.Duration value such as `300ms`, `1.5h`, or `2h45m`;
diff --git a/modules/caddyevents/app.go b/modules/caddyevents/app.go
index db2192ce..6b10460d 100644
--- a/modules/caddyevents/app.go
+++ b/modules/caddyevents/app.go
@@ -201,6 +201,9 @@ func (app *App) On(eventName string, handler Handler) error {
 // Emit creates and dispatches an event named eventName to all relevant handlers with
 // the metadata data. Events are emitted and propagated synchronously. The returned Event
 // value will have any additional information from the invoked handlers.
+//
+// Note that the data map is not copied, for efficiency. After Emit() is called, the
+// data passed in should not be changed in other goroutines.
 func (app *App) Emit(ctx caddy.Context, eventName string, data map[string]any) Event {
 	logger := app.logger.With(zap.String("name", eventName))
 
@@ -212,11 +215,11 @@ func (app *App) Emit(ctx caddy.Context, eventName string, data map[string]any) E
 	eventName = strings.ToLower(eventName)
 
 	e := Event{
+		Data:   data,
 		id:     id,
 		ts:     time.Now(),
 		name:   eventName,
 		origin: ctx.Module(),
-		data:   data,
 	}
 
 	logger = logger.With(
@@ -244,12 +247,12 @@ func (app *App) Emit(ctx caddy.Context, eventName string, data map[string]any) E
 		case "event.module":
 			return e.origin.CaddyModule().ID, true
 		case "event.data":
-			return e.data, true
+			return e.Data, true
 		}
 
 		if strings.HasPrefix(key, "event.data.") {
 			key = strings.TrimPrefix(key, "event.data.")
-			if val, ok := data[key]; ok {
+			if val, ok := e.Data[key]; ok {
 				return val, true
 			}
 		}
@@ -257,7 +260,7 @@ func (app *App) Emit(ctx caddy.Context, eventName string, data map[string]any) E
 		return nil, false
 	})
 
-	logger.Debug("event", zap.Any("data", e.data))
+	logger.Debug("event", zap.Any("data", e.Data))
 
 	// invoke handlers bound to the event by name and also all events; this for loop
 	// iterates twice at most: once for the event name, once for "" (all events)
@@ -314,26 +317,40 @@ func (app *App) Emit(ctx caddy.Context, eventName string, data map[string]any) E
 }
 
 // Event represents something that has happened or is happening.
+// An Event value is not synchronized, so it should be copied if
+// being used in goroutines.
+//
+// EXPERIMENTAL: As with the rest of this package, events are
+// subject to change.
 type Event struct {
-	id     uuid.UUID
-	ts     time.Time
-	name   string
-	origin caddy.Module
-	data   map[string]any
-
 	// If non-nil, the event has been aborted, meaning
 	// propagation has stopped to other handlers and
 	// the code should stop what it was doing. Emitters
 	// may choose to use this as a signal to adjust their
 	// code path appropriately.
 	Aborted error
+
+	// The data associated with the event. Usually the
+	// original emitter will be the only one to set or
+	// change these values, but the field is exported
+	// so handlers can have full access if needed.
+	// However, this map is not synchronized, so
+	// handlers must not use this map directly in new
+	// goroutines; instead, copy the map to use it in a
+	// goroutine.
+	Data map[string]any
+
+	id     uuid.UUID
+	ts     time.Time
+	name   string
+	origin caddy.Module
 }
 
 // CloudEvent exports event e as a structure that, when
 // serialized as JSON, is compatible with the
 // CloudEvents spec.
 func (e Event) CloudEvent() CloudEvent {
-	dataJSON, _ := json.Marshal(e.data)
+	dataJSON, _ := json.Marshal(e.Data)
 	return CloudEvent{
 		ID:              e.id.String(),
 		Source:          e.origin.CaddyModule().String(),
diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go
index 290a4083..84b0b941 100644
--- a/modules/caddyhttp/app.go
+++ b/modules/caddyhttp/app.go
@@ -505,24 +505,64 @@ func (app *App) Stop() error {
 		app.logger.Debug("servers shutting down with eternal grace period")
 	}
 
-	// shut down servers
-	for _, server := range app.Servers {
+	// goroutines aren't guaranteed to be scheduled right away,
+	// so we'll use one WaitGroup to wait for all the goroutines
+	// to start their server shutdowns, and another to wait for
+	// them to finish; we'll always block for them to start so
+	// that when we return the caller can be confident* that the
+	// old servers are no longer accepting new connections
+	// (* the scheduler might still pause them right before
+	// calling Shutdown(), but it's unlikely)
+	var startedShutdown, finishedShutdown sync.WaitGroup
+
+	// these will run in goroutines
+	stopServer := func(server *Server) {
+		defer finishedShutdown.Done()
+		startedShutdown.Done()
+
 		if err := server.server.Shutdown(ctx); err != nil {
 			app.logger.Error("server shutdown",
 				zap.Error(err),
 				zap.Strings("addresses", server.Listen))
 		}
+	}
+	stopH3Server := func(server *Server) {
+		defer finishedShutdown.Done()
+		startedShutdown.Done()
 
-		if server.h3server != nil {
-			// TODO: CloseGracefully, once implemented upstream (see https://github.com/lucas-clemente/quic-go/issues/2103)
-			if err := server.h3server.Close(); err != nil {
-				app.logger.Error("HTTP/3 server shutdown",
-					zap.Error(err),
-					zap.Strings("addresses", server.Listen))
-			}
+		if server.h3server == nil {
+			return
+		}
+		// TODO: CloseGracefully, once implemented upstream (see https://github.com/lucas-clemente/quic-go/issues/2103)
+		if err := server.h3server.Close(); err != nil {
+			app.logger.Error("HTTP/3 server shutdown",
+				zap.Error(err),
+				zap.Strings("addresses", server.Listen))
 		}
 	}
 
+	for _, server := range app.Servers {
+		startedShutdown.Add(2)
+		finishedShutdown.Add(2)
+		go stopServer(server)
+		go stopH3Server(server)
+	}
+
+	// block until all the goroutines have been run by the scheduler;
+	// this means that they have likely called Shutdown() by now
+	startedShutdown.Wait()
+
+	// if the process is exiting, we need to block here and wait
+	// for the grace periods to complete, otherwise the process will
+	// terminate before the servers are finished shutting down; but
+	// we don't really need to wait for the grace period to finish
+	// if the process isn't exiting (but note that frequent config
+	// reloads with long grace periods for a sustained length of time
+	// may deplete resources)
+	if caddy.Exiting() {
+		finishedShutdown.Wait()
+	}
+
 	return nil
 }
 
diff --git a/modules/caddyhttp/autohttps.go b/modules/caddyhttp/autohttps.go
index 3d476cb1..be229ea8 100644
--- a/modules/caddyhttp/autohttps.go
+++ b/modules/caddyhttp/autohttps.go
@@ -378,19 +378,29 @@ redirServersLoop:
 		// we'll create a new server for all the listener addresses
 		// that are unused and serve the remaining redirects from it
 		for _, srv := range app.Servers {
-			if srv.hasListenerAddress(redirServerAddr) {
-				// find the index of the route after the last route with a host
-				// matcher, then insert the redirects there, but before any
-				// user-defined catch-all routes
-				// see https://github.com/caddyserver/caddy/issues/3212
-				insertIndex := srv.findLastRouteWithHostMatcher()
-				srv.Routes = append(srv.Routes[:insertIndex], append(routes, srv.Routes[insertIndex:]...)...)
+			// only look at servers which listen on an address which
+			// we want to add redirects to
+			if !srv.hasListenerAddress(redirServerAddr) {
+				continue
+			}
 
-				// append our catch-all route in case the user didn't define their own
-				srv.Routes = appendCatchAll(srv.Routes)
+			// find the index of the route after the last route with a host
+			// matcher, then insert the redirects there, but before any
+			// user-defined catch-all routes
+			// see https://github.com/caddyserver/caddy/issues/3212
+			insertIndex := srv.findLastRouteWithHostMatcher()
 
-				continue redirServersLoop
+			// add the redirects at the insert index, except for when
+			// we have a catch-all for HTTPS, in which case the user's
+			// defined catch-all should take precedence. See #4829
+			if len(uniqueDomainsForCerts) != 0 {
+				srv.Routes = append(srv.Routes[:insertIndex], append(routes, srv.Routes[insertIndex:]...)...)
 			}
+
+			// append our catch-all route in case the user didn't define their own
+			srv.Routes = appendCatchAll(srv.Routes)
+
+			continue redirServersLoop
 		}
 
 		// no server with this listener address exists;



Test patch:
diff --git a/caddytest/caddytest.go b/caddytest/caddytest.go
index 4fb33941..e560ab4a 100644
--- a/caddytest/caddytest.go
+++ b/caddytest/caddytest.go
@@ -43,7 +43,7 @@ type Defaults struct {
 
 // Default testing values
 var Default = Defaults{
-	AdminPort:          2019,
+	AdminPort:          2999, // different from what a real server also running on a developer's machine might be
 	Certifcates:        []string{"/caddy.localhost.crt", "/caddy.localhost.key"},
 	TestRequestTimeout: 5 * time.Second,
 	LoadRequestTimeout: 5 * time.Second,
diff --git a/caddytest/integration/caddyfile_test.go b/caddytest/integration/caddyfile_test.go
index 27588833..77ac54ef 100644
--- a/caddytest/integration/caddyfile_test.go
+++ b/caddytest/integration/caddyfile_test.go
@@ -16,6 +16,7 @@ func TestRespond(t *testing.T) {
   {
     http_port     9080
     https_port    9443
+    grace_period  1
   }
   
   localhost:9080 {
@@ -37,6 +38,7 @@ func TestRedirect(t *testing.T) {
   {
     http_port     9080
     https_port    9443
+    grace_period  1
   }
   
   localhost:9080 {
@@ -86,6 +88,7 @@ func TestReadCookie(t *testing.T) {
   {
     http_port     9080
     https_port    9443
+    grace_period  1
   }
   
   localhost:9080 {
@@ -109,6 +112,7 @@ func TestReplIndex(t *testing.T) {
   {
     http_port     9080
     https_port    9443
+    grace_period  1
   }
 
   localhost:9080 {
diff --git a/caddytest/integration/handler_test.go b/caddytest/integration/handler_test.go
index fb986c7c..e295dc55 100644
--- a/caddytest/integration/handler_test.go
+++ b/caddytest/integration/handler_test.go
@@ -13,6 +13,7 @@ func TestBrowse(t *testing.T) {
 	{
 		http_port     9080
 		https_port    9443
+		grace_period  1
 	}
 	http://localhost:9080 {
 		file_server browse
diff --git a/caddytest/integration/map_test.go b/caddytest/integration/map_test.go
index afed0c35..05959576 100644
--- a/caddytest/integration/map_test.go
+++ b/caddytest/integration/map_test.go
@@ -13,6 +13,7 @@ func TestMap(t *testing.T) {
 	tester.InitServer(`{
 		http_port     9080
 		https_port    9443
+		grace_period  1
 	}
 
 	localhost:9080 {
diff --git a/caddytest/integration/reverseproxy_test.go b/caddytest/integration/reverseproxy_test.go
index ca11287a..dfc8df0d 100644
--- a/caddytest/integration/reverseproxy_test.go
+++ b/caddytest/integration/reverseproxy_test.go
@@ -18,6 +18,7 @@ func TestSRVReverseProxy(t *testing.T) {
 	{
 		"apps": {
 		  "http": {
+			"grace_period": 1,
 			"servers": {
 			  "srv0": {
 				"listen": [
@@ -50,6 +51,7 @@ func TestSRVWithDial(t *testing.T) {
 	{
 		"apps": {
 		  "http": {
+			"grace_period": 1,
 			"servers": {
 			  "srv0": {
 				"listen": [
@@ -115,6 +117,7 @@ func TestDialWithPlaceholderUnix(t *testing.T) {
 	{
 		"apps": {
 		  "http": {
+			"grace_period": 1,
 			"servers": {
 			  "srv0": {
 				"listen": [
@@ -156,6 +159,7 @@ func TestReverseProxyWithPlaceholderDialAddress(t *testing.T) {
 	{
 		"apps": {
 			"http": {
+				"grace_period": 1,
 				"servers": {
 					"srv0": {
 						"listen": [
@@ -239,6 +243,7 @@ func TestReverseProxyWithPlaceholderTCPDialAddress(t *testing.T) {
 	{
 		"apps": {
 			"http": {
+				"grace_period": 1,
 				"servers": {
 					"srv0": {
 						"listen": [
@@ -321,6 +326,7 @@ func TestSRVWithActiveHealthcheck(t *testing.T) {
 	{
 		"apps": {
 		  "http": {
+			"grace_period": 1,
 			"servers": {
 			  "srv0": {
 				"listen": [
diff --git a/caddytest/integration/sni_test.go b/caddytest/integration/sni_test.go
index 813e9a05..aa47c75e 100644
--- a/caddytest/integration/sni_test.go
+++ b/caddytest/integration/sni_test.go
@@ -15,6 +15,7 @@ func TestDefaultSNI(t *testing.T) {
       "http": {
         "http_port": 9080,
         "https_port": 9443,
+        "grace_period": 1,
         "servers": {
           "srv0": {
             "listen": [
@@ -112,6 +113,7 @@ func TestDefaultSNIWithNamedHostAndExplicitIP(t *testing.T) {
       "http": {
         "http_port": 9080,
         "https_port": 9443,
+        "grace_period": 1,
         "servers": {
           "srv0": {
             "listen": [
@@ -212,6 +214,7 @@ func TestDefaultSNIWithPortMappingOnly(t *testing.T) {
       "http": {
         "http_port": 9080,
         "https_port": 9443,
+        "grace_period": 1,
         "servers": {
           "srv0": {
             "listen": [
diff --git a/caddytest/integration/stream_test.go b/caddytest/integration/stream_test.go
index cfd9d361..0cb1db2e 100644
--- a/caddytest/integration/stream_test.go
+++ b/caddytest/integration/stream_test.go
@@ -27,6 +27,7 @@ func TestH2ToH2CStream(t *testing.T) {
       "http": {
         "http_port": 9080,
         "https_port": 9443,
+		"grace_period": 1,
         "servers": {
           "srv0": {
             "listen": [
@@ -216,6 +217,7 @@ func TestH2ToH1ChunkedResponse(t *testing.T) {
     "http": {
       "http_port": 9080,
       "https_port": 9443,
+	  "grace_period": 1,
       "servers": {
         "srv0": {
           "listen": [