From 9e178711fefa5d5497528341e454a542f98cb0eb Mon Sep 17 00:00:00 2001 From: jangrui Date: Sat, 30 May 2026 08:19:22 +0800 Subject: [PATCH] fix(auth): address LDAP security and stability issues from code review - Prevent spring-boot-starter-data-ldap AutoConfiguration side effects by setting spring.ldap.urls="" to avoid connection attempts when disabled - Add conditional LdapAutoConfiguration that only creates LdapTemplate when skillhub.ldap.enabled=true - Fix resource leaks in LdapAuthService (DirContext, NamingEnumeration) - Add safeLogHost() to prevent credential exposure in logs - Add connection timeouts (5s connect, 10s read) to prevent hangs - Add LDAP injection prevention via isValidUsername() validation Signed-off-by: jangrui --- .../src/main/resources/application.yml | 6 ++ .../auth/config/LdapAutoConfiguration.java | 40 ++++++++++ .../skillhub/auth/ldap/LdapAuthService.java | 80 ++++++++++++++++--- .../skillhub/auth/local/LocalAuthService.java | 26 +++++- 4 files changed, 137 insertions(+), 15 deletions(-) create mode 100644 server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapAutoConfiguration.java diff --git a/server/skillhub-app/src/main/resources/application.yml b/server/skillhub-app/src/main/resources/application.yml index 66af1081..3b8f21e6 100644 --- a/server/skillhub-app/src/main/resources/application.yml +++ b/server/skillhub-app/src/main/resources/application.yml @@ -15,6 +15,10 @@ spring: basename: messages application: name: skillhub + ldap: + # Prevent spring-boot-starter-data-ldap AutoConfiguration from attempting + # to initialize LDAP connections when LDAP is not configured + urls: "" lifecycle: timeout-per-shutdown-phase: 30s jpa: @@ -105,6 +109,8 @@ skillhub: email-from-address: ${SKILLHUB_AUTH_PASSWORD_RESET_FROM_ADDRESS:noreply@skillhub.local} email-from-name: ${SKILLHUB_AUTH_PASSWORD_RESET_FROM_NAME:SkillHub} ldap: + # LDAP authentication configuration + # Set enabled to true and configure url/base/username/password to enable LDAP authentication enabled: ${SKILLHUB_LDAP_ENABLED:false} url: ${SKILLHUB_LDAP_URL:} base: ${SKILLHUB_LDAP_BASE:} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapAutoConfiguration.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapAutoConfiguration.java new file mode 100644 index 00000000..ecccb619 --- /dev/null +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/config/LdapAutoConfiguration.java @@ -0,0 +1,40 @@ +package com.iflytek.skillhub.auth.config; + +import org.springframework.boot.autoconfigure.condition.ConditionalOnProperty; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.ldap.core.LdapTemplate; +import org.springframework.ldap.core.support.LdapContextSource; + +/** + * LDAP auto-configuration that creates LdapTemplate only when LDAP is enabled. + * This avoids the startup side effects of spring-boot-starter-data-ldap's + * auto-configuration when LDAP is disabled. + */ +@Configuration +@ConditionalOnProperty(name = "skillhub.ldap.enabled", havingValue = "true") +public class LdapAutoConfiguration { + + /** + * Creates an LdapContextSource configured from skillhub.ldap properties. + */ + @Bean + public LdapContextSource ldapContextSource(LdapProperties ldapProperties) { + LdapContextSource contextSource = new LdapContextSource(); + contextSource.setUrl(ldapProperties.getUrl()); + contextSource.setBase(ldapProperties.getBase()); + if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) { + contextSource.setUserDn(ldapProperties.getUsername()); + contextSource.setPassword(ldapProperties.getPassword()); + } + return contextSource; + } + + /** + * Creates an LdapTemplate for LDAP operations. + */ + @Bean + public LdapTemplate ldapTemplate(LdapContextSource ldapContextSource) { + return new LdapTemplate(ldapContextSource); + } +} diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java index 3ea272d6..bf590475 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/ldap/LdapAuthService.java @@ -74,8 +74,8 @@ public class LdapAuthService { throw new AuthFlowException(HttpStatus.SERVICE_UNAVAILABLE, "error.auth.ldap.disabled"); } - log.debug("LDAP URL: {}, Base: {}, SearchBase: {}, SearchAttr: {}", - ldapProperties.getUrl(), + log.debug("LDAP host: {}, base: {}, searchBase: {}, searchAttr: {}", + safeLogHost(ldapProperties.getUrl()), ldapProperties.getBase(), ldapProperties.getUserSearchBase(), ldapProperties.getUserSearchAttribute()); @@ -138,6 +138,12 @@ public class LdapAuthService { * Finds the DN (Distinguished Name) of a user in LDAP. */ private String findUserDn(String username) { + // LDAP injection prevention: validate username before search + if (!isValidUsername(username)) { + log.warn("Invalid username format for LDAP search: {}", username); + return null; + } + DirContext ctx = null; javax.naming.NamingEnumeration results = null; try { @@ -181,12 +187,16 @@ public class LdapAuthService { env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); env.put(Context.PROVIDER_URL, ldapProperties.getUrl()); env.put(Context.SECURITY_AUTHENTICATION, "simple"); - + + // Connection timeout: 5 seconds for connect, 10 seconds for read + env.put("com.sun.jndi.ldap.connect.timeout", "5000"); + env.put("com.sun.jndi.ldap.read.timeout", "10000"); + if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) { env.put(Context.SECURITY_PRINCIPAL, ldapProperties.getUsername()); env.put(Context.SECURITY_CREDENTIALS, ldapProperties.getPassword()); } - + return new InitialDirContext(env); } @@ -203,10 +213,34 @@ public class LdapAuthService { } } + /** + * Safely extracts host from LDAP URL for logging, avoiding credential exposure. + * Handles formats like: ldap://host:389, ldap://user:pass@host:389, ldaps://host + */ + private String safeLogHost(String url) { + if (url == null || url.isEmpty()) { + return ""; + } + try { + // Remove protocol prefix + String withoutProtocol = url.replaceFirst("^ldaps?://", ""); + // Extract host:port or just host + int atIndex = withoutProtocol.indexOf('@'); + if (atIndex > 0) { + withoutProtocol = withoutProtocol.substring(atIndex + 1); + } + int colonIndex = withoutProtocol.indexOf(':'); + return colonIndex > 0 ? withoutProtocol.substring(0, colonIndex) : withoutProtocol; + } catch (Exception e) { + return "[url-parse-error]"; + } + } + /** * Authenticates a user against the LDAP server using their DN and password. */ private boolean authenticateLdap(String userDn, String password) { + DirContext ctx = null; try { Hashtable env = new Hashtable<>(); env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); @@ -214,12 +248,15 @@ public class LdapAuthService { env.put(Context.SECURITY_AUTHENTICATION, "simple"); env.put(Context.SECURITY_PRINCIPAL, userDn); env.put(Context.SECURITY_CREDENTIALS, password); + env.put("com.sun.jndi.ldap.connect.timeout", "5000"); + env.put("com.sun.jndi.ldap.read.timeout", "10000"); - DirContext ctx = new InitialDirContext(env); - ctx.close(); + ctx = new InitialDirContext(env); return true; } catch (NamingException e) { return false; + } finally { + closeContext(ctx); } } @@ -227,24 +264,28 @@ public class LdapAuthService { * Retrieves user attributes from LDAP. */ private Attributes getUserAttributes(String userDn) { + DirContext ctx = null; try { Hashtable env = new Hashtable<>(); env.put(Context.INITIAL_CONTEXT_FACTORY, "com.sun.jndi.ldap.LdapCtxFactory"); env.put(Context.PROVIDER_URL, ldapProperties.getUrl()); env.put(Context.SECURITY_AUTHENTICATION, "simple"); - + env.put("com.sun.jndi.ldap.connect.timeout", "5000"); + env.put("com.sun.jndi.ldap.read.timeout", "10000"); + // Use bind DN if configured, otherwise anonymous bind if (ldapProperties.getUsername() != null && !ldapProperties.getUsername().isEmpty()) { env.put(Context.SECURITY_PRINCIPAL, ldapProperties.getUsername()); env.put(Context.SECURITY_CREDENTIALS, ldapProperties.getPassword()); } - - DirContext ctx = new InitialDirContext(env); + + ctx = new InitialDirContext(env); Attributes attrs = ctx.getAttributes(new LdapName(userDn)); - ctx.close(); return attrs; } catch (Exception e) { return null; + } finally { + closeContext(ctx); } } @@ -272,8 +313,11 @@ public class LdapAuthService { // If not found, create a new user if (user == null) { - // Use a unique identifier based on username if email is missing - // This prevents creating duplicate accounts for users without email + // For LDAP users without email, use "ldap:{username}@internal" as a unique identifier. + // This format: + // 1. Prevents duplicate accounts when email attribute is missing + // 2. Clearly identifies the account origin (LDAP vs local) + // 3. Follows email format to satisfy the email NOT NULL constraint String normalizedEmail = email != null ? email.toLowerCase() : "ldap:" + username + "@internal"; user = new UserAccount( @@ -322,4 +366,16 @@ public class LdapAuthService { roles ); } + + /** + * Validates username to prevent LDAP injection attacks. + * Allows only alphanumeric characters and underscores, 3-64 characters. + */ + private boolean isValidUsername(String username) { + if (username == null || username.isEmpty()) { + return false; + } + // Allow alphanumeric, underscore, hyphen, dot, and @ for UPN formats + return username.matches("^[A-Za-z0-9_@.\\-]{3,64}$"); + } } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java index 398b75e1..db6b57de 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/local/LocalAuthService.java @@ -134,9 +134,9 @@ public class LocalAuthService { // Fallback to LDAP authentication if enabled if (ldapProperties.isEnabled()) { log.info("Local user not found, attempting LDAP authentication for username: {}", username); - log.debug("LDAP enabled: {}, URL: {}, Base: {}", - ldapProperties.isEnabled(), - ldapProperties.getUrl(), + log.debug("LDAP enabled: {}, host: {}, base: {}", + ldapProperties.isEnabled(), + safeLogHost(ldapProperties.getUrl()), ldapProperties.getBase()); try { PlatformPrincipal ldapPrincipal = ldapAuthService.login(username, password); @@ -271,4 +271,24 @@ public class LocalAuthService { throw new AuthFlowException(HttpStatus.BAD_REQUEST, "validation.auth.local.email.invalid"); } } + + /** + * Safely extracts host from LDAP URL for logging, avoiding credential exposure. + */ + private static String safeLogHost(String url) { + if (url == null || url.isEmpty()) { + return ""; + } + try { + String withoutProtocol = url.replaceFirst("^ldaps?://", ""); + int atIndex = withoutProtocol.indexOf('@'); + if (atIndex > 0) { + withoutProtocol = withoutProtocol.substring(atIndex + 1); + } + int colonIndex = withoutProtocol.indexOf(':'); + return colonIndex > 0 ? withoutProtocol.substring(0, colonIndex) : withoutProtocol; + } catch (Exception e) { + return "[url-parse-error]"; + } + } }