-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[improve][broker/proxy] Optionally prevent role/originalPrincipal logging #23386
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
base: master
Are you sure you want to change the base?
Conversation
Do you have a chance to share some details of the use case where this is needed? Does the role contain sensitive information? Is this a compliance requirement to not log it? |
pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java
Outdated
Show resolved
Hide resolved
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/ServerCnx.java
Outdated
Show resolved
Hide resolved
pulsar-proxy/src/main/java/org/apache/pulsar/proxy/server/ProxyConnection.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/RedactedRole.java
Outdated
Show resolved
Hide resolved
We use token-based authN/authZ, so the role contains the token. This PR will remove logging them and reduce log storage as we produce like 30 log/s just for logging role content (with a 500bytes token). |
7e68070
to
29687ce
Compare
@lhotari I defined two configuration keys for broker and proxy. One anonymizes using SHA-256. The other put REDACTED. Is that ok? |
pulsar-broker-common/src/main/java/org/apache/pulsar/broker/ServiceConfiguration.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/RoleObfuscator.java
Outdated
Show resolved
Hide resolved
pulsar-common/src/main/java/org/apache/pulsar/common/util/RoleObfuscator.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/pulsar/common/util/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java
Outdated
Show resolved
Hide resolved
...ava/org/apache/pulsar/common/util/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java
Outdated
Show resolved
Hide resolved
b4c3ff3
to
368d2e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @KannarFr! 2 minor comments remaining. It would be good to document this as a PIP since most new features are documented that way. You can take a minimal approach for the PIP document and cover the relevant details that are provided in this PR.
...c/main/java/org/apache/pulsar/common/configuration/anonymizer/DefaultRoleAnonymizerType.java
Show resolved
Hide resolved
...pache/pulsar/common/configuration/anonymizer/DefaultAuthenticationRoleLoggingAnonymizer.java
Outdated
Show resolved
Hide resolved
7af5587
to
e153616
Compare
Done! |
Please split the PIP document to a separate PR. That's what is usually done. The PIP document PR would get merged before the implementation PR. |
When you create the PIP document PR, please make the file name pip-XXX.md since most PIP docs have such file name. You should check the mailing list and existing PRs to find a PIP number that isn't used yet. PIP-385 is already taken. |
Motivation
Add option in broker and proxy to optionally prevent them from logging role or orignalPrincipal. If the option is activated they will log
[REDACTED]
.If I missed some logs, please share them.
Verifying this change
This change is a trivial rework / code cleanup without any test coverage.
Does this pull request potentially affect one of the following parts:
Documentation
doc
doc-required
doc-not-needed
doc-complete