etcdserver/auth: check empty password #5196

gyuho · 2016-04-26T14:49:23Z

gyuho · 2016-04-26T14:51:46Z

Easiest way to reproduce:

./bin/etcdctl set foo value

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/users/foo -d '{"user":"foo", "password":"foo"}'
# auth: created user foo

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/roles/foo -d '{"role":"foo", "permissions": {"kv": {"read": ["/foo/*"]}}}'
# auth: created new role foo

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/users/foo -d '{"user": "foo", "grant": ["foo"]}'
# auth: updated user foo

./bin/etcdctl user add root:a
# auth: created user root

./bin/etcdctl auth enable
# auth: auth: enabled auth

curl -L -u foo:foo  http://127.0.0.1:2379/v2/keys/foo/
curl -L -u foo:  http://127.0.0.1:2379/v2/keys/foo/  
# THIS LAST COMMAND DOES NOT FAIL WITHOUT THIS PATCH

bcrypt.CompareHashAndPassword returns nil error for empty string.
This patch adds additional checks for empty string when the auth is enabled.

err := bcrypt.CompareHashAndPassword([]byte("$2a$10$Zzmtqsix/KthzodN0t21iOHIUlejJeRESPVoH7LIFvc7RkyUK3zIG"), []byte(""))
fmt.Println(err == nil) // true

xiang90 · 2016-04-26T14:52:38Z

etcdserver/api/v2http/client_auth.go

@@ -90,8 +90,7 @@ func hasKeyPrefixAccess(sec auth.Store, r *http.Request, key string, recursive b
 		plog.Warningf("auth: no such user: %s.", username)
 		return false
 	}
-	authAsUser := sec.CheckPassword(user, password)


we should probable fix CheckPassword function, not patch it in this func?

right. Will fix the CheckPassword.

xiang90 · 2016-04-26T15:02:45Z

etcdserver/auth/auth.go

