diff --git a/server/channels/app/post_metadata_test.go b/server/channels/app/post_metadata_test.go index 03951699af7..109443a1ff9 100644 --- a/server/channels/app/post_metadata_test.go +++ b/server/channels/app/post_metadata_test.go @@ -2590,7 +2590,7 @@ func TestGetLinkMetadata(t *testing.T) { assert.Nil(t, img) assert.Error(t, err) assert.IsType(t, &url.Error{}, err) - assert.Equal(t, httpservice.ErrAddressForbidden, err.(*url.Error).Err) + assert.ErrorContains(t, err, httpservice.ErrAddressForbidden.Error()) requestURL = th.App.GetSiteURL() + "/api/v4/image?url=" + url.QueryEscape(requestURL) diff --git a/server/public/shared/httpservice/client.go b/server/public/shared/httpservice/client.go index 39b7ef3d169..b483b4c342d 100644 --- a/server/public/shared/httpservice/client.go +++ b/server/public/shared/httpservice/client.go @@ -12,6 +12,7 @@ import ( "net/http" "net/netip" "net/url" + "strings" "time" "golang.org/x/net/http/httpproxy" @@ -117,7 +118,10 @@ type DialContextFunction func(ctx context.Context, network, addr string) (net.Co var ErrAddressForbidden = errors.New("address forbidden, you may need to set AllowedUntrustedInternalConnections to allow an integration access to your internal network") -func dialContextFilter(dial DialContextFunction, allowHost func(host string) bool, allowIP func(ip net.IP) bool) DialContextFunction { +// dialContextFilter wraps a dial function to filter connections based on host and IP validation. +// It first checks if the host is allowed, then resolves the hostname to IPs and validates each one. +// Returns detailed error messages when connections are rejected for security reasons. +func dialContextFilter(dial DialContextFunction, allowHost func(host string) bool, allowIP func(ip net.IP) error) DialContextFunction { return func(ctx context.Context, network, addr string) (net.Conn, error) { host, port, err := net.SplitHostPort(addr) if err != nil { @@ -133,7 +137,8 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo return nil, err } - var firstErr error + var firstDialErr error + var forbiddenReasons []string for _, ip := range ips { select { case <-ctx.Done(): @@ -141,7 +146,13 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo default: } - if allowIP == nil || !allowIP(ip) { + if allowIP == nil { + forbiddenReasons = append(forbiddenReasons, fmt.Sprintf("IP %s is not allowed", ip)) + continue + } + + if err := allowIP(ip); err != nil { + forbiddenReasons = append(forbiddenReasons, err.Error()) continue } @@ -149,14 +160,18 @@ func dialContextFilter(dial DialContextFunction, allowHost func(host string) boo if err == nil { return conn, nil } - if firstErr == nil { - firstErr = err + if firstDialErr == nil { + firstDialErr = err } } - if firstErr == nil { + if firstDialErr == nil { + // If we didn't find an allowed IP address, return an error explaining why + if len(forbiddenReasons) > 0 { + return nil, fmt.Errorf("%s: %s", ErrAddressForbidden.Error(), strings.Join(forbiddenReasons, "; ")) + } return nil, ErrAddressForbidden } - return nil, firstErr + return nil, firstDialErr } } @@ -166,7 +181,7 @@ func getProxyFn() func(r *http.Request) (*url.URL, error) { // TODO: Consider removing this code once MM-61938 is fixed upstream. if r.URL != nil { if addr, err := netip.ParseAddr(r.URL.Hostname()); err == nil && addr.Is6() && addr.Zone() != "" { - return nil, fmt.Errorf("invalid IPv6 address in URL: %q", addr.String()) + return nil, fmt.Errorf("invalid IPv6 address in URL: %q", addr) } } @@ -174,7 +189,8 @@ func getProxyFn() func(r *http.Request) (*url.URL, error) { } } -func NewTransport(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) bool) *MattermostTransport { +// NewTransport creates a new MattermostTransport with detailed error messages for IP check failures +func NewTransport(enableInsecureConnections bool, allowHost func(host string) bool, allowIP func(ip net.IP) error) *MattermostTransport { dialContext := (&net.Dialer{ Timeout: ConnectTimeout, KeepAlive: 30 * time.Second, diff --git a/server/public/shared/httpservice/client_test.go b/server/public/shared/httpservice/client_test.go index 709fc5f5e72..49fc1485226 100644 --- a/server/public/shared/httpservice/client_test.go +++ b/server/public/shared/httpservice/client_test.go @@ -60,13 +60,13 @@ func TestHTTPClient(t *testing.T) { t.Run("checks", func(t *testing.T) { allowHost := func(_ string) bool { return true } rejectHost := func(_ string) bool { return false } - allowIP := func(_ net.IP) bool { return true } - rejectIP := func(_ net.IP) bool { return false } + allowIP := func(_ net.IP) error { return nil } + rejectIP := func(_ net.IP) error { return fmt.Errorf("IP not allowed") } testCases := []struct { description string allowHost func(string) bool - allowIP func(net.IP) bool + allowIP func(net.IP) error expectedAllowed bool }{ {"allow with no checks", nil, nil, true}, @@ -88,7 +88,7 @@ func TestHTTPClient(t *testing.T) { require.NoError(t, err) } else { require.IsType(t, &url.Error{}, err) - require.Equal(t, ErrAddressForbidden, err.(*url.Error).Err) + require.Contains(t, err.(*url.Error).Err.Error(), "address forbidden") } }) } @@ -156,7 +156,12 @@ func TestDialContextFilter(t *testing.T) { filter := dialContextFilter(func(ctx context.Context, network, addr string) (net.Conn, error) { didDial = true return nil, nil - }, func(host string) bool { return host == "10.0.0.1" }, func(ip net.IP) bool { return !IsReservedIP(ip) }) + }, func(host string) bool { return host == "10.0.0.1" }, func(ip net.IP) error { + if IsReservedIP(ip) { + return fmt.Errorf("IP %s is reserved", ip) + } + return nil + }) _, err := filter(context.Background(), "", tc.Addr) if tc.IsValid { @@ -164,7 +169,7 @@ func TestDialContextFilter(t *testing.T) { require.True(t, didDial) } else { require.Error(t, err) - require.Equal(t, err, ErrAddressForbidden) + require.Contains(t, err.Error(), "address forbidden") require.False(t, didDial) } } @@ -226,8 +231,9 @@ func TestIsOwnIP(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, _ := IsOwnIP(tt.ip) - assert.Equalf(t, tt.want, got, "IsOwnIP() = %v, want %v for IP %s", got, tt.want, tt.ip.String()) + got, err := IsOwnIP(tt.ip) + require.NoError(t, err) + assert.Equal(t, tt.want, got) }) } } diff --git a/server/public/shared/httpservice/httpservice.go b/server/public/shared/httpservice/httpservice.go index 6e6c7b90ce0..d886b45b3b5 100644 --- a/server/public/shared/httpservice/httpservice.go +++ b/server/public/shared/httpservice/httpservice.go @@ -4,6 +4,7 @@ package httpservice import ( + "fmt" "net" "net/http" "slices" @@ -84,31 +85,31 @@ func (h *HTTPServiceImpl) MakeTransport(trustURLs bool) *MattermostTransport { return slices.Contains(strings.FieldsFunc(*h.configService.Config().ServiceSettings.AllowedUntrustedInternalConnections, splitFields), host) } - allowIP := func(ip net.IP) bool { + allowIP := func(ip net.IP) error { reservedIP := IsReservedIP(ip) - ownIP, err := IsOwnIP(ip) - // If there is an error getting the self-assigned IPs, default to the secure option + ownIP, err := IsOwnIP(ip) if err != nil { - return false + // If there is an error getting the self-assigned IPs, default to the secure option + return fmt.Errorf("unable to determine if IP is own IP: %w", err) } // If it's not a reserved IP and it's not self-assigned IP, accept the IP if !reservedIP && !ownIP { - return true - } - - if h.configService.Config().ServiceSettings.AllowedUntrustedInternalConnections == nil { - return false + return nil } // In the case it's the self-assigned IP, enforce that it needs to be explicitly added to the AllowedUntrustedInternalConnections - for _, allowed := range strings.FieldsFunc(*h.configService.Config().ServiceSettings.AllowedUntrustedInternalConnections, splitFields) { + for _, allowed := range strings.FieldsFunc(model.SafeDereference(h.configService.Config().ServiceSettings.AllowedUntrustedInternalConnections), splitFields) { if _, ipRange, err := net.ParseCIDR(allowed); err == nil && ipRange.Contains(ip) { - return true + return nil } } - return false + + if reservedIP { + return fmt.Errorf("IP %s is in a reserved range and not in AllowedUntrustedInternalConnections", ip) + } + return fmt.Errorf("IP %s is a self-assigned IP and not in AllowedUntrustedInternalConnections", ip) } return NewTransport(insecure, allowHost, allowIP)