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.

Sign up for GitHub

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: Bring enforce_origin and origins to admin config #3595

Merged
merged 6 commits into from
Aug 3, 2020
Merged

httpcaddyfile: Bring enforce_origin and origins to admin config #3595

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dd1190f
Bring `ensure_origin` and `origins` to caddyfile admin config
Vigilans Jul 31, 2020
f51a2dc
Add unit test for caddyfile admin config update
Vigilans Jul 31, 2020
2d23e35
Add caddyfile adapt test for typical admin setup
Vigilans Jul 31, 2020
158a2f3
httpcaddyfile: Replace admin config error message when there's more a…
Vigilans Aug 1, 2020
7ab8ff6
Merge branch 'master' into vigilans/caddyfile-admin
Vigilans Aug 1, 2020
047cc26
Merge branch 'master' into vigilans/caddyfile-admin
Vigilans Aug 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions caddyconfig/httpcaddyfile/httptype.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,12 +261,8 @@ func (st ServerType) Setup(inputServerBlocks []caddyfile.ServerBlock,
storageCvtr.(caddy.Module).CaddyModule().ID.Name(),
&warnings)
}
if adminConfig, ok := options["admin"].(string); ok && adminConfig != "" {
if adminConfig == "off" {
cfg.Admin = &caddy.AdminConfig{Disabled: true}
} else {
cfg.Admin = &caddy.AdminConfig{Listen: adminConfig}
}
if adminConfig, ok := options["admin"].(*caddy.AdminConfig); ok && adminConfig != nil {
cfg.Admin = adminConfig
}
if len(customLogs) > 0 {
if cfg.Logging == nil {
Expand Down
52 changes: 52 additions & 0 deletions caddyconfig/httpcaddyfile/httptype_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,58 @@ func TestGlobalOptions(t *testing.T) {
expectWarn: false,
expectError: true,
},
{
input: `
{
admin {
enforce_origin
origins 192.168.1.1:2020 127.0.0.1:2020
}
}
:80
`,
expectWarn: false,
expectError: false,
},
{
input: `
{
admin 127.0.0.1:2020 {
enforce_origin
origins 192.168.1.1:2020 127.0.0.1:2020
}
}
:80
`,
expectWarn: false,
expectError: false,
},
{
input: `
{
admin 192.168.1.1:2020 127.0.0.1:2020 {
enforce_origin
origins 192.168.1.1:2020 127.0.0.1:2020
}
}
:80
`,
expectWarn: false,
expectError: true,
},
{
input: `
{
admin off {
enforce_origin
origins 192.168.1.1:2020 127.0.0.1:2020
}
}
:80
`,
expectWarn: false,
expectError: true,
},
} {

adapter := caddyfile.Adapter{
Expand Down
38 changes: 30 additions & 8 deletions caddyconfig/httpcaddyfile/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,17 +223,39 @@ func parseOptSingleString(d *caddyfile.Dispenser) (interface{}, error) {
}

func parseOptAdmin(d *caddyfile.Dispenser) (interface{}, error) {
if d.Next() {
var listenAddress string
if !d.AllArgs(&listenAddress) {
return "", d.ArgErr()
adminCfg := new(caddy.AdminConfig)
for d.Next() {
if d.NextArg() {
listenAddress := d.Val()
if listenAddress == "off" {
adminCfg.Disabled = true
if d.Next() { // Do not accept any remaining options including block
return nil, d.Err("No more option is allowed after turning off admin config")
}
mholt marked this conversation as resolved.
Show resolved Hide resolved
} else {
adminCfg.Listen = listenAddress
if d.NextArg() { // At most 1 arg is allowed
return nil, d.Err("At most 1 argument is allowed in admin config")
Vigilans marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
if listenAddress == "" {
listenAddress = caddy.DefaultAdminListen
for nesting := d.Nesting(); d.NextBlock(nesting); {
switch d.Val() {
case "enforce_origin":
adminCfg.EnforceOrigin = true

case "origins":
adminCfg.Origins = d.RemainingArgs()

default:
return nil, d.Errf("unrecognized parameter '%s'", d.Val())
mholt marked this conversation as resolved.
Show resolved Hide resolved
}
}
return listenAddress, nil
}
return "", nil
if adminCfg.Listen == "" && !adminCfg.Disabled {
adminCfg.Listen = caddy.DefaultAdminListen
}
return adminCfg, nil
}

func parseOptOnDemand(d *caddyfile.Dispenser) (interface{}, error) {
Expand Down
80 changes: 80 additions & 0 deletions caddytest/integration/caddyfile_adapt/global_options_admin.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
{
debug
http_port 8080
https_port 8443
default_sni localhost
order root first
storage file_system {
root /data
}
acme_ca https://example.com
acme_ca_root /path/to/ca.crt

email [email protected]
admin {
origins localhost:2019 [::1]:2019 127.0.0.1:2019 192.168.10.128
}
Comment on lines +14 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This admin config typically does:

  • Listen to default address localhost:2019 (address argument left empty in the admin <address> | off { ... })
  • Disable ensure_origin (not set in the caddyfile)
  • Allow outbound IP with default HTTP(s) port as origin (192.168.10.128 without port), in ordered to be called remotely via reverse_proxy.

Copy link
Member

Choose a reason for hiding this comment

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

Allow outbound IP with default HTTP(s) port as origin (192.168.10.128 without port), in ordered to be called remotely via reverse_proxy.

I'm not sure I understand this part...

FYI reverse proxying to the admin endpoint within the same Caddy instance/config will not work (because of a chicken-egg problem). I am not sure if that is what you are referring to, but just wanted to clarify that.

Copy link
Contributor Author

@Vigilans Vigilans Aug 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not sure what kind of scenario you are referring to, may I just clarify my usecase here:

  1. I have established a Virtual LAN using ZeroTierOne with all trusted devices joined, and the IP of my caddy VPS is 10.147.17.12.
  2. I started an https server on 10.147.17.12, with admin endpoint reverse proxied on path /api/caddy, so only trusted devices can get to the admin endpoint of my caddy server, and the data is encrypted in the public network. The config is as follows: 
    https://10.147.17.12 {
  route /api/caddy/* {
    uri strip_prefix /api/caddy
    reverse_proxy http://localhost:2019
  }
}
  3. I tried to GET the caddy config through the private VLAN IP, but host not allowed reported: 
    $ curl -k https://10.147.17.12/api/caddy/config/
{"error":"host not allowed: 10.147.17.12"}
  4. I added the 10.147.17.12 to the admin config: 
    {
  admin {
    origins localhost:2019 [::1]:2019 127.0.0.1:2019 10.147.17.12
  }
}
    And now I can get the caddy config from remote machine: 
    $ curl -k https://10.147.17.12/api/caddy/config/
{"admin":{"listen":"localhost:2019","origins":["localhost:2019","[::1]:2019","127.0.0.1:2019","10.147.17.12"]}, "apps": ... }
    NOTE: If I replace 10.147.17.12 with 10.147.17.12:443 in origins field, I cannot get the config whether I specified port in client request or not: 
    $ curl -k https://10.147.17.12/api/caddy/config/
{"error":"host not allowed: 10.147.17.12"}
$ curl -k https://10.147.17.12:443/api/caddy/config/
{"error":"host not allowed: 10.147.17.12"}
    I cannot accurately interpret this behavior, and I think maybe caddy automatically removed the 443 port and pass 10.147.17.12 as host for checking, so only 10.147.17.12 without specifying port with work.

Copy link
Member

@mholt mholt Aug 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for explaining!

`reverse_proxy http://localhost:2019`

This will be problematic. Config changes through this proxy will wait for the admin endpoint to return, obviously, but the admin endpoint will be waiting for the reverse proxy to return, for a graceful reload. So it's a chicken-and-egg problem that I don't know how to solve yet.

Unrelated to this PR, but I thought you should know.

but host not allowed reported:

Correct; alternatively, you can have the proxy change the Host and Origin headers as they go upstream to the backend.

NOTE: If I replace 10.147.17.12 with 10.147.17.12:443 in origins field, I cannot get the config whether I specified port in client request or not:
...
I cannot accurately interpret this behavior, and I think maybe caddy automatically removed the 443 port and pass 10.147.17.12 as host for checking, so only 10.147.17.12 without specifying port with work.

Caddy is not removing the port. The Host header won't usually include a port for standard ports (that is up to the HTTP client). In this case, curl does not append :443 to the header. Caddy does not change them.

on_demand_tls {
ask https://example.com
interval 30s
burst 20
}
local_certs
key_type ed25519
}

:80
----------
{
"admin": {
"listen": "localhost:2019",
"origins": [
"localhost:2019",
"[::1]:2019",
"127.0.0.1:2019",
"192.168.10.128"
]
},
"logging": {
"logs": {
"default": {
"level": "DEBUG"
}
}
},
"storage": {
"module": "file_system",
"root": "/data"
},
"apps": {
"http": {
"http_port": 8080,
"https_port": 8443,
"servers": {
"srv0": {
"listen": [
":80"
]
}
}
},
"tls": {
"automation": {
"policies": [
{
"issuer": {
"module": "internal"
}
}
],
"on_demand": {
"rate_limit": {
"interval": 30000000000,
"burst": 20
},
"ask": "https://example.com"
}
}
}
}
}