Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHOENIX-5772 Streamline the kerberos logic in thin client java code #20

Closed
wants to merge 5 commits into from

Conversation

stoty
Copy link
Contributor

@stoty stoty commented Mar 12, 2020

move hbase config parameter processing to python
remove hadoop dependency from queryserver-client
use shaded avatica client everywhere
update sqlline to 1.9
don't shade sqlline + deps into thin client
remove unnecessary interdependencies between modules

move hbase config parameter processing to python
remove hadoop dependency from queryserver-client
use shaded avatica client everywhere
update sqlline to 1.9
don't shade sqlline + deps into thin client
remove unnecessary interdependencies between modules
Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

I like it! Restating what all I see done here to make sure I didn't miss anything:

  • Repackage sqlline to bundle all dependencies together
  • Update sqlline version
  • Use JAAS to do a ticket-cache based login for sqllline-thin
  • Provide sqlline-thin python args for principal+keytab, instead of forcing them through the JDBC url.

bin/sqlline-thin.py Show resolved Hide resolved
bin/sqlline-thin.py Show resolved Hide resolved
bin/sqlline-thin.py Outdated Show resolved Hide resolved
@@ -0,0 +1,5 @@
ThinClient {
com.sun.security.auth.module.Krb5LoginModule required
Copy link
Member

Choose a reason for hiding this comment

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

I think this will break on IBM java. They do something annoying like have Krb5LoginModule in a different package, I think com.ibm.security.auth.module.Krb5LoginModule.

Also annoying, the semantics on what you options you may/must provide in the configuration block are different between vendors.

Copy link
Member

Choose a reason for hiding this comment

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

I think these options are OK...

https://www.ibm.com/support/knowledgecenter/SSYKE2_7.1.0/com.ibm.java.security.api.71.doc/jgss/com/ibm/security/auth/module/Krb5LoginModule.html seems to indicate that it's actually useKeytab on IBM.

Probably easiest to just acknowledge that we only expect this to work on openjdk/oraclejdk (and maybe azul?). All of those have the exact same JAAS config semantics for krb5, AFAIK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right.
I cobbled together a solution without external config file, and with IBM JRE support based on the hadoop minicluster config. It mostly works with IBM JRE now.
Getting the IBM JRE actually talk to a HTTPS server was also fun. (Only old-timers remember the export-only limited cipher brouhaha, that IBM still has)

bin/thin_client_jaas.conf Outdated Show resolved Hide resolved
@stoty
Copy link
Contributor Author

stoty commented Mar 13, 2020

Some general notes:

We don't repackage sqlline, we just use the pre-packaged uberjar provided by them.

You did not spell out exactly in your summary, but the major win here is using the pre-shaded avatcia jar instead of shading ourselves, and omitting hbase, hadoop, and sqlline from the JDBC driver JAR.

Copy link
Member

@joshelser joshelser left a comment

Choose a reason for hiding this comment

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

Sorry for the delay getting back here.

@asfgit asfgit closed this in 8ad2347 Apr 6, 2020
@stoty stoty deleted the PHOENIX-5772 branch April 7, 2020 07:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants