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

Password salting does not work with a property on customer or AdminUser #441

Closed
phillipuniverse opened this issue Oct 16, 2013 · 4 comments

Comments

@phillipuniverse
Copy link
Contributor

This works fine with a global salt but if you try to use a specific salt on a per-customer basis then you won't be able to login successfully.

The current salt paradigm (which only exists in CustomerService and AdminUserService) should be refactored to utilize Spring's SaltSource.

@ghost ghost assigned phillipuniverse Nov 7, 2013
@phillipuniverse
Copy link
Contributor Author

Slightly more complicated than originally anticipated, because the salt source should operate on the primary key of customer (which will never change), rather than just operating on the username field. For maximum portability and cleanest integration with Spring, I think that the following should happen:

  1. The Customer interface should extend from UserDetails (Spring interface)
  2. Add a List<CustomerRole> onto CustomerImpl (this won't cause any DB changes)
  3. For the getAuthorities() method implementation it should convert between CustomerRole and GrantedAuthority (see Broadleaf's UserDetailsServiceImpl for how to do this)
  4. Make CustomerService implement Spring's UserDetailsService which just requires the loadUserByUsername and just returns a Customer
  5. Change the references in applicationContext-security in DemoSite to have a SaltSource and use the blCustomerService rather than a new blUserDetailsService:
<!--  The BLC Authentication manager.   -->
<sec:authentication-manager alias="blAuthenticationManager">
    <sec:authentication-provider user-service-ref="blUserDetailsService">
        <sec:password-encoder ref="blPasswordEncoder">
            <sec:salt-source ref="blSaltSource" />
        </sec:password-encoder>
    </sec:authentication-provider>
</sec:authentication-manager>

<bean id="blSaltSource" class="org.springframework.security.authentication.dao.ReflectionSaltSource">
    <property name="userPropertyToUse" value="id" />
</bean>
  1. Remove the jdbc-user-service element within applicationContext-security in DemoSite
  2. Inject blSaltSource into CustomerService to use the same salting when creating Customers

The same types of modifications should be made to the AdminUser domain and applicationContext-security-admin.xml.

Also, all of the changes that I referenced should be just fine to make in the 3.0.x line.

@bpolster
Copy link
Member

bpolster commented Nov 7, 2013

@phillipuniverse IMO, our customer should not depend on spring security. Using the username is fine (typical) as a salt. Implementors should require a password change if the username is being changed.

@bpolster
Copy link
Member

bpolster commented Nov 7, 2013

@phillipuniverse This is also helpful and suggests using a Random salt. http://security.stackexchange.com/questions/8015/what-should-be-used-as-a-salt

@phillipuniverse
Copy link
Contributor Author

Rather than implement the Spring Security interfaces on our Broadleaf services and domain (like CustomerService and Customer), individual implementors can provide implementations of Springs UserDetailsService and UserDetails object so they can override the salting mechanism themselves. If they wanted to extend it they would just have to override what we did in the framework anyway; makes more sense for them to override what's already provided in Spring Security rather than another layer.

Note that I changed the issue title here to reflect problems in the admin as well. In general, our password approach is:

  1. Site - salt based on username (disallows username changes out of the box)
  2. Admin - salt based on primary key
  3. Default encoder is plaintext
  4. Configurable password encoders via properties file (site.password.encoder and admin.site.encoder)
  5. The framework does not provide the salt configuration but does utilize it

I also provided some default Sha1 admin passwords and the Spring Security config via BroadleafCommerce/DemoSite#43 for all demo sites going forward.

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

No branches or pull requests

2 participants