Todd Lipcon has posted comments on this change. Change subject: KUDU-1844: /varz should not expose potentially sensitive configs ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/server/webserver-test.cc File src/kudu/server/webserver-test.cc: Line 139: ASSERT_STR_CONTAINS(buf_.ToString(), kudu::kRedactionMessage); can we search specifically for "--test_sensitive_flag=<redacted>" here? just to be extra clear that it's getting applied in the right place http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flag_tags-test.cc File src/kudu/util/flag_tags-test.cc: Line 27: #include "logging.h" kudu/util/logging.h Line 123: gflags::FlagSaver s; no need for FlagSaver here since this is inheriting from KuduTest, which already does flags saver for every test case. (I guess the above ones are doing it unnecessarily) Line 125: ASSERT_STR_CONTAINS(CommandlineFlagsIntoString(), kRedactionMessage); same - check for the specific flag http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flags.cc File src/kudu/util/flags.cc: Line 111: DEFINE_bool(redact_sensitive_flags, false, let's set this to default true -- I think the flags which are worth redacting are only going to be set in secure environments, and I'd rather be secure by default. PS1, Line 112: would s/would/will/ PS1, Line 113: using /varz to dump them" and when logging the configuration at daemon startup Line 322: string CheckFlagsAndRedact(const string& tag, bool shouldRedact) { what's the purpose of this extra function? seems like it's only called from CommandlineFlagsIntoString below, so may as well just put the body of it there. Line 460: args << "--" << flag.name << '=' << flag.current_value; I think we'd need to redact here as well (since the daemons print the non-default flags into the log on startup) http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/logging.h File src/kudu/util/logging.h: PS1, Line 283: // Writes all command-line flags to the log at level INFO. If --redact_sensitive_flags : // is set to true, the flags with sensitive tag, would be logged with redacted value. : void LogCommandLineFlags(); it looks like we aren't using this function anywhere, let's just remove it entirely. -- To view, visit http://gerrit.cloudera.org:8080/6043 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7661e0409c79559f22b07e7c12737e804544186b Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Hao Hao <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
