[SPARK-27748][SS] Kafka consumer/producer password/token redaction. #24627

gaborgsomogyi · 2019-05-16T15:01:11Z

What changes were proposed in this pull request?

Kafka parameters are logged at several places and the following parameters has to be redacted:

Delegation token
ssl.truststore.password
ssl.keystore.password
ssl.key.password

This PR contains:

Spark central redaction framework used to redact passwords (spark.redaction.regex)
Custom redaction added to handle sasl.jaas.config (delegation token)
Redaction code added into consumer/producer code
Test refactor

How was this patch tested?

Existing + additional unit tests.

gaborgsomogyi · 2019-05-16T15:03:36Z

...rnal/kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaTokenUtil.scala

      | useTicketCache=true
      | serviceName="${clusterConf.kerberosServiceName}";
-      """.stripMargin.replace("\n", "")
+      """.stripMargin.replace("\n", "").trim


This is unrelated fix. There were additional spaces at the end of the generated string. This change removes it. Worth to mention the code works both way but thought it's just ugly.

gaborgsomogyi · 2019-05-16T15:04:28Z

...rnal/kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaTokenUtil.scala

      | password="$password";
-      """.stripMargin.replace("\n", "")
-    logDebug(s"Scram JAAS params: ${params.replaceAll("password=\".*\"", "password=\"[hidden]\"")}")
+      """.stripMargin.replace("\n", "").trim


This is unrelated fix. There were additional spaces at the end of the generated string. This change removes it. Worth to mention the code works both way but thought it's just ugly.

SparkQA · 2019-05-16T17:11:04Z

Test build #105457 has finished for PR 24627 at commit 7e445f7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-05-18T17:28:54Z

Retest this please.

SparkQA · 2019-05-18T18:45:06Z

Test build #105517 has finished for PR 24627 at commit 7e445f7.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-05-18T20:32:53Z

Retest this please.

SparkQA · 2019-05-18T22:39:55Z

Test build #105518 has finished for PR 24627 at commit 7e445f7.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

dongjoon-hyun · 2019-05-19T05:05:30Z

.../kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaRedactionUtil.scala

