Details
Description
ReconfigurableBase will log old and new configuration values, which may cause sensitive parameters (most notably cloud storage keys, though there may be other instances) to get included in the logs.
Given the currently small list of reconfigurable properties, an argument could be made for simply not logging the property values at all, but this is not the only instance where potentially sensitive configuration gets written somewhere else in plaintext. I think a generic mechanism for redacting sensitive information for textual display will be useful to some of the web UIs too.
Attachments
Attachments
- HADOOP-13494-branch-2.7.001.patch
- 10 kB
- Sean Mackrory
- HADOOP-13494-branch-2.6.001.patch
- 10 kB
- Sean Mackrory
- HADOOP-13494.004.patch
- 10 kB
- Sean Mackrory
- HADOOP-13494.003.patch
- 10 kB
- Sean Mackrory
- HADOOP-13494.002.patch
- 7 kB
- Sean Mackrory
- HADOOP-13494.001.patch
- 7 kB
- Sean Mackrory
Issue Links
- is related to
-
HDFS-11080 Update HttpFS to use ConfigRedactor
- Resolved
Activity
Attaching a patch with a simple approach. It'd be nice to have some mechanism where modules that define config parameters (e.g. Constants.java under s3a) can say which ones are sensitive. But that seems a recipe for over-engineering to me... I think a list of patterns will be best. Any suggestions of other patterns that belong on the list would be welcome...
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 13s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 6m 33s | trunk passed |
+1 | compile | 6m 43s | trunk passed |
+1 | checkstyle | 0m 23s | trunk passed |
+1 | mvnsite | 0m 53s | trunk passed |
+1 | mvneclipse | 0m 12s | trunk passed |
+1 | findbugs | 1m 17s | trunk passed |
+1 | javadoc | 0m 44s | trunk passed |
+1 | mvninstall | 0m 37s | the patch passed |
+1 | compile | 6m 45s | the patch passed |
+1 | javac | 6m 45s | the patch passed |
-0 | checkstyle | 0m 23s | hadoop-common-project/hadoop-common: The patch generated 8 new + 4 unchanged - 1 fixed = 12 total (was 5) |
+1 | mvnsite | 0m 52s | the patch passed |
+1 | mvneclipse | 0m 13s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
-1 | findbugs | 1m 29s | hadoop-common-project/hadoop-common generated 1 new + 0 unchanged - 0 fixed = 1 total (was 0) |
+1 | javadoc | 0m 46s | the patch passed |
+1 | unit | 8m 2s | hadoop-common in the patch passed. |
+1 | asflicense | 0m 22s | The patch does not generate ASF License warnings. |
37m 51s |
Reason | Tests |
---|---|
FindBugs | module:hadoop-common-project/hadoop-common |
Write to static field org.apache.hadoop.conf.ConfigRedactor.compiledPatterns from instance method new org.apache.hadoop.conf.ConfigRedactor() At ConfigRedactor.java:from instance method new org.apache.hadoop.conf.ConfigRedactor() At ConfigRedactor.java:[line 46] |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12823535/HADOOP-13494.001.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux cfd55624f234 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / d677b68 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/10247/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt |
findbugs | https://builds.apache.org/job/PreCommit-HADOOP-Build/10247/artifact/patchprocess/new-findbugs-hadoop-common-project_hadoop-common.html |
Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/10247/testReport/ |
modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/10247/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 14s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 8m 19s | trunk passed |
+1 | compile | 8m 17s | trunk passed |
+1 | checkstyle | 0m 26s | trunk passed |
+1 | mvnsite | 0m 53s | trunk passed |
+1 | mvneclipse | 0m 13s | trunk passed |
+1 | findbugs | 1m 22s | trunk passed |
+1 | javadoc | 0m 46s | trunk passed |
+1 | mvninstall | 0m 40s | the patch passed |
+1 | compile | 6m 58s | the patch passed |
+1 | javac | 6m 58s | the patch passed |
-0 | checkstyle | 0m 24s | hadoop-common-project/hadoop-common: The patch generated 3 new + 4 unchanged - 1 fixed = 7 total (was 5) |
+1 | mvnsite | 0m 52s | the patch passed |
+1 | mvneclipse | 0m 12s | the patch passed |
+1 | whitespace | 0m 1s | The patch has no whitespace issues. |
+1 | findbugs | 1m 34s | the patch passed |
+1 | javadoc | 0m 49s | the patch passed |
+1 | unit | 8m 15s | hadoop-common in the patch passed. |
+1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
41m 58s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12823716/HADOOP-13494.002.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle |
uname | Linux adaec842cab8 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 9f29f42 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/10249/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt |
Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/10249/testReport/ |
modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/10249/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks for working on this Sean, this is a pretty concerning bug, fix looks good overall. A few review comments:
ConfigRedactor:
- Class javadoc's first sentence should end with a period. Also needs a "<p>" tag if you want to linebreak the paragraph.
- How do you feel about making the list of regexes themselves configurable? Users can put whatever keys they want into their Configuration (which might also be sensitive), so ideally redaction also handles this case. It makes the logic in ReconfigurableBase a little more complicated though, since we'll need per-Configuration redactors.
I also did a quick grep for "password" in DFSConfigKeys and turned up a few, we should consider redacting those as well.
Thanks for the review.
Fixed Javadoc and a couple of other checkstyle issues. Also made the list configurable, simplified some of the defaults to improve readability without practically expanding what would get matched, and added 'password' to the list (to catch the ssl.*password properties you saw in DFSConfigKeys.java)
It makes the logic in ReconfigurableBase a little more complicated though, since we'll need per-Configuration redactors.
I'm not entirely sure I caught your meaning here. I had the list just get parsed from the property, with commas to separate expressions, and then used the list as before. Did you have something else in mind?
+1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 11s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 6m 52s | trunk passed |
+1 | compile | 6m 45s | trunk passed |
+1 | checkstyle | 0m 24s | trunk passed |
+1 | mvnsite | 0m 54s | trunk passed |
+1 | mvneclipse | 0m 12s | trunk passed |
+1 | findbugs | 1m 20s | trunk passed |
+1 | javadoc | 0m 44s | trunk passed |
+1 | mvninstall | 0m 37s | the patch passed |
+1 | compile | 6m 47s | the patch passed |
+1 | javac | 6m 47s | the patch passed |
-0 | checkstyle | 0m 24s | hadoop-common-project/hadoop-common: The patch generated 1 new + 115 unchanged - 1 fixed = 116 total (was 116) |
+1 | mvnsite | 0m 52s | the patch passed |
+1 | mvneclipse | 0m 12s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | xml | 0m 1s | The patch has no ill-formed XML file. |
+1 | findbugs | 1m 24s | the patch passed |
+1 | javadoc | 0m 44s | the patch passed |
+1 | unit | 7m 40s | hadoop-common in the patch passed. |
+1 | asflicense | 0m 21s | The patch does not generate ASF License warnings. |
37m 50s |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12823781/HADOOP-13494.003.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml |
uname | Linux 16989ba04c2c 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 864f878 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/10253/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt |
Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/10253/testReport/ |
modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/10253/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
Thanks for the rev Sean, a few more comments:
- Normally you want to pass in the Configuration to the constructor, since making a Configuration is pretty heavy-weight (all the parsing and deprecation logic), and it also makes unit testing easier.
- The tricky bit I was alluding to was how when we have both oldConf and newConf, they can have different redaction patterns configured. I think the right behavior here is to respect the redaction settings in the selfsame config. This also relates to passing in the Configuration to the constructor.
- I think "password" might also be too broad of a pattern, since it catches okay keys like "hadoop.security.credstore.java-keystore-provider.password-file".
Aahh I understand now. Passing in the configuration object now, and anchoring password to the end of the key - should be a much safer heuristic. Also in this patch, separate redactors for the old and new config. I originally thought it safer to go with the new config only, because none of the sensitive properties are currently reconfigurable. I don't really see it being a common case where you change your mind to stop redacting certain configs. If you add a property to redact, the old value still gets logged. I see your argument too though, so no strong opinions here. Thoughts?
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 16s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 8m 10s | trunk passed |
+1 | compile | 7m 34s | trunk passed |
+1 | checkstyle | 0m 24s | trunk passed |
+1 | mvnsite | 0m 53s | trunk passed |
+1 | mvneclipse | 0m 12s | trunk passed |
+1 | findbugs | 1m 16s | trunk passed |
+1 | javadoc | 0m 45s | trunk passed |
+1 | mvninstall | 0m 37s | the patch passed |
+1 | compile | 6m 59s | the patch passed |
+1 | javac | 6m 59s | the patch passed |
-0 | checkstyle | 0m 25s | hadoop-common-project/hadoop-common: The patch generated 1 new + 115 unchanged - 1 fixed = 116 total (was 116) |
+1 | mvnsite | 0m 53s | the patch passed |
+1 | mvneclipse | 0m 13s | the patch passed |
+1 | whitespace | 0m 0s | The patch has no whitespace issues. |
+1 | xml | 0m 1s | The patch has no ill-formed XML file. |
+1 | findbugs | 1m 33s | the patch passed |
+1 | javadoc | 0m 45s | the patch passed |
-1 | unit | 16m 57s | hadoop-common in the patch failed. |
+1 | asflicense | 0m 20s | The patch does not generate ASF License warnings. |
49m 39s |
Reason | Tests |
---|---|
Timed out junit tests | org.apache.hadoop.http.TestHttpServerLifecycle |
Subsystem | Report/Notes |
---|---|
Docker | Image:yetus/hadoop:9560f25 |
JIRA Patch URL | https://issues.apache.org/jira/secure/attachment/12823806/HADOOP-13494.004.patch |
JIRA Issue | |
Optional Tests | asflicense compile javac javadoc mvninstall mvnsite unit findbugs checkstyle xml |
uname | Linux 35011672dd59 3.13.0-36-lowlatency #63-Ubuntu SMP PREEMPT Wed Sep 3 21:56:12 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux |
Build tool | maven |
Personality | /testptch/hadoop/patchprocess/precommit/personality/provided.sh |
git revision | trunk / 9daa997 |
Default Java | 1.8.0_101 |
findbugs | v3.0.0 |
checkstyle | https://builds.apache.org/job/PreCommit-HADOOP-Build/10255/artifact/patchprocess/diff-checkstyle-hadoop-common-project_hadoop-common.txt |
unit | https://builds.apache.org/job/PreCommit-HADOOP-Build/10255/artifact/patchprocess/patch-unit-hadoop-common-project_hadoop-common.txt |
Test Results | https://builds.apache.org/job/PreCommit-HADOOP-Build/10255/testReport/ |
modules | C: hadoop-common-project/hadoop-common U: hadoop-common-project/hadoop-common |
Console output | https://builds.apache.org/job/PreCommit-HADOOP-Build/10255/console |
Powered by | Apache Yetus 0.4.0-SNAPSHOT http://yetus.apache.org |
This message was automatically generated.
No test is labelled as a failure, but it reports a timeout and previous successful runs show 3547 tests. 7 are missing in this run...
Results :
Tests run: 3540, Failures: 0, Errors: 0, Skipped: 141
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 16:49.541s
[INFO] Finished at: Tue Aug 16 02:17:48 UTC 2016
[INFO] Final Memory: 24M/287M
[INFO] ------------------------------------------------------------------------
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.17:test (default-test) on project hadoop-common: There was a timeout or other error in the fork -> [Help 1]
[ERROR]
+1 LGTM, one little nit is that I try to avoid wildcard imports since they can pull in unknown stuff, but not necessary to fix here. I'll check this in shortly.
I've committed this to trunk, branch-2, branch-2.8, but beyond that there are significant conflicts due in part to some missing ReconfigurableBase changes. I took a look at backporting these, but it seems hard.
Sean, do you mind preparing separate patches for branch-2.7 and branch-2.6? Thanks.
SUCCESS: Integrated in Jenkins build Hadoop-trunk-Commit #10286 (See https://builds.apache.org/job/Hadoop-trunk-Commit/10286/)
HADOOP-13494. ReconfigurableBase can log sensitive information. (wang: rev 4b689e7a758a55cec2ca8398727feefc8ac21bfd)
- (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/CommonConfigurationKeysPublic.java
- (add) hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfigRedactor.java
- (edit) hadoop-common-project/hadoop-common/src/main/resources/core-default.xml
- (edit) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/ReconfigurableBase.java
- (add) hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/ConfigRedactor.java
-1 overall |
Vote | Subsystem | Runtime | Comment |
---|---|---|---|
0 | reexec | 0m 20s | Docker mode activated. |
+1 | @author | 0m 0s | The patch does not contain any @author tags. |
+1 | test4tests | 0m 0s | The patch appears to include 1 new or modified test files. |
+1 | mvninstall | 8m 26s | branch-2.7 passed |
+1 | compile | 5m 25s | branch-2.7 passed with JDK v1.8.0_101 |
+1 | compile | 6m 11s | branch-2.7 passed with JDK v1.7.0_101 |
+1 | checkstyle | 0m 26s | branch-2.7 passed |
+1 | mvnsite | 0m 52s | branch-2.7 passed |
+1 | mvneclipse | 0m 18s | branch-2.7 passed |
-1 | findbugs | 1m 44s | hadoop-common-project/hadoop-common in branch-2.7 has 3 extant Findbugs warnings. |
+1 | javadoc | 0m 48s | branch-2.7 passed with JDK v1.8.0_101 |
+1 | javadoc | 0m 56s | branch-2.7 passed with JDK v1.7.0_101 |
+1 | mvninstall | 0m 40s | the patch passed |
+1 | compile | 6m 26s | the patch passed with JDK v1.8.0_101 |
+1 | javac | 6m 26s | the patch passed |
+1 | compile | 6m 37s | the patch passed with JDK v1.7.0_101 |
+1 | javac | 6m 37s | the patch passed |
-0 | checkstyle | 0m 25s | hadoop-common-project/hadoop-common: The patch generated 3 new + 120 unchanged - 1 fixed = 123 total (was 121) |
+1 | mvnsite | 0m 48s | the patch passed |
+1 | mvneclipse | 0m 14s | the patch passed |
-1 | whitespace | 0m 0s | The patch has 3449 line(s) that end in whitespace. Use git apply --whitespace=fix <<patch_file>>. Refer https://git-scm.com/docs/git-apply |
-1 | whitespace | 1m 25s | The patch 90 line(s) with tabs. |
+1 | xml | 0m 1s | The patch has no ill-formed XML file. |
+1 | findbugs | 1m 54s | the patch passed |
+1 | javadoc | 0m 47s | the patch passed with JDK v1.8.0_101 |
+1 | javadoc | 0m 58s | the patch passed with JDK v1.7.0_101 |
-1 | unit | 21m 1s | hadoop-common in the patch failed with JDK v1.7.0_101. |
+1 | asflicense | 0m 24s | The patch does not generate ASF License warnings. |
88m 28s |
Reason | Tests |
---|---|
JDK v1.8.0_101 Failed junit tests | hadoop.util.bloom.TestBloomFilters |
JDK v1.8.0_101 Timed out junit tests | org.apache.hadoop.conf.TestConfiguration |
JDK v1.7.0_101 Failed junit tests | hadoop.ipc.TestDecayRpcScheduler |
hadoop.ha.TestZKFailoverController | |
hadoop.util.bloom.TestBloomFilters | |
hadoop.ipc.TestRPC | |
JDK v1.7.0_101 Timed out junit tests | org.apache.hadoop.conf.TestConfiguration |
This message was automatically generated.
Confused by these results - the patches contains no tab characters and have only 2 lines that end in whitespace. The code compiled and TestConfigRedactor passed - other test failures appear completely unrelated, as are the findbugs issues.
Should this be in the Hadoop Common tracker so that the solution can be leveraged by both HDFS and YARN?