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.

Sign up for GitHub

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

Jump to bottom

Add kerberos auth support to kudu connector #10953

Merged
merged 4 commits into from
Feb 24, 2022
Merged

Add kerberos auth support to kudu connector #10953

merged 4 commits into from
Feb 24, 2022

Conversation

grantatspothero
Copy link
Contributor

@grantatspothero grantatspothero commented Feb 4, 2022 •
edited by hashhar
Loading

Fixes: #1237

This PR is currently blocked by upgrading the kudu client version to 1.15.0 in this PR: https://github.com/trinodb/trino/pull/10940/files

This is because support for customizing the kerberos service principal primary name was only added in java client 1.15.0:
apache/kudu@e69b3c6

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Kudu

* Add support for Kerberos authentication. ({issue}`10953`)

@cla-bot cla-bot bot added the cla-signed label Feb 4, 2022
@grantatspothero grantatspothero force-pushed the gn/kuduKerberosSupport branch 6 times, most recently from 433567c to d3bb26e Compare February 8, 2022 21:04
@grantatspothero grantatspothero force-pushed the gn/kuduKerberosSupport branch 4 times, most recently from 775e009 to 09ed27d Compare February 9, 2022 20:44
hashhar
hashhar reviewed Feb 10, 2022
View reviewed changes
Copy link
Member

@hashhar hashhar left a comment

Choose a reason for hiding this comment

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

Didn't review the kerberos config in the PT env - not an expert with it.

the only important question from me is regarding when to renew tickets and what happens if we hold on to an expired ticket.

@grantatspothero grantatspothero force-pushed the gn/kuduKerberosSupport branch 5 times, most recently from eb79cba to a2893f4 Compare February 10, 2022 19:14
wendigo
wendigo reviewed Feb 15, 2022
View reviewed changes
Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

Generally LGTM, some config names/descriptions improvements. Nice work @grantatspothero !

@grantatspothero grantatspothero force-pushed the gn/kuduKerberosSupport branch 2 times, most recently from 8eb137a to 63265ae Compare February 15, 2022 22:51
@grantatspothero
Put usages of kuduclient behind an interface
684f30b 
A subsequent commit will then introduce a kerberizedkuduclient that handles kerberos ticket renewal
@grantatspothero
Modify KuduModule to use AbstractConfigurationAwareModule
f73090e 
Subsequent commit will add kerberos config support to the kudumodule
@grantatspothero grantatspothero force-pushed the gn/kuduKerberosSupport branch 2 times, most recently from d4cbf8e to 9960f8a Compare February 16, 2022 17:42
@grantatspothero
Copy link
Contributor Author

Rebased and created docs. I also tried to take the suggestions on the config properties, let me know if the feedback I provided does not make sense.

@github-actions github-actions bot added the docs label Feb 16, 2022
hashhar
hashhar reviewed Feb 18, 2022
View reviewed changes
docs/src/main/sphinx/connector/kudu.rst Outdated
Comment on lines 60 to 82
Kerberos support
----------------

In order to connect to a kudu cluster that uses ``kerberos``
authentication, you need to configure the following kudu properties:

.. code-block:: properties

kudu.authentication.type = KERBEROS

kudu.authentication.client.principal = clientprincipalname

kudu.authentication.client.keytab = /path/to/keytab/file.keytab

kudu.authentication.config = /path/to/kerberos/krb5.conf

## Optional and defaults to "kudu"
## If kudu is running with a custom SPN this needs to be configured
kudu.authentication.server.principal.primary = kudu
Copy link
Member

Choose a reason for hiding this comment

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

@mosabua Can you PTAL?

@grantatspothero are the newlines between consecutive properties needed?

Copy link
Contributor Author

@grantatspothero grantatspothero Feb 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

No, just thought that made this more readable, can change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mosabua can you take a look

@hashhar the newlines were to match the existing style of the docs, see above in the same file

@grantatspothero
Copy link
Contributor Author

Had to rebase this commit: fca8b7b

to fix the kudu product test CI failure. There was a race condition where we reported kerberos startup was successful before the kerberos init script finished running.

@wendigo
Copy link
Contributor

wendigo commented Feb 21, 2022

