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

caddyhttp: Add server-level trusted_proxies config #5103

Merged
merged 1 commit into from
Jan 10, 2023

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Oct 1, 2022

Partial work for #4924 as per my plan in #4924 (comment)

We've had requests for defining trusted_proxies globally, so it doesn't need to be configured for each reverse_proxy. This does that.

But also, this opens the door for more places we can use trusted proxies, such as in #4924 which asks for getting the real client IP. We can do that by looking at the XFF header if the proxy is trusted. That part isn't done in this PR, edit: this is in #5104!

Also we might want to change how remote_ip forwarded works, possibly making it not trust forwarded by default (which would be a breaking change... but eh, it's known to be insecure right now). And maybe we should add client_ip to the logs. See #5104 for the logs part.

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.

Thanks - overall this LGTM, just a question about implementation. I still want to review this one more time before merging, even if no more changes are made. Not sure if it'll go into 2.6.2 or 2.6.3.

modules/caddyhttp/server.go Outdated Show resolved Hide resolved
@francislavoie
Copy link
Member Author

I'll get back to this soon. My priorities had been elsewhere for the past while.

@francislavoie
Copy link
Member Author

Alright, this is ready for another round of review. I'm pretty happy with this.

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 is looking great!

My only request is, can we use the existing Vars context value instead of creating a new one? Each time we create a new one, it makes writing mocks a little more tedious and error-prone.

@francislavoie
Copy link
Member Author

We are. That's where vars is first initialized 🤔 but maybe I don't understand what you mean.

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.

Wow you're right. I'm blind. LGTM! Thank you :)

@francislavoie francislavoie modified the milestones: v2.7.0, v2.6.3 Jan 10, 2023
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 this pull request may close these issues.

2 participants