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

v2 - add forward-related headers by default #3275

Closed
bcmedeiros opened this issue Apr 17, 2020 · 11 comments · Fixed by #3276
Closed

v2 - add forward-related headers by default #3275

bcmedeiros opened this issue Apr 17, 2020 · 11 comments · Fixed by #3276
Labels
feature ⚙️ New feature or request
Milestone

Comments

@bcmedeiros
Copy link

bcmedeiros commented Apr 17, 2020

I just found out Caddy, firstly I'd like to say it looks amazing, new reverse-proxy mode seems so handy, even certificates are automatically generated S2

I just missed one thing; I'm running this command:

caddy reverse-proxy --from https://public.host:443 --to http://localhost:8081

And my upstream server expects some headers to know the request is actually being received through a proxy. It ends up generating a URL to redirect, and it uses http://public.host, instead of https://public.host, which breaks my app.

Any chances of getting this working?

@mholt
Copy link
Member

mholt commented Apr 17, 2020

Can you be more specific what is needed? What are the headers that the backend is getting, and what are the headers it requires? By default, all the incoming headers are simply forwarded to the backend (but X-Forwarded-For is added implicitly already).

@mholt mholt added the needs info 📭 Requires more information label Apr 17, 2020
@bcmedeiros
Copy link
Author

bcmedeiros commented Apr 17, 2020

@mholt, thanks for checking that!

I couldn't debug it yet, but based on the other reverse proxies that I use successfully, and also the fact that the only part that is wrong is the protocol, I'm pretty sure the one that is missing is the X-Forwarded-Proto.

@bcmedeiros
Copy link
Author

bcmedeiros commented Apr 17, 2020

Sorry for the double post, but I believe that when the --from has https and --to has http protocols, X-Forwarded-Proto could be included by default.

@mholt
Copy link
Member

mholt commented Apr 17, 2020

We could do that. That seems like a good idea, but I'd probably just have it be set regardless of To/From protocols. I'll commit that in a moment. Edit: Actually can you please confirm that is the missing header first? You can use a Caddyfile to set the header, I think your total Caddyfile would be like 3 lines:

public.host

reverse_proxy http://localhost:8081 {
    header_up X-Forwarded-Proto https
}

and see what happens.

@bcmedeiros
Copy link
Author

@mholt I can confirm here, the above config works for my upstream server!

We could do that. That seems like a good idea, but I'd probably just have it be set regardless of To/From protocols.

Makes sense... I can't think of any side-effects, besides sending a (non-standard?) headers when not needed.

As a side note, I'm a complete newbie with Caddy, so I first tried Desktop/caddy_2.0.0-rc.3_mac_amd64/caddy run config.caddy, with config.caddy being a file with the content you suggested. Caddy started in a very weird way doing it, but no warnings about my command not making sense were issued. Trying with Desktop/caddy_2.0.0-rc.3_mac_amd64/caddy run and having a Caddyfille in the current folder started properly.

Again, great work with Caddy! I never thought I could find a reverse proxy with SSL support that would be so easy to run!

@francislavoie
Copy link
Member

francislavoie commented Apr 17, 2020

As a side note, I'm a complete newbie with Caddy, so I first tried Desktop/caddy_2.0.0-rc.3_mac_amd64/caddy run config.caddy, with config.caddy being a file with the content you suggested. Caddy started in a very weird way doing it, but no warnings about my command not making sense were issued.

See the CLI docs here:

https://caddyserver.com/docs/command-line#caddy-run

You need to specify --config <file> and if your config filename doesn't either start with Caddyfile or end with .json, you need to specify --adapter <name> as well (i.e. --adapter caddyfile or --adapter json depending on the type of config you're trying to load). If no --config is passed, it tries to load a file named Caddyfile from your current working directory, otherwise it fails.

@mholt mholt added feature ⚙️ New feature or request and removed needs info 📭 Requires more information labels Apr 17, 2020
@mholt mholt added this to the v2.0.0 milestone Apr 17, 2020
@bcmedeiros
Copy link
Author

bcmedeiros commented Apr 17, 2020

You need to specify --config <file> and if your config filename doesn't either start with Caddyfile or end with .json, you need to specify --adapter <name> as well (i.e. --adapter caddyfile or --adapter json depending on the type of config you're trying to load). If no --config is passed, it tries to load a file named Caddyfile from your current working directory, otherwise it fails.

Makes sense! Thanks for explaining that.

My weird command didn't fail, though, that's what happened:

brunojcm-macbook:~ bruno.medeiros$ Desktop/caddy_2.0.0-rc.3_mac_amd64/caddy run config.caddy
2020/04/17 14:38:51.571	INFO	admin	admin endpoint started	{"address": "tcp/localhost:2019", "enforce_origin": false, "origins": ["localhost:2019", "[::1]:2019", "127.0.0.1:2019"]}
2020/04/17 14:38:51.571	INFO	serving initial configuration

The server appeared to have started normally, but my configuration was ignored (as expected, but I couldn't understand that by the output).

Let me know if that was unexpected and you want me to raise an issue for that.

@mholt
Copy link
Member

mholt commented Apr 17, 2020

@brunojcm I've pushed a potential enhancement to #3276. Could you please give it a try? I think this can and should go into 2.0 if you can try it today that would be great :)

@mholt
Copy link
Member

mholt commented Apr 17, 2020

Regarding the config file:

run config.caddy

I guess that second argument is just being ignored. It's supposed to be --config config.caddy, but we aren't being very strict about unflagged arguments.

@bcmedeiros
Copy link
Author

@brunojcm I've pushed a potential enhancement to #3276. Could you please give it a try? I think this can and should go into 2.0 if you can try it today that would be great :)

Done, results posted on the PR.

I guess that second argument is just being ignored. It's supposed to be --config config.caddy, but we aren't being very strict about unflagged arguments.

I'd say that might trick some stupid people that don't read the docs carrefully (like me) trying to use custom config files. I'd tend to say that if the parameter is not expected, the command should be rejected. Sorry, I'm going way off-topic here, this is just minor feedback. I'm just even daring to say it because you guys seems to strive to make the experience as simple and pleasant as possible, without having to read much docs, so that might be a way to improve it :D

@mholt
Copy link
Member

mholt commented Apr 17, 2020

Yeah, we'll work on that. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants