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

feature: add the delegating password encoder for apollo-portal simple auth #3804

Merged
merged 13 commits into from
Jul 10, 2021

Conversation

vdiskg
Copy link
Contributor

@vdiskg vdiskg commented Jul 2, 2021

What's the purpose of this PR

the best algorithm for password storage should change some day in the future. the DelegatingPasswordEncoder can makes the algorithm upgrade seamless.

Brief changelog

  1. extend the ApolloPortalDB.Users.Password to 512
  2. replace the BCryptPasswordEncoder with the DelegatingPasswordEncoder backend of the BCryptPasswordEncoder
  3. add a PlaceholderPasswordEncoder which return a random string as encoded password and never matches any encoded password.
  4. replace the random string password for oidc with the DelegatingPasswordEncoder backend of the PlaceholderPasswordEncoder

Follow this checklist to help us incorporate your contribution quickly and easily:

  • Read the Contributing Guide before making this pull request.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
  • Write necessary unit tests to verify the code.
  • Run mvn clean test to make sure this pull request doesn't break anything.
  • Update the CHANGES log.

@codecov-commenter
Copy link

codecov-commenter commented Jul 2, 2021

Codecov Report

Merging #3804 (d65ad73) into master (1afa5ad) will decrease coverage by 0.24%.
The diff coverage is 0.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3804      +/-   ##
============================================
- Coverage     50.16%   49.92%   -0.25%     
+ Complexity     2446     2444       -2     
============================================
  Files           479      482       +3     
  Lines         14807    14853      +46     
  Branches       1503     1507       +4     
============================================
- Hits           7428     7415      -13     
- Misses         6858     6917      +59     
  Partials        521      521              
Impacted Files Coverage Δ
...lo/portal/spi/configuration/AuthConfiguration.java 5.47% <0.00%> (-0.09%) ⬇️
...ollo/portal/spi/oidc/OidcLocalUserServiceImpl.java 0.00% <0.00%> (ø)
...lo/portal/spi/oidc/PlaceholderPasswordEncoder.java 0.00% <0.00%> (ø)
...i/springsecurity/ApolloPasswordEncoderFactory.java 0.00% <0.00%> (ø)
...tal/spi/springsecurity/PasswordEncoderAdapter.java 0.00% <0.00%> (ø)
.../spi/springsecurity/SpringSecurityUserService.java 0.00% <0.00%> (ø)
...work/apollo/biz/message/DatabaseMessageSender.java 50.00% <0.00%> (-14.59%) ⬇️
...mework/apollo/portal/component/PortalSettings.java 57.81% <0.00%> (-7.82%) ⬇️
.../apollo/internals/RemoteConfigLongPollService.java 77.10% <0.00%> (-1.21%) ⬇️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1afa5ad...d65ad73. Read the comment docs.

@nobodyiam
Copy link
Member

This feature looks great!

However, I have one concern that if we need to change the password from "xxx" to "{bcrypt}xxx", does that mean the user could not upgrade to this version smoothly? e.g.

  1. If the password is not changed, then the new version of apollo-portal won't work
  2. If the password is changed, then the old version of apollo-portal won't work

So how about we leave the password in the original form and we let the new version of apollo-portal adapt them? Does the org.springframework.security.crypto.password.DelegatingPasswordEncoder#setDefaultPasswordEncoderForMatches work for this scenario?

@vdiskg
Copy link
Contributor Author

vdiskg commented Jul 3, 2021

This feature looks great!

However, I have one concern that if we need to change the password from "xxx" to "{bcrypt}xxx", does that mean the user could not upgrade to this version smoothly? e.g.

  1. If the password is not changed, then the new version of apollo-portal won't work
  2. If the password is changed, then the old version of apollo-portal won't work

So how about we leave the password in the original form and we let the new version of apollo-portal adapt them? Does the org.springframework.security.crypto.password.DelegatingPasswordEncoder#setDefaultPasswordEncoderForMatches work for this scenario?

add an adapter for old password.
upgrade step:

  1. all of apollo-portal is old version, and the password has no prefix, all of apollo-portal works fine.
  2. upgrade some of the apollo-portal to the new version, and the password has no prefix, the old version of the apollo-portal works fine as before and the new version of apollo-portal works fine with the help of the adapter.
  3. upgrade all of apollo-portal, and the password has no prefix, the new version of apollo-portal works fine with the help of the adapter.
  4. execute the sql scripts, change the password from "xxx" to "{bcrypt}xxx", all of the new version of apollo-portal works fine.

@vdiskg vdiskg force-pushed the delegate-password-encoder branch from 39d6482 to 586a630 Compare July 3, 2021 12:46
@nobodyiam
Copy link
Member

execute the sql scripts, change the password from "xxx" to "{bcrypt}xxx", all of the new version of apollo-portal works fine.

Is this step mandatory? What if we just leave the password as in the form of "xxx"?

@vdiskg
Copy link
Contributor Author

vdiskg commented Jul 3, 2021

execute the sql scripts, change the password from "xxx" to "{bcrypt}xxx", all of the new version of apollo-portal works fine.

Is this step mandatory? What if we just leave the password as in the form of "xxx"?

it do work at the moment of update apollo-portal to the new version.
but once the user update the password, it will be changed to "{bcrypt}xxx" by the DelegatingPasswordEncoder, and those users can only login on the new version of apollo-portal.
so it seems ok not to execute the sql scripts, but all of the apollo-portal need to update to the new version.

@nobodyiam
Copy link
Member

so it seems ok not to execute the sql scripts, but all of the apollo-portal need to update to the new version.

Right, this is a must and we should note it in the release note

@vdiskg
Copy link
Contributor Author

vdiskg commented Jul 4, 2021

execute the sql scripts, change the password from "xxx" to "{bcrypt}xxx", all of the new version of apollo-portal works fine.

Is this step mandatory? What if we just leave the password as in the form of "xxx"?

for better compatibility, we could split this pr into 2 parts.

  1. on the 1.9.0 add the DelegatingPasswordEncoder just as the decoder and just use the BCryptPasswordEncoder as the encoder as before. so the apollo-portal of 1.9.0 can work well with the old version of apollo-portal.
  2. on the next feature version, just like 1.10.0 replace the BCryptPasswordEncoder with DelegatingPasswordEncoder as the encoder. and then change the password from "xxx" to "{bcrypt}xxx". the apollo-portal of 1.9.0 and 1.10.0 can both work well.

do you think it is necessary to do that?

@nobodyiam
Copy link
Member

I think it's not necessary to split this pr into 2 parts. It should be ok as long as the new version of apollo-portal could decode the password in both xxx and {bcrypt}xxx forms so that we don't require users to do the data migration.
If the user changes the password during the apollo-portal upgrade process, then there are chances the user could not login (when accessing the old version apollo-portal) but I think this chance is trivial so it is ok.

Copy link
Member

@nobodyiam nobodyiam left a comment

Choose a reason for hiding this comment

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

LGTM

@nobodyiam nobodyiam merged commit 72a0498 into apolloconfig:master Jul 10, 2021
@github-actions github-actions bot locked and limited conversation to collaborators Jul 10, 2021
@nobodyiam nobodyiam added this to the 1.9.0 milestone Jul 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants