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.
This commit is contained in:
Jason Song 2023-10-18 17:44:36 +08:00 committed by GitHub
parent 8abc1aae4a
commit 4e98224a45
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 72 additions and 20 deletions

View File

@ -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 {

View File

@ -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),
},
}

View File

@ -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)
u, err := webhookProxy(allowedHostMatcher)(req)
if tt.wantErr {
assert.Error(t, err)
return
}
for reqURL, proxyURL := range kases {
req, err := http.NewRequest("POST", reqURL, nil)
assert.NoError(t, err)
u, err := webhookProxy()(req)
assert.NoError(t, err)
if proxyURL == "" {
assert.Nil(t, u)
} else {
assert.EqualValues(t, proxyURL, u.String())
got := ""
if u != nil {
got = u.String()
}
assert.Equal(t, tt.want, got)
})
}
}