-	return err == nil
+	matched := err == nil
+	// empty string password returns 'nil' error
+	if user.Password != "" && password == "" {


move this line before bcrypt?

gyuho · 2016-04-26T15:04:53Z

Still need to figure out why curl -L -u foo:foo http://127.0.0.1:2379/v2/keys/foo/ fails after updating the roles.

gyuho · 2016-04-26T19:31:59Z

/cc @xiang90 @heyitsanthony PTAL.

Added fix to handle empty password
Added, cleaned up cURL e2e tests

gyuho · 2016-04-26T19:32:56Z

./bin/etcdctl set foo value

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/users/foo -d '{"user":"foo", "password":"foo"}'
# auth: created user foo

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/roles/foo -d '{"role":"foo", "permissions": {"kv": {"read": ["/foo/*"]}}}'
# auth: created new role foo

curl -L -X PUT  http://127.0.0.1:2379/v2/auth/users/foo -d '{"user": "foo", "grant": ["foo"]}'
# auth: updated user foo

./bin/etcdctl user add root:a
./bin/etcdctl auth enable

# this should not error, but error
# client side: 'The request requires user authentication'
# server side: 'v2http: auth: incorrect password for user: foo.'
# FIXED
curl -L -u foo:foo  http://127.0.0.1:2379/v2/keys/foo/

# this should error, but does not error
# FIXED: it errors
# client side: 'The request requires user authentication'
# server side: 'v2http: auth: incorrect password for user: foo.'
curl -L -u foo:  http://127.0.0.1:2379/v2/keys/foo/

heyitsanthony · 2016-04-26T19:44:10Z

e2e/v2_curl_test.go

+	password string
+
+	isTLS  bool
+	isJSON bool


this doesn't look necessary-- "value=" can be prepended to the value string instead of setting isJSON : true in the request

gyuho · 2016-04-26T20:16:31Z

@heyitsanthony Cleaned up e2e tests. PTAL.

And JSON body gets Invalid JSON request error if sent with value=...
So I think we need isJSON option.

xiang90 · 2016-04-26T20:37:04Z

etcdserver/auth/auth.go

-		return old, err
+	if user.Password != "" {
+		var hash string
+		hash, err = s.HashPassword(user.Password)


i think merge should handle this? Does the behavior of hashpassword changed somehow?

Actually this check was all we need. (remove empty check in HashPassword).

https://github.com/coreos/etcd/blob/master/etcdserver/api/v2http/client_auth.go#L422-L442 passes user without password (even when user has been created with password), so we need to check this line I think.

I mean a few line under this, there is a merge call. In the merge call, the passed in user will be merged with the old user. If the old user has password, but the new user does not. The old password should be merged into new user. No?

Old password gets overwritten with the hash value of empty string if user was passed to UpdateUser with empty password.

The line here https://github.com/coreos/etcd/blob/master/etcdserver/auth/auth.go#L457-L459

So hash(empty) is not empty? And with your change, do we need that in merge at all? Or should we move your change to merge? I do not want the check to be at multiple places and one of them is wrong.

Yeah it is already very confusing...

Do we want to calculate the hash inside merge method?
I think it will be confusing too :0.

The first goal is to do this check at ONLY place, not two. Or we are making this worse.

agree. I will see if we can put all into merge method.

heyitsanthony · 2016-04-26T20:43:04Z

@gyuho sorry I mixed it up-- non-json requests would prepend value= and json requests would have the plain value. Basically, the part https://github.com/coreos/etcd/pull/5196/files#diff-b7cbda5cb5341dc6dad708e39bb62b90R172 isn't necessary because the relevant case can be folded into input the string (value) instead of passing it in a distinct flag (isJSON) as input.

gyuho · 2016-04-26T20:52:35Z

@heyitsanthony Can you point me to the line that you mention? Think the link points to the old commit. Thanks!

heyitsanthony · 2016-04-26T20:55:32Z

@gyuho the if !req.isJSON { dt = "value=" + dt } line. It doesn't check it anywhere else so I think it can be removed if the value strings are fixed.

gyuho · 2016-04-26T21:00:27Z

@heyitsanthony I think we need it for plain text. Doesn't work in my local machine.
It's weird...

Without that line, I get:

expected "{\"action\":\"set\",\"node\":{\"key\":\"/foo\",\"value\":\"bar\",\""
got ["{\"action\":\"set\",\"node\":{\"key\":\"/foo\",\"value\":\"\",\"modifiedIndex\":4,\"createdIndex\":4}}\r\n"]

heyitsanthony · 2016-04-26T21:29:21Z

@gyuho right... it's doing that because the value for a non-json curl request isn't starting with value= so the value for some PUT isn't going through. An alternative to putting value= in the test input directly would be testing the first character of value when doing a PUT and prepend value= if it's not {.

gyuho · 2016-04-26T21:34:24Z

@heyitsanthony Just fixed to check { for value=.

@xiang90 Just put hash logic into merge method.

PTAL.

Thanks all!

heyitsanthony · 2016-04-26T21:35:34Z

lgtm

xiang90 · 2016-04-26T21:37:37Z

etcdserver/auth/auth.go

@@ -448,29 +442,33 @@ func (s *store) DisableAuth() error {
 // is called and returns a new User with these modifications applied. Think of
 // all Users as immutable sets of data. Merge allows you to perform the set
 // operations (desired grants and revokes) atomically
-func (u User) merge(n User) (User, error) {
+func (s *store) merge(ou, nu User) (User, error) {


can we pass in the password interface? i think there is an interface for this. so we do not need to make this a method on store?

gyuho · 2016-04-26T21:51:37Z

etcdserver/auth/auth.go

@@ -448,29 +443,33 @@ func (s *store) DisableAuth() error {
 // is called and returns a new User with these modifications applied. Think of
 // all Users as immutable sets of data. Merge allows you to perform the set
 // operations (desired grants and revokes) atomically
-func (u User) merge(n User) (User, error) {
+func (s passwordStore) merge(ou, nu User) (User, error) {


@xiang90 Just changed this as a method to satisfy PasswordStore interface to make it not as store method.

PTAL.

func (u User) merge (n User, passwordStore pws)

Just fixed. PTAL. Thanks.

xiang90 · 2016-04-26T22:02:16Z

etcdserver/auth/auth_test.go

@@ -73,14 +88,14 @@ func TestMergeUser(t *testing.T) {
 		},
 		{
 			User{User: "foo"},
-			User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},


why do we want to change this?

because we now do the hash inside merge.

Otherwise, since this is not empty string, merge method will recalculate the hash returning different value.

that is ok. but the original test case is to ensure non-empty string password will not be overwrite. you change changes this test cases completely.

Then should we drop this test case? We can only detect the empty string, not the non-empty string is already bcrypted-password, I think.

ok. we can drop the old one. then why do we want to merge with an empty password user? that case is unclear. it is unclear about if the password is overwritten by the new user or is preserved for the existing user since both of them are empty.

gyuho · 2016-04-26T22:22:45Z

@xiang90 I just dropped the old one.

ok. we can drop the old one. then why do we want to merge with an empty password user? that case is unclear. it is unclear about if the password is overwritten by the new user or is preserved for the existing user since both of them are empty.

Yeah I had made it empty just to pass the test. I also think it doesn't make sense to have that case.

xiang90 · 2016-04-26T22:29:26Z

etcdserver/auth/auth_test.go

-			User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
-			User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
-			false,
-		},


We need to have a test case to cover your new change. We need to ensure the empty password will not overwrite the previous password.

Can we add

{ User{User: "foo"}, User{User: "foo", "ps"}, User{User: "foo", "ps"}, false, }

?

Fix etcd-io#5182.

For etcd-io#5182.

gyuho · 2016-04-26T22:37:59Z

etcdserver/auth/auth_test.go

-			User{User: "foo"},
-			User{User: "foo", Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
-			User{User: "foo", Roles: []string{}, Password: "$2a$10$aUPOdbOGNawaVSusg3g2wuC3AH6XxIr9/Ms4VgDvzrAVOJPYzZILa"},
+		{ // empty password will not overwrite the previous password


@xiang90 Just added the case to test empty password will not overwrite the previous password. PTAL. Thanks!

i do not think this is correct. the input password is not empty. it is foo.

This test merges tt.input with tt.merge. So if we want to test empty password will not overwrite the previous password, the previous password must come from the one that gets overwritten, which is tt.input.User?

tt.input is the one that would get overwritten with previous implementation. Please correct me if I am wrong.

tt.input is the one actually overwrites the correct one. Say a user has a password: AAA. We want to update its role only. So the input user has empty password but a role, the password should remain AAA, not overwrite to empty.

ok. i see what you are say... the naming in the test is really confusing.

xiang90 · 2016-04-26T22:54:16Z

LGTM

gyuho · 2016-04-26T22:54:47Z

Thanks! merging after all greens.

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from 6838186 to 49ab1c6 Compare April 26, 2016 14:59

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho changed the title ~~auth: invalid for empty password~~ [working in progress] auth: invalid for empty password Apr 26, 2016

gyuho force-pushed the password_check branch 2 times, most recently from 66cb407 to fd60840 Compare April 26, 2016 19:29

gyuho changed the title ~~[working in progress] auth: invalid for empty password~~ etcdserver/auth: check empty password Apr 26, 2016

gyuho force-pushed the password_check branch from fd60840 to 467ce0e Compare April 26, 2016 19:31

heyitsanthony reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from 467ce0e to c50b23f Compare April 26, 2016 20:14

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from c50b23f to a875ef3 Compare April 26, 2016 20:40

gyuho force-pushed the password_check branch from a875ef3 to 613ba6f Compare April 26, 2016 20:51

gyuho force-pushed the password_check branch 2 times, most recently from ae915fc to 890e07a Compare April 26, 2016 21:27

gyuho force-pushed the password_check branch from 890e07a to 695f299 Compare April 26, 2016 21:33

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from 695f299 to cc86728 Compare April 26, 2016 21:50

gyuho reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from cc86728 to d421c50 Compare April 26, 2016 21:58

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho force-pushed the password_check branch from d421c50 to a7b923a Compare April 26, 2016 22:21

xiang90 reviewed Apr 26, 2016
View reviewed changes

gyuho added 2 commits April 26, 2016 15:37

etcdserver/auth: check empty password in merge

07685bc

Fix etcd-io#5182.

e2e: add tests for issue 5182

bba08f6

For etcd-io#5182.

gyuho force-pushed the password_check branch from a7b923a to bba08f6 Compare April 26, 2016 22:37

gyuho reviewed Apr 26, 2016
View reviewed changes

gyuho merged commit c8ab6c3 into etcd-io:master Apr 26, 2016

gyuho deleted the password_check branch April 26, 2016 22:56

xiang90 mentioned this pull request Apr 27, 2016

etcd serves stale read requests before log is caught up during recovery/startup #3879

Closed

etcdserver/auth: check empty password #5196

etcdserver/auth: check empty password #5196

Conversation

gyuho commented Apr 26, 2016

gyuho commented Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Apr 26, 2016

gyuho commented Apr 26, 2016

gyuho commented Apr 26, 2016

Choose a reason for hiding this comment

gyuho commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heyitsanthony commented Apr 26, 2016 • edited Loading

gyuho commented Apr 26, 2016

heyitsanthony commented Apr 26, 2016

gyuho commented Apr 26, 2016

heyitsanthony commented Apr 26, 2016

gyuho commented Apr 26, 2016

heyitsanthony commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho Apr 26, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gyuho commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Apr 26, 2016

gyuho commented Apr 26, 2016

gyuho commented Apr 26, 2016 •

edited

Loading

heyitsanthony commented Apr 26, 2016 •

edited

Loading

gyuho Apr 26, 2016 •

edited

Loading