esrally must not log clear text user passwords #464
Conversation
…rprise environments.
…rprise environments.
esrally/client.py
Outdated
| @@ -12,7 +12,12 @@ class EsClientFactory: | |||
| Abstracts how the Elasticsearch client is created. Intended for testing. | |||
| """ | |||
| def __init__(self, hosts, client_options): | |||
| logger.info("Creating ES client connected to %s with options [%s]" % (hosts, client_options)) | |||
| masked_client_options = dict(client_options) | |||
| if masked_client_options.has_key("basic_auth_password"): | |||
There was a problem hiding this comment.
Can you please change this to "basic_auth_password" in masked_client_options?
There was a problem hiding this comment.
Yes. I learned it's more pyhtonic and Pyhton 3 compatible: https://stackoverflow.com/questions/1323410/has-key-or-in
esrally/client.py
Outdated
| masked_client_options = dict(client_options) | ||
| if masked_client_options.has_key("basic_auth_password"): | ||
| masked_client_options["basic_auth_password"] = "*****" | ||
| if masked_client_options.has_key("http_auth"): |
There was a problem hiding this comment.
Can you please change this to "http_auth" in masked_client_options?
esrally/client.py
Outdated
| if masked_client_options.has_key("basic_auth_password"): | ||
| masked_client_options["basic_auth_password"] = "*****" | ||
| if masked_client_options.has_key("http_auth"): | ||
| masked_client_options["http_auth"] = (client_options["basic_auth_user"], "*****") |
There was a problem hiding this comment.
I think this should be masked_client_options["http_auth"] = (client_options["http_auth"][0], "*****") as you are modifying the http_auth entry and a basic_auth_user does not necessarily exist then.
danielmitterdorfer
added
enhancement
esrally/client.py
Outdated
| masked_client_options["basic_auth_password"] = "*****" | ||
| if masked_client_options.has_key("http_auth"): | ||
| masked_client_options["http_auth"] = (client_options["basic_auth_user"], "*****") | ||
| logger.info("Creating ES client connected to %s with options [%s]" % (hosts, masked_client_options)) |
There was a problem hiding this comment.
Very minor comment/ask, the current code is not consistent at the moment, but while you are at it, would it be possible to use the comma format and have the options as arguments, as documented here? The idea is that moving forwards we move away from % for strings, so in your case it'd be:
logger.info("Creating ES client connected to %s with options [%s]", hosts, masked_client_options)
Thank you!
|
Thanks for the review. I applied the requested changes. |
|
Thanks for your contribution and iterating on the PR! The changes looks fine to me. We'll merge the PR soon. It will be included in the next release 0.10.0 of Rally. |
|
Thanks for accepting my commit. Looking forward to run 0.10.0 soonish :). |
esrally 0.9.4 logs the basic auth password in clear text:
esrally must not do this to enable usage in enterprise environments.
Please accept PR (following).