You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@kudu.apache.org by da...@apache.org on 2017/10/25 19:10:12 UTC
kudu git commit: KUDU-1957: Clarify web UI redaction in --redact flag
help
Repository: kudu
Updated Branches:
refs/heads/master 15f3f9b2f -> e63d2be62
KUDU-1957: Clarify web UI redaction in --redact flag help
This patch refines the definition of --redact flag to control the
context of redaction. The available options now are 'all', 'log'
and 'none'. If 'all' is specified, sensitive data (sensitive
configuration flags and row data) will be redacted from the
web UI as well as glog and error messages. If 'log' is specified,
sensitive data will only be redacted from glog and error messages.
And the option 'flag' is removed from --redact flag to let this
flag only control redaction context.
Change-Id: I32fe0c4e65f22d6f9f3d188018b119f07300e26f
Reviewed-on: http://gerrit.cloudera.org:8080/6755
Tested-by: Kudu Jenkins
Reviewed-by: Dan Burkert <da...@apache.org>
Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/e63d2be6
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/e63d2be6
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/e63d2be6
Branch: refs/heads/master
Commit: e63d2be624f46a37cbf7ceda5c2affdab0d3281c
Parents: 15f3f9b
Author: hahao <ha...@cloudera.com>
Authored: Thu Oct 19 23:46:31 2017 -0700
Committer: Dan Burkert <da...@apache.org>
Committed: Wed Oct 25 19:09:36 2017 +0000
----------------------------------------------------------------------
src/kudu/mini-cluster/external_mini_cluster.cc | 4 +--
src/kudu/server/default_path_handlers.cc | 6 ++--
src/kudu/server/webserver-test.cc | 1 +
src/kudu/server/webserver.cc | 10 +++++-
src/kudu/util/flag_tags-test.cc | 1 +
src/kudu/util/flag_tags.h | 2 +-
src/kudu/util/flags-test.cc | 1 +
src/kudu/util/flags.cc | 35 +++++++++------------
src/kudu/util/flags.h | 10 +++---
src/kudu/util/logging.cc | 3 +-
src/kudu/util/logging.h | 15 ++++++---
src/kudu/util/test_util.cc | 4 +--
12 files changed, 51 insertions(+), 41 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/mini-cluster/external_mini_cluster.cc
----------------------------------------------------------------------
diff --git a/src/kudu/mini-cluster/external_mini_cluster.cc b/src/kudu/mini-cluster/external_mini_cluster.cc
index 25ce80f..dae51a3 100644
--- a/src/kudu/mini-cluster/external_mini_cluster.cc
+++ b/src/kudu/mini-cluster/external_mini_cluster.cc
@@ -715,8 +715,8 @@ Status ExternalDaemon::StartProcess(const vector<string>& user_flags) {
// Disable minidumps by default since many tests purposely inject faults.
argv.emplace_back("--enable_minidumps=false");
- // Disable log redaction.
- argv.emplace_back("--redact=flag");
+ // Disable redaction.
+ argv.emplace_back("--redact=none");
// Enable metrics logging.
argv.emplace_back("--metrics_log_interval_ms=1000");
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/server/default_path_handlers.cc
----------------------------------------------------------------------
diff --git a/src/kudu/server/default_path_handlers.cc b/src/kudu/server/default_path_handlers.cc
index 040aaaf..a76a565 100644
--- a/src/kudu/server/default_path_handlers.cc
+++ b/src/kudu/server/default_path_handlers.cc
@@ -127,9 +127,9 @@ static void LogsHandler(const Webserver::WebRequest& req, Webserver::WebResponse
}
// Registered to handle "/flags", and prints out all command-line flags and their HTML
-// escaped values. If --redact is set with 'flag', the values of flags tagged as
-// sensitive will be redacted. The values would not be HTML escaped if in the raw text
-// mode, e.g. "/varz?raw".
+// escaped values. If --redact indicates that redaction is enabled for the web UI, the
+// values of flags tagged as sensitive will be redacted. The values would not be HTML
+// escaped if in the raw text mode, e.g. "/varz?raw".
static void FlagsHandler(const Webserver::WebRequest& req,
Webserver::PrerenderedWebResponse* resp) {
std::ostringstream* output = resp->output;
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/server/webserver-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/server/webserver-test.cc b/src/kudu/server/webserver-test.cc
index ca80822..1875a4c 100644
--- a/src/kudu/server/webserver-test.cc
+++ b/src/kudu/server/webserver-test.cc
@@ -176,6 +176,7 @@ TEST_F(WebserverTest, TestDefaultPaths) {
}
TEST_F(WebserverTest, TestRedactFlagsDump) {
+ kudu::g_should_redact = kudu::RedactContext::ALL;
// Test varz -- check for the sensitive flag is redacted and HTML-escaped.
ASSERT_OK(curl_.FetchURL(strings::Substitute("http://$0/varz", addr_.ToString()),
&buf_));
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/server/webserver.cc
----------------------------------------------------------------------
diff --git a/src/kudu/server/webserver.cc b/src/kudu/server/webserver.cc
index 4812001..f8b7cee 100644
--- a/src/kudu/server/webserver.cc
+++ b/src/kudu/server/webserver.cc
@@ -51,6 +51,7 @@
#include "kudu/util/env.h"
#include "kudu/util/flag_tags.h"
#include "kudu/util/locks.h"
+#include "kudu/util/logging.h"
#include "kudu/util/net/net_util.h"
#include "kudu/util/net/sockaddr.h"
#include "kudu/util/url-coding.h"
@@ -458,7 +459,14 @@ int Webserver::RunPathHandler(const PathHandler& handler,
ostringstream content;
PrerenderedWebResponse resp { HttpStatusCode::Ok, HttpResponseHeaders{}, &content };
- handler.callback()(req, &resp);
+ // Enable or disable redaction from the web UI based on the setting of --redact.
+ // This affects operations like default value and scan predicate pretty printing.
+ if (kudu::g_should_redact == kudu::RedactContext::ALL) {
+ handler.callback()(req, &resp);
+ } else {
+ ScopedDisableRedaction s;
+ handler.callback()(req, &resp);
+ }
string full_content;
if (use_style) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/flag_tags-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/flag_tags-test.cc b/src/kudu/util/flag_tags-test.cc
index a06a05e..4626d0d 100644
--- a/src/kudu/util/flag_tags-test.cc
+++ b/src/kudu/util/flag_tags-test.cc
@@ -126,6 +126,7 @@ TEST_F(FlagTagsTest, TestUnlockFlags) {
TEST_F(FlagTagsTest, TestSensitiveFlags) {
// Setting a sensitive flag should return a redacted value.
{
+ kudu::g_should_redact = kudu::RedactContext::LOG;
ASSERT_STR_CONTAINS(CommandlineFlagsIntoString(EscapeMode::NONE), strings::Substitute(
"--test_sensitive_flag=$0", kRedactionMessage));
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/flag_tags.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/flag_tags.h b/src/kudu/util/flag_tags.h
index ba5c870..bf0c400 100644
--- a/src/kudu/util/flag_tags.h
+++ b/src/kudu/util/flag_tags.h
@@ -82,7 +82,7 @@
//
// - "sensitive":
// The values of these flags are considered sensitive and will be redacted
-// if --redact is set with 'flag'.
+// if redaction is enabled.
//
// A given flag may have zero or more tags associated with it. The system does
// not make any attempt to check integrity of the tags - for example, it allows
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/flags-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/flags-test.cc b/src/kudu/util/flags-test.cc
index 60ebebb..9ebc178 100644
--- a/src/kudu/util/flags-test.cc
+++ b/src/kudu/util/flags-test.cc
@@ -91,6 +91,7 @@ TEST_F(FlagsTest, TestNonDefaultFlags) {
// Setting a sensitive flag with non-default value should return
// a redacted value.
FLAGS_test_sensitive_flag = true;
+ kudu::g_should_redact = kudu::RedactContext::LOG;
std::string result = GetNonDefaultFlags(default_flags);
for (const auto& expected : expected_flags) {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/flags.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/flags.cc b/src/kudu/util/flags.cc
index 957ea7a..edb232b 100644
--- a/src/kudu/util/flags.cc
+++ b/src/kudu/util/flags.cc
@@ -128,18 +128,17 @@ TAG_FLAG(unlock_unsafe_flags, advanced);
TAG_FLAG(unlock_unsafe_flags, stable);
DEFINE_string(redact, "all",
- "Comma-separated list of redactions. Supported options are 'flag', "
- "'log', 'all', and 'none'. If 'flag' is specified, configuration flags which may "
- "include sensitive data will be redacted whenever server configuration "
- "is emitted. If 'log' is specified, row data will be redacted from log "
- "and error messages. If 'all' is specified, all of above will be redacted. "
+ "Comma-separated list that controls redaction context. Supported options "
+ "are 'all','log', and 'none'. If 'all' is specified, sensitive data "
+ "(sensitive configuration flags and row data) will be redacted from "
+ "the web UI as well as glog and error messages. If 'log' is specified, "
+ "sensitive data will only be redacted from glog and error messages. "
"If 'none' is specified, no redaction will occur.");
TAG_FLAG(redact, advanced);
TAG_FLAG(redact, evolving);
static bool ValidateRedact(const char* /*flagname*/, const string& value) {
- kudu::g_should_redact_log = false;
- kudu::g_should_redact_flag = false;
+ kudu::g_should_redact = kudu::RedactContext::NONE;
// Flag value is case insensitive.
string redact_flags;
@@ -147,8 +146,7 @@ static bool ValidateRedact(const char* /*flagname*/, const string& value) {
// 'all', 'none', and '' must be specified without any other option.
if (redact_flags == "ALL") {
- kudu::g_should_redact_log = true;
- kudu::g_should_redact_flag = true;
+ kudu::g_should_redact = kudu::RedactContext::ALL;
return true;
}
if (redact_flags == "NONE" || redact_flags.empty()) {
@@ -157,16 +155,14 @@ static bool ValidateRedact(const char* /*flagname*/, const string& value) {
for (const auto& t : strings::Split(redact_flags, ",", strings::SkipEmpty())) {
if (t == "LOG") {
- kudu::g_should_redact_log = true;
- } else if (t == "FLAG") {
- kudu::g_should_redact_flag = true;
+ kudu::g_should_redact = kudu::RedactContext::LOG;
} else if (t == "ALL" || t == "NONE") {
LOG(ERROR) << "Invalid redaction options: "
<< value << ", '" << t << "' must be specified by itself.";
return false;
} else {
- LOG(ERROR) << "Invalid redaction type: " << t <<
- ". Available types are 'flag', 'log', 'all', and 'none'.";
+ LOG(ERROR) << "Invalid redaction context: " << t <<
+ ". Available types are 'all', 'log', and 'none'.";
return false;
}
}
@@ -433,15 +429,15 @@ void RunCustomValidators() {
}
}
-// Redact the flag tagged as 'sensitive', if --redact is set
-// with 'flag'. Otherwise, return its value as-is. If EscapeMode
-// is set to HTML, return HTML escaped string.
+// If --redact indicates, redact the flag tagged as 'sensitive'.
+// Otherwise, return its value as-is. If EscapeMode is set to HTML,
+// return HTML escaped string.
string CheckFlagAndRedact(const CommandLineFlagInfo& flag, EscapeMode mode) {
string ret_value;
unordered_set<string> tags;
GetFlagTags(flag.name, &tags);
- if (ContainsKey(tags, "sensitive") && g_should_redact_flag) {
+ if (ContainsKey(tags, "sensitive") && KUDU_SHOULD_REDACT()) {
ret_value = kRedactionMessage;
} else {
ret_value = flag.current_value;
@@ -544,8 +540,7 @@ string GetNonDefaultFlags(const GFlagsMap& default_flags) {
args << '\n';
}
- // Redact the flags tagged as sensitive, if --redact is set
- // with 'flag'.
+ // Redact the flags tagged as sensitive, if redaction is enabled.
string flag_value = CheckFlagAndRedact(flag, EscapeMode::NONE);
args << "--" << flag.name << '=' << flag_value;
}
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/flags.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/flags.h b/src/kudu/util/flags.h
index 0c3028a..2c5159a 100644
--- a/src/kudu/util/flags.h
+++ b/src/kudu/util/flags.h
@@ -57,17 +57,17 @@ enum class EscapeMode {
NONE
};
-// Stick the flags into a string. If --redact is set with 'flag',
-// the values of flags tagged as sensitive will be redacted. Otherwise,
-// the values will be written to the string as-is. The values will
-// be HTML escaped if EscapeMode is HTML.
+// Stick the flags into a string. If redaction is enabled, the values of
+// flags tagged as sensitive will be redacted. Otherwise, the values
+// will be written to the string as-is. The values will be HTML escaped
+// if EscapeMode is HTML.
std::string CommandlineFlagsIntoString(EscapeMode mode);
typedef std::unordered_map<std::string, google::CommandLineFlagInfo> GFlagsMap;
// Get all the flags different from their defaults. The output is a nicely
// formatted string with --flag=value pairs per line. Redact any flags that
-// are tagged as sensitive, if --redact is set with 'flag'.
+// are tagged as sensitive, if redaction is enabled.
std::string GetNonDefaultFlags(const GFlagsMap& default_flags);
GFlagsMap GetFlagsMap();
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/logging.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/logging.cc b/src/kudu/util/logging.cc
index f53ee64..02b6b95 100644
--- a/src/kudu/util/logging.cc
+++ b/src/kudu/util/logging.cc
@@ -79,8 +79,7 @@ using base::SpinLockHolder;
namespace kudu {
__thread bool tls_redact_user_data = true;
-bool g_should_redact_log;
-bool g_should_redact_flag;
+kudu::RedactContext g_should_redact;
const char* const kRedactionMessage = "<redacted>";
namespace {
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/logging.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/logging.h b/src/kudu/util/logging.h
index bbef2d8..bfc4c59 100644
--- a/src/kudu/util/logging.h
+++ b/src/kudu/util/logging.h
@@ -60,7 +60,8 @@
// Evaluates to 'true' if the caller should redact any user data in the current scope.
// Most callers should instead use KUDU_REDACT(...) defined below, but this can be useful
// to short-circuit expensive logic.
-#define KUDU_SHOULD_REDACT() (kudu::g_should_redact_log && kudu::tls_redact_user_data)
+#define KUDU_SHOULD_REDACT() ((kudu::g_should_redact == kudu::RedactContext::ALL || \
+ kudu::g_should_redact == kudu::RedactContext::LOG) && kudu::tls_redact_user_data)
// Either evaluate and return 'expr', or return the string "<redacted>", depending on whether
// redaction is enabled in the current scope.
@@ -87,11 +88,14 @@ extern __thread bool tls_redact_user_data;
// Redacted log messages are replaced with this constant.
extern const char* const kRedactionMessage;
-// Flag for checking if log redaction is enabled or disabled.
-extern bool g_should_redact_log;
+enum class RedactContext {
+ ALL,
+ LOG,
+ NONE
+};
-// Flag for checking if flag redaction is enabled or disabled.
-extern bool g_should_redact_flag;
+// Flag to indicate which redaction context is enabled.
+extern kudu::RedactContext g_should_redact;
class ScopedDisableRedaction {
public:
@@ -103,6 +107,7 @@ class ScopedDisableRedaction {
~ScopedDisableRedaction() {
tls_redact_user_data = old_val_;
}
+
private:
bool old_val_;
};
http://git-wip-us.apache.org/repos/asf/kudu/blob/e63d2be6/src/kudu/util/test_util.cc
----------------------------------------------------------------------
diff --git a/src/kudu/util/test_util.cc b/src/kudu/util/test_util.cc
index b7d4c57..51fe5a1 100644
--- a/src/kudu/util/test_util.cc
+++ b/src/kudu/util/test_util.cc
@@ -83,8 +83,8 @@ KuduTest::KuduTest()
// Disabling fsync() speeds up tests dramatically, and it's safe to do as no
// tests rely on cutting power to a machine or equivalent.
{"never_fsync", "true"},
- // Disable log redaction.
- {"redact", "flag"},
+ // Disable redaction.
+ {"redact", "none"},
// Reduce default RSA key length for faster tests. We are using strong/high
// TLS v1.2 cipher suites, so minimum possible for TLS-related RSA keys is
// 768 bits. However, for the external mini cluster we use 1024 bits because