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 support for 2-way SSL when connecting with LdapServer #11070

Merged
merged 2 commits into from
Apr 5, 2022
Merged

Add support for 2-way SSL when connecting with LdapServer #11070

merged 2 commits into from
Apr 5, 2022

Conversation

Praveen2112
Copy link
Member

@Praveen2112 Praveen2112 commented Feb 16, 2022 •
edited
Loading

Description

Is this change a fix, improvement, new feature, refactoring, or other?

New Feature

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

Specific to LDAP Password Authenticator.

How would you describe this change to a non-technical end user or system administrator?

Adding support for 2-way SSL to LDAP Password Authenticator.

Related issues, pull requests, and links

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:

# LDAP
* Adding support for 2-way SSL to LDAP Password Authenticator.

@cla-bot cla-bot bot added the cla-signed label Feb 16, 2022
@Praveen2112 Praveen2112 mentioned this pull request Feb 16, 2022
Merged
@Praveen2112 Praveen2112 force-pushed the praveen/open_ldap_ssl branch 2 times, most recently from 8f7adfd to 4cb2fe5 Compare March 10, 2022 09:30
@Praveen2112 Praveen2112 force-pushed the praveen/open_ldap_ssl branch from 4cb2fe5 to 306c221 Compare March 24, 2022 06:00
@Praveen2112 Praveen2112 marked this pull request as ready for review March 24, 2022 06:00
@github-actions github-actions bot added the docs label Mar 24, 2022
lukasz-walkiewicz
lukasz-walkiewicz reviewed Mar 24, 2022
View reviewed changes
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ssl/SslUtils.java Outdated Show resolved Hide resolved
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ssl/SslUtils.java
{
private SslUtils() {}

public static SSLContext createSSLContext(
Copy link
Member

Choose a reason for hiding this comment

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

How about creating builder instead of this factory method?

Copy link
Member

Choose a reason for hiding this comment

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

Then it will be a different refactor. Now it is simple extraction. Maybe separate PR or commit if we go with the builder approach?

Copy link
Member Author

Choose a reason for hiding this comment

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

At most of the places we build it from config - so builder might not help us. It makes it a bit more complicated.

docs/src/main/sphinx/security/ldap.rst Outdated Show resolved Hide resolved
...in/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapConfig.java Show resolved Hide resolved
docs/src/main/sphinx/security/ldap.rst Outdated Show resolved Hide resolved
...in/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapConfig.java Outdated Show resolved Hide resolved
...rino-password-authenticators/src/test/java/io/trino/plugin/password/ldap/TestLdapConfig.java Outdated Show resolved Hide resolved
@Praveen2112 Praveen2112 force-pushed the praveen/open_ldap_ssl branch from 306c221 to bd11d2b Compare April 1, 2022 07:31
@Praveen2112
Copy link
Member Author

@lukasz-walkiewicz AC

@Praveen2112 Praveen2112 force-pushed the praveen/open_ldap_ssl branch from bd11d2b to 0f4a6c4 Compare April 1, 2022 08:46
kokosing
kokosing approved these changes Apr 3, 2022
View reviewed changes
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ssl/SslUtils.java
{
private SslUtils() {}

public static SSLContext createSSLContext(
Copy link
Member

Choose a reason for hiding this comment

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

Then it will be a different refactor. Now it is simple extraction. Maybe separate PR or commit if we go with the builder approach?

@Praveen2112 Praveen2112 force-pushed the praveen/open_ldap_ssl branch from 0f4a6c4 to 309c8e9 Compare April 4, 2022 05:23
@Praveen2112 Praveen2112 requested a review from mosabua April 4, 2022 05:24
@Praveen2112
Copy link
Member Author

cc: @mosabua Can you please take a look at the docs changes ?

aczajkowski
aczajkowski approved these changes Apr 4, 2022
View reviewed changes
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 (not very important)

...d-authenticators/src/main/java/io/trino/plugin/password/ldap/JdkLdapAuthenticatorClient.java
SSLContext sslContext = SSLContext.getInstance("SSL");
sslContext.init(null, trustManagers, null);
return sslContext;
return SslUtils.createSSLContext(Optional.empty(), Optional.empty(), Optional.of(trustCertificate), Optional.empty());
Copy link
Member

Choose a reason for hiding this comment

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

nit: maybe we could move try catch in SslUtils and pass SSL initialisation error cause as optional parameter.

@Praveen2112 Praveen2112 merged commit 9525d8f into trinodb:master Apr 5, 2022
@github-actions github-actions bot added this to the 376 milestone Apr 5, 2022
@mosabua mosabua mentioned this pull request Apr 5, 2022
Merged
@maltesander maltesander mentioned this pull request Apr 12, 2022
Closed
7 tasks
@brokenjacobs brokenjacobs mentioned this pull request Apr 21, 2022
Closed
@mosabua
Copy link
Member

mosabua commented Apr 22, 2022

Just a heads up to everyone involved here. This change of property name and break up into multiple is essentially a breaking change for upgrades. If possible, it would be good to change this so the old config name works as legacy config or so .. but I am aware .. that might not be possible. Also props to @kpayne for finding this.

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

@kokosing kokosing kokosing approved these changes

@lukasz-walkiewicz lukasz-walkiewicz lukasz-walkiewicz approved these changes

@aczajkowski aczajkowski aczajkowski approved these changes

@s2lomon s2lomon Awaiting requested review from s2lomon

@electrum electrum Awaiting requested review from electrum

@mosabua mosabua Awaiting requested review from mosabua

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

Successfully merging this pull request may close these issues.
5 participants
@Praveen2112 @mosabua @kokosing @lukasz-walkiewicz @aczajkowski