etcd-io / etcd

Distributed reliable key-value store for the most critical data of a distributed system
https://etcd.io
Apache License 2.0
47.78k stars 9.77k forks source link

auth/simple_token.go token not removed when etcdctl session closes #6554

Closed vimalk78 closed 7 years ago

vimalk78 commented 8 years ago

The Token generated for a user during Authentication is being cached in auth/authStore All tokens for a user are being removed when a user is deleted, or the user changes password, but not removed when the user session closes, or the same user gets authenticated again.

ideally the token should be removed when the authenticated session is over. at least it should be removed when the same user gets a new Token.

currently, if etcdctl is used for many operations with auth enabled, the map size keeps on growing.

 46 func (as *authStore) assignSimpleTokenToUser(username, token string) {
 47     as.simpleTokensMu.Lock()
 48 
 49     _, ok := as.simpleTokens[token]
 50     if ok {
 51         plog.Panicf("token %s is alredy used", token)
 52     }
 53 
 54     as.simpleTokens[token] = username
 55     plog.Errorf("added user %s , total = %d", username, len(as.simpleTokens))
 56     as.simpleTokensMu.Unlock()
 57 }

The above modified code prints the following log.

17:35:39 etcd1 | 2016-09-29 17:35:39.365732 E | auth: added user root , total = 1 17:35:39 etcd2 | 2016-09-29 17:35:39.377584 E | auth: added user root , total = 1 17:35:39 etcd3 | 2016-09-29 17:35:39.379829 E | auth: added user root , total = 1 17:35:52 etcd1 | 2016-09-29 17:35:52.018620 E | auth: added user root , total = 2 17:35:52 etcd3 | 2016-09-29 17:35:52.023928 E | auth: added user root , total = 2 17:35:52 etcd2 | 2016-09-29 17:35:52.037939 E | auth: added user root , total = 2 17:35:53 etcd3 | 2016-09-29 17:35:53.858867 E | auth: added user root , total = 3 17:35:53 etcd1 | 2016-09-29 17:35:53.871008 E | auth: added user root , total = 3 17:35:53 etcd2 | 2016-09-29 17:35:53.880090 E | auth: added user root , total = 3 17:35:55 etcd1 | 2016-09-29 17:35:55.794764 E | auth: added user root , total = 4 17:35:55 etcd2 | 2016-09-29 17:35:55.799397 E | auth: added user root , total = 4 17:35:55 etcd3 | 2016-09-29 17:35:55.812799 E | auth: added user root , total = 4 17:35:57 etcd3 | 2016-09-29 17:35:57.196612 E | auth: added user root , total = 5 17:35:57 etcd2 | 2016-09-29 17:35:57.221568 E | auth: added user root , total = 5 17:35:57 etcd1 | 2016-09-29 17:35:57.229336 E | auth: added user root , total = 5 17:35:58 etcd2 | 2016-09-29 17:35:58.512297 E | auth: added user root , total = 6 17:35:58 etcd3 | 2016-09-29 17:35:58.526757 E | auth: added user root , total = 6 17:35:58 etcd1 | 2016-09-29 17:35:58.528447 E | auth: added user root , total = 6 17:35:59 etcd2 | 2016-09-29 17:35:59.872109 E | auth: added user root , total = 7 17:35:59 etcd1 | 2016-09-29 17:35:59.877785 E | auth: added user root , total = 7 17:35:59 etcd3 | 2016-09-29 17:35:59.885849 E | auth: added user root , total = 7

vimalk78 commented 8 years ago

i think this is a problem even when using embedded v3 client.

mitake commented 8 years ago

@vimalk78 yes, current simple token has a problem of not having timeout mechanism. I'm planning to add the mechanism after finishing jwt token support: https://github.com/coreos/etcd/pull/6082

xiang90 commented 8 years ago

@mitake Can we fix this for simple token first? We will support jwt in 3.2 not 3.1

vimalk78 commented 8 years ago

@xiang90 , @mitake what is your idea about invalidating the tokens, remove after session is closed, or have a keep alive time and remove after timeout

mitake commented 8 years ago

@xiang90 ok, then I'll fix the simple token first.

@vimalk78 removing after a timeout would be good. Do you have any ideas?

vimalk78 commented 8 years ago

@mitake , removing the token as soon as the session closes will be best. next best thing is to remove earlier token for the same user when user gets authenticated again. this way we wont need a timer based approach. with timer based approach we will also need mechanism for clients to reset the timer, if they choose to continue the session, else forcing to authenticate again.

vimalk78 commented 8 years ago

@xiang90 , @mitake may i work on this and send a PR ?

mitake commented 8 years ago

@vimalk78 of course, it will be great.

mitake commented 8 years ago

@xiang90 should we finish it before releasing 3.1.0? Then I should work on it instead of @vimalk78 for finishing soon. If backporting is ok, I can cooperate with @vimalk78

vimalk78 commented 8 years ago

my solution direction is to remove the simpleToken when the client closes connection. so was trying to find the hook in grpc for the same. https://groups.google.com/forum/#!topic/grpc-io/EIcQlLqlNQg

for now i can finish by checking for any existing token for username in the auth.assignSimpleTokenToUser and removing it before adding the new Token

xiang90 commented 7 years ago

Fixed by https://github.com/coreos/etcd/pull/6574