Repository navigation
Let OAuth reach Basecamp's own hosts through fake-IP DNS - #889
Conversation
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
You’re at about 95% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
There was a problem hiding this comment.
🔵 Needs a closer look
The implementation appears sound, but changing OAuth credential egress and SSRF boundaries warrants final human security review.
0 open findings
What changed in this PR
Allows OAuth to reach trusted Basecamp hosts through fake-IP DNS while retaining strict SSRF controls elsewhere.
Changes:
- Adds a host- and port-scoped policy for
198.18.0.0/15. - Routes each OAuth request and redirect through the appropriate transport.
- Adds policy, routing, redirect, and lane-wiring tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/auth/client.go |
Adds and routes the restricted first-party OAuth policy. |
internal/auth/client_test.go |
Tests admitted addresses, routing boundaries, and redirects. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
jeremy
left a comment
jeremy
left a comment
There was a problem hiding this comment.
🤖 Security review, including an adversarial pass. No blocking findings at 8be8c45.
Checked and holding:
- Who gains what. The relaxed policy only gets a request to a TCP connect and TLS handshake at
198.18.0.0/15. A credential is sent only after the peer presents a certificate trusted for the exact first-party name. The actor who controls fake-IP DNS could already answer with an ordinary public address under the old policy, so this PR gives them nothing new. - Host matching (
isFirstPartyOAuthURL) uses the sameHostname()/Port()the dialer uses. Trailing dot, IDN/punycode, IPv6 literals, subdomains, look-alike suffixes and:0443all fall through to the strict lane. A danglinghost:matches but dials 443, and token endpoints already reject userinfo. - Per-hop selection. Token, refresh and device POSTs don't follow redirects. GET redirects go back through
oauthTransport, so each hop is judged by its own URL. A discovery document can name a first-party host, but the traffic then goes to that host under TLS, not back to whoever wrote the document. - Separate pools. The strict and first-party transports keep separate connection pools. No proxy is used unless
BASECAMP_OAUTH_USE_PROXY=1is set, and that path is unchanged. - NAT64 and anycast. NAT64 is admitted only when the IPv4 it embeds is admitted, and the local-use prefix stays refused. Leaving out
IANASpecialUsealso admits AS112/AMT, as the comment says, and TLS still applies there. - Stored and exchanged endpoints are validated before use, so an http or non-443 first-party endpoint never gets the relaxation.
One non-blocking suggestion: the argument rests on TLS verification, and no test proves the first-party transport verifies certificates. A later InsecureSkipVerify or custom TLSClientConfig on that path would leave these tests green. A test that fails a handshake against a self-signed server on the first-party transport would lock that in. Cheaper: assert the transport carries no insecure TLS config. Optionally, add isFirstPartyOAuthURL table rows for userinfo, host:, :0443, Unicode/IDN and an IPv6 literal.
go test ./internal/auth/ passes locally; CI is green.
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.
|
@jeremy thanks, both suggestions are in 6f6abdd. TLS verification. I took the handshake test rather than the config assertion, because a config check only sees the one spelling it inspects. I broke the code three ways and the test caught all three:
URL table.
|
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
You’re at about 99% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/auth/client_test.go">
<violation number="1" location="internal/auth/client_test.go:951">
P2: The test server listens on an ephemeral port, but this policy permits only port 443; `AllowLoopback()` does not relax that port restriction, so surfguard rejects the request before TLS and the certificate-error assertion fails. Allow the test server's port in this test policy.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| t.Cleanup(srv.Close) | ||
|
|
||
| m := laneManager(t, "https://3.basecampapi.com") | ||
| m.firstPartyPolicy = func() surfguard.Policy { return firstPartyOAuthPolicy().AllowLoopback() } |
There was a problem hiding this comment.
P2: The test server listens on an ephemeral port, but this policy permits only port 443; AllowLoopback() does not relax that port restriction, so surfguard rejects the request before TLS and the certificate-error assertion fails. Allow the test server's port in this test policy.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At internal/auth/client_test.go, line 951:
<comment>The test server listens on an ephemeral port, but this policy permits only port 443; `AllowLoopback()` does not relax that port restriction, so surfguard rejects the request before TLS and the certificate-error assertion fails. Allow the test server's port in this test policy.</comment>
<file context>
@@ -920,3 +929,48 @@ func TestLanes_InstallTheFirstPartyPolicy(t *testing.T) {
+ 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) {
</file context>
Problem
On networks that use fake-IP DNS, every hostname resolves into
198.18.0.0/15, and the connection is carried on to the real host. That includes mihomo/Clash Meta and sing-box in fake-ip mode, and the egress of hosted agent machines such as Grok's. Since v0.10.0 (#656), every OAuth request is checked againstoauth.DefaultIssuerPolicy, which refuses that range. So on those networks:auth loginand token refresh toapp.basecamp.comandlaunchpad.37signals.comare refused before anything is sent:token endpoint "https://app.basecamp.com/oauth/tokens" resolves to an address refused by the address policy.BASECAMP_OAUTH_USE_PROXY=1only helps whenHTTPS_PROXYnames a real proxy. On a transparent fake-IP network nothing is configured, so the only way out today is to run a local proxy. That also turns the address check off entirely for every OAuth request, including endpoints named by discovery documents. The workaround is weaker than what this PR does.Fixes #849, #867, #868. All three report this same failure. #867 is a Grok Bot customer, and we have more cases in support.
Change
All of it is in
internal/auth/client.go:firstPartyOAuthHostsis a list compiled into the CLI:3.basecampapi.com,api.basecamp.com,3.basecamp.com(stored credentials namehttps://3.basecamp.com/oauth/tokens),app.basecamp.comandlaunchpad.37signals.com.firstPartyOAuthPolicy:surfguard.Policy{}.Allow(198.18.0.0/15).AllowPorts(443). That's surfguard's default deny tables plus the fake-IP range, on port 443 only. It leaves outIANASpecialUsebecause surfguard'sAllownever overrides that table, soDefaultIssuerPolicy().Allow(...)would still refuse the range.isFirstPartyOAuthURLis true only forhttps, on the default port or:443, with a hostname exactly in the list (case-insensitive). Plain HTTP, any other port, a trailing dot and look-alike names all return false.oauthTransport.directpicks the transport per request, from that request's URL: first-party URLs get the first-party transport, everything else gets the lane's own strict transport. Proxy handling is unchanged. Opt-out mode still sends a first-party request through the proxy, and protected mode still ignores the proxy (it now uses the per-host direct transport).buildLaneClient.There is no new setting. Fake-IP networks work without any configuration.
Security review, against #656
#656 says what it protects against: "a malicious BC5 discovery document could still steer the device-authorization and token POSTs (carrying
client_id,device_code, refresh token) into private address space." basecamp-sdk#810 adds: "One public host buys private space, and what arrives is credentials rather than a blind GET." Here is how this change stands against that, and against the one actor it does give something to.1. A malicious discovery document (the #656 threat): unchanged
issuer.example,app.basecamp.com.evil.example,evil.app.basecamp.com, a198.18.0.1literal andhttp://app.basecamp.com, and all of them land on the strict transport.RoundTripwith the new URL and gets the strict policy. The redirect test has a first-party endpoint redirect tohttps://198.18.0.1/…; the real production lane refuses that withErrBlockedand never opens a socket. The existingcheckAuthClientRedirectstill refuses to replay a credential-bearing POST to any redirect target.NewExchanger, the device flow andNewDiscoverer(…, WithIssuerHTTPClient(…))all use the CLI's lane client when one is passed in, and the CLI passes one at every OAuth call site (auth.go,agent.go,agent_connect.go,revoke.go).2. Addresses newly reachable, for first-party hosts only
The test checks one address from every block in surfguard's IANA special-use table. The strict policy refuses all of them. The first-party policy admits only:
198.18.0.0/15, the fake-IP range;192.31.196.0/24,192.52.193.0/24,192.175.48.0/24,2001:3::/32,2001:4:112::/48,2620:4f:8000::/48). These are public internet services, not internal space.64:ff9b::/96) whose embedded IPv4 address is itself admitted. The strict policy refuses that prefix whole because it's in the IANA table; without the table, surfguard decodes the embedded address and judges that instead (it already did this for SIIT under both policies). That's how an IPv6-only network with DNS64 reaches a public host, so it's a side benefit. A private, loopback or metadata address inside a NAT64 address is still refused, and a test covers those.Still refused for first-party hosts, each one tested: 10/8, 172.16/12, 192.168/16, loopback, 100.64/10 (CGNAT), 169.254.169.254 and
fd00:ec2::254(cloud metadata), 168.63.129.16 (Azure wireserver), 0.0.0.0, multicast, broadcast, ULA, link-local, and IPv4-mapped and NAT64 spellings of a private address. On top of that, only port 443 is allowed.3. Someone who controls DNS on the machine: the one actor who gains anything
A hostile resolver or network could now point
app.basecamp.comat a198.18.xaddress and the CLI would connect. What they get:app.basecamp.com. A machine at that address can't present one, so the handshake fails before the request body, which holds the credential, is sent. The most they get is a TLS connection attempt to a benchmarking-range address on 443. That is the "blind" case Dangerous mode: let your agent run commands, when only you can give it work #810 calls acceptable, not the "credentials arrive" case.SSL_CERT_FILE. That proxy already sees every API call, and on hosted agent machines it is the egress the operator chose.4. Compared with today's workaround
BASECAMP_OAUTH_USE_PROXY=1with a proxy, which is what affected customers run now, turns the address check off for every OAuth request, discovered endpoints included. After this PR those customers can drop it and keep the strict check on everything except one range for five hosts we operate.Rejected alternatives
=1plus a proxy already switches the check off. But someone could set10.0.0.0/8or0.0.0.0/0to make an error go away, and configuration is exactly where Token refresh fails behind an egress proxy: token endpoint "resolves to an address refused by the address policy" (v0.13.0) #867 and auth login fails behind fake-IP DNS proxy: token endpoint refused by address policy (198.18.0.0/15) #868 went wrong. A hard-coded/15avoids both.Allowoverride the IANA table. That would weakenIANASpecialUsefor every program using surfguard, not just this one.Tests
They're in
internal/auth/client_test.go:TestFirstPartyOAuthPolicy_AdmitsTheFakeIPRangeAndNoInternalSpace: every IANA special-use block; checks the exact list above.TestFirstPartyOAuthPolicy_StillRefusesInternalTargets: the addresses an SSRF goes after, including their IPv6 and NAT64 spellings, and both ends of the fake-IP range.TestFirstPartyOAuthPolicy_DialGate: surfguard's connect-time check.198.18.0.1:443passes,:80doesn't, andDefaultIssuerPolicystill refuses198.18.0.1:443(the OAuth login and token refresh refused when DNS returns fake-IP addresses (198.18.0.0/15), with no supported way to trust them #849 failure).TestIsFirstPartyOAuthURL: every first-party host, plus scheme, port, case, trailing-dot and look-alike cases.TestOAuthTransport_PicksTheDirectTransportByRequestHost: routing with no proxy, with a proxy in protected mode, and with the opt-out.TestFirstPartyRedirectToAForeignHostIsJudgedByTheLanePolicy: uses the real production lane.TestLanes_InstallTheFirstPartyPolicy: drives the first-party transport each lane actually builds (BC5 and Launchpad, under a production and a local base URL), so wiring the wrong policy intobuildLaneClientfails. It uses only dial-time refusals, so it never touches the network.TestFirstPartyOAuthPolicy_JudgesTranslatedIPv4ByItsEmbeddedAddress: NAT64 addresses embedding public and fake-IP addresses are admitted; ones embedding private, loopback or metadata addresses are refused.Checking that the tests catch a broken fix. I broke the code five ways and ran the new tests against each. All five were caught:
IANASpecialUsein the first-party policyAn independent review (gpt-6.1-sol at xhigh) found two more breakages the suite didn't catch: wiring the strict policy into the lanes' first-party transport, and wiring one that allows loopback. Both now fail
TestLanes_InstallTheFirstPartyPolicy. The same review found the NAT64 admission above, which the first version of this description left out. It found no way for a non-first-party host, or a private address, to get the relaxed policy.bin/cipasses locally.Not covered
Allow.app.basecamp.comthat resolves into198.18/15, because that needs control of the system resolver.oauth.DefaultIssuerPolicy. That's a cosmetic follow-up in basecamp-sdk.BASECAMP_OAUTH_USE_PROXY=1is how fake-IP networks get through. Once this lands, that sentence should say fake-IP DNS works without configuration.