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: Only append TLS conn policy if it's non-empty #3319

Merged
merged 1 commit into from
May 5, 2020

Conversation

mholt
Copy link
Member

@mholt mholt commented Apr 27, 2020

This can lead to nicer, smaller JSON output for Caddyfiles like this:

a {
	tls internal
}
b {
	tls foo@bar.com
}

i.e. where the tls directive only configures automation policies, and
is merely meant to enable TLS on a server block (if it wasn't implied).
This helps keeps implicit config implicit.

Needs a little more testing to ensure it doesn't break anything
important.

Diff: https://www.diffchecker.com/U9uyyjkT

JSON before:

{
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":443"
          ],
          "routes": [
            {
              "match": [
                {
                  "host": [
                    "a"
                  ]
                }
              ],
              "terminal": true
            },
            {
              "match": [
                {
                  "host": [
                    "b"
                  ]
                }
              ],
              "terminal": true
            }
          ],
          "tls_connection_policies": [
            {
              "match": {
                "sni": [
                  "a"
                ]
              }
            },
            {
              "match": {
                "sni": [
                  "b"
                ]
              }
            },
            {}
          ]
        }
      }
    },
    "tls": {
      "automation": {
        "policies": [
          {
            "subjects": [
              "a"
            ],
            "issuer": {
              "module": "internal"
            }
          },
          {
            "subjects": [
              "b"
            ],
            "issuer": {
              "email": "foo@bar.com",
              "module": "acme"
            }
          }
        ]
      }
    }
  }
}

JSON after:

{
  "apps": {
    "http": {
      "servers": {
        "srv0": {
          "listen": [
            ":443"
          ],
          "routes": [
            {
              "match": [
                {
                  "host": [
                    "a"
                  ]
                }
              ],
              "terminal": true
            },
            {
              "match": [
                {
                  "host": [
                    "b"
                  ]
                }
              ],
              "terminal": true
            }
          ]
        }
      }
    },
    "tls": {
      "automation": {
        "policies": [
          {
            "subjects": [
              "a"
            ],
            "issuer": {
              "module": "internal"
            }
          },
          {
            "subjects": [
              "b"
            ],
            "issuer": {
              "email": "foo@bar.com",
              "module": "acme"
            }
          }
        ]
      }
    }
  }
}

17 line savings in this case.

This can lead to nicer, smaller JSON output for Caddyfiles like this:

	a {
		tls internal
	}
	b {
		tls foo@bar.com
	}

i.e. where the tls directive only configures automation policies, and
is merely meant to enable TLS on a server block (if it wasn't implied).
This helps keeps implicit config implicit.

Needs a little more testing to ensure it doesn't break anything
important.
@mholt mholt added the under review 🧐 Review is pending before merging label Apr 27, 2020
@mholt mholt added this to the 2.1 milestone Apr 27, 2020
@francislavoie
Copy link
Member

Is this to fix the {} thing? Would be helpful to see the JSON output before/after if you have a minute 😅

@mholt
Copy link
Member Author

mholt commented Apr 27, 2020

@francislavoie Not exactly, no (that empty policy is definitely there for a reason, but I think there are some cases where we can remove it). I updated my post with the before/after.

@mholt
Copy link
Member Author

mholt commented Apr 27, 2020

@henrocker Would you have a chance to try this PR soon, perchance? (Build artifacts are available for download.) Just want to make sure we don't repeat #3249. I don't think we will but then again my mind is mush today.

And then after that, could you also help us make sure things are working by testing then deploying the latest HEAD on master with your sites? We're getting ready to tag 2.0 next week.

@high3eam
Copy link

high3eam commented Apr 28, 2020

UPDATE: Same result with HEAD on master :-)

@mholt I've just deployed https://github.com/caddyserver/caddy/suites/633332206/artifacts/5178946 and it is working as expected for all of my sites! PHP, as well as static sites work fine, and customized cert path sites in combination with LE cert sites work, too 👍

Caddyfile: https://gist.github.com/Henrocker/b8abff26447e2c65fadb66bdca6e949c

Logs:

/etc/caddy/caddy_rc3_test run --config /etc/caddy/Caddyfile --adapter caddyfile
2020/04/28 06:59:18.451 INFO    using provided configuration    {"config_file": "/etc/caddy/Caddyfile", "config_adapter": "caddyfile"}
2020/04/28 06:59:18.471 INFO    admin   admin endpoint started  {"address": "tcp/localhost:2019", "enforce_origin": false, "origins": ["127.0.0.1:2019", "localhost:2019", "[::1]:2019"]}
2020/04/28 08:59:18 [INFO][cache:0xc000a0ec30] Started certificate maintenance routine
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "ytmp3.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "mail.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "skip.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "www.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "proxy.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "fotos.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "zeit.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "dns.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "ip.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "mta-sts.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "draw.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "frequencies.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "postfix.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "t-bot.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    skipping automatic certificate management because one or more matching certificates are already loaded  {"domain": "sql.hnrk.io", "server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    enabling automatic HTTP->HTTPS redirects        {"server_name": "srv1"}
2020/04/28 06:59:18.482 INFO    http    server is listening only on the HTTP port, so no automatic HTTPS will be applied to this server {"server_name": "srv0", "http_port": 80}
2020/04/28 06:59:18.482 WARN    http    user server is listening on same interface as automatic HTTP->HTTPS redirects; user-configured routes might override these redirects    {"server_name": "srv0", "interface": "tcp/:80"}
2020/04/28 06:59:18.496 INFO    tls     cleaned up storage units
2020/04/28 06:59:18.496 INFO    http    enabling experimental HTTP/3 listener   {"addr": ":443"}
2020/04/28 06:59:18.496 INFO    http    enabling automatic TLS certificate management   {"domains": ["die-reikiquellen.de", "www.harmoniks.de", "www.die-reikiquellen.de", "harmoniks.de", "www.weather-frogs.de", "weather-frogs.de"]}
2020/04/28 06:59:18.511 INFO    autosaved config        {"file": "/root/.config/caddy/autosave.json"}
2020/04/28 06:59:18.511 INFO    serving initial configuration

@mholt
Copy link
Member Author

mholt commented Apr 28, 2020

@henrocker Awesome, thank you very much for trying both! I feel a little more confident now, thanks to your help. :)

@mholt mholt removed the under review 🧐 Review is pending before merging label May 5, 2020
@mholt mholt merged commit 2f59467 into master May 5, 2020
@mholt mholt deleted the tls-conn-policies branch May 5, 2020 18:37
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

Successfully merging this pull request may close these issues.

3 participants