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

auth, etcdserver: hash password in the API layer #11943

Merged
merged 2 commits into from
Jul 20, 2020

Conversation

mitake
Copy link
Contributor

@mitake mitake commented May 24, 2020

With this change etcd gets hashed password for UserAdd and UserChangePassword. Log entries of WAL will be like below:

   2            10      norm    header:<ID:3632560226236973574 > auth_user_add:<name:"u1" password:"$2a$10$tc.gSL/vyM48WXtMzDTKZuXWyWIbMqPQPWLR20GY8he0OzSpU4jgi" options:<> >

Note that this PR is still WIP (conversion between string and byte array in the API layer).

@mitake
Copy link
Contributor Author

mitake commented May 24, 2020

Note that this PR changes the content of log entry and state machine behavior so cannot be backported to the stable releases.

@mitake mitake force-pushed the bcrypt-in-api branch 3 times, most recently from b085cf6 to 7a75e48 Compare June 10, 2020 14:42
@mitake mitake changed the title WIP RFC auth, etcdserver: hash password in the API layer auth, etcdserver: hash password in the API layer Jun 17, 2020
@mitake
Copy link
Contributor Author

mitake commented Jun 17, 2020

I think it's ready to be reviewed, could you take a look @jingyih @gyuho @spzala ?

@mitake
Copy link
Contributor Author

mitake commented Jun 24, 2020

kindly ping, could you take a look? @xiang90 @jingyih @gyuho @spzala

etcdserver/v3_server.go Outdated Show resolved Hide resolved
@jingyih
Copy link
Contributor

jingyih commented Jun 30, 2020

Note that this PR changes the content of log entry and state machine behavior so cannot be backported to the stable releases.

I just added feature label to this PR (usually we do not backport features). Maybe we want to create a do-not-backport label for this type of changes?

@jingyih
Copy link
Contributor

jingyih commented Jun 30, 2020

Does this feature prevent user from performing rolling upgrade to their etcd clusters?

@tangcong
Copy link
Contributor

tangcong commented Jul 1, 2020

Does this feature prevent user from performing rolling upgrade to their etcd clusters?

yes,if new member applies a UserAddRequest from old member, it is failed to add user and cause data inconsistency. if r.HashPassword == "" && r.Password != "" ,we have to be compatible with old version logic.how do you think so? @mitake

@mitake
Copy link
Contributor Author

mitake commented Jul 1, 2020

@jingyih

Maybe we want to create a do-not-backport label for this type of changes?

Yes, creating such a label is valuable. PRs which introduce changed behavior of the state machine should be labeled with it.

@tangcong

yes,if new member applies a UserAddRequest from old member, it is failed to add user and cause data inconsistency. if r.HashPassword == "" && r.Password != "" ,we have to be compatible with old version logic.how do you think so? @mitake

That's a good idea, I'll make a commit for implementing the strategy.

@mitake
Copy link
Contributor Author

mitake commented Jul 5, 2020

@tangcong @jingyih updated for the compatibility issue, could you take a look?

auth/store.go Outdated Show resolved Hide resolved
etcdserver/v3_server.go Show resolved Hide resolved
@mitake
Copy link
Contributor Author

mitake commented Jul 9, 2020

@jingyih could you take a look when you have a time? I think it's ok to merge this PR (the failed test isn't related to this PR).

@mitake
Copy link
Contributor Author

mitake commented Jul 12, 2020

cc @spzala

Copy link
Member

@spzala spzala left a comment

Choose a reason for hiding this comment

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

@mitake thank you, lgtm but can you please fix the build error - https://travis-ci.com/github/etcd-io/etcd/jobs/358895971#L413 I think you just need to remove the else loop on LOC 386 which is unnecessary. Thanks!

@mitake
Copy link
Contributor Author

mitake commented Jul 13, 2020

@spzala oops sorry for that, I fixed it in the latest commit

@spzala
Copy link
Member

spzala commented Jul 13, 2020

@spzala oops sorry for that, I fixed it in the latest commit

@mitake :) no problem at all and Thanks!
@jingyih hi, any final comments from you? Thanks!

@spzala
Copy link
Member

spzala commented Jul 20, 2020

@mitake thanks much! Merging it. @gyuho @xiang90 @jingyih

@spzala spzala merged commit ef866a6 into etcd-io:master Jul 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

5 participants