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

Add configuration hide-user-data-from-log to hide user data from server logs #877

Merged
merged 22 commits into from
Sep 2, 2024

Conversation

naglera
Copy link
Contributor

@naglera naglera commented Aug 8, 2024

Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

Copy link

codecov bot commented Aug 8, 2024

Codecov Report

Attention: Patch coverage is 27.58621% with 21 lines in your changes missing coverage. Please review.

Project coverage is 70.42%. Comparing base (04d76d8) to head (f8e62fe).
Report is 25 commits behind head on unstable.

Files with missing lines Patch % Lines
src/debug.c 0.00% 19 Missing ⚠️
src/replication.c 66.66% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##           unstable     #877      +/-   ##
============================================
- Coverage     70.43%   70.42%   -0.02%     
============================================
  Files           113      114       +1     
  Lines         61728    61647      -81     
============================================
- Hits          43479    43413      -66     
+ Misses        18249    18234      -15     
Files with missing lines Coverage Δ
src/acl.c 88.89% <100.00%> (ø)
src/config.c 78.69% <ø> (ø)
src/networking.c 88.34% <100.00%> (-0.04%) ⬇️
src/server.h 100.00% <ø> (ø)
src/replication.c 87.13% <66.66%> (+0.01%) ⬆️
src/debug.c 53.67% <0.00%> (-0.65%) ⬇️

... and 25 files with indirect coverage changes

@madolson
Copy link
Member

madolson commented Aug 8, 2024

In the client info we have two extra pieces of information, the name and the user, we need to decide if this counts as user data and should also be redacted from the log.

@madolson
Copy link
Member

madolson commented Aug 8, 2024

@valkey-io/core-team Overall would like a vote for this. In addition, some specific questions for this feature:

  1. What should the config name be. My recommendation is hide-user-data-in-log
  2. Is the user and name attached a client considered highly sensitive data and we should avoid logging. We marked the username as sensitive here: 3c0fd25.

If we don't believe users should be logged, we should also hide them from the client list output that is also in the log.

@madolson madolson added major-decision-pending Major decision pending by TSC team release-notes This issue should get a line item in the release notes labels Aug 9, 2024
@naglera naglera changed the title Secure logs: Mask PII Secure logs: Mask user data Aug 11, 2024
@PingXie
Copy link
Member

PingXie commented Aug 12, 2024

  1. What should the config name be. My recommendation is hide-user-data-in-log

If Redis has already picked this name and the behavior is similar, I think this name is good (so users don't have to learn a new concept).

Though from a quick look at the PR, we also use this config to decide whether to log the stack trace, which makes "user-data" a bit far fetched IMO - just a nit.

@hwware
Copy link
Member

hwware commented Aug 12, 2024

@valkey-io/core-team Overall would like a vote for this. In addition, some specific questions for this feature:

  1. What should the config name be. My recommendation is hide-user-data-in-log
  2. Is the user and name attached a client considered highly sensitive data and we should avoid logging. We marked the username as sensitive here: 3c0fd25.

If we don't believe users should be logged, we should also hide them from the client list output that is also in the log.

  1. hide-user-data-in-log is fine for me, and it aligns to Redis
  2. username can be sensitive data, please reference function isSensitiveCommand (valkey-cli.c), which username, passpword, and sentinel-user are all sensitive

@madolson madolson added major-decision-approved Major decision approved by TSC team and removed major-decision-pending Major decision pending by TSC team labels Aug 12, 2024
@madolson
Copy link
Member

username can be sensitive data, please reference function isSensitiveCommand (valkey-cli.c), which username, passpword, and sentinel-user are all sensitive

I'm aligned with this. Let's remove the name and username from the client output as well.

Copy link
Contributor Author

@naglera naglera left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When it comes to masking the client's name and username from the client output, should we apply it to all instances where client information is printed, or only to the cases where the server voluntarily reveals such details?
i.e. should client list and client info work normaly when feature is enabeld?
@hwware @madolson

@madolson
Copy link
Member

@naglera I think a bunch of tests are failing because we changed the default value, which is breaking a bunch of log scans for tests. I think for testing we can default to having it on.

Copy link
Member

@enjoy-binbin enjoy-binbin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new code LGTM.

@enjoy-binbin enjoy-binbin changed the title Secure logs: Mask user data Secure logs: Mask user data and add hide-user-data-from-log configuration item Aug 27, 2024
Copy link
Member

@ranshid ranshid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Signed-off-by: Madelyn Olson <[email protected]>
@madolson madolson added the run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP) label Sep 2, 2024
Minor changes to wording

Signed-off-by: Madelyn Olson <[email protected]>
@madolson madolson changed the title Secure logs: Mask user data and add hide-user-data-from-log configuration item Add configuration hide-user-data-from-log to hide user data from server logs Sep 2, 2024
@madolson madolson merged commit 5fdb47c into valkey-io:unstable Sep 2, 2024
56 checks passed
madolson pushed a commit that referenced this pull request Sep 3, 2024
…er logs (#877)

Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

---------

Signed-off-by: Amit Nagler <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Sep 5, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
enjoy-binbin added a commit that referenced this pull request Sep 5, 2024
Minor cleanup, introduced in #877.

Signed-off-by: Binbin <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 13, 2024
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 13, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…er logs (valkey-io#877)

Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

---------

Signed-off-by: Amit Nagler <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…er logs (valkey-io#877)

Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

---------

Signed-off-by: Amit Nagler <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
…er logs (valkey-io#877)

Implement data masking for user data in server logs and diagnostic output. This change prevents potential exposure of confidential information, such as PII, and enhances privacy protection. It masks all command arguments, client names, and client usernames.

Added a new hide-user-data-from-log configuration item, default yes.

---------

Signed-off-by: Amit Nagler <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
enjoy-binbin added a commit to enjoy-binbin/valkey that referenced this pull request Sep 14, 2024
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit to PingXie/valkey that referenced this pull request Sep 14, 2024
Minor cleanup, introduced in valkey-io#877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
PingXie pushed a commit that referenced this pull request Sep 15, 2024
Minor cleanup, introduced in #877.

Signed-off-by: Binbin <[email protected]>
Signed-off-by: Ping Xie <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-decision-approved Major decision approved by TSC team release-notes This issue should get a line item in the release notes run-extra-tests Run extra tests on this PR (Runs all tests from daily except valgrind and RESP)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants