BE: Auth: Support LDAPS for AD #840
Conversation
kapybro
bot
added
status/triage
|
@Haarolean PTAL |
fix sonar issues
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| private static final X509Certificate[] CERTS = new X509Certificate[0]; | ||
|
|
||
| @Override | ||
| public void checkClientTrusted(X509Certificate[] x509Certificates, String s) { |
Check failure
Code scanning / SonarCloud
Server certificates should be verified during SSL/TLS connections High
| } | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(X509Certificate[] x509Certificates, String s) { |
Check failure
Code scanning / SonarCloud
Server certificates should be verified during SSL/TLS connections High
| public CustomSslSocketFactory() { | ||
| try { | ||
| SSLContext ctx = SSLContext.getInstance("TLS"); | ||
| ctx.init(null, new TrustManager[] { new DisabledX509TrustManager() }, new SecureRandom()); |
There was a problem hiding this comment.
I think we can achieve the same like that:
SslContext context = SslContextBuilder.forClient()
.trustManager(InsecureTrustManagerFactory.INSTANCE)
.build();
and if there's a way to build a SSLSocketFactory via supplying a SslContext, we can reduce all this copypaste.
| super.start(); | ||
|
|
||
| if (sslEnabled) { | ||
| setCustomCertAndRestartServer(); |
There was a problem hiding this comment.
does it make sense to set the things up before start to avoid restarting?
| import org.springframework.test.context.ContextConfiguration; | ||
|
|
||
| @ContextConfiguration(initializers = {ActiveDirectoryLdapTest.Initializer.class}) | ||
| public class ActiveDirectoryLdapTest extends AbstractActiveDirectoryIntegrationTest { |
There was a problem hiding this comment.
I feel like we're missing some tests in this test class :)
There was a problem hiding this comment.
I moved the tests to AbstractActiveDirectoryIntegrationTest.
They are run for both classes ActiveDirectoryLdapTest/ActiveDirectoryLdapsTest.
Or did I misunderstand?
There was a problem hiding this comment.
Ah, this is kinda counter-intuitive. Usually abstract test classes are used as a test base for setting something up before executing tests. Here, ActiveDirectoryLdapTest looks like it doesn't test anything due to the test itself being pulled up to AbstractActiveDirectoryIntegrationTest instead.
Haarolean
changed the title
Haarolean
added
type/enhancement
What changes did you make? (Give an overview)
In this PR I added
CustomSslSocketFactorythat trusts all certificates by default.This allows you to use LDAPS to connect to an AD server without further specifying the path to the truststore/password.
Is there anything you'd like reviewers to focus on?
How Has This Been Tested? (put an "x" (case-sensitive!) next to an item)
Checklist (put an "x" (case-sensitive!) next to all the items, otherwise the build will fail)
Check out Contributing and Code of Conduct
A picture of a cute animal (not mandatory but encouraged)