Skip to content

Conversation

@turboFei
Copy link
Member

@turboFei turboFei commented Apr 1, 2025

Why are the changes needed?

Backport apache/hive#3749

It is not possible to create SSL connection with Kerberos authentication when the server certificate is not issued to the canonical host name but to an alternative domain name.

See details about the exception and steps for reproducing in the HIVE-26723

Hive JDBC client validates the host name by its canonical name by default. This behaviour leads to SSLHandshakeExcpetion when trying to connect using alias name with Kerberos authentication. To solve this issue a new connection property is introduced to be able disabling canonical host name check: enableCanonicalHostnameCheck having default value true.

When the property is not given in connection string (or its value is true) then the original behaviour is applied i.e. checking canonical host name.

How was this patch tested?

There are no new unit tests because the fix is in the HiveConnection constructor which contains lot of logic inside and also builds new SSL connections.
IMO it would have been far too much effort to mock the whole environment for creating unit tests against this tiny change. :(

There wasn't any already existing test against HiveConnection that could be extended with this new feature/bugfix. It is misleading that there is a class having name TestHiveConnection but there is no any tests that would test the class HiveConnection itself.

BTW It was tested manually: after this fix when the steps in JIRA are executed again using the new JARs then the SSL connection is created successfully, and I was able to execute queries.

Does this PR introduce any user-facing change?

A new JDBC connection URL property has been introduced: enableCanonicalHostnameCheck to be able to turn off the canonical host name checking. Its default value is true so if it is not set the canonical host name is checked when building up the SSL connection.

To turn off the canonical host name checking just add this property to the connection string, i.e:

./beeline -u "jdbc:hive2://hs2.subdomain.example.com:443/default;transportMode=http;httpPath=cliservice;socketTimeout=60;ssl=true;retries=1;principal=myhiveprincipal/mydomain.example.com;enableCanonicalHostnameCheck=false;"

Was this patch authored or co-authored using generative AI tooling?

No.

Copy link
Member

@pan3793 pan3793 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a clean backport from upstream Hive, LGTM

@pan3793 pan3793 added this to the v1.9.4 milestone Apr 1, 2025
@pan3793 pan3793 closed this in 1937dd9 Apr 1, 2025
pan3793 pushed a commit that referenced this pull request Apr 1, 2025
…ing.

### Why are the changes needed?

Backport apache/hive#3749

It is not possible to create SSL connection with Kerberos authentication when the server certificate is not issued to the canonical host name but to an alternative domain name.

See details about the exception and steps for reproducing in the [HIVE-26723](https://issues.apache.org/jira/browse/HIVE-26723)

Hive JDBC client validates the host name by its canonical name by default. This behaviour leads to SSLHandshakeExcpetion when trying to connect using alias name with Kerberos authentication. To solve this issue a new connection property is introduced to be able disabling canonical host name check: enableCanonicalHostnameCheck having default value true.

When the property is not given in connection string (or its value is true) then the original behaviour is applied i.e. checking canonical host name.

### How was this patch tested?

There are no new unit tests because the fix is in the HiveConnection constructor which contains lot of logic inside and also builds new SSL connections.
IMO it would have been far too much effort to mock the whole environment for creating unit tests against this tiny change. :(

There wasn't any already existing test against HiveConnection that could be extended with this new feature/bugfix. It is misleading that there is a class having name TestHiveConnection but there is no any tests that would test the class HiveConnection itself.

BTW It was tested manually: after this fix when the steps in JIRA are executed again using the new JARs then the SSL connection is created successfully, and I was able to execute queries.

### Does this PR introduce any user-facing change?
A new JDBC connection URL property has been introduced: enableCanonicalHostnameCheck to be able to turn off the canonical host name checking. Its default value is true so if it is not set the canonical host name is checked when building up the SSL connection.

To turn off the canonical host name checking just add this property to the connection string, i.e:

```
./beeline -u "jdbc:hive2://hs2.subdomain.example.com:443/default;transportMode=http;httpPath=cliservice;socketTimeout=60;ssl=true;retries=1;principal=myhiveprincipal/mydomain.example.com;enableCanonicalHostnameCheck=false;"
```
### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #7009 from turboFei/kerberos_can.

Closes #7009

40cd488 [Wang, Fei] Backport HIVE-26723: Configurable canonical name checking.

Authored-by: Wang, Fei <[email protected]>
Signed-off-by: Cheng Pan <[email protected]>
(cherry picked from commit 1937dd9)
Signed-off-by: Cheng Pan <[email protected]>
@pan3793 pan3793 modified the milestones: v1.9.4, v1.10.2 Apr 1, 2025
@pan3793
Copy link
Member

pan3793 commented Apr 1, 2025

Merged to master/1.10.2

@turboFei turboFei deleted the kerberos_can branch April 1, 2025 05:57
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 6 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (196b47e) to head (40cd488).
Report is 10 commits behind head on master.

Files with missing lines Patch % Lines
.../org/apache/kyuubi/jdbc/hive/KyuubiConnection.java 0.00% 6 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##           master   #7009   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files         693     694    +1     
  Lines       42735   42748   +13     
  Branches     5816    5819    +3     
======================================
- Misses      42735   42748   +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants