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

httpcaddyfile: Add auto_https global option #3284

Merged

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Apr 20, 2020

Fixes #3219

Adds a new global option to the Caddyfile to configure Automatic HTTPS:

{
	auto_https [off|disable_redirects]
}

Disabling redirects from Caddyfile:

{
	auto_https disable_redirects
}

localhost

JSON:

{
	"apps": {
		"http": {
			"servers": {
				"srv0": {
					"listen": [
						":443"
					],
					"routes": [
						{
							"match": [
								{
									"host": [
										"localhost"
									]
								}
							],
							"terminal": true
						}
					],
					"automatic_https": {
						"disable_redirects": true
					}
				}
			}
		}
	}
}

Disabling Auto HTTPS completely from Caddyfile:

{
	auto_https off
}

localhost

JSON:

{
	"apps": {
		"http": {
			"servers": {
				"srv0": {
					"listen": [
						":443"
					],
					"routes": [
						{
							"match": [
								{
									"host": [
										"localhost"
									]
								}
							],
							"terminal": true
						}
					],
					"tls_connection_policies": [
						{}
					],
					"automatic_https": {
						"disable": true
					}
				}
			}
		}
	}
}

@francislavoie francislavoie added this to the 2.1 milestone Apr 20, 2020
@francislavoie francislavoie force-pushed the caddyfile-autohttps-disable-redir branch from bafaefa to 6d28332 Compare April 20, 2020 15:21
@mholt
Copy link
Member

mholt commented Apr 20, 2020

@francislavoie Thanks for working on this!!

The extra TLS conn policy is intentional as you discovered, but in this case I think it's because we don't look inside to see that the only connection policy is empty except for the ServerName filter. It's not really something I want to change right now but if we find out that it's safe to take out the extra policy, we can do so. (The extra one in this case might actually be the first policy, not the empty second one.) I might be wrong, but I think that logic of adding the empty one at the end is mostly for cases where the non-empty one actually sets something about the TLS connection that is different from the default.

Anyway, I'll look at this more after 2.0, but one thing I'm not sure about already is whether HTTP redirects belong being configured in the tls directive.

