From 985a028b8b8b16e9fa0c08de338d20cd28d54b85 Mon Sep 17 00:00:00 2001 From: Pierre Mauduit Date: Thu, 14 Mar 2024 11:32:10 +0100 Subject: [PATCH 1/2] ldap - avoid providing sensitive (salted password) on the /whoami endpoint This is a kind of revisit of the PR#88. --- .../accounts/admin/AccountManager.java | 3 +- .../ldap/NoPasswordLdapUserDetailsMapper.java | 11 +++ .../LdapAuthenticatorProviderBuilder.java | 3 +- .../admin/CreateAccountUserCustomizerIT.java | 8 +++ ...tpHeadersBase64EncodedCreateAccountIT.java | 8 +++ .../ldap/basic/BasicLdapAuthenticationIT.java | 44 ++++++++++++ .../ExtendedLdapAuthenticationIT.java | 71 +++++++++++++++++++ .../test/resources/application-basicldap.yml | 39 ++++++++++ .../resources/application-createaccount.yml | 8 ++- 9 files changed, 192 insertions(+), 3 deletions(-) create mode 100644 gateway/src/main/java/org/georchestra/gateway/security/ldap/NoPasswordLdapUserDetailsMapper.java create mode 100644 gateway/src/test/java/org/georchestra/gateway/security/ldap/basic/BasicLdapAuthenticationIT.java create mode 100644 gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java create mode 100644 gateway/src/test/resources/application-basicldap.yml diff --git a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/AccountManager.java b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/AccountManager.java index 6074c642..dee2e77a 100644 --- a/gateway/src/main/java/org/georchestra/gateway/accounts/admin/AccountManager.java +++ b/gateway/src/main/java/org/georchestra/gateway/accounts/admin/AccountManager.java @@ -38,7 +38,8 @@ public interface AccountManager { * @param mappedUser the user {@link ResolveGeorchestraUserGlobalFilter} * resolved by calling * {@link GeorchestraUserMapper#resolve(Authentication)} - * @return the stored version of the user if it exists, otherwise an empty Optional + * @return the stored version of the user if it exists, otherwise an empty + * Optional */ Optional find(GeorchestraUser mappedUser); diff --git a/gateway/src/main/java/org/georchestra/gateway/security/ldap/NoPasswordLdapUserDetailsMapper.java b/gateway/src/main/java/org/georchestra/gateway/security/ldap/NoPasswordLdapUserDetailsMapper.java new file mode 100644 index 00000000..d80b4ad2 --- /dev/null +++ b/gateway/src/main/java/org/georchestra/gateway/security/ldap/NoPasswordLdapUserDetailsMapper.java @@ -0,0 +1,11 @@ +package org.georchestra.gateway.security.ldap; + +import org.springframework.security.ldap.userdetails.LdapUserDetailsMapper; + +public class NoPasswordLdapUserDetailsMapper extends LdapUserDetailsMapper { + + @Override + protected String mapPassword(Object passwordValue) { + return null; + } +} diff --git a/gateway/src/main/java/org/georchestra/gateway/security/ldap/basic/LdapAuthenticatorProviderBuilder.java b/gateway/src/main/java/org/georchestra/gateway/security/ldap/basic/LdapAuthenticatorProviderBuilder.java index 7d99a597..aa33ecca 100644 --- a/gateway/src/main/java/org/georchestra/gateway/security/ldap/basic/LdapAuthenticatorProviderBuilder.java +++ b/gateway/src/main/java/org/georchestra/gateway/security/ldap/basic/LdapAuthenticatorProviderBuilder.java @@ -21,6 +21,7 @@ import static java.util.Objects.requireNonNull; import org.georchestra.ds.users.AccountDao; +import org.georchestra.gateway.security.ldap.NoPasswordLdapUserDetailsMapper; import org.georchestra.gateway.security.ldap.extended.ExtendedLdapAuthenticationProvider; import org.georchestra.gateway.security.ldap.extended.ExtendedPasswordPolicyAwareContextSource; import org.springframework.ldap.core.support.BaseLdapPathContextSource; @@ -72,7 +73,7 @@ public ExtendedLdapAuthenticationProvider build() { final GrantedAuthoritiesMapper rolesMapper = ldapAuthoritiesMapper(); provider.setAuthoritiesMapper(rolesMapper); - provider.setUserDetailsContextMapper(new LdapUserDetailsMapper()); + provider.setUserDetailsContextMapper(new NoPasswordLdapUserDetailsMapper()); provider.setAccountDao(accountDao); return provider; } diff --git a/gateway/src/test/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizerIT.java b/gateway/src/test/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizerIT.java index 74cd65b0..66cdc697 100644 --- a/gateway/src/test/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizerIT.java +++ b/gateway/src/test/java/org/georchestra/gateway/accounts/admin/CreateAccountUserCustomizerIT.java @@ -16,6 +16,8 @@ import org.springframework.context.ApplicationContext; import org.springframework.ldap.NameNotFoundException; import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; import org.springframework.test.web.reactive.server.WebTestClient; import java.util.Map; @@ -46,6 +48,12 @@ public class CreateAccountUserCustomizerIT { ldap.stop(); } + @DynamicPropertySource + static void registerLdap(DynamicPropertyRegistry registry) { + registry.add("testcontainers.georchestra.ldap.host", () -> "127.0.0.1"); + registry.add("testcontainers.georchestra.ldap.port", ldap::getMappedLdapPort); + } + private static final Map NOT_EXISTING_ACCOUNT_HEADERS = Map.of( // "sec-georchestra-preauthenticated", "true", // "preauth-username", "pmartin", // diff --git a/gateway/src/test/java/org/georchestra/gateway/accounts/admin/PreauthHttpHeadersBase64EncodedCreateAccountIT.java b/gateway/src/test/java/org/georchestra/gateway/accounts/admin/PreauthHttpHeadersBase64EncodedCreateAccountIT.java index 74def65b..ee594cd9 100644 --- a/gateway/src/test/java/org/georchestra/gateway/accounts/admin/PreauthHttpHeadersBase64EncodedCreateAccountIT.java +++ b/gateway/src/test/java/org/georchestra/gateway/accounts/admin/PreauthHttpHeadersBase64EncodedCreateAccountIT.java @@ -13,6 +13,8 @@ import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; import org.springframework.boot.test.context.SpringBootTest; import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; import org.springframework.test.web.reactive.server.WebTestClient; @SpringBootTest(classes = GeorchestraGatewayApplication.class) @@ -34,6 +36,12 @@ class PreauthHttpHeadersBase64EncodedCreateAccountIT { ldap.stop(); } + @DynamicPropertySource + static void registerLdap(DynamicPropertyRegistry registry) { + registry.add("testcontainers.georchestra.ldap.host", () -> "127.0.0.1"); + registry.add("testcontainers.georchestra.ldap.port", ldap::getMappedLdapPort); + } + @Test void testPreauthenticatedHeaders_AccentedChars() throws Exception { testClient.get().uri("/whoami")// diff --git a/gateway/src/test/java/org/georchestra/gateway/security/ldap/basic/BasicLdapAuthenticationIT.java b/gateway/src/test/java/org/georchestra/gateway/security/ldap/basic/BasicLdapAuthenticationIT.java new file mode 100644 index 00000000..acaafe87 --- /dev/null +++ b/gateway/src/test/java/org/georchestra/gateway/security/ldap/basic/BasicLdapAuthenticationIT.java @@ -0,0 +1,44 @@ +package org.georchestra.gateway.security.ldap.basic; + +import org.georchestra.gateway.app.GeorchestraGatewayApplication; +import org.georchestra.testcontainers.ldap.GeorchestraLdapContainer; +import org.hamcrest.Matchers; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.web.reactive.server.WebTestClient; + +@SpringBootTest(classes = GeorchestraGatewayApplication.class) +@ActiveProfiles({ "basicldap" }) +@AutoConfigureWebTestClient(timeout = "PT20S") +@Disabled("ExtendedLdapAuthenticationProvider being built instead of a Basic one after https://github.com/georchestra/georchestra-gateway/pull/50/files ?") +public class BasicLdapAuthenticationIT { + + public static GeorchestraLdapContainer ldap = new GeorchestraLdapContainer(); + + private @Autowired WebTestClient testClient; + + public static @BeforeAll void startUpContainers() { + ldap.start(); + } + + public static @AfterAll void shutDownContainers() { + ldap.stop(); + } + + public @Test void testWhoamiNoPasswordRevealed() { + testClient.get().uri("/whoami")// + .header("Authorization", "Basic dGVzdGFkbWluOnRlc3RhZG1pbg==") // testadmin:testadmin + .exchange()// + .expectStatus()// + .is2xxSuccessful()// + .expectBody(String.class)// + .value(Matchers.not(Matchers.containsString("{SHA}"))); + } + +} diff --git a/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java b/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java new file mode 100644 index 00000000..417c69f0 --- /dev/null +++ b/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java @@ -0,0 +1,71 @@ +package org.georchestra.gateway.security.ldap.extended; + +import org.georchestra.gateway.app.GeorchestraGatewayApplication; +import org.georchestra.gateway.filter.headers.providers.JsonPayloadHeadersContributor; +import org.georchestra.gateway.model.GatewayConfigProperties; +import org.georchestra.testcontainers.ldap.GeorchestraLdapContainer; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.autoconfigure.web.reactive.AutoConfigureWebTestClient; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.ApplicationContext; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.DynamicPropertyRegistry; +import org.springframework.test.context.DynamicPropertySource; +import org.springframework.test.web.reactive.server.WebTestClient; +import org.springframework.web.context.WebApplicationContext; +import org.testcontainers.containers.GenericContainer; +import org.testcontainers.utility.DockerImageName; + +import java.util.Arrays; +import java.util.Optional; + +import static org.junit.jupiter.api.Assertions.assertNotNull; + +@SpringBootTest(classes = GeorchestraGatewayApplication.class) +@ActiveProfiles({ "createaccount" }) +@AutoConfigureWebTestClient(timeout = "PT20S") +public class ExtendedLdapAuthenticationIT { + public static GeorchestraLdapContainer ldap = new GeorchestraLdapContainer(); + + private @Autowired WebTestClient testClient; + + public static @BeforeAll void startUpContainers() { + ldap.start(); + } + + public static @AfterAll void shutDownContainers() { + ldap.stop(); + } + + @DynamicPropertySource + static void registerLdap(DynamicPropertyRegistry registry) { + registry.add("testcontainers.georchestra.ldap.host", () -> "127.0.0.1"); + registry.add("testcontainers.georchestra.ldap.port", ldap::getMappedLdapPort); + } + + public @Test void testWhoami() { + testClient.get().uri("/whoami")// + .header("Authorization", "Basic dGVzdGFkbWluOnRlc3RhZG1pbg==") // testadmin:testadmin + .exchange()// + .expectStatus()// + .is2xxSuccessful()// + .expectBody()// + .jsonPath("$.GeorchestraUser.username").isEqualTo("testadmin"); + } + + public @Test void testWhoamiNoPasswordRevealed() { + testClient.get().uri("/whoami")// + .header("Authorization", "Basic dGVzdGFkbWluOnRlc3RhZG1pbg==") // testadmin:testadmin + .exchange()// + .expectStatus()// + .is2xxSuccessful()// + .expectBody()// + .jsonPath( + "$.['org.georchestra.gateway.security.ldap.extended.GeorchestraUserNamePasswordAuthenticationToken'].principal.password") + .isEmpty(); + } + +} diff --git a/gateway/src/test/resources/application-basicldap.yml b/gateway/src/test/resources/application-basicldap.yml new file mode 100644 index 00000000..3e1cf715 --- /dev/null +++ b/gateway/src/test/resources/application-basicldap.yml @@ -0,0 +1,39 @@ + # This YAML file configures the geOrchestra gateway with + # a basic LDAP (extended set to false). + # +georchestra: + gateway: + default-headers: + # Default security headers to append to proxied requests + proxy: true + username: true + roles: true + org: true + orgname: true + global-access-rules: + - intercept-url: + - "/**" + - "/proxy/?url=*" + anonymous: true + security: + ldap: + default: + enabled: true + extended: false + url: ldap://${ldapHost}:${ldapPort}/ + baseDn: dc=georchestra,dc=org + adminDn: cn=admin,dc=georchestra,dc=org + adminPassword: secret +spring: + cloud: + gateway: + enabled: true + default-filters: + - SecureHeaders + - TokenRelay + - RemoveSecurityHeaders + # AddSecHeaders appends sec-* headers to proxied requests based on the + # georchestra.gateway.default-headers and georchestra.gateway.servies..headers config properties + - AddSecHeaders + httpclient.wiretap: true + httpserver.wiretap: false diff --git a/gateway/src/test/resources/application-createaccount.yml b/gateway/src/test/resources/application-createaccount.yml index 0b774fad..9ceb7117 100644 --- a/gateway/src/test/resources/application-createaccount.yml +++ b/gateway/src/test/resources/application-createaccount.yml @@ -1,3 +1,9 @@ + # This YAML file configures the geOrchestra gateway with + # * a geOrchestra extended LDAP + # * authentication via HTTP headers + # + # It is mainly used for integration testing the preauthentication mechanisms. + # georchestra: gateway: default-headers: @@ -21,7 +27,7 @@ georchestra: default: enabled: true extended: true - url: ldap://${ldapHost}:${ldapPort}/ + url: ldap://${testcontainers.georchestra.ldap.host}:${testcontainers.georchestra.ldap.port}/ baseDn: dc=georchestra,dc=org adminDn: cn=admin,dc=georchestra,dc=org adminPassword: secret From fcec37ac09192267c77ae7eb42d7313e9e59046b Mon Sep 17 00:00:00 2001 From: Pierre Mauduit Date: Thu, 14 Mar 2024 16:38:22 +0100 Subject: [PATCH 2/2] adding an IT for testing login via email instead of uid Should have been done in the context of PR #50, but better late than never. --- .../ldap/extended/ExtendedLdapAuthenticationIT.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java b/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java index 417c69f0..cdd5615e 100644 --- a/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java +++ b/gateway/src/test/java/org/georchestra/gateway/security/ldap/extended/ExtendedLdapAuthenticationIT.java @@ -56,6 +56,16 @@ static void registerLdap(DynamicPropertyRegistry registry) { .jsonPath("$.GeorchestraUser.username").isEqualTo("testadmin"); } + public @Test void testWhoamiUsingEmail() { + testClient.get().uri("/whoami")// + .header("Authorization", "Basic cHNjK3Rlc3RhZG1pbkBnZW9yY2hlc3RyYS5vcmc6dGVzdGFkbWlu") // psc+testadmin@georchestra.org:testadmin + .exchange()// + .expectStatus()// + .is2xxSuccessful()// + .expectBody()// + .jsonPath("$.GeorchestraUser.username").isEqualTo("testadmin"); + } + public @Test void testWhoamiNoPasswordRevealed() { testClient.get().uri("/whoami")// .header("Authorization", "Basic dGVzdGFkbWluOnRlc3RhZG1pbg==") // testadmin:testadmin