Fix webhook SSRF protections - Security Hardening

src/jobservice/job/impl/notification/http_helper.go

@@ -15,13 +15,18 @@
 package notification
 
 import (
+	"context"
+	"crypto/tls"
+	"net"
 	"net/http"
 	"os"
 	"strconv"
+	"sync"
 	"time"
 
 	commonhttp "github.com/goharbor/harbor/src/common/http"
 	"github.com/goharbor/harbor/src/jobservice/logger"
+	"github.com/goharbor/harbor/src/lib"
 )
 
 const (
@@ -65,11 +70,120 @@ func init() {
 		clients: map[string]*http.Client{},
 	}
 	httpHelper.clients[secure] = &http.Client{
-		Transport: commonhttp.GetHTTPTransport(),
-		Timeout:   timeout,
+		Transport:     webhookTransport(false),
+		Timeout:       timeout,
+		CheckRedirect: noRedirect,
 	}
 	httpHelper.clients[insecure] = &http.Client{
-		Transport: commonhttp.GetHTTPTransport(commonhttp.WithInsecure(true)),
-		Timeout:   timeout,
+		Transport:     webhookTransport(true),
+		Timeout:       timeout,
+		CheckRedirect: noRedirect,
 	}
 }
+
+type contextKey string
+
+const (
+	useProxyKey contextKey = "useProxy"
+)
+
+var (
+	dialer = &net.Dialer{
+		Timeout:   30 * time.Second,
+		KeepAlive: 30 * time.Second,
+	}
+)
+
+func webhookTransport(insecure bool) http.RoundTripper {
+	opts := []func(*http.Transport){
+		func(tr *http.Transport) {
+			tr.Proxy = http.ProxyFromEnvironment
+			tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
+				if useProxy, ok := ctx.Value(useProxyKey).(bool); ok && useProxy {
+					return dialer.DialContext(ctx, network, addr)
+				}
+				return lib.PublicDialContext(ctx, network, addr)
+			}
+		},
+	}
+	if insecure {
+		opts = append(opts, commonhttp.WithInsecureSkipVerify(true))
+	}
+	underlying := commonhttp.NewTransport(opts...)
+	return &ssrfProxyRoundTripper{
+		insecure:   insecure,
+		underlying: underlying,
+	}
+}
+
+type ssrfProxyRoundTripper struct {
+	insecure     bool
+	underlying   http.RoundTripper
+	transportsMu sync.Mutex
+	transports   map[string]*http.Transport
+}
+
+func (s *ssrfProxyRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) {
+	useProxy, ok := req.Context().Value(useProxyKey).(bool)
+	if !ok || !useProxy {
+		return s.underlying.RoundTrip(req)
+	}
+
+	host := req.URL.Hostname()
+	port := req.URL.Port()
+	if port == "" {
+		if req.URL.Scheme == "https" {
+			port = "443"
+		} else {
+			port = "80"
+		}
+	}
+
+	// Resolve and validate target host, pinning the IP address to prevent DNS-rebinding SSRF
+	dialAddr, err := lib.PublicDialAddress(req.Context(), host, port)
+	if err != nil {
+		return nil, err
+	}
+
+	ip, _, err := net.SplitHostPort(dialAddr)
+	if err != nil {
+		return nil, err
+	}
+
+	s.transportsMu.Lock()
+	if s.transports == nil {
+		s.transports = make(map[string]*http.Transport)
+	}
+	tr, exists := s.transports[host]
+	if !exists {
+		tr = &http.Transport{
+			Proxy: http.ProxyFromEnvironment,
+			DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) {
+				return dialer.DialContext(ctx, network, addr)
+			},
+			TLSClientConfig: &tls.Config{
+				ServerName:         host,
+				InsecureSkipVerify: s.insecure,
+			},
+			MaxIdleConns:          100,
+			IdleConnTimeout:       90 * time.Second,
+			TLSHandshakeTimeout:   10 * time.Second,
+			ExpectContinueTimeout: 1 * time.Second,
+		}
+		s.transports[host] = tr
+	}
+	s.transportsMu.Unlock()
+
+	clonedReq := req.Clone(req.Context())
+	if clonedReq.Host == "" {
+		clonedReq.Host = req.URL.Host
+	}
+
+	clonedReq.URL.Host = net.JoinHostPort(ip, port)
+
+	return tr.RoundTrip(clonedReq)
+}
+
+func noRedirect(_ *http.Request, _ []*http.Request) error {
+	return http.ErrUseLastResponse
+}

