[SPARK-54293][SQL] Make ThriftHttpCLIService SNI host check configurable#55308
[SPARK-54293][SQL] Make ThriftHttpCLIService SNI host check configurable#55308yadavay-amzn wants to merge 4 commits into
Conversation
1a3a880 to
1f7e24c
Compare
|
@LuciferYang Could you take a look when you get a chance? Thanks! |
| Arrays.toString(sslContextFactoryServer.getExcludeProtocols())); | ||
| sslContextFactoryServer.setKeyStorePath(keyStorePath); | ||
| sslContextFactoryServer.setKeyStorePassword(keyStorePassword); | ||
| // SPARK-54293: Disable SNI host check, which defaults to true since Jetty 10. |
|
can we follow SPARK-56528 (#55396) to make it configurable? |
Address review feedback from @pan3793 (PR apache#55308): follow the pattern established in SPARK-56528 and make the SNI host check configurable rather than unconditionally disabled. Introduces spark.sql.hive.thriftServer.http.sniHostCheckEnabled (default: false, preserving the behavior from the first commit on this branch). Operators who want stricter host checking for security can opt in by setting the flag to true.
63c708a to
2a05403
Compare
Address review feedback from @pan3793 (PR apache#55308): follow the pattern established in SPARK-56528 and make the SNI host check configurable rather than unconditionally disabled. Introduces spark.sql.hive.thriftServer.http.sniHostCheckEnabled (default: false, preserving the behavior from the first commit on this branch). Operators who want stricter host checking for security can opt in by setting the flag to true.
|
Done — updated to follow the SPARK-56528 pattern. Added |
| "connector. Since SPARK-45522 (Jetty 10+), Spark has disabled SNI host check to " + | ||
| "preserve backward compatibility. Set to true to enforce SNI host checking for " + | ||
| "stricter security. See SPARK-54293.") | ||
| .version("4.2.0") |
There was a problem hiding this comment.
code change lgtm, this requires a PMC member to justify whether to include this in 4.2 as a late-arriving feature.
Address review feedback from @pan3793 (PR apache#55308): follow the pattern established in SPARK-56528 and make the SNI host check configurable rather than unconditionally disabled. Introduces spark.sql.hive.thriftServer.http.sniHostCheckEnabled (default: false, preserving the behavior from the first commit on this branch). Operators who want stricter host checking for security can opt in by setting the flag to true.
2a05403 to
b648511
Compare
|
Updated |
|
@pan3793 Gentle reminder, could you please take a look when you get a chance and see if this PR is ready to merge? |
Disable SNI host check in ThriftHttpCLIService's Jetty SSL connector, consistent with the fix applied to JettyUtils.scala in SPARK-45522. Since Jetty 10, SniHostCheck defaults to true. This was fixed in the Spark UI server but not in ThriftHttpCLIService, which also creates a Jetty server with SSL support. Note: RestSubmissionServer (also mentioned in the JIRA) does not have SSL support, so it does not need this fix.
Address review feedback from @pan3793 (PR apache#55308): follow the pattern established in SPARK-56528 and make the SNI host check configurable rather than unconditionally disabled. Introduces spark.sql.hive.thriftServer.http.sniHostCheckEnabled (default: false, preserving the behavior from the first commit on this branch). Operators who want stricter host checking for security can opt in by setting the flag to true.
b648511 to
8b88fe0
Compare
| "stricter security. See SPARK-54293.") | ||
| .version("5.0.0") |
There was a problem hiding this comment.
as default value does not change the current behavior, we can deliver this in 4.3.0. and it's not necessary to mention which JIRA ticket added the config in doc.
| "stricter security. See SPARK-54293.") | |
| .version("5.0.0") | |
| "stricter security.") | |
| .version("4.3.0") |
Address review feedback from @pan3793 (PR apache#55308): follow the pattern established in SPARK-56528 and make the SNI host check configurable rather than unconditionally disabled. Introduces spark.sql.hive.thriftServer.http.sniHostCheckEnabled (default: false, preserving the behavior from the first commit on this branch). Operators who want stricter host checking for security can opt in by setting the flag to true.
8b88fe0 to
49892b4
Compare
|
Done, removed JIRA references from the doc string and updated version to 4.3.0. Thanks @pan3793! |
|
PR title needs to be updated. |
Co-authored-by: Cheng Pan <pan3793@gmail.com>
|
@pan3793 Sorry for not applying the recommended change as is earlier, I used the apply suggestion button this time to do so. |
pan3793
left a comment
There was a problem hiding this comment.
LGTM, will leave this open for another one day before merging.
### What changes were proposed in this pull request? Make the SNI host check in `ThriftHttpCLIService` configurable via `spark.sql.hive.thriftServer.http.sniHostCheckEnabled`, following the same pattern as SPARK-56528 which made it configurable for the Spark UI. ### Why are the changes needed? SPARK-45522 (Jetty 10+ upgrade) silently enabled SNI host checking, which causes HTTPS connection failures when the certificate CN does not match the hostname. This was made configurable in the Spark UI server (SPARK-56528) but `ThriftHttpCLIService` had no way to disable it. This PR adds the same configurability: default `true` (preserving the current SPARK-45522 behavior), with the option to set `false` to restore pre-SPARK-45522 behavior. ### Does this PR introduce _any_ user-facing change? A new internal configuration `spark.sql.hive.thriftServer.http.sniHostCheckEnabled` (default: `true`) is added. Users who need to disable SNI host checking can set it to `false`. ### How was this patch tested? Added `ThriftHttpCLIServiceSuite` with 3 tests: - SNI host check enabled by default - SNI host check disabled when configured - Jetty 10+ default behavior verification ### Was this patch authored or co-authored using generative AI tooling? Yes Closes #55308 from yadavay-amzn/fix/SPARK-54293-sni-host-check. Authored-by: Anupam Yadav <anupamya@amazon.com> Signed-off-by: Cheng Pan <chengpan@apache.org> (cherry picked from commit fa1373e) Signed-off-by: Cheng Pan <chengpan@apache.org>
|
thanks, merged to master/4.x |
What changes were proposed in this pull request?
Make the SNI host check in
ThriftHttpCLIServiceconfigurable viaspark.sql.hive.thriftServer.http.sniHostCheckEnabled, following the same pattern as SPARK-56528 which made it configurable for the Spark UI.Why are the changes needed?
SPARK-45522 (Jetty 10+ upgrade) silently enabled SNI host checking, which causes HTTPS connection failures when the certificate CN does not match the hostname. This was made configurable in the Spark UI server (SPARK-56528) but
ThriftHttpCLIServicehad no way to disable it.This PR adds the same configurability: default
true(preserving the current SPARK-45522 behavior), with the option to setfalseto restore pre-SPARK-45522 behavior.Does this PR introduce any user-facing change?
A new internal configuration
spark.sql.hive.thriftServer.http.sniHostCheckEnabled(default:true) is added. Users who need to disable SNI host checking can set it tofalse.How was this patch tested?
Added
ThriftHttpCLIServiceSuitewith 3 tests:Was this patch authored or co-authored using generative AI tooling?
Yes