georchestra / georchestra-gateway

GNU General Public License v3.0
0 stars 5 forks source link

ldap - avoid providing sensitive (hashed password) on the /whoami endpoint #110

Open pmauduit opened 5 months ago

pmauduit commented 5 months ago

This is a kind of a revisit of the PR #88. The default mapper provided by Spring security maps all the fields retrieved from the LDAP, including the userPassword. Even if hashed, I think it it safer not to have the endpoint revealing such information.

This PR suggests to use a mapper which always return null when calling mapPassword.

Note: When working on this one, I first began to patch the case when authenticated onto an Extended/geOrchestra LDAP, then I introduced a new profile to test a case with an LDAP not declared as extended (simply a copy/paste of the previous one, with LDAP extended property set to false), and stumbled upon some modifications introduced in https://github.com/georchestra/georchestra-gateway/pull/50/files that I could not make sense of: from the basic ldap package classes, we are creating a ExtendedLdapAuthenticationProvider ? For me it does not make sense, and we should revert the previously introduced modifications onto the LdapAuthenticatorProviderBuilder.java class.

pmauduit commented 5 months ago

Tests are broken, because the testcontainer LDAP is running on a different port than the one set into the java properties, weirdly enough.

pmauduit commented 5 months ago

Tests are broken, because the testcontainer LDAP is running on a different port than the one set into the java properties, weirdly enough.

Race condition when launching tests in parallel, using java properties to pass the infos related to connecting to the LDAP is not thread-safe ; using the following maven-failsafe configuration makes it, but takes longer:

+        <configuration>
+          <forkCount>1</forkCount>
+          <reuseForks>false</reuseForks>
+        </configuration>
landryb commented 5 months ago

ouch. scary :)

pmauduit commented 5 months ago

Note: When working on this one, I first began to patch the case when authenticated onto an Extended/geOrchestra LDAP, then I introduced a new profile to test a case with an LDAP not declared as extended (simply a copy/paste of the previous one, with LDAP extended property set to false), and stumbled upon some modifications introduced in https://github.com/georchestra/georchestra-gateway/pull/50/files that I could not make sense of: from the basic ldap package classes, we are creating a ExtendedLdapAuthenticationProvider ? For me it does not make sense, and we should revert the previously introduced modifications onto the LdapAuthenticatorProviderBuilder.java class.

For the record, the modifications have been introduced in order to allow a connection using the email instead of the uid. I have the feeling that:

Might need revisit, if authentication by e-mail is still a need ? Not even sure. But in the scope of another PR.