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

BCrypt encode calls increase startup times #4915

Closed
philwebb opened this issue Dec 18, 2017 · 2 comments
Closed

BCrypt encode calls increase startup times #4915

philwebb opened this issue Dec 18, 2017 · 2 comments
Assignees
@rwinch
Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Milestone
5.0.1

Comments

@philwebb
Copy link
Member

philwebb commented Dec 18, 2017 •
edited
Loading

Summary

Whilst investigating Spring Boot startup time regressions it became apparent that BCrypt.encode is a hot method.

Actual Behavior

DaoAuthenticationProvider currently generates an encoded password in doAfterPropertiesSet. This encoded password is decoded when a UsernameNotFoundException is throw in order to prevent timing attacks (a user that isn't found takes about the same time to process as one that is).

Expected Behavior

It would be nice to remove the encode call from the application startup critical-path so that application startup times are not increased. One possible option might be to make userNotFoundEncodedPassword late binding and to calculate it on the first call to retrieveUser.

I believe (although I'm by no means an expert here) that this should allow faster startup without being susceptible to timing attacks. The first call to retrieveUser would be a little slower, but this will be constant regardless of if a user is ultimately found or not. Subsequent call will have the same behavior as the current implementation.

Configuration

Spring Boot 2.0.M7 running spring-boot-sample-actuator-ui.

Version

5.0.0.RELEASE

Sample

https://github.com/spring-projects/spring-boot/tree/master/spring-boot-samples/spring-boot-sample-actuator-ui
@rwinch rwinch added this to the 5.0.1 milestone Dec 18, 2017
@philwebb philwebb mentioned this issue Dec 18, 2017
Closed
@rwinch rwinch self-assigned this Dec 18, 2017
@rwinch rwinch added type: enhancement A general enhancement in: web An issue in web modules (web, webmvc) in: core An issue in spring-security-core and removed in: web An issue in web modules (web, webmvc) labels Dec 18, 2017
@philwebb philwebb mentioned this issue Dec 22, 2017
Closed
@philwebb
Copy link
Member Author

With PR #4927 applied Spring Boot startup improves from

Started SampleActuatorUiApplication in 3.373 seconds (JVM running for 3.781)

to

Started SampleActuatorUiApplication in 3.142 seconds (JVM running for 3.545)

@philwebb philwebb mentioned this issue Jan 8, 2018
Closed
philwebb added a commit to philwebb/spring-security that referenced this issue Jan 9, 2018
@philwebb
Lazily initialize userNotFoundEncodedPassword
cff6f18 
Update `DaoAuthenticationProvider` so that `userNotFoundEncodedPassword`
is lazily initialized on the first call to `retrieveUser`, rather than
in `doAfterPropertiesSet`.

Since some `PasswordEncoder` implementations can be slow, this change
can help to improve application startup times and the expense of some
delay with the first login.

Note that `userNotFoundEncodedPassword` creation occurs on the first
user retrieval, regardless of whether the user is ultimately found. This
ensures consistent processing times, regardless of the outcome.

First Call:
	Found      = encode(userNotFound) + decode(supplied)
	Not-Found  = encode(userNotFound) + decode(userNotFound)

Subsequent Call:
	Found      = decode(supplied)
	Not-Found  = decode(userNotFound)

Fixes spring-projectsgh-4915
@rwinch rwinch closed this as completed in fd78d05 Jan 24, 2018
rwinch added a commit that referenced this issue Jan 24, 2018
@rwinch
Polish userNotFoundEncodedPassword
6ba225b 
Ensure that if passwordEncoder is set that userNotFoundEncodedPassword
is encoded again if already set.

Issue: gh-4915
@philwebb
Copy link
Member Author

Sweeeeeet!! Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rwinch rwinch

Labels
in: core An issue in spring-security-core type: enhancement A general enhancement
Projects
None yet
Milestone
5.0.1
Development

Successfully merging a pull request may close this issue.

Lazily initialize userNotFoundEncodedPassword philwebb/spring-security
2 participants
@rwinch @philwebb