Hao Hao has posted comments on this change. Change subject: KUDU-1844: /varz should not expose potentially sensitive configs ......................................................................
Patch Set 1: (24 comments) http://gerrit.cloudera.org:8080/#/c/6043/1//COMMIT_MSG Commit Message: PS1, Line 12: redact > Nit: "<redacted>" right? Not "<redact>"? Done http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/server/default-path-handlers.cc File src/kudu/server/default-path-handlers.cc: Line 46: using boost::replace_all; > warning: using decl 'replace_all' is unused [misc-unused-using-decls] Not quite follow the comments? PS1, Line 115: If --redact_sensitive_flags is true for a flag with sensitive tag > Nit: the way this is worded, it sounds like --redact_sensitive_flags is som Done 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? jus Done 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 Done Line 27: #include "logging.h" > Should be kudu/util/logging.h Done Line 123: gflags::FlagSaver s; > no need for FlagSaver here since this is inheriting from KuduTest, which al Done Line 125: ASSERT_STR_CONTAINS(CommandlineFlagsIntoString(), kRedactionMessage); > same - check for the specific flag Done PS1, Line 125: ASSERT_STR_CONTAINS > Can we be more specific and assert that the output contains "--test_sensiti Done http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flag_tags.h File src/kudu/util/flag_tags.h: PS1, Line 84: These flags are considering sensitive and their value would be redacted if : // --redact_sensitive_flags is enabled. > Nit: how about "The values of these flags are considered sensitive and will Done http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flags.cc File src/kudu/util/flags.cc: Line 40: #include "kudu/util/logging.h" > Keep this list sorted alphabetically. Done Line 111: DEFINE_bool(redact_sensitive_flags, false, > Shouldn't this default to true? That's what we did for --log_redact_user_da I agree we may want to make it more general for different redaction purpose. Do you think it make sense to do in another commit? Line 111: DEFINE_bool(redact_sensitive_flags, false, > let's set this to default true -- I think the flags which are worth redacti Done PS1, Line 112: would > s/would/will/ Done PS1, Line 113: using /varz to dump them" > and when logging the configuration at daemon startup Done Line 115: TAG_FLAG(redact_sensitive_flags, stable); > I would be hesitant to introduce any new flag as "stable". Let's use "evolv Done Line 318: // Stick the flags into a string. During the process, check that > Apart from the redaction, does this produce the exact same output as google Yes, it is. Line 322: string CheckFlagsAndRedact(const string& tag, bool shouldRedact) { > what's the purpose of this extra function? seems like it's only called from Done PS1, Line 322: const string& tag, > What's the value of allowing this to be customized if it's only ever called Removed CheckFlagsAndRedact function. Line 440: return CheckFlagsAndRedact("sensitive", FLAGS_redact_sensitive_flags); > Why not embed CheckFlagsAndRedact() into this function? Since it's only bei Done Line 460: args << "--" << flag.name << '=' << flag.current_value; > I think we'd need to redact here as well (since the daemons print the non-d Done http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/flags.h File src/kudu/util/flags.h: PS1, Line 53: If any flags tagged with : // 'sensitive' and --redact_sensitive_flags is enabled, : // redact the flag > How about "If --redact_sensitive_flags is true, the values of flags tagged Done http://gerrit.cloudera.org:8080/#/c/6043/1/src/kudu/util/logging.h File src/kudu/util/logging.h: PS1, Line 284: is set to true, the flags with sensitive tag, would be logged with redacted value. > "...is set to true, the values of flags tagged as sensitive will be redacte Removed it as Todd suggested. 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 Done -- 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: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Hao Hao <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
