Skip to content

Commit

Permalink
Addressed Review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
madhukarbharti committed Oct 18, 2020
1 parent fc5d0d2 commit b24731d
Show file tree
Hide file tree
Showing 7 changed files with 281 additions and 162 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

import org.carlspring.strongbox.authentication.support.AuthenticationContextInitializer;
import org.carlspring.strongbox.config.UsersConfig;
import org.carlspring.strongbox.configuration.StrongboxDelegatingPasswordEncoder;
import org.carlspring.strongbox.configuration.WebSecurityConfig;
import org.carlspring.strongbox.users.domain.SystemRole;

import org.junit.jupiter.api.Test;
Expand All @@ -25,6 +25,7 @@
import org.springframework.ldap.core.LdapTemplate;
import org.springframework.security.core.authority.SimpleGrantedAuthority;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.security.ldap.userdetails.LdapUserDetails;
import org.springframework.security.ldap.userdetails.LdapUserDetailsImpl;
import org.springframework.security.ldap.userdetails.LdapUserDetailsService;
Expand All @@ -37,7 +38,8 @@
* @author sbespalov
*/
@SpringBootTest
@ContextHierarchy({ @ContextConfiguration(classes = UsersConfig.class),
@ContextHierarchy({ @ContextConfiguration(classes = { UsersConfig.class,
WebSecurityConfig.class }),
@ContextConfiguration(locations = "classpath:/ldapServerApplicationContext.xml"),
@ContextConfiguration(initializers = LdapAuthenticationProviderTest.TestContextInitializer.class, locations = "classpath:/org/carlspring/strongbox/authentication/external/ldap/strongbox-authentication-providers.xml") })
@ActiveProfiles(profiles = "test")
Expand All @@ -49,12 +51,15 @@ public class LdapAuthenticationProviderTest
@Inject
private ContextSource contextSource;

@Inject
private PasswordEncoder passwordEncoder;

@Inject
private LdapUserDetailsService ldapUserDetailsService;

@Test
public void embeddedLdapServerCreationContainsExpectedContextSourceAndData()
throws Exception
throws Exception
{
LdapTemplate template = new LdapTemplate(contextSource);
Object ldapObject = template.lookup("uid=przemyslaw.fusik,ou=Users");
Expand All @@ -63,7 +68,8 @@ public void embeddedLdapServerCreationContainsExpectedContextSourceAndData()
assertThat(ldapObject).isInstanceOf(DirContextAdapter.class);
DirContextAdapter dirContextAdapter = (DirContextAdapter) ldapObject;
assertThat(dirContextAdapter.getDn().toString()).isEqualTo("uid=przemyslaw.fusik,ou=Users");
assertThat(dirContextAdapter.getNameInNamespace()).isEqualTo("uid=przemyslaw.fusik,ou=Users,dc=carlspring,dc=com");
assertThat(dirContextAdapter.getNameInNamespace()).isEqualTo(
"uid=przemyslaw.fusik,ou=Users,dc=carlspring,dc=com");
}

@Test
Expand All @@ -75,19 +81,20 @@ public void embeddedLdapServerRegistersExpectedAuthenticationProvider()
LdapUserDetails ldapUserDetails = (LdapUserDetailsImpl) ldapUser;

assertThat(ldapUserDetails.getDn()).isEqualTo("uid=przemyslaw.fusik,ou=Users,dc=carlspring,dc=com");
assertThat(ldapUserDetails.getPassword()).isEqualTo("{SHA-256}{mujKRdqeWWYAWhczNwVnBl6L6dHNwWO5eIGZ/G7pnBg=}bb63813f5b6f64ae306ebbbb23dcbb1c6f49eb9b989fc466b1b1a24a011bb2ce");
assertThat(ldapUserDetails.getPassword()).isEqualTo(
"{SHA-256}{mujKRdqeWWYAWhczNwVnBl6L6dHNwWO5eIGZ/G7pnBg=}bb63813f5b6f64ae306ebbbb23dcbb1c6f49eb9b989fc466b1b1a24a011bb2ce");
assertThat(passwordEncoder.matches("password", ldapUserDetails.getPassword())).isEqualTo(true);
assertThat(ldapUserDetails.getUsername()).isEqualTo("przemyslaw.fusik");
assertThat(((List<SimpleGrantedAuthority>)ldapUser.getAuthorities()))
assertThat(((List<SimpleGrantedAuthority>) ldapUser.getAuthorities()))
.contains(
new SimpleGrantedAuthority(SystemRole.REPOSITORY_MANAGER.name()),
new SimpleGrantedAuthority("USER_ROLE")
);
}

@Test
public void base64EncodedPasswordsShouldWork()
public void base64EncodedPasswordAfterAlgorithmShouldWork()
{
StrongboxDelegatingPasswordEncoder passwordEncoder = new StrongboxDelegatingPasswordEncoder();
UserDetails ldapUser = ldapUserDetailsService.loadUserByUsername("base64encoded-issue-1840");

assertThat(ldapUser).isInstanceOf(LdapUserDetailsImpl.class);
Expand All @@ -97,14 +104,36 @@ public void base64EncodedPasswordsShouldWork()
assertThat(ldapUserDetails.getPassword()).isEqualTo("{MD5}X03MO1qnZdYdgyfeuILPmQ==");
assertThat(ldapUserDetails.getUsername()).isEqualTo("base64encoded-issue-1840");
assertThat(passwordEncoder.matches("password", ldapUserDetails.getPassword())).isEqualTo(true);
assertThat(((List<SimpleGrantedAuthority>)ldapUser.getAuthorities()))
assertThat(((List<SimpleGrantedAuthority>) ldapUser.getAuthorities()))
.contains(
new SimpleGrantedAuthority(SystemRole.REPOSITORY_MANAGER.name()),
new SimpleGrantedAuthority("USER_ROLE")
);
}

@Test
public void base64EncodedPasswordWithAlgorithmShouldWork()
{
UserDetails ldapUser = ldapUserDetailsService.loadUserByUsername("base64withalgorithm-issue-1840");

assertThat(ldapUser).isInstanceOf(LdapUserDetailsImpl.class);
LdapUserDetails ldapUserDetails = (LdapUserDetailsImpl) ldapUser;

assertThat(ldapUserDetails.getDn()).isEqualTo(
"uid=base64withalgorithm-issue-1840,ou=Users,dc=carlspring,dc=com");
assertThat(ldapUserDetails.getPassword()).isEqualTo(
"e2JjcnlwdH0kMmEkMTAkbHB3bHh5anZYS3pOMWNjQ3J3MlBCdVp4LmVWZXNXYmZtVGJzckNib01VLmdzTldWY1pXTWk=");
assertThat(ldapUserDetails.getUsername()).isEqualTo("base64withalgorithm-issue-1840");
assertThat(passwordEncoder.matches("password", ldapUserDetails.getPassword())).isEqualTo(true);
assertThat(((List<SimpleGrantedAuthority>) ldapUser.getAuthorities()))
.contains(
new SimpleGrantedAuthority(SystemRole.REPOSITORY_MANAGER.name()),
new SimpleGrantedAuthority("USER_ROLE")
);
}

public static class TestContextInitializer extends AuthenticationContextInitializer
public static class TestContextInitializer
extends AuthenticationContextInitializer
{

public TestContextInitializer()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ givenName: Martin
surname: Todorov
userPassword: {bcrypt}$2a$10$lpwlxyjvXKzN1ccCrw2PBuZx.eVesWbfmTbsrCboMU.gsNWVcZWMi

# Create users with base64 encoded password of uid mtodorov
# Create users with base64 encoded password after algorithm(passwords: password)
dn: uid=base64encoded-issue-1840,ou=Users,dc=carlspring,dc=com
objectClass: inetOrgPerson
objectClass: organizationalPerson
Expand All @@ -37,6 +37,19 @@ givenName: Martin
surname: Todorov
userPassword: {MD5}X03MO1qnZdYdgyfeuILPmQ==

# Create users with base64 encoded password including algorithm(passwords: password)
dn: uid=base64withalgorithm-issue-1840,ou=Users,dc=carlspring,dc=com
objectClass: inetOrgPerson
objectClass: organizationalPerson
objectClass: person
objectClass: top
uid: base64withalgorithm-issue-1840
cn: base64withalgorithm-issue-1840
mail: mtodorov@carlspring.com
givenName: Martin
surname: Todorov
userPassword: e2JjcnlwdH0kMmEkMTAkbHB3bHh5anZYS3pOMWNjQ3J3MlBCdVp4LmVWZXNXYmZtVGJzckNib01VLmdzTldWY1pXTWk=

dn: uid=stodorov,ou=Users,dc=carlspring,dc=com
objectClass: inetOrgPerson
objectClass: organizationalPerson
Expand Down Expand Up @@ -96,6 +109,7 @@ uniqueMember: uid=mtodorov,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=stodorov,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=przemyslaw.fusik,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=base64encoded-issue-1840,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=base64withalgorithm-issue-1840,ou=Users,dc=carlspring,dc=com

# Employees -> Contributors
dn: ou=Contributors, ou=Employees, ou=Groups, dc=carlspring, dc=com
Expand All @@ -105,6 +119,7 @@ description: All contributors
objectClass: groupOfUniqueNames
uniqueMember: uid=przemyslaw.fusik,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=base64encoded-issue-1840,ou=Users,dc=carlspring,dc=com
uniqueMember: uid=base64withalgorithm-issue-1840,ou=Users,dc=carlspring,dc=com

# Employees -> Managers
dn: ou=Managers, ou=Employees, ou=Groups, dc=carlspring, dc=com
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,207 @@
package org.carlspring.strongbox.configuration;

import java.util.Base64;

import org.apache.commons.lang3.StringUtils;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.security.crypto.codec.Hex;
import org.springframework.security.crypto.codec.Utf8;
import org.springframework.security.crypto.password.PasswordEncoder;
import org.springframework.stereotype.Component;

/**
* This class encodes given raw String or Base64 encoded string.
* If given string is Base64 Encoded it will attempt to decode the password and finally delegate it to the password encoder.
* This handles all possible cases
* <p>
* {ALG}md5/sha1/bcrypt(mypassword)
* {ALG}base64.encode(md5/sha1/bcrypt(mypassword))
* base64.encode({ALG}md5/sha1/bcrypt(mypassword))
*
* @author mbharti
* @date 07/10/20
*/
@Component
public class Base64PasswordEncoderDelegate
implements PasswordEncoder
{

private static final String PREFIX = "{";

private static final String SUFFIX = "}";

private final PasswordEncoder passwordEncoder;

private static final Logger logger = LoggerFactory.getLogger(Base64PasswordEncoderDelegate.class);


/**
* Constructs PasswordEncoder with given PasswordEncoder
*
* @param passwordEncoder
*/
public Base64PasswordEncoderDelegate(PasswordEncoder passwordEncoder)
{
this.passwordEncoder = passwordEncoder;
}

/**
* This Encodes the raw password. In case given rawCharSequence is Base64 encoded,
* then it will decode and then encode the decoded string.
*
* @param rawCharSequence Raw String or Base64 encoded String
* @return Encoded String
*/
@Override
public String encode(CharSequence rawCharSequence)
{
return passwordEncoder.encode(rawCharSequence);
}

/**
* Verify the encoded password obtained from storage matches the submitted raw password after it too is encoded.
* Returns true if the passwords match, false if they do not. The stored password itself is never decoded.
* In case given rawCharSequence is Base64 encoded, then it decode it first then verifies.
*
* @param rawCharSequence Raw password
* @param encodedString Encoded Hash String or hash containing Base64 encoded String
* @return true if the passwords match
* false if the password do not match
*/
@Override
public boolean matches(CharSequence rawCharSequence,
String encodedString)
{
boolean matches = matchesEncodedPassword(rawCharSequence, encodedString);

// When the default password encoder does not match - attempt to decode password and retry.
// Edge case for https://github.com/strongbox/strongbox/issues/1840}
if (!matches)
{
matches = matchesBase64EncodedPasswordAfterAlgorithm(rawCharSequence, encodedString);

if (!matches)
{
matches = matchesBase64EncodedPassword(rawCharSequence, encodedString);
}
}

return matches;
}

private boolean matchesEncodedPassword(CharSequence rawCharSequence,
String encodedString)
{
try
{
return passwordEncoder.matches(rawCharSequence, encodedString);
}
catch (Exception e)
{
logger.warn("Failed to match password");
return false;
}
}


private boolean matchesBase64EncodedPasswordAfterAlgorithm(CharSequence rawCharSequence,
CharSequence prefixEncodedPassword)
{
if (prefixEncodedPassword == null || rawCharSequence == null)
{
return false;
}

try
{
String prefixEncodedPasswordString = prefixEncodedPassword.toString();

String algorithmUsed = extractId(prefixEncodedPasswordString);

String base64DecodedPasswordAfterAlgorithm;

String extractBase64EncodedHash = prefixEncodedPasswordString;

if (StringUtils.isNotEmpty(algorithmUsed))
{
extractBase64EncodedHash = extractEncodedPassword(prefixEncodedPasswordString);

base64DecodedPasswordAfterAlgorithm =
PREFIX + algorithmUsed + SUFFIX + decodeBase64EncodedHashWithHex(extractBase64EncodedHash);
}
else
{
base64DecodedPasswordAfterAlgorithm = decodeBase64EncodedHashWithHex(extractBase64EncodedHash);
}

return passwordEncoder.matches(rawCharSequence, base64DecodedPasswordAfterAlgorithm);
}
catch (Exception e)
{
logger.warn("Failed to match password after decoding base64encoded hash after algorithm");
return false;
}
}

private boolean matchesBase64EncodedPassword(CharSequence rawCharSequence,
CharSequence prefixEncodedPassword)
{
if (prefixEncodedPassword == null || rawCharSequence == null)
{
return false;
}
try
{
String base64DecodedPassword = new String(
Base64.getDecoder().decode(Utf8.encode(prefixEncodedPassword.toString())));

return passwordEncoder.matches(rawCharSequence, base64DecodedPassword);
}
catch (Exception e)
{
logger.warn("Failed to match password after decoding base64 hash included algorithm");
return false;
}
}

private String decodeBase64EncodedHashWithHex(String base64EncodedHash)
{
try
{
return new String(Hex.encode(Base64.getDecoder().decode(Utf8.encode(base64EncodedHash))));
}
catch (Exception ex)
{
logger.warn("Failed to do Base64Decoding " + ex.getMessage());
}

return base64EncodedHash;
}

private String extractEncodedPassword(String prefixEncodedPassword)
{
int start = prefixEncodedPassword.indexOf(SUFFIX);

return prefixEncodedPassword.substring(start + 1);
}

private String extractId(String prefixEncodedPassword)
{
int start = prefixEncodedPassword.indexOf(PREFIX);

if (start != 0)
{
return StringUtils.EMPTY;
}

int end = prefixEncodedPassword.indexOf(SUFFIX, start);

if (end < 0)
{
return StringUtils.EMPTY;
}

return prefixEncodedPassword.substring(start + 1, end);
}
}

0 comments on commit b24731d

Please sign in to comment.