derphttp: add TLSConfigBypassesTLSDial opt-in flag#116
Open
ibdafna wants to merge 1 commit intocoder:mainfrom
Open
derphttp: add TLSConfigBypassesTLSDial opt-in flag#116ibdafna wants to merge 1 commit intocoder:mainfrom
ibdafna wants to merge 1 commit intocoder:mainfrom
Conversation
ibdafna
force-pushed
the
nflx/derp-tls-bypass-tlsdial
branch
from
fb73dba to
31f9e66
Compare
ibdafna
commented
May 4, 2026
| } | ||
| }) | ||
|
|
||
| t.Run("readers never observe inconsistent pairing under concurrent writers", func(t *testing.T) { |
Author
There was a problem hiding this comment.
This test may be an overkill here -- happy to remove if it's deemed unnecesary.
Member
There was a problem hiding this comment.
Yeah, this test is probably overkill. I'd be worried about it potentially causing flakes, so we should probably just remove it. We won't be changing these values after the connection is started, so there's no risk of it happening in Coder anyway
ibdafna
force-pushed
the
nflx/derp-tls-bypass-tlsdial
branch
2 times, most recently
from
df77af4 to
2f6322f
Compare
Today derphttp.Client.tlsConfig always passes the caller-supplied
TLSConfig through tlsdial.Config, which wraps it with a VerifyConnection
hook that runs system-root verification with a baked-in Let's Encrypt
fallback. tlsdial.Config also panics on base configs that already set
InsecureSkipVerify or VerifyConnection.
That contract works well when DERP is reachable directly over a publicly
trusted PKI, but it breaks for callers who legitimately need to perform
their own server verification — for example when DERP is fronted by a
reverse proxy that presents a non-publicly-trusted certificate, or when
authenticating with an mTLS framework that uses custom CAs / SPIFFE-style
identity / app-name verification (i.e. the standard Go pattern of
InsecureSkipVerify=true paired with a custom VerifyPeerCertificate).
This adds an opt-in TLSConfigBypassesTLSDial bool. When true (and
TLSConfig is non-nil), the supplied config is used as-is after a Clone +
ServerName fallback. tlsdial.Config is bypassed entirely. node-level
InsecureForTests is still honored; node.CertName (a tlsdial-specific
domain-fronting hook) is intentionally ignored on the bypass path —
callers bringing their own verifier are expected to encode any cert
pinning in their own VerifyPeerCertificate / VerifyConnection. The doc
comment explicitly warns that with bypass=true the supplied TLSConfig is
the sole source of server verification.
Plumbing:
- derphttp.Client.TLSConfigBypassesTLSDial bool.
- magicsock.Conn stores the (TLSConfig, bypass) pair as a single
atomic.Pointer[derpTLSPair] so reconnects observe a coherent pair.
Existing SetDERPTLSConfig(cfg) remains the default path and stores
(cfg, false); new SetDERPTLSConfigWithBypass(cfg, bypass) stores the
pair together for callers that need the bypass path.
- magicsock/derp.go reads the pair atomically and propagates both onto
the constructed derphttp.Client.
- netcheck.Client gains DERPTLSConfigBypassesTLSDial alongside the
existing DERPTLSConfig field, plus an optional GetDERPTLSConfig
callback for clients (like magicsock) whose DERP TLS config can be
updated after netcheck.Client construction. magicsock.NewConn wires
this callback to the same atomic pair used by real DERP connections,
so health checks / `coder netcheck` don't panic or drift from the
DERP connection path when DERPTLSConfig is a custom-verifier config.
Backward compatible: bypass=false (the default) preserves the existing
behavior and existing callers see no change. Existing SetDERPTLSConfig(cfg)
callers continue to work; their effective bypass state is false.
Test coverage:
- derp/derphttp/derphttp_test.go: TestTLSConfigBypassesTLSDial covers
the default path, the bypass path, ServerName behavior, the nil-config
fallback, and node.InsecureForTests under bypass.
- wgengine/magicsock/derp_tls_pair_test.go: TestSetDERPTLSConfigPair
covers the legacy setter, the combined setter, and a -race-friendly
concurrent writers/readers test that verifies the (cfg, bypass)
atomic-pair invariant.
- net/netcheck/netcheck_test.go: TestProbeTLSConfigBypass covers the
live GetDERPTLSConfig path with a TLS config that would panic inside
tlsdial.Config if bypass were not propagated.
ibdafna
force-pushed
the
nflx/derp-tls-bypass-tlsdial
branch
from
2f6322f to
193f513
Compare
deansheather
reviewed
May 6, 2026
| t.Run("bypass path uses caller config as-is", func(t *testing.T) { | ||
| // A typical "I'm bringing my own verifier" config that would make | ||
| // tlsdial.Config panic. | ||
| var verifyCalls int |
Member
There was a problem hiding this comment.
This counter isn't checked in the test
| } | ||
| }) | ||
|
|
||
| t.Run("readers never observe inconsistent pairing under concurrent writers", func(t *testing.T) { |
Member
There was a problem hiding this comment.
Yeah, this test is probably overkill. I'd be worried about it potentially causing flakes, so we should probably just remove it. We won't be changing these values after the connection is started, so there's no risk of it happening in Coder anyway
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Today
derphttp.Client.tlsConfigalways passes the caller-supplied TLSConfig throughtlsdial.Config, which wraps it with a VerifyConnection hook that runs system-root verification with a baked-in Let's Encrypt fallback.tlsdial.Configalso panics on base configs that already set InsecureSkipVerify or VerifyConnection.That contract works well when DERP is reachable directly over a publicly trusted PKI, but it breaks for callers who legitimately need to perform their own server verification -- for example when DERP is fronted by a reverse proxy that presents a non-publicly-trusted certificate, or when authenticating with an mTLS framework that uses custom CAs / SPIFFE-style identity / app-name verification (i.e. the standard Go pattern of
InsecureSkipVerify=truepaired with a custom VerifyPeerCertificate).This adds an opt-in TLSConfigBypassesTLSDial bool. When true (and TLSConfig is non-nil), the supplied config is used as-is after a Clone + ServerName fallback.
tlsdial.Configis bypassed entirely. node-levelInsecureForTestsis still honored;node.CertName(a tlsdial-specific domain-fronting hook) is intentionally ignored on the bypass path -- callers bringing their own verifier are expected to encode any cert pinning in their own VerifyPeerCertificate / VerifyConnection.Plumbing:
derphttp.Client.TLSConfigBypassesTLSDial boolmagicsock.Conn.derpTLSConfigBypassesTLSDialatomic +Conn.SetDERPTLSConfigBypassesTLSDialsettermagicsock/derp.gopropagates the flag onto the constructed DERP client alongside the existing TLSConfig plumbing.Backward compatible: bypass=false (the default) preserves the existing behavior and existing callers see no change. New unit tests cover the default path, the bypass path, ServerName behavior, the nil-config fallback, and node.InsecureForTests under bypass.
This code was authored with Cursor under my supervision