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

Fix: Add support for running CHProxy behind another proxy #225

Merged
32 changes: 32 additions & 0 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,9 @@ type Server struct {
// Optional metrics handler configuration
Metrics Metrics `yaml:"metrics,omitempty"`

// Optional Proxy configuration
Proxy Proxy `yaml:"proxy,omitempty"`

// Catches all undefined fields
XXX map[string]interface{} `yaml:",inline"`
}
Expand Down Expand Up @@ -307,6 +310,35 @@ func (c *Metrics) UnmarshalYAML(unmarshal func(interface{}) error) error {
return checkOverflow(c.XXX, "metrics")
}

type Proxy struct {
// Enable enables parsing proxy headers. In proxy mode, CHProxy will try to
// parse the X-Forwarded-For, X-Real-IP or Forwarded header to extract the IP. If an other header is configured
// in the proxy settings, CHProxy will use that header instead.
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO, you should add the fact that the first ip the list will be chosen

Enable bool `yaml:"enable,omitempty"`

// Header allows for configuring an alternative header to parse the remote IP from, e.g.
// CF-Connecting-IP. If this is set, Enable must be set to true otherwise this setting
// will be ignored.
Header string `yaml:"header,omitempty"`

// Catches all undefined fields and must be empty after parsing.
XXX map[string]interface{} `yaml:",inline"`
}

// UnmarshalYAML implements the yaml.Unmarshaler interface.
func (c *Proxy) UnmarshalYAML(unmarshal func(interface{}) error) error {
type plain Proxy
if err := unmarshal((*plain)(c)); err != nil {
return err
}

if !c.Enable && c.Header != "" {
return fmt.Errorf("`proxy_header` cannot be set without enabling proxy settings")
}

return checkOverflow(c.XXX, "proxy")
}

// Cluster describes CH cluster configuration
// The simplest configuration consists of:
//
Expand Down
12 changes: 12 additions & 0 deletions config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ var fullConfig = Config{
Metrics: Metrics{
NetworksOrGroups: []string{"office"},
},
Proxy: Proxy{
Enable: true,
Header: "CF-Connecting-IP",
},
},
LogDebug: true,

Expand Down Expand Up @@ -455,6 +459,11 @@ func TestBadConfig(t *testing.T) {
"testdata/bad.wildcarded_users.no.wildcard.yml",
"user name \"analyst_named\" marked 'is_wildcared' does not match 'prefix_*'",
},
{
"proxy header without enabling proxy settings",
"testdata/bad.proxy_settings.yml",
"`proxy_header` cannot be set without enabling proxy settings",
},
}

for _, tc := range testCases {
Expand Down Expand Up @@ -723,6 +732,9 @@ func TestConfigString(t *testing.T) {
metrics:
allowed_networks:
- office
proxy:
enable: true
header: CF-Connecting-IP
clusters:
- name: first cluster
scheme: http
Expand Down
15 changes: 15 additions & 0 deletions config/testdata/bad.proxy_settings.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
server:
http:
listen_addr: ":8080"
proxy:
header: CF-Connecting-IP

users:
- name: "dummy"
allowed_networks: ["1.2.3.4"]
to_cluster: "cluster"
to_user: "default"

clusters:
- name: "cluster"
nodes: ["127.0.1.1:8123"]
6 changes: 6 additions & 0 deletions config/testdata/full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,12 @@ server:
allowed_networks: ["office"]
namespace: ""

# Proxy settings enable parsing proxy headers in cases where
# CHProxy is run behind another proxy.
proxy:
enable: true
header: CF-Connecting-IP

# Configs for input users.
users:
# Name and password are used to authorize access via BasicAuth or
Expand Down
3 changes: 2 additions & 1 deletion config/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@ func (n Networks) Contains(addr string) bool {

h, _, err := net.SplitHostPort(addr)
if err != nil {
panic(fmt.Sprintf("BUG: unexpected error while parsing RemoteAddr: %s", err))
// If we only have an IP address. This happens when the proxy middleware is enabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the err comes due to other reason ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

you should remove the mention of middleware

h = addr
}

ip := net.ParseIP(h)
Expand Down
6 changes: 6 additions & 0 deletions docs/content/en/configuration/default.md
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,12 @@ server:
allowed_networks: ["office"]
namespace: ""

# Proxy settings enable parsing proxy headers in cases where
# CHProxy is run behind another proxy.
proxy:
enable: true
header: CF-Connecting-IP

# Configs for input users.
users:
# Name and password are used to authorize access via BasicAuth or
Expand Down
32 changes: 32 additions & 0 deletions docs/content/en/configuration/proxy.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

title: Network Proxy
category: Configuration
position: 207
---

By default, `Chproxy` extracts the HTTP requests remote address. `Chproxy` can be configured to run behind another network proxy as well, by including the following configuration:

```yml
server:
proxy:
enable: true
```

When `Chproxy` is run with proxy mode enabled, `Chproxy` will try to parse the following headers to extract the remote address:

- `X-Forwarded-For`
- `X-Real-IP`
- `Forwarded`

If multiple remote address are found `Chproxy` assumes the first IP address is the actual remote address. For example in the case where `X-Forwarded-For: 10.0.0.1, 10.3.2.1`, `Chproxy` assumes `10.0.0.1` is the correct address.

If you have a custom header that contains the real remote address, it is possible to configure `Chproxy` to parse that header instead of the common proxy headers:

```yml
server:
proxy:
enable: true
header: X-MyCustomHeader
```

`Chproxy` assumes the header contains the remote address and doesn't apply any parsing logic to extract the remote address from the header.
19 changes: 16 additions & 3 deletions main.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var (
allowedNetworksHTTP atomic.Value
allowedNetworksHTTPS atomic.Value
allowedNetworksMetrics atomic.Value
proxyHandler atomic.Value
)

func main() {
Expand Down Expand Up @@ -94,7 +95,7 @@ var autocertManager *autocert.Manager

func newAutocertManager(cfg config.Autocert) *autocert.Manager {
if len(cfg.CacheDir) > 0 {
if err := os.MkdirAll(cfg.CacheDir, 0700); err != nil {
if err := os.MkdirAll(cfg.CacheDir, 0o700); err != nil {
log.Fatalf("error while creating folder %q: %s", cfg.CacheDir, err)
}
}
Expand Down Expand Up @@ -133,7 +134,9 @@ func newListener(listenAddr string) net.Listener {

func serveTLS(cfg config.HTTPS) {
ln := newListener(cfg.ListenAddr)

h := http.HandlerFunc(serveHTTP)

tlsCfg := newTLSConfig(cfg)
tln := tls.NewListener(ln, tlsCfg)
log.Infof("Serving https on %q", cfg.ListenAddr)
Expand All @@ -145,6 +148,7 @@ func serveTLS(cfg config.HTTPS) {
func serve(cfg config.HTTP) {
var h http.Handler
ln := newListener(cfg.ListenAddr)

h = http.HandlerFunc(serveHTTP)
if cfg.ForceAutocertHandler {
if autocertManager == nil {
Expand Down Expand Up @@ -188,8 +192,8 @@ func newTLSConfig(cfg config.HTTPS) *tls.Config {
return &tlsCfg
}

func listenAndServe(ln net.Listener, h http.Handler, cfg config.TimeoutCfg) error {
s := &http.Server{
func newServer(ln net.Listener, h http.Handler, cfg config.TimeoutCfg) *http.Server {
return &http.Server{
TLSNextProto: make(map[string]func(*http.Server, *tls.Conn, http.Handler)),
Handler: h,
ReadTimeout: time.Duration(cfg.ReadTimeout),
Expand All @@ -200,6 +204,10 @@ func listenAndServe(ln net.Listener, h http.Handler, cfg config.TimeoutCfg) erro
// must handle all these errors in the code.
ErrorLog: log.NilLogger,
}
}

func listenAndServe(ln net.Listener, h http.Handler, cfg config.TimeoutCfg) error {
s := newServer(ln, h, cfg)
return s.Serve(ln)
}

Expand Down Expand Up @@ -234,6 +242,10 @@ func serveHTTP(rw http.ResponseWriter, r *http.Request) {
promHandler.ServeHTTP(rw, r)
case "/", "/query":
var err error

proxyHandler := proxyHandler.Load().(*ProxyHandler)
r.RemoteAddr = proxyHandler.GetRemoteAddr(r)

var an *config.Networks
if r.TLS != nil {
an = allowedNetworksHTTPS.Load().(*config.Networks)
Expand Down Expand Up @@ -274,6 +286,7 @@ func applyConfig(cfg *config.Config) error {
allowedNetworksHTTP.Store(&cfg.Server.HTTP.AllowedNetworks)
allowedNetworksHTTPS.Store(&cfg.Server.HTTPS.AllowedNetworks)
allowedNetworksMetrics.Store(&cfg.Server.Metrics.AllowedNetworks)
proxyHandler.Store(NewProxyHandler(&cfg.Server.Proxy))
log.SetDebug(cfg.LogDebug)
log.Infof("Loaded config:\n%s", cfg)

Expand Down
45 changes: 35 additions & 10 deletions main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ func TestServe(t *testing.T) {
name string
file string
testFn func(t *testing.T)
listenFn func() (net.Listener, chan struct{})
listenFn func() (*http.Server, chan struct{})
}{
{
"https request",
Expand Down Expand Up @@ -711,6 +711,29 @@ func TestServe(t *testing.T) {
},
startHTTP,
},
{
"http request with default proxy headers",
Copy link
Collaborator

@mga-chka mga-chka Sep 11, 2022

Choose a reason for hiding this comment

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

I might miss something but were do you check in this test that the remote addr is modified?

Also, you should add the test that when the proxy is disabled, nothing is changed

Copy link
Collaborator Author

@Blokje5 Blokje5 Sep 11, 2022

Choose a reason for hiding this comment

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

The test works because allowed_networks: ["10.0.0.0/24"] is set in the config. It would return a 403 if the remote addr wasn't modified.

The proxy disabled setting should be covered by the other tests (as the middleware is always used).

"testdata/https.proxy-enabled.yml",
func(t *testing.T) {
query := "SELECT 1"
req, err := http.NewRequest("GET", "https://127.0.0.1:8443?query="+url.QueryEscape(query), nil)
checkErr(t, err)
req.Close = true
req.SetBasicAuth("default", "qwerty")
req.Header.Add("X-Forwarded-For", "10.0.0.1")

resp, err := tlsClient.Do(req)
checkErr(t, err)
defer resp.Body.Close()

if resp.StatusCode != http.StatusOK {
t.Fatalf("unexpected status code: %d; expected: %d", resp.StatusCode, http.StatusOK)
}

checkResponse(t, resp.Body, expectedOkResp)
},
startTLS,
},
}

// Wait until CHServer starts.
Expand All @@ -734,10 +757,10 @@ func TestServe(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
*configFile = tc.file
ln, done := tc.listenFn()
s, done := tc.listenFn()
defer func() {
if err := ln.Close(); err != nil {
t.Fatalf("unexpected error while closing listener: %s", err)
if err := s.Shutdown(context.Background()); err != nil {
t.Fatalf("unexpected error while closing server: %s", err)
}
<-done
}()
Expand Down Expand Up @@ -778,7 +801,7 @@ var tlsClient = &http.Client{Transport: &http.Transport{
TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
}}

func startTLS() (net.Listener, chan struct{}) {
func startTLS() (*http.Server, chan struct{}) {
cfg, err := loadConfig()
if err != nil {
panic(fmt.Sprintf("error while loading config: %s", err))
Expand All @@ -794,14 +817,15 @@ func startTLS() (net.Listener, chan struct{}) {
tlsCfg := newTLSConfig(cfg.Server.HTTPS)
tln := tls.NewListener(ln, tlsCfg)
h := http.HandlerFunc(serveHTTP)
s := newServer(tln, h, config.TimeoutCfg{})
go func() {
listenAndServe(tln, h, config.TimeoutCfg{})
s.Serve(tln)
close(done)
}()
return tln, done
return s, done
}

func startHTTP() (net.Listener, chan struct{}) {
func startHTTP() (*http.Server, chan struct{}) {
cfg, err := loadConfig()
if err != nil {
panic(fmt.Sprintf("error while loading config: %s", err))
Expand All @@ -815,11 +839,12 @@ func startHTTP() (net.Listener, chan struct{}) {
panic(fmt.Sprintf("cannot listen for %q: %s", cfg.Server.HTTP.ListenAddr, err))
}
h := http.HandlerFunc(serveHTTP)
s := newServer(ln, h, config.TimeoutCfg{})
go func() {
listenAndServe(ln, h, config.TimeoutCfg{})
s.Serve(ln)
close(done)
}()
return ln, done
return s, done
}

// TODO randomise port for each instance of the mock
Expand Down
Loading