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: 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

merged 6 commits into from
Aug 3, 2020

Conversation

Vigilans
Copy link
Contributor

Currently, admin field in caddyfile's global option could only affects two fields in caddy.AdminConfig: Disabled and Listen. This PR makes modification of EnsureOrigin and Origins field available to caddyfile config.

@CLAassistant
Copy link

CLAassistant commented Jul 23, 2020

CLA assistant check
All committers have signed the CLA.

@Vigilans Vigilans changed the title Bring ensure_origin and origins to caddyfile admin config httpcaddyfile: Bring ensure_origin and origins to admin config Jul 23, 2020
@mholt mholt added the under review 🧐 Review is pending before merging label Jul 25, 2020
Copy link
Contributor Author

@Vigilans Vigilans left a comment

Choose a reason for hiding this comment

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

Have added an adapt test for admin configs referred from "Host not allowed" when calling the API remotely - Help - Caddy Community.

Comment on lines +14 to +16
admin {
origins localhost:2019 [::1]:2019 127.0.0.1:2019 192.168.10.128
}
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

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

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.

caddyconfig/httpcaddyfile/options.go Show resolved Hide resolved
caddyconfig/httpcaddyfile/options.go Outdated Show resolved Hide resolved
caddyconfig/httpcaddyfile/options.go Show resolved Hide resolved
@mholt mholt changed the title httpcaddyfile: Bring ensure_origin and origins to admin config httpcaddyfile: Bring enforce_origin and origins to admin config Jul 31, 2020
mholt
mholt previously approved these changes Jul 31, 2020
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.

This looks like a great change, thank you! The tests look good too, we appreciate that.

I'm willing to accept it as-is but I left a couple comments you might want to look over before we merge it, in case you want to change anything.

caddyconfig/httpcaddyfile/options.go Outdated Show resolved Hide resolved
Comment on lines +14 to +16
admin {
origins localhost:2019 [::1]:2019 127.0.0.1:2019 192.168.10.128
}
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.

@mholt mholt added this to the 2.x milestone Jul 31, 2020
…rguments than needed


Replace d.Err() to d.ArgErr() since the latter provides similarly informative error message

Co-authored-by: Matt Holt <[email protected]>
@mholt mholt removed the under review 🧐 Review is pending before merging label Aug 1, 2020
@mholt mholt merged commit 8b80a32 into caddyserver:master Aug 3, 2020
@mholt
Copy link
Member

mholt commented Aug 3, 2020

Thank you very much for the great contribution! It'll go out with 2.2 beta soon.

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