See failures in suite-1: io.trino.tempto.query.QueryExecutionException: java.sql.SQLException: Query failed (#20220218_224931_02583_g3jkd): line 1:1: Catalog 'kudu' does not exist

wendigo
wendigo reviewed Feb 21, 2022
View reviewed changes
...oduct-tests/src/main/java/io/trino/tests/product/kudu/TestKuduConnectoKerberosSmokeTest.java Outdated
assertThat(result).updatedRowsCountIsEqualTo(25);
assertThat(onTrino().executeQuery(format("SELECT count(*) FROM %s", kuduTable))).containsExactlyInOrder(row(25));
// Kerberos tickets are configured to expire after 60 seconds, this should expire the ticket
Thread.sleep(120_000L);
Copy link
Contributor

Choose a reason for hiding this comment

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

can we make it shorter? (configurable)? Can we expire it programmatically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I configured the default ticket expiry in tests to be 1 min, see kerberos_init.sh, I can make this lower just wanted to be very safe.

I'll look a bit more into useTicketCache in JAAS, it is possible we can force jaas to use the ticket cache then destroy the ticket cache programmatically
https://docs.oracle.com/javase/8/docs/jre/api/security/jaas/spec/com/sun/security/auth/module/Krb5LoginModule.html

Copy link
Contributor Author

@grantatspothero grantatspothero Feb 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

When I tried to configure useTicketCache with debug logs on, it attempted to read a ticket cache file that did not exist.

It looks like the java kerberos client implementation does not ever write tickets to the ticket cache, it only supports reading from an existing ticket cache, see here for some more details:
https://stackoverflow.com/questions/45463635/jaas-fails-to-persist-kerberos-ticket-to-cache-file-and-unable-to-create-cach/45618313#45618313

Going down that path doesn't seem promising.

I'm not a kerberos expert, if other people have ideas let me know! Revocation of kerberos tickets is not usually done because ticket lifetimes are so short.

@grantatspothero
Copy link
Contributor Author

grantatspothero commented Feb 22, 2022 •
edited
Loading

@wendigo there were permissions problems with the kudu keytab files that only happened in CI for some reason, fixed and tests are green now.

wendigo
wendigo approved these changes Feb 23, 2022
View reviewed changes
Copy link
Contributor

@wendigo wendigo left a comment

Choose a reason for hiding this comment

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

LGTM % modernizer violations:

Error:  /home/runner/work/trino/trino/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduAuthenticationConfig.java:39: Use buildOrThrow() instead, as it makes it clear that it will throw on duplicated values
Error:  /home/runner/work/trino/trino/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduKerberosConfig.java:55: Use buildOrThrow() instead, as it makes it clear that it will throw on duplicated values
Error:  /home/runner/work/trino/trino/plugin/trino-kudu/src/test/java/io/trino/plugin/kudu/TestKuduKerberosConfig.java:74: Use buildOrThrow() instead, as it makes it clear that it will throw on duplicated values

@grantatspothero
Adds kerberos support for kudu connector
5d065b2 
Includes handling of kerberos ticket expiration and product tests
demonstrating ticket renewal
@grantatspothero
Extract SystemProperties into plugin-toolkit module
44b872c 
Allows reduction of boilerplate related to system properties
@hashhar hashhar merged commit b738961 into trinodb:master Feb 24, 2022
@hashhar hashhar mentioned this pull request Feb 24, 2022
Closed
@github-actions github-actions bot added this to the 372 milestone Feb 24, 2022
@mosabua mosabua mentioned this pull request Feb 24, 2022
Merged
@hashhar hashhar mentioned this pull request Mar 3, 2022
Closed
findepi
findepi reviewed Feb 2, 2023
View reviewed changes
...oduct-tests/src/main/java/io/trino/tests/product/kudu/TestKuduConnectoKerberosSmokeTest.java
assertThat(result).updatedRowsCountIsEqualTo(25);
assertThat(onTrino().executeQuery(format("SELECT count(*) FROM %s", kuduTable))).containsExactlyInOrder(row(25));
// Kerberos tickets are configured to expire after 60 seconds, this should expire the ticket
Thread.sleep(70_000L);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't have a test that sleeps 70 seconds, should we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@findepi We could reduce the max ticket lifetime a bit, see MAX_TICKET_LIFETIME="1min" in kerberos init.

You cannot set this ticket lifetime too low or problems can occur. For example, setting the max ticket lifetime to 5s will cause the ticket to get refreshed repeatedly.

@findepi findepi mentioned this pull request Feb 2, 2023
Closed
@findepi
Copy link
Member

findepi commented Feb 2, 2023

FYI, the product test is or started to be flaky -- #14441

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

@findepi findepi findepi left review comments

@Praveen2112 Praveen2112 Praveen2112 left review comments

@wendigo wendigo wendigo approved these changes

@hashhar hashhar hashhar approved these changes

Assignees
No one assigned
Labels
cla-signed docs
Milestone
372
Development

Successfully merging this pull request may close these issues.

Support Kerberos authentication for Kudu Connector
5 participants
@grantatspothero @wendigo @findepi @Praveen2112 @hashhar