From 4e98224a4501238286ee3fd5ed911958951da9b0 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Wed, 18 Oct 2023 17:44:36 +0800 Subject: [PATCH] Support allowed hosts for webhook to work with proxy (#27655) When `webhook.PROXY_URL` has been set, the old code will check if the proxy host is in `ALLOWED_HOST_LIST` or reject requests through the proxy. It requires users to add the proxy host to `ALLOWED_HOST_LIST`. However, it actually allows all requests to any port on the host, when the proxy host is probably an internal address. But things may be even worse. `ALLOWED_HOST_LIST` doesn't really work when requests are sent to the allowed proxy, and the proxy could forward them to any hosts. This PR fixes it by: - If the proxy has been set, always allow connectioins to the host and port. - Check `ALLOWED_HOST_LIST` before forwarding. --- modules/hostmatcher/http.go | 18 +++++++-- services/webhook/deliver.go | 9 +++-- services/webhook/deliver_test.go | 65 +++++++++++++++++++++++++------- 3 files changed, 72 insertions(+), 20 deletions(-) diff --git a/modules/hostmatcher/http.go b/modules/hostmatcher/http.go index 65f5f78b1..c743f6efb 100644 --- a/modules/hostmatcher/http.go +++ b/modules/hostmatcher/http.go @@ -7,12 +7,17 @@ import ( "context" "fmt" "net" + "net/url" "syscall" "time" ) // NewDialContext returns a DialContext for Transport, the DialContext will do allow/block list check func NewDialContext(usage string, allowList, blockList *HostMatchList) func(ctx context.Context, network, addr string) (net.Conn, error) { + return NewDialContextWithProxy(usage, allowList, blockList, nil) +} + +func NewDialContextWithProxy(usage string, allowList, blockList *HostMatchList, proxy *url.URL) func(ctx context.Context, network, addr string) (net.Conn, error) { // How Go HTTP Client works with redirection: // transport.RoundTrip URL=http://domain.com, Host=domain.com // transport.DialContext addrOrHost=domain.com:80 @@ -26,11 +31,18 @@ func NewDialContext(usage string, allowList, blockList *HostMatchList) func(ctx Timeout: 30 * time.Second, KeepAlive: 30 * time.Second, - Control: func(network, ipAddr string, c syscall.RawConn) (err error) { - var host string - if host, _, err = net.SplitHostPort(addrOrHost); err != nil { + Control: func(network, ipAddr string, c syscall.RawConn) error { + host, port, err := net.SplitHostPort(addrOrHost) + if err != nil { return err } + if proxy != nil { + // Always allow the host of the proxy, but only on the specified port. + if host == proxy.Hostname() && port == proxy.Port() { + return nil + } + } + // in Control func, the addr was already resolved to IP:PORT format, there is no cost to do ResolveTCPAddr here tcpAddr, err := net.ResolveTCPAddr(network, ipAddr) if err != nil { diff --git a/services/webhook/deliver.go b/services/webhook/deliver.go index 19c34772c..8f728d3aa 100644 --- a/services/webhook/deliver.go +++ b/services/webhook/deliver.go @@ -239,7 +239,7 @@ var ( hostMatchers []glob.Glob ) -func webhookProxy() func(req *http.Request) (*url.URL, error) { +func webhookProxy(allowList *hostmatcher.HostMatchList) func(req *http.Request) (*url.URL, error) { if setting.Webhook.ProxyURL == "" { return proxy.Proxy() } @@ -257,6 +257,9 @@ func webhookProxy() func(req *http.Request) (*url.URL, error) { return func(req *http.Request) (*url.URL, error) { for _, v := range hostMatchers { if v.Match(req.URL.Host) { + if !allowList.MatchHostName(req.URL.Host) { + return nil, fmt.Errorf("webhook can only call allowed HTTP servers (check your %s setting), deny '%s'", allowList.SettingKeyHint, req.URL.Host) + } return http.ProxyURL(setting.Webhook.ProxyURLFixed)(req) } } @@ -278,8 +281,8 @@ func Init() error { Timeout: timeout, Transport: &http.Transport{ TLSClientConfig: &tls.Config{InsecureSkipVerify: setting.Webhook.SkipTLSVerify}, - Proxy: webhookProxy(), - DialContext: hostmatcher.NewDialContext("webhook", allowedHostMatcher, nil), + Proxy: webhookProxy(allowedHostMatcher), + DialContext: hostmatcher.NewDialContextWithProxy("webhook", allowedHostMatcher, nil, setting.Webhook.ProxyURLFixed), }, } diff --git a/services/webhook/deliver_test.go b/services/webhook/deliver_test.go index ee63975ad..72aa00478 100644 --- a/services/webhook/deliver_test.go +++ b/services/webhook/deliver_test.go @@ -14,35 +14,72 @@ import ( "code.gitea.io/gitea/models/db" "code.gitea.io/gitea/models/unittest" webhook_model "code.gitea.io/gitea/models/webhook" + "code.gitea.io/gitea/modules/hostmatcher" "code.gitea.io/gitea/modules/setting" api "code.gitea.io/gitea/modules/structs" webhook_module "code.gitea.io/gitea/modules/webhook" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestWebhookProxy(t *testing.T) { + oldWebhook := setting.Webhook + t.Cleanup(func() { + setting.Webhook = oldWebhook + }) + setting.Webhook.ProxyURL = "http://localhost:8080" setting.Webhook.ProxyURLFixed, _ = url.Parse(setting.Webhook.ProxyURL) setting.Webhook.ProxyHosts = []string{"*.discordapp.com", "discordapp.com"} - kases := map[string]string{ - "https://discordapp.com/api/webhooks/xxxxxxxxx/xxxxxxxxxxxxxxxxxxx": "http://localhost:8080", - "http://s.discordapp.com/assets/xxxxxx": "http://localhost:8080", - "http://github.com/a/b": "", + allowedHostMatcher := hostmatcher.ParseHostMatchList("webhook.ALLOWED_HOST_LIST", "discordapp.com,s.discordapp.com") + + tests := []struct { + req string + want string + wantErr bool + }{ + { + req: "https://discordapp.com/api/webhooks/xxxxxxxxx/xxxxxxxxxxxxxxxxxxx", + want: "http://localhost:8080", + wantErr: false, + }, + { + req: "http://s.discordapp.com/assets/xxxxxx", + want: "http://localhost:8080", + wantErr: false, + }, + { + req: "http://github.com/a/b", + want: "", + wantErr: false, + }, + { + req: "http://www.discordapp.com/assets/xxxxxx", + want: "", + wantErr: true, + }, } + for _, tt := range tests { + t.Run(tt.req, func(t *testing.T) { + req, err := http.NewRequest("POST", tt.req, nil) + require.NoError(t, err) - for reqURL, proxyURL := range kases { - req, err := http.NewRequest("POST", reqURL, nil) - assert.NoError(t, err) + u, err := webhookProxy(allowedHostMatcher)(req) + if tt.wantErr { + assert.Error(t, err) + return + } - u, err := webhookProxy()(req) - assert.NoError(t, err) - if proxyURL == "" { - assert.Nil(t, u) - } else { - assert.EqualValues(t, proxyURL, u.String()) - } + assert.NoError(t, err) + + got := "" + if u != nil { + got = u.String() + } + assert.Equal(t, tt.want, got) + }) } }