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.

Sign up for GitHub

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

Jump to bottom

Added support for specifying LDAP UPN in properties file #683

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added support for specifying LDAP UPN in properties file #683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions ...auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,17 +120,28 @@ public class AuthenticationProviderService {
* If required properties are missing, and thus the user DN cannot be
* determined.
*/
private Dn getUserBindDN(LDAPConfiguration config, String username)
throws GuacamoleException {
private Dn getUserBindDN(LDAPConfiguration config, String username, String password) throws GuacamoleException {

// If a search DN is provided, search the LDAP directory for the DN
// corresponding to the given username
String searchBindLogon = config.getSearchBindDN();

String searchBindLogon;
String searchBindPassword;

if(confService.getUPNDomain() != "" && confService.getUPNDomain() != null){
Copy link
Contributor

Choose a reason for hiding this comment

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

The test confService.getUPNDomain() != "" performs a by-reference comparison against the String object for "", not a comparison of the content of those strings. The proper test for whether a String is empty is isEmpty(), however the null check shown here would have to occur first for that call to not throw a NullPointerException in the event the string is indeed null.

searchBindLogon = username + "@" + confService.getUPNDomain();
searchBindPassword = password;
Comment on lines +132 to +133
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this should be done. The search bind DN is a very specific thing - it's the service account that Guacamole uses to resolve the DN of a user. It shouldn't be replaced with the user's own account.

If the idea here is to allow logins via UPN and avoid the search bind DN entirely, then the solution would be to allow user authentication to proceed without a user search by directly binding with the provided UPN - essentially an alternative to the direct user mapping already supported.

}else{
searchBindLogon = config.getSearchBindDN();
searchBindPassword = config.getSearchBindPassword();
}
if (searchBindLogon != null) {

// Create an LDAP connection using the search account
LdapNetworkConnection searchConnection = ldapService.bindAs(config,
searchBindLogon, config.getSearchBindPassword());
searchBindLogon,
searchBindPassword
);

// Warn of failure to find
if (searchConnection == null) {
Expand Down Expand Up @@ -219,7 +230,8 @@ private UserLDAPConfiguration getLDAPConfiguration(String username,
config.getServerHostname(), username, translatedUsername);

// Derive DN of user within this LDAP server
Dn bindDn = getUserBindDN(config, translatedUsername);

Dn bindDn = getUserBindDN(config, username, password);
if (bindDn == null || bindDn.isEmpty()) {
logger.info("Unable to determine DN of user \"{}\" using LDAP "
+ "server \"{}\". Proceeding with next server...",
Expand Down
16 changes: 16 additions & 0 deletions ...ole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/ConfigurationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,20 @@ else if (cachedConfigurations != null) {

}

/**
* Returns the UPN domain name used to authenticate via LDAP,
* or null by default.
*
* @return
* The UPN domain name of the LDAP accounts when authenticating per-user.
*
* @throws GuacamoleException
* If guacamole.properties connect be parsed.
*/
public String getUPNDomain() throws GuacamoleException {
return environment.getProperty(
LDAPGuacamoleProperties.LDAP_UPN_DOMAIN,
null
);
}
}
13 changes: 12 additions & 1 deletion ...-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LDAPGuacamoleProperties.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,17 @@ private LDAPGuacamoleProperties() {}
@Override
public String getName() { return "ldap-hostname"; }

};

/**
* The domain used for a UPN username for LDAP server to connect to when authenticating users.
*/
public static final StringGuacamoleProperty LDAP_UPN_DOMAIN =
new StringGuacamoleProperty() {

@Override
public String getName() { return "ldap-upn-domain"; }

};

/**
Expand Down Expand Up @@ -298,5 +309,5 @@ private LDAPGuacamoleProperties() {}
public String getName() { return "ldap-member-attribute-type"; }

};

}