+    val redactionPattern = SparkEnv.get.conf.get(SECRET_REDACTION_PATTERN)
+    params.map { case (key, value) =>
+      if (key.equalsIgnoreCase(SaslConfigs.SASL_JAAS_CONFIG)) {
+        (key, redactJaasParam(value.asInstanceOf[String]))


Hi, @gaborgsomogyi .
Is this the only reason why we cannot use Utils.redact directly?

@dongjoon-hyun
Not sure what you mean only reason. The short answer is yes.

A little but more detailed SaslConfigs.SASL_JAAS_CONFIG has different format than any other property. A normal property looks like the following:
Key=ssl.truststore.password, Value=secret.
SaslConfigs.SASL_JAAS_CONFIG however have the following syntax:
Key=sasl.jaas.config, Value=org.apache.kafka.common.security.scram.ScramLoginModule required tokenauth=true serviceName="kafka" username="admin" password="admin-secret";
Utils.redact makes a malformed and unreadable string out of it.

HeartSaVioR · 2019-05-21T07:09:20Z

I know this patch just apply redaction to all the places which print configuration as of now, but I'm feeling that Kafka module is too verbose on configuration, especially KafkaConfigUpdater. Assuming debug log level is turned on, you would majorly want to track which consumers are being acquired or evicted, or closed. Logging in KafkaConfigUpdater doesn't help either.

Let's revisit the purpose of logging. I guess we are logging to find when consumer/producer is created, and when it's evicted/closed. (either tracking each instance, or counting for each key). Though we use whole configuration as a key of cache, we (as human) cannot check every key-value pairs to find same instance. Some pairs play as a differentiator and we (again, as human) just assume other pairs are not different.

So IMHO we don't even need to print out all configurations. Especially, if we really want to track each instance, explicit ID (as #19096 introduces) would work better and much helpful to debug.

gaborgsomogyi · 2019-05-21T08:14:29Z

So IMHO we don't even need to print out all configurations.

Without printing out all the configurations I'm wondering how do you think one can find out why a consumer/producer creation fail?

explicit ID (as #19096 introduces) would work better and much helpful to debug.

I agree, introducing an ID would be enough but only when consumer/producer already created. Such case not necessary to log all configurations. This is the next PR in my queue but didn't want to mix up the 2 changes.

Kafka module is too verbose on configuration, especially KafkaConfigUpdater.

Yeah, this can be discussed.

HeartSaVioR · 2019-05-21T08:28:15Z

So IMHO we don't even need to print out all configurations.

Without printing out all the configurations I'm wondering how do you think one can find out why a consumer/producer creation fail?

Maybe I was not clear, more clearly I don't think we should print out configurations all the time. Does Kafka module support configuration change in runtime? If not a data source needs to print out these configuration only once, instead of for every creation/termination of instances.

gaborgsomogyi · 2019-05-21T08:41:58Z

Maybe I was not clear, more clearly I don't think we should print out configurations all the time.

Same understanding. So this means redaction is needed.

Does Kafka module support configuration change in runtime?

I don't think so but I'll sit together with Kafka guys, have a deeper understanding and based on that we can go forward. That said my intention is to reduce log size drastically and introducing ID tracking but I suggest to do it in an additional PR to keep the focus.

HeartSaVioR · 2019-05-21T08:45:56Z

I meant Kafka module is spark-sql-kafka, as we are concerning about config in spark-sql-kafka. Does we assume the config can be changed after query is being run? I have been simply guessing we don't support it, but the risk of supporting it may be just cache miss, so I'm not sure it is unsupported.

gaborgsomogyi · 2019-05-21T08:52:21Z

From Spark side we're not changing the configuration otherwise caching doesn't make sense.

HeartSaVioR · 2019-05-22T09:57:54Z

As this patch fixes the issue the current code has, IMHO we can go with this patch, and apply the new idea of improvements in another issues/PRs. I can help dealing with applying new idea as well.

gaborgsomogyi · 2019-05-22T10:02:19Z

@dongjoon-hyun do you have any other comments?

gaborgsomogyi · 2019-05-24T09:12:44Z

not much movement, maybe @vanzin ?

vanzin

These logs do seem a bit verbose, but since they're already there...

external/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/CachedKafkaProducer.scala

.../kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaRedactionUtil.scala

SparkQA · 2019-05-30T19:55:58Z

Test build #105974 has finished for PR 24627 at commit c591632.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

.../kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaRedactionUtil.scala

SparkQA · 2019-06-03T10:34:03Z

Test build #106101 has finished for PR 24627 at commit 67aa337.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2019-06-03T22:42:47Z

merging to master.

[SPARK-27748][SS] Kafka consumer/producer password/token redaction.

7e445f7

gaborgsomogyi commented May 16, 2019

View reviewed changes

dongjoon-hyun reviewed May 19, 2019

View reviewed changes

vanzin reviewed May 29, 2019

View reviewed changes

external/kafka-0-10-sql/src/main/scala/org/apache/spark/sql/kafka010/CachedKafkaProducer.scala Outdated Show resolved Hide resolved

.../kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaRedactionUtil.scala Outdated Show resolved Hide resolved

Fix

c591632

vanzin reviewed May 30, 2019

View reviewed changes

.../kafka-0-10-token-provider/src/main/scala/org/apache/spark/kafka010/KafkaRedactionUtil.scala Outdated Show resolved Hide resolved

Redact non String params as well + added size assertion

67aa337

vanzin closed this in 911fadf Jun 3, 2019

HeartSaVioR mentioned this pull request Sep 26, 2019

[SPARK-27748][SS][FOLLOWUP] Correct the order of logging token as debug log #25935

Closed

[SPARK-27748][SS] Kafka consumer/producer password/token redaction. #24627

[SPARK-27748][SS] Kafka consumer/producer password/token redaction. #24627

Uh oh!

Conversation

gaborgsomogyi commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

gaborgsomogyi May 16, 2019

Choose a reason for hiding this comment

Uh oh!

gaborgsomogyi May 16, 2019

Choose a reason for hiding this comment

Uh oh!

SparkQA commented May 16, 2019

Uh oh!

dongjoon-hyun commented May 18, 2019

Uh oh!

SparkQA commented May 18, 2019

Uh oh!

dongjoon-hyun commented May 18, 2019

Uh oh!

SparkQA commented May 18, 2019

Uh oh!

dongjoon-hyun May 19, 2019

Choose a reason for hiding this comment

Uh oh!

gaborgsomogyi May 20, 2019

Choose a reason for hiding this comment

Uh oh!

HeartSaVioR commented May 21, 2019

Uh oh!

gaborgsomogyi commented May 21, 2019

Uh oh!

HeartSaVioR commented May 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gaborgsomogyi commented May 21, 2019

Uh oh!

HeartSaVioR commented May 21, 2019

Uh oh!

gaborgsomogyi commented May 21, 2019

Uh oh!

HeartSaVioR commented May 22, 2019

Uh oh!

gaborgsomogyi commented May 22, 2019

Uh oh!

gaborgsomogyi commented May 24, 2019

Uh oh!

vanzin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

SparkQA commented May 30, 2019

Uh oh!

Uh oh!

SparkQA commented Jun 3, 2019

Uh oh!

vanzin commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gaborgsomogyi commented May 16, 2019 •

edited

Loading

HeartSaVioR commented May 21, 2019 •

edited

Loading