From f1b4e3ccc687ed806cf3bbb671bbcab13230aebf Mon Sep 17 00:00:00 2001 From: "stonezdj(Daojun Zhang)" Date: Fri, 16 Oct 2026 15:06:05 +0800 Subject: [PATCH] Fix webhook egress allow all (#24126) fix(webhook): default webhook egress policy level to allow_all When the webhook egress policy is empty or unset, default to egressAllowAll to preserve Harbor's default egress behavior and avoid regressions for internal and test webhook targets. Signed-off-by: stonezdj --- src/common/http/private_network.go | 4 +- src/common/http/private_network_test.go | 47 +++++++++++++++---- .../job/impl/notification/ssrf_test.go | 4 +- 3 files changed, 41 insertions(+), 14 deletions(-) diff --git a/src/common/http/private_network.go b/src/common/http/private_network.go index 0eef9417ab..6b61a5c000 100644 --- a/src/common/http/private_network.go +++ b/src/common/http/private_network.go @@ -33,7 +33,7 @@ import ( ) // webhookEgressPolicyEnv selects which destinations the webhook and Slack notification clients -// may reach: allow_all, block_restricted (the default) or public_only. See egressLevel. +// may reach: allow_all (the default), block_restricted or public_only. See egressLevel. const webhookEgressPolicyEnv = "HARBOR_WEBHOOK_EGRESS_POLICY" // webhookEgressAllowlistEnv lists destinations the notification clients may always reach, whatever @@ -216,7 +216,7 @@ func parseEgressLevel(policyEnv, legacyEnv string) egressLevel { } value := strings.TrimSpace(legacyEnv) if value == "" { - return egressBlockRestricted + return egressAllowAll } allowed, err := strconv.ParseBool(value) switch { diff --git a/src/common/http/private_network_test.go b/src/common/http/private_network_test.go index 85f5f3c07c..cbfb9e0784 100644 --- a/src/common/http/private_network_test.go +++ b/src/common/http/private_network_test.go @@ -112,10 +112,32 @@ func TestValidateNetworkTarget(t *testing.T) { } } -// TestValidateNetworkTargetDefault covers the default policy: internal webhook receivers keep -// working, while targets that reach the host itself or cloud metadata stay blocked. +// TestValidateNetworkTargetDefault covers the default policy (allow_all): all targets are allowed. func TestValidateNetworkTargetDefault(t *testing.T) { setEgressEnv(t, "", "", "") + resolver := &stubNetworkResolver{answers: map[string][]netip.Addr{ + "rebind.example.com": {netip.MustParseAddr("169.254.169.254")}, + }} + + for _, host := range []string{ + "8.8.8.8", + "10.0.0.1", + "127.0.0.1", + "169.254.169.254", + "localhost", + "metadata.google.internal", + "rebind.example.com", + } { + t.Run("allows "+host, func(t *testing.T) { + require.NoError(t, ValidateNetworkTarget(context.Background(), resolver, host)) + }) + } +} + +// TestValidateNetworkTargetBlockRestricted covers the block_restricted policy: internal webhook receivers keep +// working, while targets that reach the host itself or cloud metadata stay blocked. +func TestValidateNetworkTargetBlockRestricted(t *testing.T) { + setEgressEnv(t, "block_restricted", "", "") resolver := &stubNetworkResolver{answers: map[string][]netip.Addr{ "receiver.ns.svc.cluster.local": {netip.MustParseAddr("10.96.12.7")}, "rebind.example.com": {netip.MustParseAddr("169.254.169.254")}, @@ -174,8 +196,8 @@ func TestEgressLevelPrecedence(t *testing.T) { policy, legacy string want egressLevel }{ - {"", "", egressBlockRestricted}, - {" ", " ", egressBlockRestricted}, + {"", "", egressAllowAll}, + {" ", " ", egressAllowAll}, {"", "true", egressAllowAll}, {"", "1", egressAllowAll}, {"", "false", egressPublicOnly}, @@ -202,7 +224,7 @@ func TestValidateNetworkTargetPerLevel(t *testing.T) { {"allow_all", "", true, true, true}, {"", "true", true, true, true}, {"block_restricted", "", true, true, false}, - {"", "", true, true, false}, + {"", "", true, true, true}, {"public_only", "", true, false, false}, {"", "false", true, false, false}, } { @@ -271,7 +293,7 @@ func TestValidateNetworkTargetAllowlist(t *testing.T) { } func TestCheckEgress(t *testing.T) { - setEgressEnv(t, "", "", "") + setEgressEnv(t, "block_restricted", "", "") require.NoError(t, checkEgress("tcp4", "10.0.0.1:443", nil)) require.NoError(t, checkEgress("tcp6", "[fd12::1]:443", nil)) require.ErrorContains(t, checkEgress("tcp4", "169.254.169.254:80", nil), "restricted network address") @@ -280,10 +302,15 @@ func TestCheckEgress(t *testing.T) { setEgressEnv(t, "public_only", "", "") require.ErrorContains(t, checkEgress("tcp4", "10.0.0.1:443", nil), "private network address") require.NoError(t, checkEgress("tcp4", "8.8.8.8:443", nil)) + + setEgressEnv(t, "", "", "") + require.NoError(t, checkEgress("tcp4", "10.0.0.1:443", nil)) + require.NoError(t, checkEgress("tcp4", "169.254.169.254:80", nil)) + require.NoError(t, checkEgress("tcp4", "127.0.0.1:80", nil)) } func TestWithEgressGuard(t *testing.T) { - setEgressEnv(t, "", "", "") + setEgressEnv(t, "block_restricted", "", "") listener, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) t.Cleanup(func() { _ = listener.Close() }) @@ -396,7 +423,7 @@ func TestWithEgressGuardProxyBlocksPrivateTargets(t *testing.T) { // proxy itself: a direct request to the proxy's address must not inherit the dial exemption the // proxy gets once it has been used. func TestWithEgressGuardProxyAddressNotReachableDirectly(t *testing.T) { - setEgressEnv(t, "", "", "") + setEgressEnv(t, "block_restricted", "", "") proxy := newRecordingProxy(t) transport := &http.Transport{Proxy: func(req *http.Request) (*url.URL, error) { if req.URL.Host == proxy.url.Host { @@ -419,7 +446,7 @@ func TestWithEgressGuardProxyAddressNotReachableDirectly(t *testing.T) { // TestWithEgressGuardNoProxyStillGuardsDial covers NO_PROXY: when the proxy function declines a // target, the direct dial keeps the resolved-address check. func TestWithEgressGuardNoProxyStillGuardsDial(t *testing.T) { - setEgressEnv(t, "", "", "") + setEgressEnv(t, "block_restricted", "", "") target := newRecordingProxy(t) transport := &http.Transport{Proxy: func(*http.Request) (*url.URL, error) { return nil, nil }} withEgressGuard(&stubNetworkResolver{})(transport) @@ -449,7 +476,7 @@ func TestWithEgressGuardProxyPrivateAllowed(t *testing.T) { func TestParseEgressPolicy(t *testing.T) { policy := parseEgressPolicy("", "", "10.0.0.0/8 ::ffff:172.16.0.0/108, 192.168.1.10, fe80::1%eth0, *.svc.cluster.local, Hooks.Example.COM., bad host, -, *.") - assert.Equal(t, egressBlockRestricted, policy.level) + assert.Equal(t, egressAllowAll, policy.level) assert.Equal(t, []netip.Prefix{ netip.MustParsePrefix("10.0.0.0/8"), netip.MustParsePrefix("172.16.0.0/12"), diff --git a/src/jobservice/job/impl/notification/ssrf_test.go b/src/jobservice/job/impl/notification/ssrf_test.go index 2c5d85370b..5b7330d0cc 100644 --- a/src/jobservice/job/impl/notification/ssrf_test.go +++ b/src/jobservice/job/impl/notification/ssrf_test.go @@ -123,10 +123,10 @@ func TestNotificationClientsKeepProxy(t *testing.T) { } // TestNotificationClientsGuardRestrictedTargets asserts the production notification clients carry -// the egress dial guard: with the default policy, which allows private networks, a loopback target +// the egress dial guard: under block_restricted, which allows private networks, a loopback target // is still refused before any request reaches it. func TestNotificationClientsGuardRestrictedTargets(t *testing.T) { - t.Setenv("HARBOR_WEBHOOK_EGRESS_POLICY", "") + t.Setenv("HARBOR_WEBHOOK_EGRESS_POLICY", "block_restricted") t.Setenv("HARBOR_ALLOW_PRIVATE_NETWORK_ACCESS", "") t.Setenv("HARBOR_WEBHOOK_EGRESS_ALLOWLIST", "")