src/jobservice/job/impl/notification/http_helper_test.go

@@ -1,10 +1,15 @@
 package notification
 
 import (
+	"context"
+	"io"
+	"net/http"
+	"strings"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestHttpHelper(t *testing.T) {
@@ -19,3 +24,66 @@ func TestHttpHelper(t *testing.T) {
 	_, ok := httpHelper.clients["notExists"]
 	assert.False(t, ok)
 }
+
+func TestSsrfProxyRoundTripper(t *testing.T) {
+	t.Run("no proxy key", func(t *testing.T) {
+		dummyRT := roundTripFunc(func(req *http.Request) (*http.Response, error) {
+			// Unmodified request should have original URL host
+			assert.Equal(t, "example.com", req.URL.Host)
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader("ok")),
+			}, nil
+		})
+
+		rt := &ssrfProxyRoundTripper{
+			insecure:   false,
+			underlying: dummyRT,
+		}
+
+		req, err := http.NewRequest(http.MethodGet, "https://example.com/foo", nil)
+		require.NoError(t, err)
+
+		resp, err := rt.RoundTrip(req)
+		require.NoError(t, err)
+		defer resp.Body.Close()
+		assert.Equal(t, http.StatusOK, resp.StatusCode)
+	})
+
+	t.Run("with proxy key and private IP", func(t *testing.T) {
+		rt := &ssrfProxyRoundTripper{
+			insecure:   false,
+			underlying: http.DefaultTransport,
+		}
+
+		req, err := http.NewRequest(http.MethodGet, "https://127.0.0.1/foo", nil)
+		require.NoError(t, err)
+		req = req.WithContext(context.WithValue(req.Context(), useProxyKey, true))
+
+		_, err = rt.RoundTrip(req)
+		assert.Error(t, err)
+		assert.Contains(t, err.Error(), "blocked non-public host")
+	})
+
+	t.Run("with proxy key and public host", func(t *testing.T) {
+		rt := &ssrfProxyRoundTripper{
+			insecure:   false,
+			underlying: http.DefaultTransport,
+		}
+
+		req, err := http.NewRequest(http.MethodGet, "https://example.com/foo", nil)
+		require.NoError(t, err)
+		req = req.WithContext(context.WithValue(req.Context(), useProxyKey, true))
+
+		resp, err := rt.RoundTrip(req)
+		if err == nil {
+			defer resp.Body.Close()
+			// Status can be 200 or 404 or any other HTTP status since it reached example.com
+			assert.True(t, resp.StatusCode > 0)
+		} else {
+			// If network is not reachable (e.g. offline builder), we allow dial/connect errors,
+			// but we shouldn't get validation errors.
+			assert.True(t, strings.Contains(err.Error(), "dial") || strings.Contains(err.Error(), "connect") || strings.Contains(err.Error(), "lookup") || strings.Contains(err.Error(), "no such host") || strings.Contains(err.Error(), "timeout"))
+		}
+	})
+}

src/jobservice/job/impl/notification/slack_job.go

