Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
proxyprotocol: use github.com/pires/go-proxyproto #5915
proxyprotocol: use github.com/pires/go-proxyproto #5915
Changes from all commits
fbfcb35
fb6589f
67e567b
bc04e74
8facd51
3a63b42
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't do Caddy and I only do the bare minimum of Go. This came to my attention via haproxy/haproxy#2450 and if I understand the code correctly, the
USE
policy is a footgun will result in security vulnerabilities, as per my comment on the HAProxy issue. My understanding is that it uses the PROXY header if one is sent and does not use it, if it is missing, allowing a malicious client to add the PROXY header itself if a gateway in front of Caddy is trusted, but does not add the PROXY header for whatever reason.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also find
REJECT
somewhat questionable, because it effectively isSKIP
, but worse: It would reject a connection if the first few bytes of the connection happen to match a valid PROXY header by chance, instead of just passing it to the underlying protocol handler.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user knows their Caddy instance is accessible on public networks, they should configure
allow
to limit PROXY protocol to only those IP ranges.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're only concerned with HTTP and TLS in this case, so that's not a concern. We have a different implementation for https://github.com/mholt/caddy-l4 which allows any TCP/UDP traffic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that the
allow
configuration option sets theUSE
policy for a given client as perhttps://github.com/caddyserver/caddy/pull/5915/files#diff-39cd21a1e121836c0740aa16ed05549fb146c932b9c0f93f5409a6ccb902c0a9R92-R97
Now consider the following:
allow 10.10.10.10
to preserve IP addresses from my trusted gateway running at10.10.10.10
.USE
policy, which to my understanding takes the client information from the PROXY header if present and accepts the connection without doing anything special if the PROXY header is not present.10.10.10.10
does not add the PROXY header for whatever reason (bug, configuration mistake, ...).10.10.10.10
and prepend a valid PROXY header to their request. This PROXY header would be forwarded as-is to Caddy and then accepted.If instead the
REQUIRE
policy would be configured, I would notice at step (3), because all connections from my trusted gateway would be rejected as the PROXY header is missing, thus alerting my monitoring.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for coming late to this discussion. The long-winded discussion is getting me lost. It's also hard to follow as it's going on multiple links covering multiple related yet separate subjects and multiple technologies.
Is the summary:
REQUIRE
andSKIP
are blessed, good, and safe to keep?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
I believe I summarized the whole discussion here: mholt/caddy-l4#176 (comment)
with additional clarification in a collapsed details block:
I then expand a bit on policy concerns, but wrap it up with:
I don't think I got around to raising that bug report. I had been preparing one but my browser crashed.
Traefik example
This was months ago so I can't quite recall all the details and don't have time to refresh my memory 😓 (below is based off recall of past discussion, I may slip up)
The default with Traefik for Layer 4 traffic forwarding (the only concern here) is to blindly forward IIRC.
For clarity with the Traefik example:
It's similar for Caddy, but I use Traefik due to the Layer 4 support for both inbound and outbound, as that is where PROXY protocol applies. Layer 4 is a little bit muddy with Caddy IIRC which is presently more oriented around Layer 7 for the most part (unless using the L4 plugin).
Keep
USE
Disagree,
USE
is valid.TL;DR is a third-party that is misconfigured allows for an exploit of trust when you have a more relaxed policy. That's not a reason to remove support for
USE
.No opinion on the others as I was focused on a scenario with a trusted proxy (Traefik) that didn't forward PROXY protocol headers from it's own inbound connections (Traefik entrypoint), ensuring that PROXY protocol header was only provided to Caddy when appropriate (a connection that could not use a Layer 7 router).
If you're going to remove a policy because it's considered unsafe, please demonstrate an understanding of why it's unsafe. A firewall or anti-virus can be disabled and make a system vulnerable, that doesn't mean it shouldn't be possible to opt-out. AFAIK the whole debate was around a misconfigured deployment, misconfigurations causing vulnerabilities aren't surprising.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mohammed90
@polarathene With all due respect, you are misrepresenting the situation and I won't go through this once more with you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correction, looking at the code: IGNORE is also unsafe (it's a worse version of USE).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TL;DR:
USE
can be used securely 🙄If I had time to go through the discussion properly I would, but I believe I gave a decent reproduction example where that issue is closed with a summary and conclusion and a very verbose collapsed details block.
At a glance, here are some highlighted snippets for reference if anyone is interested:
It's easy to dismiss my input, but I have shown a scenario where
USE
is valid and useful, without the vulnerability concerns that @TimWolla raised 🤷♂️ :That's it. That's what the big discussion was over
USE
policy wasn't a vulnerability in the configuration I was seeking clarity on.USE
is valid for a trusted host as the client to Caddy when that trusted host is already sanitizing the real client connection)I wrote a small program to act as a malicious client that injects it's own PROXY protocol header for the TLS connections, it could not be used to exploit a vulnerability via the scenario I argued was valid for a flexible
USE
policy. It would work if I had a misconfiguration, but as stated that is not something that should be surprising. If you set trusted hosts to allow anyone, you're vulnerable too 🤷♂️I understand everyone is busy and my responses are frequently verbose. This topic was already concluded, the information is there for anyone that wants to spend time combing over it, I'm not interested in debating it all over again.