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

Add remote IP to on_demand_tls ask endpoint HTTP headers #5955

Closed
RobertoNovelo opened this issue Nov 28, 2023 · 3 comments
Closed

Add remote IP to on_demand_tls ask endpoint HTTP headers #5955

RobertoNovelo opened this issue Nov 28, 2023 · 3 comments
Labels
declined 🚫 Not a fit for this project feature ⚙️ New feature or request

Comments

@RobertoNovelo
Copy link

When Caddy sends the web request to the ask endpoint for on_demand_tls, there is no (easy) way to obtain the remote IP triggering the on demand TLS logic.

My proposal is to add this (and any other relevant) information to the request HTTP headers by default, but maybe it could make sense to add a configuration option.

@francislavoie francislavoie added the feature ⚙️ New feature or request label Nov 28, 2023
@francislavoie
Copy link
Member

Matt and I discussed this internally last month, and I think the conclusion was that we should not include it in the actual ask request. Matt's opinion was that it's not appropriate to use the remote IP as a decision vector because it's unrelated to the task at hand.

Instead, we can log this info (at debug level) with a specific logger name (like tls.ask) and you can configure a logger which only includes those logs, and that can contain the remote IP. You can read/process those logs as needed to do rate limiting.

@mholt
Copy link
Member

mholt commented Nov 29, 2023

Yeah; thanks for the question, but I do think the client IP is irrelevant in the decision to obtain a certificate and will lead to a false sense of security. The remote IP can't always be trusted either. Ultimately I think it would be a false sense of security. Edit: Can you tell how tired I was when I wrote this.

I'm potentially willing to be convinced otherwise, but it'd have to be a very strong argument IMO.

Closing for now, but feel free to continue discussion and we can reopen if needed.

@mholt mholt closed this as not planned Won't fix, can't repro, duplicate, stale Nov 29, 2023
@mholt mholt added the declined 🚫 Not a fit for this project label Nov 29, 2023
@francislavoie
Copy link
Member

For ref #5940 is adding client_ip to the logs.

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

No branches or pull requests

3 participants