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

proxyprotocol: use github.com/pires/go-proxyproto #5915

Merged
merged 6 commits into from
Dec 13, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ require (
github.com/google/uuid v1.3.1
github.com/klauspost/compress v1.17.0
github.com/klauspost/cpuid/v2 v2.2.5
github.com/mastercactapus/proxyprotocol v0.0.4
github.com/mholt/acmez v1.2.0
github.com/prometheus/client_golang v1.15.1
github.com/quic-go/quic-go v0.40.0
Expand Down Expand Up @@ -117,6 +116,7 @@ require (
github.com/mitchellh/copystructure v1.2.0 // indirect
github.com/mitchellh/go-ps v1.0.0 // indirect
github.com/mitchellh/reflectwalk v1.0.2 // indirect
github.com/pires/go-proxyproto v0.7.0
github.com/pkg/errors v0.9.1 // indirect
github.com/prometheus/client_model v0.4.0 // indirect
github.com/prometheus/common v0.42.0 // indirect
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,6 @@ github.com/lyft/protoc-gen-validate v0.0.13/go.mod h1:XbGvPuh87YZc5TdIa2/I4pLk0Q
github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
github.com/manifoldco/promptui v0.9.0 h1:3V4HzJk1TtXW1MTZMP7mdlwbBpIinw3HztaIlYthEiA=
github.com/manifoldco/promptui v0.9.0/go.mod h1:ka04sppxSGFAtxX0qhlYQjISsg9mR4GWtQEhdbn6Pgg=
github.com/mastercactapus/proxyprotocol v0.0.4 h1:qSY75IZF30ZqIU9iW1ip3I7gTnm8wRAnGWqPxCBVgq0=
github.com/mastercactapus/proxyprotocol v0.0.4/go.mod h1:X8FRVEDZz9FkrIoL4QYTBF4Ka4ELwTv0sah0/5NxCPw=
github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
github.com/mattn/go-colorable v0.1.1/go.mod h1:FuOcm+DKB9mbwrcAfNl7/TZVBZ6rcnceauSikq3lYCQ=
github.com/mattn/go-colorable v0.1.6/go.mod h1:u6P/XSegPjTcexA+o6vUJrdnUu04hMope9wVRipJSqc=
Expand Down Expand Up @@ -433,6 +431,8 @@ github.com/performancecopilot/speed v3.0.0+incompatible/go.mod h1:/CLtqpZ5gBg1M9
github.com/peterbourgon/diskv/v3 v3.0.1 h1:x06SQA46+PKIUftmEujdwSEpIx8kR+M9eLYsUxeYveU=
github.com/pierrec/lz4 v1.0.2-0.20190131084431-473cd7ce01a1/go.mod h1:3/3N9NVKO0jef7pBehbT1qWhCMrIgbYNnFAZCqQ5LRc=
github.com/pierrec/lz4 v2.0.5+incompatible/go.mod h1:pdkljMzZIN41W+lC3N2tnIh5sFi+IEE17M5jbnwPHcY=
github.com/pires/go-proxyproto v0.7.0 h1:IukmRewDQFWC7kfnb66CSomk2q/seBuilHBYFwyq0Hs=
github.com/pires/go-proxyproto v0.7.0/go.mod h1:Vz/1JPY/OACxWGQNIRY2BeyDmpoaWmEP40O9LbuiFR4=
github.com/pkg/errors v0.8.0/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.8.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
Expand Down
68 changes: 52 additions & 16 deletions modules/caddyhttp/proxyprotocol/listenerwrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,10 @@
package proxyprotocol

import (
"fmt"
"net"
"time"

"github.com/mastercactapus/proxyprotocol"
goproxy "github.com/pires/go-proxyproto"

"github.com/caddyserver/caddy/v2"
)
Expand All @@ -38,32 +37,69 @@ type ListenerWrapper struct {
// Allow is an optional list of CIDR ranges to
// allow/require PROXY headers from.
Allow []string `json:"allow,omitempty"`
allow []*net.IPNet
mohammed90 marked this conversation as resolved.
Show resolved Hide resolved

rules []proxyprotocol.Rule
// Denby is an optional list of CIDR ranges to
// deny PROXY headers from.
Deny []string `json:"deny,omitempty"`
deny []*net.IPNet

// Accepted values are: ignore, use, reject, require, skip
// default: ignore
// Policy definitions are here: https://pkg.go.dev/github.com/pires/go-proxyproto@v0.7.0#Policy
FallbackPolicy Policy `json:"fallback_policy,omitempty"`

policy goproxy.PolicyFunc
}

// Provision sets up the listener wrapper.
func (pp *ListenerWrapper) Provision(ctx caddy.Context) error {
rules := make([]proxyprotocol.Rule, 0, len(pp.Allow))
for _, s := range pp.Allow {
_, n, err := net.ParseCIDR(s)
for _, cidr := range pp.Allow {
_, ipnet, err := net.ParseCIDR(cidr)
if err != nil {
return fmt.Errorf("invalid subnet '%s': %w", s, err)
return err
}
rules = append(rules, proxyprotocol.Rule{
Timeout: time.Duration(pp.Timeout),
Subnet: n,
})
pp.allow = append(pp.allow, ipnet)
}
for _, cidr := range pp.Deny {
_, ipnet, err := net.ParseCIDR(cidr)
if err != nil {
return err
}
pp.deny = append(pp.deny, ipnet)
}
pp.policy = func(upstream net.Addr) (goproxy.Policy, error) {
ret := pp.FallbackPolicy
host, _, err := net.SplitHostPort(upstream.String())
WeidiDeng marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return goproxy.REJECT, err
}
ip := net.ParseIP(host)
mohammed90 marked this conversation as resolved.
Show resolved Hide resolved
if ip == nil {
return goproxy.REJECT, err
}
for _, ipnet := range pp.deny {
if ipnet.Contains(ip) {
return goproxy.REJECT, nil
}
}
for _, ipnet := range pp.allow {
if ipnet.Contains(ip) {
ret = PolicyUSE
break
}
}
return policyToGoProxyPolicy[ret], nil
}

pp.rules = rules

return nil
}

// WrapListener adds PROXY protocol support to the listener.
func (pp *ListenerWrapper) WrapListener(l net.Listener) net.Listener {
pl := proxyprotocol.NewListener(l, time.Duration(pp.Timeout))
pl.SetFilter(pp.rules)
pl := &goproxy.Listener{
Listener: l,
ReadHeaderTimeout: time.Duration(pp.Timeout),
}
pl.Policy = pp.policy
return pl
}
14 changes: 13 additions & 1 deletion modules/caddyhttp/proxyprotocol/module.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ func (ListenerWrapper) CaddyModule() caddy.ModuleInfo {
// proxy_protocol {
// timeout <duration>
// allow <IPs...>
// deny <IPs...>
// fallback_policy <policy>
// }
func (w *ListenerWrapper) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
for d.Next() {
Expand All @@ -57,7 +59,17 @@ func (w *ListenerWrapper) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {

case "allow":
w.Allow = append(w.Allow, d.RemainingArgs()...)

case "deny":
w.Deny = append(w.Deny, d.RemainingArgs()...)
case "fallback_policy":
if !d.NextArg() {
return d.ArgErr()
}
p, err := parsePolicy(d.Val())
if err != nil {
return d.WrapErr(err)
}
w.FallbackPolicy = p
default:
return d.ArgErr()
}
Expand Down
82 changes: 82 additions & 0 deletions modules/caddyhttp/proxyprotocol/policy.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
package proxyprotocol

import (
"errors"
"fmt"
"strings"

goproxy "github.com/pires/go-proxyproto"
)

type Policy int

// as defined in: https://pkg.go.dev/github.com/pires/go-proxyproto@v0.7.0#Policy
const (
// IGNORE address from PROXY header, but accept connection
PolicyIGNORE Policy = iota
// USE address from PROXY header
PolicyUSE
Comment on lines +17 to +18
Copy link

@TimWolla TimWolla Feb 13, 2024

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.

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 is SKIP, 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.

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

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.

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.

My understanding is that the allow configuration option sets the USE policy for a given client as per

https://github.com/caddyserver/caddy/pull/5915/files#diff-39cd21a1e121836c0740aa16ed05549fb146c932b9c0f93f5409a6ccb902c0a9R92-R97

Now consider the following:

  1. I configure allow 10.10.10.10 to preserve IP addresses from my trusted gateway running at 10.10.10.10.
  2. This configures the 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.
  3. My trusted gateway at 10.10.10.10 does not add the PROXY header for whatever reason (bug, configuration mistake, ...).
  4. Now a malicious client could connect to my trusted gateway at 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.

Copy link
Member Author

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 and SKIP are blessed, good, and safe to keep
  • all the others are recommended to be removed and considered unsafe
    ?

Copy link

Choose a reason for hiding this comment

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

Summary

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.

I believe I summarized the whole discussion here: mholt/caddy-l4#176 (comment)

From what I can tell there was a miscommunication with the vulnerability concern raised.
At least with the present discussion, there is still no clear justification that a trust policy cannot apply to a host where the PROXY header is optional.

...projects that had flexible policies that conflicted with the specification (which appears acceptable to an extent from their response, thus the specification probably needs an update).

with additional clarification in a collapsed details block:

In the example we've been discussing, the concern appears more to do with the trusted host (Traefik) being hands-off with PROXY protocol, such that a malicious client can provide their own PROXY protocol header that Traefik simply forwards to Caddy that trusts all connections to the Caddy server port from Traefik.

  • Correct fix and prevention is to ensure Traefik does not allow untrusted clients to sneak in their own PROXY protocol header.
  • Configuration issue of the trusted host, nothing to do with Caddy allowing the PROXY header to be optional for a trusted host.

I then expand a bit on policy concerns, but wrap it up with:

If REQUIRE were a default, but the user wanted Caddy to have USE for supporting connections to Caddy that don't / can't provide the PROXY header, all that's required in this vulnerable Traefik configuration is to ensure it uses IGNORE for untrusted clients (I will raise a bug report as their docs don't communicate the default behaviour doesn't check for a PROXY header at all).

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.

  • Traefik will not apply any PROXY protocol inspection unless it's configured to do so (which was a concern that @TimWolla expressed when the inbound traffic may on the off-chance by coincidence have something that appears to be PROXY protocol header, thus Traefik avoids making this assumption by default).
  • When you have external traffic for Traefik to forward and the original IP needs to be kept due to some other load balancer / proxy in front of Traefik, then you need PROXY protocol at this Traefik entrypoint. This is separate from Traefik's routers that inject / prepend their own PROXY protocol (again when configured to do so), where traffic could be handed over to Caddy.

For clarity with the Traefik example:

  • Inbound connection (client to Traefik) => (is it a trusted proxy? apply a PROXY protocol policy) => Route to outbound
  • Outbound connection (Traefik to service, eg Caddy) => (if configured to, add a PROXY protocol header with the IP traefik associates to the client) => Traffic forwarded to service

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

all the others are recommended to be removed and considered unsafe

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.

Copy link

@TimWolla TimWolla May 27, 2024

Choose a reason for hiding this comment

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

@mohammed90

Is the summary:

REQUIRE and SKIP are blessed, good, and safe to keep
all the others are recommended to be removed and considered unsafe
?

  • REQUIRE is safe.
  • SKIP is safe.
  • IGNORE is unsafe.
  • USE is unsafe.
  • REJECT is unsafe.

@polarathene With all due respect, you are misrepresenting the situation and I won't go through this once more with you.

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).

Copy link

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 🙄

  • I just doubt anyone else cares enough to verify my demonstration of that (and I don't blame them),
  • It's not like it has to be a default policy though. Dropping support for these extra policies seems unnecessary when they have their uses.

you are misrepresenting the situation

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:

image

image

image

image

image


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 🤷‍♂️ :

image

That's it. That's what the big discussion was over

  • @TimWolla was focused on a concern that wasn't relevant in the configured scenario, for software he doesn't use or have familiarity with configuration wise, hence the misunderstanding / miscommunication. His concern is valid when applicable, but USE policy wasn't a vulnerability in the configuration I was seeking clarity on.
  • If you know what traffic is flowing inbound, You can choose to enforce PROXY protocol from trusted hosts, or discard / reject any connections that present a PROXY protocol header when one is not permitted.
  • Keep in mind this is an example where two reverse proxies supporting PROXY protocol are in use (Traefik handles the client connection, it has interacts with the PROXY protocol on both inbound and outbound connections, it is a trusted host to Caddy, but USE is valid for a trusted host as the client to Caddy when that trusted host is already sanitizing the real client connection)
  • In the case of Caddy, Traefik always adding a PROXY protocol header of it's own for Layer 4 traffic inbound to Caddy ensures that PROXY protocol is used correctly. If a 2nd header were even present (instead of stripped) the actual protocol negotiation such as TLS termination handled by Caddy would just fail instead.

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.

// REJECT connection when PROXY header is sent
// Note: even though the first read on the connection returns an error if
// a PROXY header is present, subsequent reads do not. It is the task of
// the code using the connection to handle that case properly.
PolicyREJECT
// REQUIRE connection to send PROXY header, reject if not present
// Note: even though the first read on the connection returns an error if
// a PROXY header is not present, subsequent reads do not. It is the task
// of the code using the connection to handle that case properly.
PolicyREQUIRE
// SKIP accepts a connection without requiring the PROXY header
// Note: an example usage can be found in the SkipProxyHeaderForCIDR
// function.
PolicySKIP
)

var policyToGoProxyPolicy = map[Policy]goproxy.Policy{
PolicyUSE: goproxy.USE,
PolicyIGNORE: goproxy.IGNORE,
PolicyREJECT: goproxy.REJECT,
PolicyREQUIRE: goproxy.REQUIRE,
PolicySKIP: goproxy.SKIP,
}

var policyMap = map[Policy]string{
PolicyUSE: "USE",
PolicyIGNORE: "IGNORE",
PolicyREJECT: "REJECT",
PolicyREQUIRE: "REQUIRE",
PolicySKIP: "SKIP",
}

var policyMapRev = map[string]Policy{
"USE": PolicyUSE,
"IGNORE": PolicyIGNORE,
"REJECT": PolicyREJECT,
"REQUIRE": PolicyREQUIRE,
"SKIP": PolicySKIP,
}

// MarshalText implements the text marshaller method.
WeidiDeng marked this conversation as resolved.
Show resolved Hide resolved
func (x Policy) MarshalText() ([]byte, error) {
return []byte(policyMap[x]), nil
}

// UnmarshalText implements the text unmarshaller method.
func (x *Policy) UnmarshalText(text []byte) error {
name := string(text)
tmp, err := parsePolicy(name)
if err != nil {
return err
}
*x = tmp
return nil
}

func parsePolicy(name string) (Policy, error) {
if x, ok := policyMapRev[strings.ToUpper(name)]; ok {
return x, nil
}
return Policy(0), fmt.Errorf("%s is %w", name, errInvalidPolicy)
}

var errInvalidPolicy = errors.New("invalid policy")
44 changes: 21 additions & 23 deletions modules/caddyhttp/reverseproxy/httptransport.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import (
"strings"
"time"

"github.com/mastercactapus/proxyprotocol"
"github.com/pires/go-proxyproto"
"go.uber.org/zap"
"golang.org/x/net/http2"

Expand Down Expand Up @@ -207,44 +207,42 @@ func (h *HTTPTransport) NewTransport(caddyCtx caddy.Context) (*http.Transport, e
if !ok {
return nil, fmt.Errorf("failed to get proxy protocol info from context")
}

// The src and dst have to be of the some address family. As we don't know the original
// dst address (it's kind of impossible to know) and this address is generelly of very
header := proxyproto.Header{
SourceAddr: &net.TCPAddr{
IP: proxyProtocolInfo.AddrPort.Addr().AsSlice(),
Port: int(proxyProtocolInfo.AddrPort.Port()),
Zone: proxyProtocolInfo.AddrPort.Addr().Zone(),
},
}
// The src and dst have to be of the same address family. As we don't know the original
// dst address (it's kind of impossible to know) and this address is generally of very
// little interest, we just set it to all zeros.
var destIP net.IP
switch {
case proxyProtocolInfo.AddrPort.Addr().Is4():
destIP = net.IPv4zero
header.TransportProtocol = proxyproto.TCPv4
header.DestinationAddr = &net.TCPAddr{
IP: net.IPv4zero,
}
case proxyProtocolInfo.AddrPort.Addr().Is6():
destIP = net.IPv6zero
header.TransportProtocol = proxyproto.TCPv6
header.DestinationAddr = &net.TCPAddr{
IP: net.IPv6zero,
}
default:
return nil, fmt.Errorf("unexpected remote addr type in proxy protocol info")
}

// TODO: We should probably migrate away from net.IP to use netip.Addr,
// but due to the upstream dependency, we can't do that yet.
switch h.ProxyProtocol {
case "v1":
header := proxyprotocol.HeaderV1{
SrcIP: net.IP(proxyProtocolInfo.AddrPort.Addr().AsSlice()),
SrcPort: int(proxyProtocolInfo.AddrPort.Port()),
DestIP: destIP,
DestPort: 0,
}
header.Version = 1
caddyCtx.Logger().Debug("sending proxy protocol header v1", zap.Any("header", header))
_, err = header.WriteTo(conn)
case "v2":
header := proxyprotocol.HeaderV2{
Command: proxyprotocol.CmdProxy,
Src: &net.TCPAddr{IP: net.IP(proxyProtocolInfo.AddrPort.Addr().AsSlice()), Port: int(proxyProtocolInfo.AddrPort.Port())},
Dest: &net.TCPAddr{IP: destIP, Port: 0},
}
header.Version = 2
caddyCtx.Logger().Debug("sending proxy protocol header v2", zap.Any("header", header))
_, err = header.WriteTo(conn)
default:
return nil, fmt.Errorf("unexpected proxy protocol version")
}

_, err = header.WriteTo(conn)
if err != nil {
// identify this error as one that occurred during
// dialing, which can be important when trying to
Expand Down