-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
*: support creating a user without password #9817
Conversation
This PR isn't tested yet. I'll do it and add e2e test cases later. /cc @crielly @joelegasse |
e52ceb2
to
bc20594
Compare
@joelegasse I added the test cases, could you take a look? It seems that the travis failures aren't related to the change. |
Let's document this carefully. The behavior of the system for users with no password needs to be clear so that the security implications can be reasoned about easily. |
@jpbetz thanks for pointing out, I'll update the doc later. |
Updated the doc: https://github.com/coreos/etcd/pull/9817/files#diff-fa0a9438986edbb6abed85fbeea1753c |
Docs look great @mitake. Thanks! |
@mitake Can you fix the CI failure https://travis-ci.org/coreos/etcd/jobs/392115574#L898-L912? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code and doc changes look good to me. Just want to check on the backward compatibility before approving.
Thanks!
clientv3/auth.go
Outdated
@@ -61,7 +61,7 @@ type Auth interface { | |||
AuthDisable(ctx context.Context) (*AuthDisableResponse, error) | |||
|
|||
// UserAdd adds a new user to an etcd cluster. | |||
UserAdd(ctx context.Context, name string, password string) (*AuthUserAddResponse, error) | |||
UserAdd(ctx context.Context, name string, password string, noPassword bool) (*AuthUserAddResponse, error) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a source backward incompatible change in the client. Is this acceptable? The only alternative I can think of would be to introduce an additional UserAddNoPassword
function, or similar.
cc @gyuho for compatibility expectations of changes to client
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One other alternative would be to introduce a UserAddWithOptons(..., opts... UserAddOption)
function as well as a WithNoPassword
UserAddOption
. This would future proof the API against additions of more UserAdd parameters in the future. I'm not sure if that's justified here or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing out. I'll introduce UserAddWithOptons
. It would be good for expanding in the future as @jpbetz says.
clientv3/auth.go
Outdated
func (auth *authClient) UserAdd(ctx context.Context, name string, password string) (*AuthUserAddResponse, error) { | ||
resp, err := auth.remote.UserAdd(ctx, &pb.AuthUserAddRequest{Name: name, Password: password}, auth.callOpts...) | ||
func (auth *authClient) UserAdd(ctx context.Context, name string, password string, noPassword bool) (*AuthUserAddResponse, error) { | ||
resp, err := auth.remote.UserAdd(ctx, &pb.AuthUserAddRequest{Name: name, Password: password, NoPassword: noPassword}, auth.callOpts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above. Should we retain backward compatibility?
357a5b7
to
29a8e61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, this seems more complex where a simple length check on the password should work just as well.
I think having the extra flag in addition to always sending in a password argument is strange. At the CLI level, I would still suggest having the --no-password
flag just to signal that it's intended, but once at the protobuf/API level, an empty password could signal "disable password auth for this user".
Then, instead of checking an options field in the user for NoPassword
, it's just if len(u.Password) == 0 {}
.
auth/store.go
Outdated
) | ||
} else { | ||
plog.Errorf("failed to hash password: %s", err) | ||
hashed := make([]byte, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor note: var hashed []byte
should be used here instead of make
-ing a zero-length slice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll fix it soon
@joelegasse handling an empty password as a special case breaks existing semantics of the RPC so I don't want to do it. How do you think? @jpbetz @gyuho |
3709a11
to
f9b7766
Compare
I couldn't reproduce the travis fail on my local environment. It would be freaky. |
@mitake Do you have time to rebase this PR? You were correct that the current RPC allows users to be created/authenticated with an empty password, so that wouldn't be a backwards-compatible change. If not, I can take this over and get it cleaned up for more thorough testing. |
@joelegasse probably I will be able to rebase this weekend. Do you need to test it earlier? |
No, that should be fine. Thank you :-) |
b3dcbeb
to
fe92b77
Compare
@gyuho I'm still seeing a confusing build error like below, do you know why does this happen? It would be related to recent path change? I'll work on it probably tomorrow.
|
@wenjiaswe This PR add a new member to |
d8ea60d
to
bb20f87
Compare
The unit test is failing because of data race. I opened a new issue here: #10697 The issue seems to be independent from this PR. |
Codecov Report
@@ Coverage Diff @@
## master #9817 +/- ##
==========================================
+ Coverage 62.3% 63.19% +0.88%
==========================================
Files 392 392
Lines 37158 37183 +25
==========================================
+ Hits 23152 23498 +346
+ Misses 12438 12102 -336
- Partials 1568 1583 +15
Continue to review full report at Codecov.
|
This PR depends on the fix introduced in #10739 |
@mitake Sorry for delay. Will take a look by today. |
@mitake Can you also update changelog, with rebasing to trigger CIs? |
This commit adds a feature for creating a user without password. The purpose of the feature is reducing attack surface by configuring bad passwords (CN based auth will be allowed for the user). The feature can be used with `--no-password` of `etcdctl user add` command. Fix etcd-io#9590
Hi @mitake, I noticed an e2e test failure which might be related to this PR: https://semaphoreci.com/etcd-io/etcd/branches/pull-request-10797/builds/3 I took a quick look, maybe this function call also needs to be updated? |
@jingyih thanks a lot for pointing out! Yeah probably that's the reason. It is a little bit surprising for me because such an uninitialized variable is zero cleared by go. So the behavior of the function would be deterministic and the tests rely on it shouldn't be flaky. Probably protobuf layer has a pitfall related to this? I'll take a look but please let me know if you know something related to the behavior. Anyway I opened #10800 Could you take a look? BTW the link to the result of semaphore CI seems to be expired... I hope semaphore to keep results longer :( |
We should probably move all CI tests to Travis, now that it can run 13 jobs in parallel. The failed tests was |
This commit adds a feature for creating a user without password. The
purpose of the feature is reducing attack surface by configuring bad
passwords (CN based auth will be allowed for the user).
The feature can be used with
--no-password
ofetcdctl user add
command.
Fix #9590