@@ -16,6 +16,7 @@ package notification
 
 import (
 	"bytes"
+	"context"
 	"io"
 	"net/http"
 	"os"
@@ -25,6 +26,7 @@ import (
 
 	"github.com/goharbor/harbor/src/jobservice/job"
 	"github.com/goharbor/harbor/src/jobservice/logger"
+	"github.com/goharbor/harbor/src/lib"
 	"github.com/goharbor/harbor/src/lib/errors"
 )
 
@@ -94,7 +96,7 @@ func (sj *SlackJob) Run(ctx job.Context, params job.Parameters) error {
 
 	sj.logger.Info("start to run slack job")
 
-	err := sj.execute(params)
+	err := sj.execute(ctx, params)
 	if err != nil {
 		sj.logger.Errorf("exit slack job, error: %s", err)
 	} else {
@@ -121,11 +123,23 @@ func (sj *SlackJob) init(ctx job.Context, params map[string]any) error {
 }
 
 // execute slack job
-func (sj *SlackJob) execute(params map[string]any) error {
+func (sj *SlackJob) execute(ctx job.Context, params map[string]any) error {
 	payload := params["payload"].(string)
 	address := params["address"].(string)
+	validatedAddress, err := lib.ValidatePublicHTTPURL(ctx.SystemContext(), address, true)
+	if err != nil {
+		return errors.Wrap(err, "invalid slack target")
+	}
+
+	var useProxy bool
+	if dummyReq, err := http.NewRequest(http.MethodPost, validatedAddress, nil); err == nil {
+		if proxyURL, err := http.ProxyFromEnvironment(dummyReq); err == nil && proxyURL != nil {
+			useProxy = true
+		}
+	}
 
-	req, err := http.NewRequest(http.MethodPost, address, bytes.NewReader([]byte(payload)))
+	reqCtx := context.WithValue(ctx.SystemContext(), useProxyKey, useProxy)
+	req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, validatedAddress, bytes.NewReader([]byte(payload)))
 	if err != nil {
 		return errors.Wrap(err, "error to generate request")
 	}

src/jobservice/job/impl/notification/slack_job_test.go

@@ -3,7 +3,7 @@ package notification
 import (
 	"io"
 	"net/http"
-	"net/http/httptest"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -53,35 +53,65 @@ func TestSlackJobRun(t *testing.T) {
 
 	rep := &SlackJob{}
 
-	// test slack request
-	ts := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			body, _ := io.ReadAll(r.Body)
+	originalClients := httpHelper.clients
+	t.Cleanup(func() {
+		httpHelper.clients = originalClients
+	})
+	httpHelper.clients = map[string]*http.Client{}
+	httpHelper.clients[secure] = &http.Client{
+		CheckRedirect: noRedirect,
+		Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
+			body, _ := io.ReadAll(req.Body)
 
 			// test request method
-			assert.Equal(t, http.MethodPost, r.Method)
+			assert.Equal(t, http.MethodPost, req.Method)
 			// test request body
 			assert.Equal(t, string(body), `{"key": "value"}`)
-		}))
-	defer ts.Close()
+			return &http.Response{
+				StatusCode: http.StatusOK,
+				Body:       io.NopCloser(strings.NewReader("")),
+				Header:     make(http.Header),
+				Request:    req,
+			}, nil
+		}),
+	}
+	httpHelper.clients[insecure] = httpHelper.clients[secure]
+
 	params := map[string]any{
 		"skip_cert_verify": true,
 		"payload":          `{"key": "value"}`,
-		"address":          ts.URL,
+		"address":          "http://1.1.1.1",
 	}
 	// test correct slack response
 	assert.Nil(t, rep.Run(ctx, params))
 
-	tsWrong := httptest.NewServer(
-		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-			w.WriteHeader(http.StatusUnauthorized)
-		}))
-	defer tsWrong.Close()
+	httpHelper.clients[insecure] = &http.Client{
+		CheckRedirect: noRedirect,
+		Transport: roundTripFunc(func(req *http.Request) (*http.Response, error) {
+			return &http.Response{
+				StatusCode: http.StatusUnauthorized,
+				Body:       io.NopCloser(strings.NewReader("")),
+				Header:     make(http.Header),
+				Request:    req,
+			}, nil
+		}),
+	}
 	paramsWrong := map[string]any{
 		"skip_cert_verify": true,
 		"payload":          `{"key": "value"}`,
-		"address":          tsWrong.URL,
+		"address":          "http://1.1.1.1",
 	}
 	// test incorrect slack response
 	assert.NotNil(t, rep.Run(ctx, paramsWrong))
 }
+
+func TestSlackJobRunRejectsPrivateTarget(t *testing.T) {
+	ctx := &mockjobservice.MockJobContext{}
+	rep := &SlackJob{}
+	params := map[string]any{
+		"payload": `{"key": "value"}`,
+		"address": "http://169.254.169.254/latest/meta-data",
+	}
+
+	assert.NotNil(t, rep.Run(ctx, params))
+}