You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@storm.apache.org by arunmahadevan <gi...@git.apache.org> on 2018/08/11 00:43:30 UTC
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
GitHub user arunmahadevan opened a pull request:
https://github.com/apache/storm/pull/2801
STORM-3184: Mask the plaintext passwords from the logs
Introduce a `Password` config annotation and use it to mark configs that are
sensitive and mask the values while logging.
Master port of https://github.com/apache/storm/pull/2798
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/arunmahadevan/storm STORM-3184-master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/storm/pull/2801.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #2801
----
commit 9d36724e88f54ae48fd892b001eeb76313adc9da
Author: Arun Mahadevan <ar...@...>
Date: 2018-08-11T00:39:49Z
STORM-3184: Mask the plaintext passwords from the logs
Introduce a `Password` config annotation and use it to mark configs that are
sensitive and mask the values while logging.
----
---
[GitHub] storm issue #2801: STORM-3184: Mask the plaintext passwords from the logs
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on the issue:
https://github.com/apache/storm/pull/2801
@arunmahadevan Looks like we don't need to introduce Guava for master branch. I understand this is annoying to apply Java 7 / Java 8 for each branch, but let's keep this approach until we ship Storm 2.0.0.
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209424838
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+ public static Map<String, Object> maskPasswords(final Map<String, Object> conf) {
+ Maps.EntryTransformer<String, Object, Object> maskPasswords =
--- End diff --
Could we replace this with Java streams API and remove Guava dependency?
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209432875
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+ public static Map<String, Object> maskPasswords(final Map<String, Object> conf) {
+ Maps.EntryTransformer<String, Object, Object> maskPasswords =
--- End diff --
The problem is that the map is traversed and copied even if we don't intend to log the conf and I was trying to avoid it using the Guava transformer. It may not be a major issue now since the conf is logged only once at the beginning, but wanted to avoid any potential future misuse.
I could not figure out a way to do the same with java 8 Streams API. I have refactored it to return a wrapped object which will be evaluated only when its required to log. Please take a look and let me know if it makes sense or if theres a better way.
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209438801
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+ public static Map<String, Object> maskPasswords(final Map<String, Object> conf) {
+ Maps.EntryTransformer<String, Object, Object> maskPasswords =
--- End diff --
@arunmahadevan
I also think this approach can replace `Utils.radactValue` via adding `@password` to STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD.
What do you think? Looks like we don't need to have both approach.
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by HeartSaVioR <gi...@git.apache.org>.
Github user HeartSaVioR commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209438732
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+ public static Map<String, Object> maskPasswords(final Map<String, Object> conf) {
+ Maps.EntryTransformer<String, Object, Object> maskPasswords =
--- End diff --
@arunmahadevan
Ah OK that makes sense. I should have made clear that my concern was adding guava to the dependency without shading. You can revert the logic with using shaded guava.
https://github.com/apache/storm/blob/master/shaded-deps/pom.xml#L207-L218
Sorry about the confusion.
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/storm/pull/2801
---
[GitHub] storm pull request #2801: STORM-3184: Mask the plaintext passwords from the ...
Posted by arunmahadevan <gi...@git.apache.org>.
Github user arunmahadevan commented on a diff in the pull request:
https://github.com/apache/storm/pull/2801#discussion_r209442754
--- Diff: storm-client/src/jvm/org/apache/storm/utils/ConfigUtils.java ---
@@ -52,6 +79,16 @@ public static ConfigUtils setInstance(ConfigUtils u) {
return oldInstance;
}
+ public static Map<String, Object> maskPasswords(final Map<String, Object> conf) {
+ Maps.EntryTransformer<String, Object, Object> maskPasswords =
--- End diff --
@HeartSaVioR , thanks for the suggestion, we can replace STORM_ZOOKEEPER_TOPOLOGY_AUTH_PAYLOAD too.
I was not aware of the "shaded-deps" module and its usage.
Reverted to the earlier approach and using the shaded-deps now.
---