SNOW-3969787: Send SNI when probing endpoints whose host contains an underscore - #2729
sfc-gh-ckoch wants to merge 6 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
| * @param hostname host as listed in the allowlist file | ||
| * @return a host suitable for SNI, or the original host if no valid variant exists | ||
| */ | ||
| static String toSniCompatibleHost(String hostname) { |
There was a problem hiding this comment.
Is this check run only against snowflake domain names or also for some third parties? If it applies to third parties, the rewrite there may not work as expected.
There was a problem hiding this comment.
CertificateDiagnostic appears to run against third parties, so I constrained the rewrite to snowflakecomputing, but changed some of the text fixture URLs to be RFC compliant so our tests never make an SNI-less connection.
There was a problem hiding this comment.
Scoped to Snowflake hosts, so third-party entries are probed exactly as listed. In practice an underscored third-party host should not arise: the allowlist comes from SYSTEM$ALLOWLIST() and real third-party entries are real DNS names, with underscores only ever in the account identifier. See the allowlist.json thread for why our test fixture was the exception.
sfc-gh-rkowalski
left a comment
There was a problem hiding this comment.
Two things I'd like resolved before merge:
- The PR description doesn't match the code. It describes
toSniCompatibleHost, reusingSNIHostNameas a validator, a warning on the no-valid-variant path, and aCertificateDiagnosticCheckTestbut none of which are in the diff. The CHANGELOG is accurate but description is outdated. ALLOW_UNDERSCORES_IN_HOSTis silently neutered. After this change,SnowflakeConnectStringno longer reads the flag, sosetAllowUnderscoresInHost(true)becomes inert for*.snowflakecomputing.*hosts. That flag was added deliberately (#703, SNOW-544896 regionless-URL support) as an escape hatch to preserve underscore hosts, and its default was re-tuned in #1189.. Either honor it (see inline suggestion) or make its removal an explicit as this will probably be a BCR.
fe9d37a to
856d319
Compare
|
One behavior change worth calling out that nobody asked about: the old code only rewrote when |
856d319 to
21b050c
Compare
sfc-gh-rkowalski
left a comment
There was a problem hiding this comment.
Both earlier change requests are resolved: the description now matches the code, and ALLOW_UNDERSCORES_IN_HOST is a live opt-out again in both the connect string and the diagnostic checks. Implementation is clean and well-tested. Approving; one non-blocking consistency note inline.
| // variant that Snowflake also serves for account names containing underscores. | ||
| String hostname = snowflakeEndpoint.getHost(); | ||
| if (!this.proxyConf.isAllowUnderscoresInHost()) { | ||
| hostname = SnowflakeUtil.normalizeSnowflakeHost(hostname); |
There was a problem hiding this comment.
Non-blocking: this normalization is applied here and in HttpAndHttpsDiagnosticCheck, but DnsDiagnosticCheck and TcpDiagnosticCheck still probe snowflakeEndpoint.getHost() raw. So for an underscored Snowflake account a single diagnostic run splits: TLS/HTTP probe the hyphenated host the client actually connects to, while DNS/TCP probe the underscored name (legal as a DNS label, but not the name a real client uses). Not in the SNI scope of this PR, but either normalize there too or add a line to the PR notes stating DNS/TCP intentionally stay on the literal allowlist host. Fine to merge either way.
There was a problem hiding this comment.
I had decided not to change the Tcp or Dns check because they don't exercise the TLS path. Given that we are only normalizing Snowflake hostnames and this assumption is baked all throughout our client/driver code base, I think it should be safe like this.
…underscore CertificateDiagnosticCheck built its https:// probe URL from the raw allowlist host. Snowflake account names may contain underscores, and an underscore is not a Letter-Digit-Hyphen character, so SNIHostName rejects such a host and the JDK silently completes the handshake with no server_name extension at all. Two consequences. The check reports on whatever default certificate the endpoint serves for an SNI-less handshake rather than the certificate a real client would be presented, which is the opposite of what a connectivity diagnostic is for. It also emits handshakes that stand out from every other client on the wire, since the driver's normal Apache HttpClient path does send SNI. Reuse SNIHostName itself as the validator so this cannot drift from the rule the JDK actually applies, and fall back to the hyphenated form of the host, which Snowflake also serves for account names containing underscores. Hosts that are already valid are probed exactly as listed. When no valid variant exists the original host is kept and the SNI-less handshake is logged as a warning instead of happening silently, so the endpoint the allowlist asked for is still the one probed. Note that HttpAndHttpsDiagnosticCheck already applied the same underscore-to-hyphen substitution; this brings the certificate check in line with it. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
.... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
Replace the three separate underscore-to-hyphen host implementations (CertificateDiagnosticCheck.toSniCompatibleHost, the blind replace in HttpAndHttpsDiagnosticCheck, and the ALLOW_UNDERSCORES_IN_HOST flag logic in SnowflakeConnectString) with a single SnowflakeUtil.normalizeSnowflakeHost helper. The helper is scoped to Snowflake hosts, so third-party allowlist hosts (cloud storage, OCSP responders, Duo, ...) are left unchanged instead of being blindly rewritten. Drops the dead ALLOW_UNDERSCORES_IN_HOST flag. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
The Snowsight and Duo hosts in the diagnostic test fixtures used underscores, which are not valid in hostnames (RFC 952 Letter-Digit-Hyphen). Real third-party hosts never contain underscores, so replace them with hyphens in allowlist.json, DiagnosticContextLatestIT (kept in lockstep for the containsAll assertion), and the SnowflakeUtilTest third-party cases. Third-party URLs are still never rewritten by normalizeSnowflakeHost, which is scoped to Snowflake hosts. .... Generated with [Cortex Code](https://docs.snowflake.com/en/user-guide/cortex-code/cortex-code) Co-Authored-By: Cortex Code <noreply@snowflake.com>
…uard The unification dropped two behaviors that the previous per-call-site implementations had. Restore both. allowUnderscoresInHost is not a dead flag: it is the opt-out for deployments whose DNS only resolves the underscored name, which PrivateLink customers rely on. Removing the read made the public setAllowUnderscoresInHost(true) a silent no-op, a behavior change for anyone who had set it. It is honoured again for connect strings, and now also for the diagnostic checks - a customer who set it is telling us their DNS only resolves the underscored host, so diagnosing the hyphenated variant would report on an endpoint they never connect to. The flag rides on ProxyConfig, which DiagnosticContext already builds from the connection property map and hands to every check. normalizeSnowflakeHost stays a pure function rather than taking the flag as a parameter: SnowflakeUtil is static with no session in scope, so callers own the decision. The host is again only rewritten when it starts with the account identifier. The host need not contain the account at all - it may be an IP address, or a name that merely routes to the account - and rewriting one we cannot tie to the account points the driver at a name Snowflake never promised to serve. The guard belongs only on the connect-string path, where host and account come from the same URL; in the diagnostics they are independent inputs and the allowlist's underscored entry is the regionless org-form host, which frequently shares no prefix with the connection's account. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…derscores DiagnosticContextLatestIT.testRunDiagnosticContextMethods is the only test that calls runDiagnostics(), and it does so against src/test/resources/allowlist.json - opening real DNS, TCP, TLS and HTTP connections to every host that file lists. Third-party hosts are deliberately never rewritten, because only Snowflake serves an equivalent hyphenated name, so an underscored one is probed as listed and the JDK completes the handshake with no server_name extension. duosecurity.com resolves through a wildcard, so CI emitted a genuine SNI-less handshake to a third party on every run; this was visible as DNS queries for duo_security.duosecurity.com in VPC flow logs. Those names are also fiction - a real SYSTEM$ALLOWLIST carries underscores only in the Snowflake account identifier. Record that reasoning on the test that makes the connections, since JSON cannot carry a comment, and leave a pointer where mockEndpoints mirrors the fixture for the containsAll assertion. The Snowflake hosts in the fixture keep their underscores: they are what exercises the normalization, and they are probed as the hyphenated variant that carries SNI. Coverage that third-party hosts are left alone moves to SnowflakeUtilTest, where it is a string assertion that opens no sockets. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
21b050c to
e4a4045
Compare
Overview
SNOW-3969787
CertificateDiagnosticCheckbuilt itshttps://probe URL from the raw allowlist host. An underscore is not a Letter-Digit-Hyphen character, soSNIHostNamerejects such a host and the JDK silently completes the handshake with noserver_nameextension.So the check reported on whatever default certificate the endpoint serves for an SNI-less handshake, not the one a real client gets — the opposite of what a connectivity diagnostic is for.
Snowflake also serves the account name with each underscore replaced by a hyphen, so the check now probes that. The substitution lives in one place,
SnowflakeUtil.normalizeSnowflakeHost, shared withHttpAndHttpsDiagnosticCheck(which had its own unscopedreplace('_', '-')) andSnowflakeConnectString.It only applies to hosts containing
.snowflakecomputing.— only Snowflake guarantees the hyphenated variant, so third-party hosts are probed as listed.allowUnderscoresInHoststill opts out, and now does so for the diagnostics too.Notes
.snowflakecomputing., and hyphenates the whole host rather than only the account prefix. Neither is reachable for a real Snowflake host. Thehost.startsWith(account)guard is unchanged.allowlist.jsonare hyphenated:testRunDiagnosticContextMethodsopens real connections to every host in that file, andduosecurity.comwildcard-resolves, so CI was emitting a real SNI-less handshake every run. The underscored Snowflake hosts stay.Pre-review self checklist
masterbranchmvn -P check-style validate)mvn verifyand inspecttarget/japicmp/japicmp.html)SNOW-XXXX:Please describe how your code solves the related issue.
normalizeSnowflakeHostreturns the hyphenated form for Snowflake hosts and leaves everything else alone; each call site gates it onallowUnderscoresInHost. Covered bySnowflakeUtilTest,ConnectStringParseTest(both flag states, account-prefix guard) andDiagnosticContextTest.