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.

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

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pmauduit
Copy link
Member

@pmauduit pmauduit commented Mar 14, 2024

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 pmauduit force-pushed the ldap-avoid-provide-sensitive-info-on-whoami branch from 5c10f69 to 8f15546 Compare March 14, 2024 12:10
@pmauduit
Copy link
Member Author

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

@pmauduit
Copy link
Member Author

pmauduit commented Mar 14, 2024

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
Copy link
Member

landryb commented Mar 14, 2024

ouch. scary :)

…point

This is a kind of revisit of the PR#88.
@pmauduit pmauduit force-pushed the ldap-avoid-provide-sensitive-info-on-whoami branch from 8f15546 to 985a028 Compare March 14, 2024 14:22
@pmauduit
Copy link
Member Author

pmauduit commented Mar 14, 2024

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.

Should have been done in the context of PR #50, but better late than
never.
@pmauduit pmauduit force-pushed the ldap-avoid-provide-sensitive-info-on-whoami branch from 0bd1984 to fcec37a Compare March 14, 2024 15:52
@pmauduit pmauduit requested review from groldan and emmdurin March 14, 2024 15:56
@pmauduit pmauduit marked this pull request as ready for review March 14, 2024 15:57
@f-necas f-necas force-pushed the main branch 2 times, most recently from 74698bf to 4660e3a Compare July 11, 2024 09:45
@pmauduit pmauduit requested a review from f-necas December 18, 2024 12:52
Copy link
Collaborator

@f-necas f-necas left a comment

Choose a reason for hiding this comment

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

Tested and works for me :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants