Add support for 2-way SSL when connecting with LdapServer#11070
Add support for 2-way SSL when connecting with LdapServer#11070Praveen2112 merged 2 commits intotrinodb:masterfrom
Conversation
Praveen2112
force-pushed
the
praveen/open_ldap_ssl
branch
2 times, most recently
from
8f7adfd to
4cb2fe5
Compare
Praveen2112
force-pushed
the
praveen/open_ldap_ssl
branch
from
4cb2fe5 to
306c221
Compare
lib/trino-plugin-toolkit/src/main/java/io/trino/plugin/base/ssl/SslUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
How about creating builder instead of this factory method?
There was a problem hiding this comment.
Then it will be a different refactor. Now it is simple extraction. Maybe separate PR or commit if we go with the builder approach?
There was a problem hiding this comment.
At most of the places we build it from config - so builder might not help us. It makes it a bit more complicated.
...in/trino-password-authenticators/src/main/java/io/trino/plugin/password/ldap/LdapConfig.java
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
force-pushed
the
praveen/open_ldap_ssl
branch
from
306c221 to
bd11d2b
Compare
Praveen2112
force-pushed
the
praveen/open_ldap_ssl
branch
from
bd11d2b to
0f4a6c4
Compare
There was a problem hiding this comment.
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
force-pushed
the
praveen/open_ldap_ssl
branch
from
0f4a6c4 to
309c8e9
Compare
|
cc: @mosabua Can you please take a look at the docs changes ? |
aczajkowski
left a comment
aczajkowski
left a comment
There was a problem hiding this comment.
LGTM % one nit (not very important)
| SSLContext sslContext = SSLContext.getInstance("SSL"); | ||
| sslContext.init(null, trustManagers, null); | ||
| return sslContext; | ||
| return SslUtils.createSSLContext(Optional.empty(), Optional.empty(), Optional.of(trustCertificate), Optional.empty()); |
There was a problem hiding this comment.
nit: maybe we could move try catch in SslUtils and pass SSL initialisation error cause as optional parameter.
|
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. |
5 participants
Description
New Feature
Specific to LDAP Password Authenticator.
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: