[NEEDS REVIEW] Fixes JENKINS-14520 - LDAP StartTLS support #97

AnalogJ · 2021-06-13T17:38:45Z

Hi!
This PR fixes JENKINS-14520.
The Jenkins LDAP Plugin does not currently support LDAP over TLS (StartTLS), only the non-standardized (and deprecated) LDAPS.

In this PR, I've modified both the javax.naming.ldap.LdapContext and Spring-LDAP usage to support STARTTLS. I don't have a LDAPS server configured to test if LDAPS support has been broken

The changes are fairly minimal, and do not touch the authn/authz mechanisms at all, just the connection configuration. I've also packaged this code and it's confirmed working on a Jenkins server.

I'm not really familiar with Spring (or Java development really), so an edit/review of the code would be appreciated (especially the error handling and .close() functionality).

Links

Most of my code came from research and examples that I found online, which I've compiled and collected here: https://github.com/AnalogJ/docker-jenkins-playground/tree/ldap_starttls

I used some of @mjalbanna code (master...mjalbanna:master) but his code was based on 1.x rather than 2.x, so it didn't have any spring-ldap fixes.

Tests

I have not written any tests, mostly because my thrown-together Java development environment doesn't seem to like the existing test suite. I had to package the plugin with mvn package -Dmaven.test.skip=true. I'm happy to add tests if someone can provide some guidance.

Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
Ensure that the pull request title represents the desired changelog entry
Please describe what you did
Link to relevant issues in GitHub or Jira
Link to relevant pull requests, esp. upstream and downstream changes
Ensure you have provided tests - that demonstrates feature works or fixes the issue

…avax.naming.ldap.LdapContext usage, not the Spring LDAP usage (which will be in a separate commit).

jtnord

it would appear that this will break users who do not use a TLS enabled server as it is not opt in.

I am also not following why you need the same property in both the LdapContext and the props hashtable.

jtnord · 2021-09-10T17:39:11Z

src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java

+                StartTlsResponse tls = (StartTlsResponse) ctx.extendedOperation(new StartTlsRequest());
+                SSLSession session = tls.negotiate();
+
+                if (!session.isValid()) {


does this not break for anyone using non SSL LDAP? (in other words this should be available for users to opt into, or opt out of).
For the AD plugin we also need to close the StartTlsResponse https://github.com/jenkinsci/active-directory-plugin/blob/19e9fbcf0518537c6c4bd24fb1fd09901e7fd60e/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java#L627-L644

agree definitely using startTLS must be optional and not per default.

agreed, this definitely breaks non-STARTTLS implementations. I commented below with some additional context for this PR.

jtnord · 2021-09-10T17:40:38Z

src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java

+                if (!session.isValid()) {
+                    throw new IOException("Couldn't negotiate StartTls session, session is invalid");
+                }
+                ctx.addToEnvironment(Context.SECURITY_AUTHENTICATION, "simple");


is this needed, it would appear to be an unrelated change and would preclude the use of any stronger sasl algorithms?

ctx.addToEnvironment(Context.SECURITY_AUTHENTICATION, "simple");

is copy pasted from https://docs.oracle.com/javase/jndi/tutorial/ldap/ext/starttls.html#TLS%20with%20Simple%20Authentication

I did see the External SASL Authentication section in that link, however I wasn't familiar enough with SASL or the Jenkins UI to add additional form controls.

olamy · 2021-09-12T22:13:12Z

src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java

+                StartTlsResponse tls = (StartTlsResponse) ctx.extendedOperation(new StartTlsRequest());
+                SSLSession session = tls.negotiate();
+
+                if (!session.isValid()) {


agree definitely using startTLS must be optional and not per default.

olamy · 2021-09-12T22:14:43Z

src/main/java/jenkins/security/plugins/ldap/LDAPConfiguration.java

+        // pooled connections do not work with StartTLS
+        contextSource.setPooled(false);


interesting. so you mean com.sun.jndi.ldap.connect.pool doesn't work anymore? it creates connectivity issues or just ignored? If so there are few usage this property in the code.

yeah, this one took me a while to figure out, but every example i was able to find on the internet related to StartTLS and Spring had setPooled(false)

Explicitly disable connection pooling, or Spring may attempt to StartTLS twice on the same connection.

Also there seems to be some known issues as documented in the spring-ldap docs: https://docs.spring.io/spring-ldap/docs/1.3.2.RELEASE/reference/html/pooling.html

AnalogJ · 2021-09-15T04:47:36Z

hey @jtnord @olamy
Thanks so much for the comments on the PR.
I definitely agree, the PR is not ready for merging as-is. To be honest, I'm not a Java developer, I was just lucky enough to find some good examples & forked code that I could merge together to get a working STARTTLS build.

I was hoping someone with a stronger Java background would be willing to pick up my crude implementation and finesse it into something that would be usable. Given the fact that LDAPS has been deprecated for a while, and there seems to be alot of people in the community interested in this fix, I hope that's a reasonable ask.

AnalogJ · 2022-01-14T16:09:49Z

Any feedback/update on this PR?

wbehrens-on-gh · 2022-06-28T19:45:13Z

Speaking of LDAP over SSL, that appears to currently be broken (in master that is) as there is no documentation to enable it and using an ldaps:// url causes an error in the configuration page

Tyrael · 2022-07-21T15:45:00Z

the configuration page reporting error with the current plugin version is a separate issue, currently that always happen regardless if the settings are correct: https://issues.jenkins.io/browse/JENKINS-68748

AnalogJ added 2 commits June 13, 2021 09:46

updating LDAPConfiguration to support starttls. This only fixes the j…

31b742c

…avax.naming.ldap.LdapContext usage, not the Spring LDAP usage (which will be in a separate commit).

Updating Spring LDAP usage to support StartTLS.

a311669

digennarot approved these changes Aug 2, 2021

View reviewed changes

jtnord reviewed Sep 10, 2021

View reviewed changes

olamy requested changes Sep 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEEDS REVIEW] Fixes JENKINS-14520 - LDAP StartTLS support #97

[NEEDS REVIEW] Fixes JENKINS-14520 - LDAP StartTLS support #97

AnalogJ commented Jun 13, 2021 •

edited

Loading

jtnord left a comment

jtnord Sep 10, 2021

olamy Sep 12, 2021

AnalogJ Sep 15, 2021

jtnord Sep 10, 2021

AnalogJ Sep 15, 2021

olamy Sep 12, 2021

olamy Sep 12, 2021

AnalogJ Sep 15, 2021

AnalogJ commented Sep 15, 2021

AnalogJ commented Jan 14, 2022

wbehrens-on-gh commented Jun 28, 2022

Tyrael commented Jul 21, 2022

		// pooled connections do not work with StartTLS
		contextSource.setPooled(false);

[NEEDS REVIEW] Fixes JENKINS-14520 - LDAP StartTLS support #97

Are you sure you want to change the base?

[NEEDS REVIEW] Fixes JENKINS-14520 - LDAP StartTLS support #97

Conversation

AnalogJ commented Jun 13, 2021 • edited Loading

Links

Tests

jtnord left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AnalogJ commented Sep 15, 2021

AnalogJ commented Jan 14, 2022

wbehrens-on-gh commented Jun 28, 2022

Tyrael commented Jul 21, 2022

AnalogJ commented Jun 13, 2021 •

edited

Loading