In general, I suppose people would rather disable them because they don't have permission to bind to the HTTP port, or don't want an extra socket to be bound in the first place. For that case, a global option seems to be a better fit. I haven't yet seen a use case where people want to disable it per-site in a way where they can't use another server block (i.e. http://example.com) for that to override the implicit redirects.

@mholt mholt added the under review 🧐 Review is pending before merging label Apr 20, 2020
@francislavoie
Copy link
Member Author

IMO tls and automatic_https feel like inextricably linked concepts, I don't think it's much of a stretch to put it there.

I do concede that it's not ideal that to set disable_redirects it adds a "tls_connection_policies" item to the JSON that otherwise wouldn't be there.

I don't love the global option idea, it feels incongruent to the way site blocks work.

@mholt
Copy link
Member

mholt commented Apr 20, 2020

The tls directive should deal only with the transport layer, not the HTTP application. We cannot break that abstraction cleanly.

The HTTP->HTTPS redirects don't occur within the user's site block anyways, because they happen on a port different than what the user has configured in their Caddyfile. Global options were made for things that can't be cleanly configure inside site blocks. So a global option is a better solution because A) it's more useful for solving @sqs 's original request, and B) it changes something that doesn't occur in any of the site blocks in the Caddyfile.

@mholt
Copy link
Member

mholt commented Apr 20, 2020

Don't put a lot of work into overhauling this PR yet though -- it needs more careful thought and consideration I think.

@mholt
Copy link
Member

mholt commented May 18, 2020

As I fiddle with this more, I'm thinking this should be a global option on our first pass, at least, since the point of disabling auto-redirects is almost always to avoid even binding to the extra socket. With this implementation, you'd have to add this to all your server blocks.

I think I'm envisioning this instead:

{
    auto_https disable_redirects
    auto_https off
}

at least, for a first pass I think this would be useful. WDYT?

@francislavoie
Copy link
Member Author

Sounds okay to me.

I think there would still be value in running the routine to collect the domains for redirects even if disable_redirects is set, so that it can be populated to be used in a manner like in #3246.

Does that make sense to you?

@mholt
Copy link
Member

mholt commented May 18, 2020

I don't know yet - but that's totally separate, so I don't want to be concerned with it in this issue/PR. For now, a way to disable the redirects is enough.

@francislavoie
Copy link
Member Author

That's what I mean though, with disable_redirects it can just do the work up to the point where it would it to the config, then it just doesn't add it.

Anyways, okay I'll rework this PR soon.

@francislavoie francislavoie force-pushed the caddyfile-autohttps-disable-redir branch from 6d28332 to f949858 Compare May 18, 2020 20:51
@francislavoie francislavoie reopened this May 18, 2020
@francislavoie francislavoie changed the title httpcaddyfile: Add disable_redirects option to tls httpcaddyfile: Add auto_https global option May 18, 2020
@francislavoie francislavoie force-pushed the caddyfile-autohttps-disable-redir branch from f949858 to 8336003 Compare May 18, 2020 20:59
@francislavoie francislavoie force-pushed the caddyfile-autohttps-disable-redir branch from 8336003 to 23a501c Compare May 18, 2020 20:59
@francislavoie
Copy link
Member Author

@mholt okay this should do it 👍

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome, this is looking much better! Just one suggestion.

Comment on lines +357 to +365
if autoHTTPS != "on" {
srv.AutoHTTPS = new(caddyhttp.AutoHTTPSConfig)
if autoHTTPS == "off" {
srv.AutoHTTPS.Disabled = true
}
if autoHTTPS == "disable_redirects" {
srv.AutoHTTPS.DisableRedir = true
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Consider using switch here, and be sure to reject unrecognized values.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Caddyfile parse step already rejects other values, but sure, doublechecking aint bad.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I hadn't noticed that. I guess that should be fine then...

@mholt mholt merged commit fae0642 into caddyserver:master May 19, 2020
@mholt mholt removed the under review 🧐 Review is pending before merging label May 19, 2020
@francislavoie francislavoie deleted the caddyfile-autohttps-disable-redir branch May 19, 2020 23:00
@averri
Copy link

averri commented May 25, 2020

I have tried this Caddyfile:

{
  auto_https disable_redirects
}


:443 {
  tls /etc/caddy/fullchain.pem /etc/caddy/privkey.pem
}

# Other config

... and the following error occur:

{"level":"info","ts":1590408669.3928888,"msg":"using provided configuration","config_file":"/etc/caddy/Caddyfile","config_adapter":"caddyfile"}
run: adapting config using caddyfile: unrecognized parameter name: auto_https

What is the correct configuration to disable the redirect from port 80?

@francislavoie
Copy link
Member Author

@averri this was only recently merged. v2.0 doesn't have this feature. It'll come in v2.1

For now you can compile from source or use the latest build from CI.

@davidpanic
Copy link

Is there an option to disable it per subdomain yet? One of my clients does not support https, but on the same subdomain most others do. Therefore I would need to disable redirects for all my domains and subdomains and manually add them back for each one except that one subdomain which is kind of annoying.

@francislavoie
Copy link
Member Author

francislavoie commented Sep 22, 2020

@psrcek just set up an http://sub.example.com site block for that one subdomain to override the redirect.

Next time, please ask your usage questions on the Caddy community forums. We prefer to keep the GitHub issue board for bugs and feature requests. Don't forget to fill out the thread template so we can help you!

@solariz
Copy link

solariz commented Nov 8, 2020

@averri this was only recently merged. v2.0 doesn't have this feature. It'll come in v2.1

Is there any news? v2.2.1 still "unrecognized directive"

@francislavoie
Copy link
Member Author

@solariz Please ask your usage questions on the Caddy community forums. We prefer to keep the GitHub issue board for bugs and feature requests. Don't forget to fill out the thread template so we can help you!

@caddyserver caddyserver locked as resolved and limited conversation to collaborators Nov 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Disabling http-to-https redirect from Caddyfile
5 participants