From f7dc8e7cf4f343ee590a3cb6c316ce93b30f6860 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 12 Oct 2026 15:48:08 -0400 Subject: [PATCH 1/3] Let OAuth reach Basecamp's own hosts through fake-IP DNS Since v0.10 (#656) every OAuth request is judged by DefaultIssuerPolicy, which refuses 198.18.0.0/15. Fake-IP DNS (mihomo/Clash, sing-box, the egress of hosted agent machines such as Grok's) resolves every name into that range, so on those networks login and token refresh to app.basecamp.com and launchpad.37signals.com are refused before a byte is sent, and an agent that had been working locks out at its first refresh. #656 guards against a discovery document steering credential-bearing requests into private space. Our own hostnames are not something a discovery document chooses, so requests to a fixed, compiled-in set of first-party hosts over HTTPS on 443 now use surfguard's default deny tables plus the fake-IP range, instead of the IANA special-use table, which surfguard's Allow cannot override. Every other host, every redirect hop to another host, and anything a base URL override names keep the lane's strict policy. Private, loopback, link-local and metadata, CGNAT and multicast addresses stay refused for first-party hosts too, and TLS still has to verify the first-party name. Fixes #849, #867, #868. --- internal/auth/client.go | 94 +++++++++++++--- internal/auth/client_test.go | 211 +++++++++++++++++++++++++++++++++++ 2 files changed, 289 insertions(+), 16 deletions(-) diff --git a/internal/auth/client.go b/internal/auth/client.go index 0d7fc81f9..960b06384 100644 --- a/internal/auth/client.go +++ b/internal/auth/client.go @@ -3,6 +3,7 @@ package auth import ( "fmt" "net/http" + "net/netip" "net/url" "os" "strings" @@ -76,6 +77,52 @@ func oauthEndpointPolicy(anchorURL, label string) (surfguard.Policy, error) { return policy, nil } +// firstPartyOAuthHosts are the hosts 37signals operates that the OAuth flows +// reach: the BC5 API and its issuer under both the original and the current +// domains (credentials stored before the move name +// https://3.basecamp.com/oauth/tokens), and Launchpad. The set is fixed at +// build time. Neither a configured base URL nor anything a discovery document +// names can add to it, which is what keeps firstPartyOAuthPolicy from ever +// reaching an endpoint a remote peer chose. +var firstPartyOAuthHosts = map[string]bool{ + "3.basecampapi.com": true, + "api.basecamp.com": true, + "3.basecamp.com": true, + "app.basecamp.com": true, + "launchpad.37signals.com": true, +} + +// fakeIPRange is 198.18.0.0/15, the RFC 2544 benchmarking block. Fake-IP DNS +// (mihomo/Clash Meta and sing-box in fake-ip mode, and the egress of hosted +// agent machines such as Grok's) answers every name from it and carries the +// connection on to the real host, so on those networks app.basecamp.com +// resolves here. +var fakeIPRange = netip.MustParsePrefix("198.18.0.0/15") + +// firstPartyOAuthPolicy is the address policy for requests to +// firstPartyOAuthHosts: surfguard's default deny tables (private, loopback, +// link-local and cloud metadata, CGNAT, multicast) with the fake-IP range +// admitted, on port 443 only. It leaves out IANASpecialUse because surfguard's +// Allow cannot override that table, and the only other blocks that table adds +// to the default ones are globally reachable anycast services (AS112, AMT), +// so the fake-IP range is the one meaningful admission. TLS still verifies the certificate +// against the first-party name, so a connection that lands anywhere else fails +// its handshake before a credential is sent. +func firstPartyOAuthPolicy() surfguard.Policy { + return surfguard.Policy{}.Allow(fakeIPRange).AllowPorts(443) +} + +// isFirstPartyOAuthURL reports whether u is an HTTPS request on the default +// port to one of firstPartyOAuthHosts. Plain HTTP (no certificate vouches for +// the answer), an explicit non-default port, and near-miss spellings such as a +// trailing dot all keep the lane's own policy. +func isFirstPartyOAuthURL(u *url.URL) bool { + if u.Scheme != "https" || (u.Port() != "" && u.Port() != "443") { + return false + } + return firstPartyOAuthHosts[strings.ToLower(u.Hostname())] +} + // proxyEnvState is the Manager's one construction-time snapshot of the proxy // environment: the resolver both lanes route and warn against, and whether // the operator opted OAuth traffic out of address enforcement when a proxy @@ -127,10 +174,14 @@ func (m *Manager) warnf(format string, args ...any) { } // oauthTransport is the per-lane egress RoundTripper for OAuth traffic. It -// owns two sub-transports: the lane's address-policed direct transport -// (surfguard dial-time enforcement; Proxy nil by construction), and — only -// when the operator set BASECAMP_OAUTH_USE_PROXY=1 — a proxying clone of -// http.DefaultTransport. +// owns two address-policed direct transports (surfguard dial-time +// enforcement; Proxy nil by construction): the lane's own, and one under +// firstPartyOAuthPolicy for firstPartyOAuthHosts. Only when the operator set +// BASECAMP_OAUTH_USE_PROXY=1 does it also own a proxying clone of +// http.DefaultTransport. Which direct transport carries a request is decided +// from that request's URL, so every redirect hop is judged afresh, and a +// lane's anchor never lends the first-party allowance to an endpoint a +// discovery document named. // // Routing is per-request and fail-closed: // @@ -155,11 +206,12 @@ func (m *Manager) warnf(format string, args ...any) { // rebuilt per call; idle connections are bounded by each sub-transport's // IdleConnTimeout, so a process-lifetime CLI Manager needs no explicit Close. type oauthTransport struct { - guarded http.RoundTripper - proxied http.RoundTripper - resolve func(*url.URL) (*url.URL, error) - optOut bool - warnf func(format string, args ...any) + guarded http.RoundTripper + firstParty http.RoundTripper + proxied http.RoundTripper + resolve func(*url.URL) (*url.URL, error) + optOut bool + warnf func(format string, args ...any) mu sync.Mutex warned map[string]bool @@ -172,7 +224,7 @@ func (t *oauthTransport) RoundTrip(req *http.Request) (*http.Response, error) { } switch { case proxyURL == nil: - return t.guarded.RoundTrip(req) + return t.direct(req.URL).RoundTrip(req) case t.optOut: // Deduplicated per endpoint: the device poll re-POSTs the same URL // every few seconds, and one downgrade notice per endpoint records @@ -183,8 +235,17 @@ func (t *oauthTransport) RoundTrip(req *http.Request) (*http.Response, error) { default: t.warnOnce(req.URL, "warning: ignoring proxy %s for OAuth request to %s: the SSRF address policy requires direct egress; set %s=1 to route OAuth through the proxy without address enforcement", redactedProxy(proxyURL), redactedEndpoint(req.URL), oauthUseProxyEnv) - return t.guarded.RoundTrip(req) + return t.direct(req.URL).RoundTrip(req) + } +} + +// direct picks the address-policed transport for a request that is not going +// through a proxy. +func (t *oauthTransport) direct(u *url.URL) http.RoundTripper { + if isFirstPartyOAuthURL(u) { + return t.firstParty } + return t.guarded } func (t *oauthTransport) warnOnce(u *url.URL, format string, args ...any) { @@ -284,11 +345,12 @@ func (m *Manager) buildLaneClient(anchorURL, label string) (*http.Client, error) } st := m.proxyState() tr := &oauthTransport{ - guarded: policy.RoundTripper(), - resolve: st.resolve, - optOut: st.optOut, - warnf: m.warnf, - warned: map[string]bool{}, + guarded: policy.RoundTripper(), + firstParty: firstPartyOAuthPolicy().RoundTripper(), + resolve: st.resolve, + optOut: st.optOut, + warnf: m.warnf, + warned: map[string]bool{}, } if st.optOut { tr.proxied = proxiedTransport(st.resolve) diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go index 275f010de..c6455a1c2 100644 --- a/internal/auth/client_test.go +++ b/internal/auth/client_test.go @@ -647,3 +647,214 @@ func TestWarnf_SanitizesRenderedMessage(t *testing.T) { assert.Contains(t, w, "evil31m.example", "C1 control stripped, host otherwise intact") } } + +// TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace walks one +// address from every block in surfguard's IANA special-purpose snapshot (the +// table DefaultIssuerPolicy applies and firstPartyOAuthPolicy leaves out). The +// strict policy must refuse every one. The first-party policy may admit only +// the fake-IP range and the globally reachable anycast service blocks, which +// pins exactly what dropping the table opens up. +func TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace(t *testing.T) { + ianaSpecialUse := []string{ + "0.0.0.0/8", "0.0.0.0/32", "10.0.0.0/8", "100.64.0.0/10", "127.0.0.0/8", + "169.254.0.0/16", "172.16.0.0/12", "192.0.0.0/24", "192.0.0.0/29", + "192.0.0.8/32", "192.0.0.9/32", "192.0.0.10/32", "192.0.0.170/32", + "192.0.0.171/32", "192.0.2.0/24", "192.31.196.0/24", "192.52.193.0/24", + "192.88.99.0/24", "192.88.99.2/32", "192.168.0.0/16", "192.175.48.0/24", + "198.18.0.0/15", "198.51.100.0/24", "203.0.113.0/24", "240.0.0.0/4", + "255.255.255.255/32", + "::1/128", "::/128", "::ffff:0:0/96", "64:ff9b::/96", "64:ff9b:1::/48", + "100::/64", "100:0:0:1::/64", "2001::/23", "2001::/32", "2001:1::1/128", + "2001:1::2/128", "2001:1::3/128", "2001:2::/48", "2001:3::/32", + "2001:4:112::/48", "2001:10::/28", "2001:20::/28", "2001:30::/28", + "2001:db8::/32", "2002::/16", "2620:4f:8000::/48", "3fff::/20", + "5f00::/16", "fc00::/7", "fe80::/10", + } + admitted := map[string]bool{ + "198.18.0.0/15": true, + // AS112 and AMT: anycast services on the public internet. + "192.31.196.0/24": true, + "192.52.193.0/24": true, + "192.175.48.0/24": true, + "2001:3::/32": true, + "2001:4:112::/48": true, + "2620:4f:8000::/48": true, + } + + strict := oauth.DefaultIssuerPolicy() + firstParty := firstPartyOAuthPolicy() + for _, block := range ianaSpecialUse { + addr := netip.MustParsePrefix(block).Addr() + assert.True(t, strict.Blocked(addr), "strict policy must refuse %s (%s)", addr, block) + assert.Equal(t, !admitted[block], firstParty.Blocked(addr), "first-party policy on %s (%s)", addr, block) + } +} + +// TestFirstPartyOAuthPolicy_StillRefusesInternalTargets names the targets an +// SSRF is after, including their IPv6 and NAT64 spellings, and checks both +// ends of the fake-IP range are admitted. +func TestFirstPartyOAuthPolicy_StillRefusesInternalTargets(t *testing.T) { + firstParty := firstPartyOAuthPolicy() + for _, a := range []string{ + "10.0.0.1", "172.16.0.1", "192.168.1.1", "127.0.0.1", "100.64.0.1", + "169.254.169.254", "168.63.129.16", "0.0.0.0", "224.0.0.1", "255.255.255.255", + "::1", "fe80::1", "fc00::1", "fd00:ec2::254", + "::ffff:10.0.0.1", "64:ff9b::a00:1", + } { + assert.True(t, firstParty.Blocked(netip.MustParseAddr(a)), "first-party policy must refuse %s", a) + } + for _, a := range []string{"198.18.0.0", "198.18.0.1", "198.19.255.255"} { + assert.False(t, firstParty.Blocked(netip.MustParseAddr(a)), "first-party policy must admit %s", a) + } +} + +// TestFirstPartyOAuthPolicy_DialGate drives the check surfguard runs as the +// socket connects: the fake-IP address is reachable on 443 only under the +// first-party policy, and the strict policy still refuses it, which is the +// failure #849 reports. +func TestFirstPartyOAuthPolicy_DialGate(t *testing.T) { + firstParty := firstPartyOAuthPolicy() + require.NoError(t, firstParty.Control("tcp4", "198.18.0.1:443", nil)) + assert.ErrorIs(t, firstParty.Control("tcp4", "198.18.0.1:80", nil), surfguard.ErrBlocked, "443 only") + assert.ErrorIs(t, firstParty.Control("tcp4", "10.0.0.1:443", nil), surfguard.ErrBlocked) + assert.ErrorIs(t, firstParty.Control("tcp4", "169.254.169.254:443", nil), surfguard.ErrBlocked) + + assert.ErrorIs(t, oauth.DefaultIssuerPolicy().Control("tcp4", "198.18.0.1:443", nil), surfguard.ErrBlocked) +} + +func TestIsFirstPartyOAuthURL(t *testing.T) { + cases := []struct { + url string + want bool + }{ + {"https://app.basecamp.com/oauth/tokens", true}, + {"https://3.basecamp.com/oauth/tokens", true}, + {"https://3.basecampapi.com/.well-known/oauth-protected-resource", true}, + {"https://api.basecamp.com/.well-known/oauth-protected-resource", true}, + {"https://launchpad.37signals.com/authorization/token", true}, + {"https://APP.Basecamp.com/oauth/tokens", true}, + {"https://app.basecamp.com:443/oauth/tokens", true}, + + {"http://app.basecamp.com/oauth/tokens", false}, + {"https://app.basecamp.com:8443/oauth/tokens", false}, + {"https://app.basecamp.com./oauth/tokens", false}, + {"https://evil.app.basecamp.com/oauth/tokens", false}, + {"https://app.basecamp.com.evil.example/oauth/tokens", false}, + {"https://basecamp.com/oauth/tokens", false}, + {"https://3.basecamp.localhost/oauth/tokens", false}, + {"https://198.18.0.1/oauth/tokens", false}, + {"https://issuer.example/oauth/tokens", false}, + } + for _, tc := range cases { + u, err := url.Parse(tc.url) + require.NoError(t, err) + assert.Equal(t, tc.want, isFirstPartyOAuthURL(u), tc.url) + } +} + +// TestOAuthTransport_PicksTheDirectTransportByRequestHost: the first-party +// transport carries only first-party URLs, whatever the proxy settings say +// about other requests. A foreign host, including a fake-IP literal or a +// look-alike name, stays on the lane's own transport, and opting out to a +// proxy still sends everything through the proxy. +func TestOAuthTransport_PicksTheDirectTransportByRequestHost(t *testing.T) { + noProxy := func(*url.URL) (*url.URL, error) { return nil, nil } + proxy := func(*url.URL) (*url.URL, error) { return url.Parse("http://proxy.corp.example:3128") } + + send := func(t *testing.T, tr *oauthTransport, rawURL string) { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, rawURL, nil) + require.NoError(t, err) + _, _ = tr.RoundTrip(req) + } + build := func(resolve func(*url.URL) (*url.URL, error), optOut bool) (*oauthTransport, *countingTransport, *countingTransport, *countingTransport) { + guarded, firstParty, proxied := &countingTransport{}, &countingTransport{}, &countingTransport{} + return &oauthTransport{ + guarded: guarded, + firstParty: firstParty, + proxied: proxied, + resolve: resolve, + optOut: optOut, + warnf: func(string, ...any) {}, + warned: map[string]bool{}, + }, guarded, firstParty, proxied + } + + t.Run("no proxy", func(t *testing.T) { + tr, guarded, firstParty, _ := build(noProxy, false) + send(t, tr, "https://app.basecamp.com/oauth/tokens") + send(t, tr, "https://launchpad.37signals.com/authorization/token") + assert.Equal(t, int32(2), firstParty.calls.Load()) + assert.Zero(t, guarded.calls.Load()) + + send(t, tr, "https://issuer.example/oauth/tokens") + send(t, tr, "https://198.18.0.1/oauth/tokens") + send(t, tr, "https://app.basecamp.com.evil.example/oauth/tokens") + send(t, tr, "http://app.basecamp.com/oauth/tokens") + assert.Equal(t, int32(4), guarded.calls.Load()) + assert.Equal(t, int32(2), firstParty.calls.Load()) + }) + + t.Run("proxy ignored in protected mode", func(t *testing.T) { + tr, guarded, firstParty, proxied := build(proxy, false) + send(t, tr, "https://app.basecamp.com/oauth/tokens") + send(t, tr, "https://issuer.example/oauth/tokens") + assert.Equal(t, int32(1), firstParty.calls.Load()) + assert.Equal(t, int32(1), guarded.calls.Load()) + assert.Zero(t, proxied.calls.Load()) + }) + + t.Run("opt-out sends first-party traffic through the proxy too", func(t *testing.T) { + tr, guarded, firstParty, proxied := build(proxy, true) + send(t, tr, "https://app.basecamp.com/oauth/tokens") + assert.Equal(t, int32(1), proxied.calls.Load()) + assert.Zero(t, firstParty.calls.Load()) + assert.Zero(t, guarded.calls.Load()) + }) +} + +// TestFirstPartyRedirectToAForeignHostIsJudgedByTheLanePolicy: a first-party +// endpoint that redirects elsewhere does not carry its allowance along. The +// next hop is chosen by its own URL and lands on the production lane's strict +// transport, which refuses a fake-IP literal with zero dials. +func TestFirstPartyRedirectToAForeignHostIsJudgedByTheLanePolicy(t *testing.T) { + clearProxyEnv(t) + + m := laneManager(t, "https://3.basecampapi.com") + client, err := m.bc5Client() + require.NoError(t, err) + tr, ok := client.Transport.(*oauthTransport) + require.True(t, ok) + require.NotNil(t, tr.firstParty, "the lane must wire the first-party transport") + + firstParty := &countingTransport{resp: func() (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusFound, + Header: http.Header{"Location": {"https://198.18.0.1/.well-known/oauth-authorization-server"}}, + Body: http.NoBody, + }, nil + }} + tr.firstParty = firstParty + + resp, err := doGet(t, client, "https://app.basecamp.com/.well-known/oauth-authorization-server") + if resp != nil { + _ = resp.Body.Close() + } + require.Error(t, err) + assert.ErrorIs(t, err, surfguard.ErrBlocked) + assert.Equal(t, int32(1), firstParty.calls.Load(), "only the first-party hop rides the first-party transport") +} + +// TestLaunchpadLane_WiresTheFirstPartyTransport: the Launchpad lane carries +// the web-flow code exchange and Launchpad refreshes, which #849 shows failing +// the same way, so it needs the same transport as the BC5 lane. +func TestLaunchpadLane_WiresTheFirstPartyTransport(t *testing.T) { + clearProxyEnv(t) + + m := laneManager(t, "https://3.basecampapi.com") + client, err := m.launchpadClient() + require.NoError(t, err) + tr, ok := client.Transport.(*oauthTransport) + require.True(t, ok) + assert.NotNil(t, tr.firstParty) +} From 8be8c4508c0ecc67c096767b6db1af63cd1622c5 Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 12 Oct 2026 16:06:45 -0400 Subject: [PATCH 2/3] Test the first-party policy each lane installs, and name NAT64 The policy tests exercised firstPartyOAuthPolicy directly, so wiring the wrong policy into buildLaneClient passed the whole suite: the strict one (fake-IP still refused) or one admitting loopback. Drive the transport each lane actually builds instead, with dial-time refusals that never reach the network. Leaving out the IANA table also lets surfguard decode NAT64 well-known addresses and judge the embedded IPv4, so a first-party host is reachable through DNS64 at a public or fake-IP address. Say so, and pin it with private embedded addresses still refused. --- internal/auth/client.go | 7 +-- internal/auth/client_test.go | 94 ++++++++++++++++++++++++++++++------ 2 files changed, 82 insertions(+), 19 deletions(-) diff --git a/internal/auth/client.go b/internal/auth/client.go index 960b06384..d34ed2523 100644 --- a/internal/auth/client.go +++ b/internal/auth/client.go @@ -103,9 +103,10 @@ var fakeIPRange = netip.MustParsePrefix("198.18.0.0/15") // firstPartyOAuthHosts: surfguard's default deny tables (private, loopback, // link-local and cloud metadata, CGNAT, multicast) with the fake-IP range // admitted, on port 443 only. It leaves out IANASpecialUse because surfguard's -// Allow cannot override that table, and the only other blocks that table adds -// to the default ones are globally reachable anycast services (AS112, AMT), -// so the fake-IP range is the one meaningful admission. TLS still verifies the certificate +// Allow cannot override that table. Leaving it out also admits globally +// reachable anycast services (AS112, AMT), and NAT64 addresses whose embedded +// IPv4 is itself admitted, which is how an IPv6-only network with DNS64 +// reaches a public host. TLS still verifies the certificate // against the first-party name, so a connection that lands anywhere else fails // its handshake before a credential is sent. func firstPartyOAuthPolicy() surfguard.Policy { diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go index c6455a1c2..9bac6f03d 100644 --- a/internal/auth/client_test.go +++ b/internal/auth/client_test.go @@ -648,12 +648,13 @@ func TestWarnf_SanitizesRenderedMessage(t *testing.T) { } } -// TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace walks one -// address from every block in surfguard's IANA special-purpose snapshot (the -// table DefaultIssuerPolicy applies and firstPartyOAuthPolicy leaves out). The -// strict policy must refuse every one. The first-party policy may admit only -// the fake-IP range and the globally reachable anycast service blocks, which -// pins exactly what dropping the table opens up. +// TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace takes the +// first address of every block in surfguard's IANA special-purpose snapshot +// (the table DefaultIssuerPolicy applies and firstPartyOAuthPolicy leaves +// out). The strict policy must refuse every one. The first-party policy may +// admit only the fake-IP range and the globally reachable anycast service +// blocks. The NAT64 prefix, whose meaning is the IPv4 address it embeds, is +// covered by TestFirstPartyOAuthPolicy_JudgesTranslatedIPv4ByItsEmbeddedAddress. func TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace(t *testing.T) { ianaSpecialUse := []string{ "0.0.0.0/8", "0.0.0.0/32", "10.0.0.0/8", "100.64.0.0/10", "127.0.0.0/8", @@ -708,6 +709,35 @@ func TestFirstPartyOAuthPolicy_StillRefusesInternalTargets(t *testing.T) { } } +// TestFirstPartyOAuthPolicy_JudgesTranslatedIPv4ByItsEmbeddedAddress: the +// strict policy refuses the NAT64 well-known prefix whole, because it is in +// the IANA table. Without that table, surfguard decodes the embedded IPv4 +// address and judges that instead, as both policies already do for SIIT. So a +// first-party host on an IPv6-only network with DNS64 is reachable through +// NAT64 at its real public address or a fake-IP one, while an embedded private +// address is still refused. +func TestFirstPartyOAuthPolicy_JudgesTranslatedIPv4ByItsEmbeddedAddress(t *testing.T) { + strict := oauth.DefaultIssuerPolicy() + firstParty := firstPartyOAuthPolicy() + cases := []struct { + addr string + admitted bool + }{ + {"64:ff9b::808:808", true}, // NAT64 of 8.8.8.8 + {"64:ff9b::c612:1", true}, // NAT64 of 198.18.0.1 + {"64:ff9b::a00:1", false}, // NAT64 of 10.0.0.1 + {"64:ff9b::a9fe:a9fe", false}, // NAT64 of 169.254.169.254 + {"64:ff9b::7f00:1", false}, // NAT64 of 127.0.0.1 + {"::ffff:0:a00:1", false}, // SIIT of 10.0.0.1 + {"64:ff9b:1::808:808", false}, // local-use NAT64, refused whole + } + for _, tc := range cases { + addr := netip.MustParseAddr(tc.addr) + assert.True(t, strict.Blocked(addr), "strict policy must refuse %s", tc.addr) + assert.Equal(t, !tc.admitted, firstParty.Blocked(addr), "first-party policy on %s", tc.addr) + } +} + // TestFirstPartyOAuthPolicy_DialGate drives the check surfguard runs as the // socket connects: the fake-IP address is reachable on 443 only under the // first-party policy, and the strict policy still refuses it, which is the @@ -845,16 +875,48 @@ func TestFirstPartyRedirectToAForeignHostIsJudgedByTheLanePolicy(t *testing.T) { assert.Equal(t, int32(1), firstParty.calls.Load(), "only the first-party hop rides the first-party transport") } -// TestLaunchpadLane_WiresTheFirstPartyTransport: the Launchpad lane carries -// the web-flow code exchange and Launchpad refreshes, which #849 shows failing -// the same way, so it needs the same transport as the BC5 lane. -func TestLaunchpadLane_WiresTheFirstPartyTransport(t *testing.T) { +// TestLanes_InstallTheFirstPartyPolicy drives the first-party transport each +// lane actually builds, not the policy helper, so wiring the wrong policy into +// buildLaneClient fails here. Every case is decided by surfguard's dial-time +// check before connect, so nothing touches the network: a fake-IP address on +// port 80 is refused for its port, which means its address was admitted, and +// loopback and private targets are refused for their address. The local-anchor +// case shows a local base URL's loopback allowance does not reach the +// first-party transport. +func TestLanes_InstallTheFirstPartyPolicy(t *testing.T) { clearProxyEnv(t) - m := laneManager(t, "https://3.basecampapi.com") - client, err := m.launchpadClient() - require.NoError(t, err) - tr, ok := client.Transport.(*oauthTransport) - require.True(t, ok) - assert.NotNil(t, tr.firstParty) + refusal := func(t *testing.T, rt http.RoundTripper, rawURL string) surfguard.Reason { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, rawURL, nil) + require.NoError(t, err) + resp, err := rt.RoundTrip(req) + if resp != nil { + _ = resp.Body.Close() + } + var violation *surfguard.Violation + require.ErrorAs(t, err, &violation, "%s must be refused by the address policy", rawURL) + return violation.Reason + } + check := func(t *testing.T, client *http.Client) { + t.Helper() + tr, ok := client.Transport.(*oauthTransport) + require.True(t, ok) + assert.Equal(t, surfguard.ReasonPort, refusal(t, tr.firstParty, "https://198.18.0.1:80/oauth/tokens")) + assert.Equal(t, surfguard.ReasonBlockedAddr, refusal(t, tr.firstParty, "https://127.0.0.1:80/oauth/tokens")) + assert.Equal(t, surfguard.ReasonBlockedAddr, refusal(t, tr.firstParty, "https://10.0.0.1/oauth/tokens")) + assert.Equal(t, surfguard.ReasonBlockedAddr, refusal(t, tr.firstParty, "https://169.254.169.254/oauth/tokens")) + } + + for _, baseURL := range []string{"https://3.basecampapi.com", "http://3.basecamp.localhost:3001"} { + t.Run(baseURL, func(t *testing.T) { + m := laneManager(t, baseURL) + bc5, err := m.bc5Client() + require.NoError(t, err) + check(t, bc5) + lp, err := m.launchpadClient() + require.NoError(t, err) + check(t, lp) + }) + } } From 6f6abddd05e994df4044a2e19ac77f388b2d906d Mon Sep 17 00:00:00 2001 From: Rob Zolkos Date: Mon, 12 Oct 2026 18:24:03 -0400 Subject: [PATCH 3/3] Prove the first-party transport verifies certificates The first-party policy admits the fake-IP range on the strength of TLS: a connection that lands anywhere but the real host fails its handshake before a credential is sent. Nothing tested that, so an insecure TLSClientConfig on that transport would have left the suite green. Drive each lane's first-party transport against a loopback server with an untrusted certificate and require the handshake to fail on verification, with no request reaching the server. The policy refuses loopback, so the Manager gains a test seam for it, like clock. Also pin the URL spellings the host match must get right: userinfo, an empty port, :0443, a homograph and its punycode, and IPv6 literals. --- internal/auth/auth.go | 7 +++++ internal/auth/client.go | 11 +++++++- internal/auth/client_test.go | 54 ++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/internal/auth/auth.go b/internal/auth/auth.go index f1e080510..06199b6bd 100644 --- a/internal/auth/auth.go +++ b/internal/auth/auth.go @@ -19,6 +19,7 @@ import ( "github.com/basecamp/basecamp-sdk/go/pkg/basecamp" "github.com/basecamp/basecamp-sdk/go/pkg/basecamp/oauth" "github.com/basecamp/cli/pkce" + surfguard "github.com/basecamp/surfguard/go" "github.com/basecamp/basecamp-cli/internal/config" "github.com/basecamp/basecamp-cli/internal/hostutil" @@ -108,6 +109,12 @@ type Manager struct { // against (renewal_hold.go). Test seam; nil means time.Now. clock func() time.Time + // firstPartyPolicy is the address policy the lanes' first-party + // transport enforces (client.go). Test seam, so a test can reach a + // loopback server through that transport; nil means + // firstPartyOAuthPolicy. + firstPartyPolicy func() surfguard.Policy + mu sync.Mutex // kindMu guards kind, which is read while building an error's remedy diff --git a/internal/auth/client.go b/internal/auth/client.go index d34ed2523..072e819c2 100644 --- a/internal/auth/client.go +++ b/internal/auth/client.go @@ -113,6 +113,15 @@ func firstPartyOAuthPolicy() surfguard.Policy { return surfguard.Policy{}.Allow(fakeIPRange).AllowPorts(443) } +// firstPartyOAuthPolicy is the Manager's first-party policy: the +// firstPartyPolicy field when a test set one. +func (m *Manager) firstPartyOAuthPolicy() surfguard.Policy { + if m.firstPartyPolicy != nil { + return m.firstPartyPolicy() + } + return firstPartyOAuthPolicy() +} + // isFirstPartyOAuthURL reports whether u is an HTTPS request on the default // port to one of firstPartyOAuthHosts. Plain HTTP (no certificate vouches for // the answer), an explicit non-default port, and near-miss spellings such as a @@ -347,7 +356,7 @@ func (m *Manager) buildLaneClient(anchorURL, label string) (*http.Client, error) st := m.proxyState() tr := &oauthTransport{ guarded: policy.RoundTripper(), - firstParty: firstPartyOAuthPolicy().RoundTripper(), + firstParty: m.firstPartyOAuthPolicy().RoundTripper(), resolve: st.resolve, optOut: st.optOut, warnf: m.warnf, diff --git a/internal/auth/client_test.go b/internal/auth/client_test.go index 9bac6f03d..ad9910fd7 100644 --- a/internal/auth/client_test.go +++ b/internal/auth/client_test.go @@ -2,6 +2,7 @@ package auth import ( "context" + "crypto/tls" "errors" "fmt" "net/http" @@ -764,9 +765,17 @@ func TestIsFirstPartyOAuthURL(t *testing.T) { {"https://launchpad.37signals.com/authorization/token", true}, {"https://APP.Basecamp.com/oauth/tokens", true}, {"https://app.basecamp.com:443/oauth/tokens", true}, + {"https://app.basecamp.com:/oauth/tokens", true}, + {"https://user@app.basecamp.com/oauth/tokens", true}, {"http://app.basecamp.com/oauth/tokens", false}, {"https://app.basecamp.com:8443/oauth/tokens", false}, + {"https://app.basecamp.com:0443/oauth/tokens", false}, + {"https://app.basecamp.com@issuer.example/oauth/tokens", false}, + {"https://аpp.basecamp.com/oauth/tokens", false}, + {"https://xn--pp-6kc.basecamp.com/oauth/tokens", false}, + {"https://[::ffff:198.18.0.1]/oauth/tokens", false}, + {"https://[64:ff9b::c612:1]/oauth/tokens", false}, {"https://app.basecamp.com./oauth/tokens", false}, {"https://evil.app.basecamp.com/oauth/tokens", false}, {"https://app.basecamp.com.evil.example/oauth/tokens", false}, @@ -920,3 +929,48 @@ func TestLanes_InstallTheFirstPartyPolicy(t *testing.T) { }) } } + +// TestLanes_FirstPartyTransportVerifiesCertificates pins the premise the +// first-party policy rests on: a connection that lands anywhere but the real +// host fails its TLS handshake before a credential is sent. It drives the +// first-party transport each lane builds against a local server presenting a +// certificate no system root trusts, so a TLSClientConfig that skips +// verification, on that transport or under it, fails here. The policy seam +// admits loopback only so the dial can reach that server. Everything else is +// the lane as buildLaneClient wires it. +func TestLanes_FirstPartyTransportVerifiesCertificates(t *testing.T) { + clearProxyEnv(t) + + var reached atomic.Int32 + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached.Add(1) + })) + t.Cleanup(srv.Close) + + m := laneManager(t, "https://3.basecampapi.com") + m.firstPartyPolicy = func() surfguard.Policy { return firstPartyOAuthPolicy().AllowLoopback() } + + check := func(t *testing.T, client *http.Client) { + t.Helper() + tr, ok := client.Transport.(*oauthTransport) + require.True(t, ok) + req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, srv.URL+"/oauth/tokens", + strings.NewReader("grant_type=refresh_token&refresh_token=secret")) + require.NoError(t, err) + resp, err := tr.firstParty.RoundTrip(req) + if resp != nil { + _ = resp.Body.Close() + } + var unverified *tls.CertificateVerificationError + require.ErrorAs(t, err, &unverified, "the handshake must fail on the certificate, not before the dial") + } + + bc5, err := m.bc5Client() + require.NoError(t, err) + check(t, bc5) + lp, err := m.launchpadClient() + require.NoError(t, err) + check(t, lp) + + assert.Zero(t, reached.Load(), "no request may reach a server whose certificate failed verification") +}