[GLUTEN-6368] Redact sensitive configs when calling `gluten::printConfig` #6793

ArnavBalyan · 2024-08-12T19:28:30Z

What changes were proposed in this pull request?

Currently Spark supports spark.redaction.regex which allows redacting sensitive info like passwords, tokens etc.
However gluten does not enforce this and logs in plaintext when debug mode is enabled.
Added support to redact in printconfig which is being used in the codebase to log the config at various places.
User can provide a regex which will be matched and a redacted string will be visitble.

Fixes: #6368

How was this patch tested?

Local Spark shell
Before:

After: (Additional regex was supplied to match spark.gluten.ugi.tokens)

github-actions · 2024-08-12T19:28:49Z

#6368

github-actions · 2024-08-12T19:29:03Z

Run Gluten Clickhouse CI

github-actions · 2024-08-12T19:40:38Z

Run Gluten Clickhouse CI

PHILO-HE

@ArnavBalyan, thanks for your work! One comment is posted.
@xushichangdesmond, could you take a review?

PHILO-HE · 2024-08-13T02:30:33Z

shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala

@@ -673,7 +674,8 @@ object GlutenConfig {
      // gcs config
      SPARK_GCS_STORAGE_ROOT_URL,
      SPARK_GCS_AUTH_TYPE,
-      SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE
+      SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE,
+      GLUTEN_REGEX_LOG_REDACTION


@ArnavBalyan, I think we should just use the existing redaction config key of Spark. Thus, this Spark config will be respected in Gluten.

See the below example to pass Spark's session timezone:
https://github.com/apache/incubator-gluten/pull/6793/files#diff-21453c657b1ccfba45172821a434e5e7cdf007a49b440155514e6641b837d77cL651

Thanks, I was trying to do this, then realized some compatibility problems with C++ regex library vs Scala matching library used by Spark.
For example: Spark typically uses something like (?i)secret\|password\|token\|access[.]key, but this fails in C++ regex library because (?i) is not supported and users need to represent the regex in a different manner. To prevent such conflicts, for now I had created a different config specific to gluten. I agree, this is not the most elegant way, can switch to single config as well, let me know thanks!

@ArnavBalyan, can we use boost::regex instead?

Yes we can use boost, there are still be parity differences, but these would be less common. Also just noticed the regex redact property is present in spark core config, it seems currently the SQL configs flow in, we might have to wire up the core configs to flow into gluten.

@ArnavBalyan, yes, we cannot guarantee the compatibility for all regex pattern. But at least, this default regex should be supported in Gluten.

yes theres differences in the c++ regex library, we could probably use a fallback to something sensible as a compromise.

Thanks for the review @PHILO-HE @xushichangdesmond, I'll wire up the spark core config and use that by default, maybe we can improve this to fallback to gluten specific configs if needed in the future

For example: Spark typically uses something like (?i)secret|password|token|access[.]key, but this fails in C++ regex library because (?i) is not supported and users need to represent the regex in a different manner.

could we overwrite Spark's default pattern with a valid regex patten in native?

There are alot of incompatibilities, it's just one of the things where c++ and java don't have complete parity, I would rather, we provide limited support for the same config, or allow users to provide regex supported by c++ using a different properties. Option 1 is better for now and later we can add support

jinchengchenghh · 2024-08-13T03:35:26Z

cpp/core/config/GlutenConfig.cc

 #include "compute/ProtobufUtils.h"
 #include "config.pb.h"
 #include "jni/JniError.h"

 namespace gluten {
+
+const std::string REDACTED_VALUE = "*********(redacted)";
+const std::string REGEX_REDACT_KEY = "spark.gluten.redaction.regex";


Duplicated https://github.com/apache/incubator-gluten/pull/6793/files#diff-738908e381e317bdb343354905fd64c5af6c4a58e035376d5dbd2df5313b4a06R67

jinchengchenghh · 2024-08-13T03:39:06Z

cpp/core/config/GlutenConfig.cc

@@ -34,12 +39,28 @@ parseConfMap(JNIEnv* env, const uint8_t* planData, const int32_t planDataLength)
  return sparkConfs;
 }

+std::optional<std::regex> getRedactionRegex(const std::unordered_map<std::string, std::string>& conf) {
+  auto it = conf.find(REGEX_REDACT_KEY);


Please move this one to anonymous namespace.

Done thanks!

jinchengchenghh · 2024-08-13T03:40:04Z

Thanks. Just several nits.

github-actions · 2024-08-13T10:11:56Z

Run Gluten Clickhouse CI

github-actions · 2024-08-13T10:19:04Z

Run Gluten Clickhouse CI

xushichangdesmond · 2024-08-15T08:22:58Z

thanks for this; think it looks good though I dont have approval rights for this repo.

ArnavBalyan · 2024-08-15T08:30:35Z

thanks for this; think it looks good though I dont have approval rights for this repo.

Ack thanks, I will incorporate @PHILO-HE's feedback and use the gluten specific configs, should not be much effort once the Spark core-configs can be exposed to the GlutenConfig, I'll update this shortly thanks!

Yohahaha

thank you, left some comments.

Yohahaha · 2024-08-15T10:45:24Z

cpp/core/config/GlutenConfig.cc

 #include "compute/ProtobufUtils.h"
 #include "config.pb.h"
 #include "jni/JniError.h"

+namespace {
+
+const std::string REGEX_REDACT_KEY = "spark.gluten.redaction.regex";


move config key definition to GlutenConfig.h

and follow others naming.

This will be changed, we will no longer rely on this config

All configs from GlutenConfig.scala should mapping to GlutenConfig.h/VeloxConfig.h, let's keep config consistent.

Yohahaha · 2024-08-15T10:46:17Z

cpp/core/config/GlutenConfig.cc

 namespace gluten {
+
+const std::string REDACTED_VALUE = "*********(redacted)";


Yohahaha · 2024-08-15T10:51:03Z

shims/common/src/main/scala/org/apache/gluten/GlutenConfig.scala

@@ -673,7 +674,8 @@ object GlutenConfig {
      // gcs config
      SPARK_GCS_STORAGE_ROOT_URL,
      SPARK_GCS_AUTH_TYPE,
-      SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE
+      SPARK_GCS_AUTH_SERVICE_ACCOUNT_JSON_KEYFILE,
+      GLUTEN_REGEX_LOG_REDACTION


For example: Spark typically uses something like (?i)secret|password|token|access[.]key, but this fails in C++ regex library because (?i) is not supported and users need to represent the regex in a different manner.

could we overwrite Spark's default pattern with a valid regex patten in native?

ArnavBalyan · 2024-08-18T08:21:02Z

cc @PHILO-HE, could you please review, wired up to existing spark confs thanks!

PHILO-HE · 2024-08-19T00:48:58Z

@ArnavBalyan, thanks for your updating! Did you verify that Spark's this config property takes effect in Gluten in a test?
BTW, let's use boost regex, as mentioned before. The boost lib has been included in some cmake files. So I guess you just need to add its header and create boost regex parser in the code.

Yohahaha

thank you!

ArnavBalyan · 2024-08-19T02:39:11Z

@ArnavBalyan, thanks for your updating! Did you verify that Spark's this config property takes effect in Gluten in a test? BTW, let's use boost regex, as mentioned before. The boost lib has been included in some cmake files. So I guess you just need to add its header and create boost regex parser in the code.

Yes this was tested, have switched to regex boost, we are able to see redaction with more flexible regex expressions thanks!

zhztheplayer · 2024-08-19T02:44:20Z

Yes this was tested, have switched to regex boost, we are able to see redaction with more flexible regex expressions thanks!

That sounds great. And does that mean we can use the same config key with vanilla Spark ?

ArnavBalyan · 2024-08-19T02:46:30Z

Yes this was tested, have switched to regex boost, we are able to see redaction with more flexible regex expressions thanks!

That sounds great. And does that mean we can use the same config key with vanilla Spark ?

Yes, the same config key (that vanilla spark supports) will now also work with Gluten, no additional changes needed for Gluten thanks

PHILO-HE

Looks good!

…fig` (apache#6793)

redact sensitive confs

961c922

github-actions bot added CORE works for Gluten Core VELOX labels Aug 12, 2024

format

2bad963

PHILO-HE reviewed Aug 13, 2024

View reviewed changes

jinchengchenghh reviewed Aug 13, 2024

View reviewed changes

comments

e938ff7

format

497d6dc

Yohahaha reviewed Aug 15, 2024

View reviewed changes

address comments

37fc994

Yohahaha previously approved these changes Aug 19, 2024

View reviewed changes

zhztheplayer changed the title ~~[GLUTEN-6368] Redact sensitive configs during logging~~ [GLUTEN-6368] Redact sensitive configs when calling gluten::printConfig Aug 19, 2024

zhztheplayer added the security label Aug 19, 2024

boost regex

85e1818

ArnavBalyan dismissed Yohahaha’s stale review via 85e1818 August 19, 2024 02:37

clang-format

2dd1db8

PHILO-HE approved these changes Aug 19, 2024

View reviewed changes

Yohahaha merged commit 5f7bb92 into apache:main Aug 19, 2024
44 checks passed

sharkdtu pushed a commit to sharkdtu/gluten that referenced this pull request Nov 11, 2024

[GLUTEN-6368] Redact sensitive configs when calling `gluten::printCon…

14df7e6

…fig` (apache#6793)

		namespace gluten {

		const std::string REDACTED_VALUE = "*********(redacted)";

[GLUTEN-6368] Redact sensitive configs when calling gluten::printConfig #6793

[GLUTEN-6368] Redact sensitive configs when calling gluten::printConfig #6793

Conversation

ArnavBalyan commented Aug 12, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

github-actions bot commented Aug 12, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

PHILO-HE Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PHILO-HE Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jinchengchenghh commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

github-actions bot commented Aug 13, 2024

xushichangdesmond commented Aug 15, 2024

ArnavBalyan commented Aug 15, 2024 • edited Loading

Yohahaha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ArnavBalyan commented Aug 18, 2024

PHILO-HE commented Aug 19, 2024

Yohahaha left a comment

Choose a reason for hiding this comment

ArnavBalyan commented Aug 19, 2024 • edited Loading

zhztheplayer commented Aug 19, 2024

ArnavBalyan commented Aug 19, 2024

PHILO-HE left a comment

Choose a reason for hiding this comment

[GLUTEN-6368] Redact sensitive configs when calling `gluten::printConfig` #6793

[GLUTEN-6368] Redact sensitive configs when calling `gluten::printConfig` #6793

ArnavBalyan commented Aug 12, 2024 •

edited

Loading

PHILO-HE Aug 13, 2024 •

edited

Loading

PHILO-HE Aug 13, 2024 •

edited

Loading

ArnavBalyan commented Aug 15, 2024 •

edited

Loading

ArnavBalyan commented Aug 19, 2024 •

edited

Loading