fix(redis): do not log the Redis password - #1316
bakiburakogun wants to merge 1 commit into
Conversation
e578398 to
3e240e6
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
There was a problem hiding this comment.
Thanks for fixing this. The valid URL cases work. I left one comment on malformed URLs: that leak predates this PR, but the startup error still exposes the password.
Please add tests for the documented examples, encoded credentials, and startup errors. Keep the list of Redis URLs in #1318 covered too.
| return parsed.toString() | ||
| } catch { | ||
| // not a parsable URL: log nothing rather than risk leaking a secret | ||
| return '<unparsable REDIS_URL>' |
There was a problem hiding this comment.
With redis://:test-password@, this helper hides the URL, but the next new URL(Config.REDIS_URL) throws an error containing the original value in input. main() then logs it, including the password.
Please return a configuration error without the raw URL and test the startup log, not just this helper.
The websocket server logs its Redis URL verbatim on every start, so a
deployment that authenticates to Redis writes the password into the journal
in clear text:
Creating Redis client with URL: redis://:s3cret@10.0.0.5:6379
Redact the credentials before logging. The host and port, which are what the
line is useful for, stay visible.
Redacting the log line is not enough on its own. The statement after it parses
the same value with new URL(), and the error that throws carries the value it
was given on its input property, which main() prints when the server fails to
start. Raise a configuration error that repeats nothing of the value instead.
A comma is legal inside a Redis password, so the value is only read as several
node URLs when every part on its own is a URL with a scheme and a host. That
leaves redis://:pass,word@host alone while still redacting each node of the
seed list used for Redis Cluster.
Signed-off-by: Baki Burak Öğün <63836730+bakiburakogun@users.noreply.github.com>
3e240e6 to
1022dca
Compare
|
You are right, and the hole was next to the one I closed rather than in it.
On the list of Redis URLs: I had to be careful there, because a comma is legal inside a password. Tests are in
Verified against the branch with the fix reverted: 11 of the 29 fail, including the malformed URL one. The full suite is 102 passing across 13 files, and Branch rebased onto current |
The problem
RedisAdapter.createRedisClient()logs the configured Redis URL verbatim every time the websocket server starts. On any deployment that authenticates to Redis, that writes the password into the journal in clear text:We hit this on a production install: the password we had just put in a
0600environment file showed up injournalctla second later, where it is readable by anyone who can read the journal and by whatever ships logs off the box.The change
Redact the credentials before logging. The host and port, which are the useful part of the line, stay visible:
A URL that cannot be parsed logs a placeholder rather than the raw string, so a malformed value with a secret in it does not leak either.
Testing