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

Add delegated Kerberos support for JDBC driver #9606

Merged

Conversation

Praveen2112
Copy link
Member

@Praveen2112 Praveen2112 commented Oct 12, 2021

This allows us to reuse the Kerberos context is setup outside trino-jdbc-driver. It is currently enabled by the setting property KerberosDelegation=true.

Supersedes #5377.

@Praveen2112 Praveen2112 force-pushed the praveen/013/jdbc_kerberos_delegation branch 3 times, most recently from d34255f to d03053f Compare October 13, 2021 07:03
@Praveen2112 Praveen2112 force-pushed the praveen/013/jdbc_kerberos_delegation branch from d03053f to 7f4f689 Compare October 13, 2021 07:17
Copy link
Member

@aczajkowski aczajkowski left a comment

Choose a reason for hiding this comment

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

LGTM % one question

@Praveen2112 Praveen2112 force-pushed the praveen/013/jdbc_kerberos_delegation branch from 7f4f689 to 8d05ecf Compare October 13, 2021 09:19
@Praveen2112
Copy link
Member Author

@kokosing , @aczajkowski AC

Copy link
Member

@aczajkowski aczajkowski left a comment

Choose a reason for hiding this comment

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

LGTM %
One NIT idea to check. In ContextBasedSubjectProvider If ticket is not renewable i would check it's validity (https://docs.oracle.com/javase/7/docs/api/javax/security/auth/kerberos/KerberosTicket.html#getEndTime()) and if not valid throw Ex. But not sure if this is a problem or not. Just thought about it.

@ankuragarawal
Copy link
Member

Thanks @Praveen2112 . This PR seems to have copied the core code from #5377.
Can you please suggest the missing pieces?

@Praveen2112
Copy link
Member Author

We have enabled this feature via a connection property, some minor code level cleanup and added a ProductTests for it.

@ankuragarawal
Copy link
Member

@electrum @Praveen2112 Hi, I would really appreciate if the author is notified about the urgency of a change before copying over the code/logic and raising a separate PR. I raised the PR last year and spent a significant amount of effort in identifying the issue, fixing and testing it and followed up numerous times initially to get this merged and I might have missed an email off late but was never aware that this was a priority and now my PR has been closed by copying over the logic/code to a new PR :-)

@Praveen2112
Copy link
Member Author

@ankuragarawal My sincerest apologies for the above miscommunication and issue due to that. I would like to thank you for your valuable contribution without which the feature might not be included in Trino.

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

Successfully merging this pull request may close these issues.

4 participants