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

Password stored in plaintext in log #10132

Closed
jxuan opened this issue Sep 28, 2018 · 18 comments
Closed

Password stored in plaintext in log #10132

jxuan opened this issue Sep 28, 2018 · 18 comments

Comments

@jxuan
Copy link

jxuan commented Sep 28, 2018

On the etcd server, the auth token should be stored in the log (WAL). We used an etcd log dump tool to see the content, and observed that every time when there is an authenticate call, a new entry will be added to the log, eg:
676 593 norm header:<ID:13926931137005064452 > authenticate:<name:"etcdplay" password:"play" simple_token:"KFHwyVMhlNdHMCtU" >

It seems that the password followed the code trace below and gets in the log:
etcdserver/v3_server.go:398 to etcdserver/v3_server.go:606 and finally raft/raft.go:969

Is it possible to remove password from the log or change it to some format other than plaintext?

@gyuho
Copy link
Contributor

gyuho commented Sep 28, 2018

We plan to add redact feature to log dump tool, but in storage layer, we just store whatever blobs were sent to the server. So, I would consider encryption in client side, before writing to etcd, for now.

@jxuan
Copy link
Author

jxuan commented Sep 28, 2018

Do you think that the fix can get into the coming release?

@gyuho
Copy link
Contributor

gyuho commented Sep 28, 2018

@jxuan Do you mean redact support for dump log tool? It will be added to tooling, not to the core etcd. So, you won't have to wait for release.

@gyuho gyuho added this to the etcd-v3.4 milestone Sep 28, 2018
@jxuan
Copy link
Author

jxuan commented Sep 28, 2018

@gyuho The root of the problem is that password is stored in plaintext in storage. You should store the encrypted version only. Hackers can still get access to the password even if you redact the tool.

Encryption on the client side will work, or just remove password from storage directly. Do you think one of these can be done for the v3.4 release? Thanks

@gyuho
Copy link
Contributor

gyuho commented Sep 28, 2018

The root of the problem is that password is stored in plaintext in storage. You should store the encrypted version only. Hackers can still get access to the password even if you redact the tool.

I am aware of that. That's why I am suggesting to implement your own for the time being.

Encryption on the client side will work

I meant on your client side not on etcd client.

We have no plan to support this for v3.4.

Or you can try https://github.com/heyitsanthony/etcdcrypto.

@herlegs
Copy link

herlegs commented Sep 29, 2018

@gyuho Hi just want to clarify that the problem is not key value pair sent to etcd is not encrypted, but "etcd user's" auth info is stored as plaintext, as in the description shows: name:"etcdplay" password:"play" (which is an user create by root user)
so I'm not sure how client side encryption can solve the problem

@jxuan
Copy link
Author

jxuan commented Sep 29, 2018

The root of the problem is that password is stored in plaintext in storage. You should store the encrypted version only. Hackers can still get access to the password even if you redact the tool.

I am aware of that. That's why I am suggesting to implement your own for the time being.

Encryption on the client side will work

I meant on your client side not on etcd client.

We have no plan to support this for v3.4.

Or you can try https://github.com/heyitsanthony/etcdcrypto.

Adding client side encryption is very heavy operationally. We need to update the client code everywhere and re-create all accounts in etcd cluster.

Can you remove password from the blob, just keep the bcrypt version of it?

@jxuan
Copy link
Author

jxuan commented Sep 29, 2018

Even if we do client-side encryption and give custom_encrypt(incoming_password) to etcd client, hackers can still extract custom_encrypt(incoming_password) from etcd storage and use it to access etcd cluster without having incoming_password.

@gyuho

@gyuho
Copy link
Contributor

gyuho commented Sep 29, 2018

give custom_encrypt(incoming_password) to etcd client

If we support client-side encryption, it will expect users to provide keys.

We will research more. If you have more ideas, please let us know!

@wenjiaswe
Copy link
Contributor

cc @jpbetz

@jpbetz
Copy link
Contributor

jpbetz commented Oct 2, 2018

@jxuan Just catching up on this. I strongly recommend treating the entire etcd data-dir as sensitive and securing it appropriately. This includes the etcd WAL log, snapshot and db files.

Everything in the data-dir contains potentially sensitive data, including to the password you just mentioned. And while one solution to the problem you mention would be to encrypt just the passwords using a secret provided to the etcd servers in a secure way, we could just take idea further and encrypt all data at rest. That would be a substantial feature to add to etcd, and would require careful design and planning.

But securing the data-dir and the host OS is the first line of defense and does secure the password you mention. Putting the data-dir on an encrypted drive could harden that further.

@justinsheehy
Copy link

Why does the solution need to be anything more complicated than "don't log the password" when logging authenticate calls?

@jxuan
Copy link
Author

jxuan commented Nov 19, 2018

I think the solution should be "Do not store the password in the original format". Hackers can still extract password from storage even if the code "does not log the password"

@jpbetz
Copy link
Contributor

jpbetz commented Nov 25, 2018

@jxuan I agree. And if we store only a hash we'd need to salt it (else someone could rainbow table the hash), and we'd need to setup a way to manage the salt securely. There's considerable effort require adding this level of additional security. Worthwhile for sure, but effort that we'd need a contributor to sign up for. In the meantime I'll stick with my recommendation that for production use that the entire data directory be locked down.

@justinsheehy The confusion here might be what we mean by log. etcd has a couple different logs (1) plaintext log (e.g. etcd.log), (2) Write ahead log (WAL) file, this is a binary database file within the same data dir of etcd that the main data files are written to and requiring the same privileges to access. We're talking about #2 here. All data that propagates between members of an etcd server cluster must pass through the WAL file, so if we didn't log it here, it wouldn't be possible to update the user/pass data across the cluster.

@xiang90
Copy link
Contributor

xiang90 commented Apr 22, 2020

Agree. We should not log the password to stdout/stderr. We still need to log the password someway to the file system though. We currently assume the on disk files are secure. If we break this assumption, multiple things need to be improved. It would be great if anyone is interested in helping!

@mitake
Copy link
Contributor

mitake commented May 24, 2020

I confirmed that #11818 removes the plaintext password of a WAL log entry of authenticate RPC (#11818 (comment)). Although other type of log entries like user_add still has a plaintext password as its field, I think CN based auth can be a solution for users who really want to avoid the problem (but doing bcrypt hashing in the API layer can contribute to it, I'm thinking about the feasibility). And I think plaintext password in WAL wouldn't be a big problem because if etcd datadir is stolen by attackers they can read any data in it as @jpbetz pointed. I think closing this issue would be fine for now. How do you think? > all

@mitake
Copy link
Contributor

mitake commented May 24, 2020

#11943 will solve the remaining problem for UserAdd and UserUpdatePassword

@mitake
Copy link
Contributor

mitake commented Jul 27, 2020

#11943 is merged now so I think it's ok to close this issue. @jxuan if you have additional concerns, feel free to comment or reopen this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants