[SPARK-17783] [SQL] Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC #15358

gatorsmile · 2016-10-05T04:51:35Z

What changes were proposed in this pull request?

We should never expose the Credentials in the EXPLAIN and DESC FORMATTED/EXTENDED command. However, below commands exposed the credentials.

In the related PR: #10452

URL patterns to specify credential seems to be vary between different databases.

Thus, we hide the whole url value if it contains the keyword password. We also hide the password property.

Before the fix, the command outputs look like:

CREATE TABLE tab1
USING org.apache.spark.sql.jdbc
OPTIONS (
 url 'jdbc:h2:mem:testdb0;user=testUser;password=testPass',
 dbtable 'TEST.PEOPLE',
 user 'testUser',
 password '$password')

DESC FORMATTED tab1
DESC EXTENDED tab1

Before the fix,

The output of SQL statement EXPLAIN

== Physical Plan ==
ExecutedCommand
   +- CreateDataSourceTableCommand CatalogTable(
	Table: `tab1`
	Created: Wed Nov 16 23:00:10 PST 2016
	Last Access: Wed Dec 31 15:59:59 PST 1969
	Type: MANAGED
	Provider: org.apache.spark.sql.jdbc
	Storage(Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass])), false

The output of DESC FORMATTED

...
|Storage Desc Parameters:    |                                                                  |       |
|  url                       |jdbc:h2:mem:testdb0;user=testUser;password=testPass               |       |
|  dbtable                   |TEST.PEOPLE                                                       |       |
|  user                      |testUser                                                          |       |
|  password                  |testPass                                                          |       |
+----------------------------+------------------------------------------------------------------+-------+

The output of DESC EXTENDED

|# Detailed Table Information|CatalogTable(
	Table: `default`.`tab1`
	Created: Wed Nov 16 23:00:10 PST 2016
	Last Access: Wed Dec 31 15:59:59 PST 1969
	Type: MANAGED
	Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)]
	Provider: org.apache.spark.sql.jdbc
	Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass]))|       |

After the fix,

The output of SQL statement EXPLAIN

== Physical Plan ==
ExecutedCommand
   +- CreateDataSourceTableCommand CatalogTable(
	Table: `tab1`
	Created: Wed Nov 16 22:43:49 PST 2016
	Last Access: Wed Dec 31 15:59:59 PST 1969
	Type: MANAGED
	Provider: org.apache.spark.sql.jdbc
	Storage(Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###])), false

The output of DESC FORMATTED

...
|Storage Desc Parameters:    |                                                                  |       |
|  url                       |###                                                               |       |
|  dbtable                   |TEST.PEOPLE                                                       |       |
|  user                      |testUser                                                          |       |
|  password                  |###                                                               |       |
+----------------------------+------------------------------------------------------------------+-------+

The output of DESC EXTENDED

|# Detailed Table Information|CatalogTable(
	Table: `default`.`tab1`
	Created: Wed Nov 16 22:43:49 PST 2016
	Last Access: Wed Dec 31 15:59:59 PST 1969
	Type: MANAGED
	Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)]
	Provider: org.apache.spark.sql.jdbc
	Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###]))|       |

How was this patch tested?

Added test cases

SparkQA · 2016-10-05T07:07:11Z

Test build #66370 has finished for PR 15358 at commit d3cc470.

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

rxin · 2016-10-05T07:56:48Z

Can you also put after the fix behavior in the description?

rxin · 2016-10-05T07:58:00Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala


  override def toString: String = {
+    val maskedProperties = properties.map {
+      case (password, _) if password.toLowerCase == "password" => (password, "###")


I think you should rename the variable "password" to just key. For a moment I thought that was the password. Same thing with url.

Also in the case of URL, do we really want to hide the URL completely? Can't we just hide the password?

Yeah, you are right.

At the beginning, I also thought maybe we just need to hide the password value instead of hiding the whole URL. Later, when I read another PR: #10452, I thought maybe we can hide the whole url. After rethinking it, users might need to see the other field values. Will follow your suggestions to hide password values only. Thanks!

In Oracle, the specification of password and user in url does not follow the same pattern we expect. Below is an example:

jdbc:oracle:thin:scott/tiger@localhost:1521:orcl

The reference link: http://docs.oracle.com/cd/B28359_01/java.111/b31224/urls.htm

So far, the dialect does not help. Let me know whether we should add more code changes for utilizing dialects. Thanks! @rxin

rxin · 2016-10-05T07:58:47Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

+   * for JDBC data sources.
+   */
+  def maskCredentials(options: Map[String, String]): Map[String, String] = {
+    options.map {


we should consolidate this code path with the one above. Otherwise the two will diverge over time. Perhaps add them to some utils class?

Great idea! I did not find a good place to combine both. Will add it into util class

gatorsmile · 2016-10-05T15:26:17Z

@rxin Sorry, I did not finish the PR description last night. The connection was broken in the train. Will fix it soon.

hvanhovell · 2016-11-16T14:47:31Z

@gatorsmile do you have time to get this ready for 2.1?

hvanhovell · 2016-11-16T14:49:41Z

@gatorsmile do you have time to get this ready for 2.1?

gatorsmile · 2016-11-16T17:09:19Z

Let me finish this now. Thanks!

SparkQA · 2016-11-17T06:52:32Z

Test build #68754 has started for PR 15358 at commit bc9a508.

gatorsmile · 2016-11-17T07:02:38Z

The PR description is updated.

gatorsmile · 2016-11-17T17:47:38Z

retest this please

SparkQA · 2016-11-17T20:14:31Z

Test build #68791 has finished for PR 15358 at commit bc9a508.

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

gatorsmile · 2016-11-18T03:39:42Z

cc @rxin @hvanhovell

gatorsmile · 2016-11-24T07:31:18Z

retest this please

SparkQA · 2016-11-24T07:32:34Z

Test build #69115 has started for PR 15358 at commit bc9a508.

gatorsmile · 2016-11-24T18:39:37Z

retest this please

SparkQA · 2016-11-24T21:02:45Z

Test build #69140 has finished for PR 15358 at commit bc9a508.

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

hvanhovell

A few minor things, otherwise LGTM

hvanhovell · 2016-11-24T21:39:32Z

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/catalog/interface.scala

-      }
+    val serdePropsToString = CatalogUtils.maskCredentials(properties) match {
+      case props if props.isEmpty => ""
+      case props => s"Properties: " + props.map(p => p._1 + "=" + p._2).mkString("[", ", ", "]")


NIT: string interpolation not needed.

hvanhovell · 2016-11-24T21:39:33Z

sql/core/src/main/scala/org/apache/spark/sql/execution/command/ddl.scala

      }
    }
  }
+


hvanhovell · 2016-11-24T21:43:09Z

sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/ddl.scala


+  override def argString: String = {
+    s"[tableIdent:$tableIdent " +
+      userSpecifiedSchema.getOrElse("") +


Does this need a space?

Yeah. Thanks! It needs a space:

ExecutedCommand +- CreateTempViewUsing [tableIdent:`jsonTable` StructType(StructField(a,IntegerType,true), StructField(b,StringType,true))replace:true provider:org.apache.spark.sql.json.DefaultSource Map(path -> /private/var/folders/4b/sgmfldk15js406vk7lw5llzw0000gn/T/spark-88202701-92c6-44f2-9945-7f388551729b)

SparkQA · 2016-11-26T02:05:09Z

Test build #69176 has finished for PR 15358 at commit 45e0ee3.

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

hvanhovell · 2016-11-28T15:04:18Z

LGTM. Merging to master/2.1.

…NDED a PERSISTENT/TEMP Table for JDBC ### What changes were proposed in this pull request? We should never expose the Credentials in the EXPLAIN and DESC FORMATTED/EXTENDED command. However, below commands exposed the credentials. In the related PR: #10452 > URL patterns to specify credential seems to be vary between different databases. Thus, we hide the whole `url` value if it contains the keyword `password`. We also hide the `password` property. Before the fix, the command outputs look like: ``` SQL CREATE TABLE tab1 USING org.apache.spark.sql.jdbc OPTIONS ( url 'jdbc:h2:mem:testdb0;user=testUser;password=testPass', dbtable 'TEST.PEOPLE', user 'testUser', password '$password') DESC FORMATTED tab1 DESC EXTENDED tab1 ``` Before the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |jdbc:h2:mem:testdb0;user=testUser;password=testPass | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |testPass | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass]))| | ``` After the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |### | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |### | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###]))| | ``` ### How was this patch tested? Added test cases Author: gatorsmile <gatorsmile@gmail.com> Closes #15358 from gatorsmile/maskCredentials. (cherry picked from commit 9f273c5) Signed-off-by: Herman van Hovell <hvanhovell@databricks.com>

hvanhovell · 2016-11-28T15:08:14Z

@gatorsmile I cannot merge this into branch 2.0. Could you open a PR if you feel that we should have this in 2.0?

gatorsmile · 2016-11-28T17:25:27Z

Will do it soon. Thanks!

…FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC ### What changes were proposed in this pull request? This PR is to backport #15358 to Spark 2.0. ------ We should never expose the Credentials in the EXPLAIN and DESC FORMATTED/EXTENDED command. However, below commands exposed the credentials. In the related PR: #10452 > URL patterns to specify credential seems to be vary between different databases. Thus, we hide the whole `url` value if it contains the keyword `password`. We also hide the `password` property. Before the fix, the command outputs look like: ``` SQL CREATE TABLE tab1 USING org.apache.spark.sql.jdbc OPTIONS ( url 'jdbc:h2:mem:testdb0;user=testUser;password=testPass', dbtable 'TEST.PEOPLE', user 'testUser', password '$password') DESC FORMATTED tab1 DESC EXTENDED tab1 ``` Before the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |jdbc:h2:mem:testdb0;user=testUser;password=testPass | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |testPass | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass]))| | ``` After the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |### | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |### | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###]))| | ``` ### How was this patch tested? Added test cases Author: gatorsmile <gatorsmile@gmail.com> Closes #16047 from gatorsmile/backPortSPARK-17783.

…NDED a PERSISTENT/TEMP Table for JDBC ### What changes were proposed in this pull request? We should never expose the Credentials in the EXPLAIN and DESC FORMATTED/EXTENDED command. However, below commands exposed the credentials. In the related PR: apache#10452 > URL patterns to specify credential seems to be vary between different databases. Thus, we hide the whole `url` value if it contains the keyword `password`. We also hide the `password` property. Before the fix, the command outputs look like: ``` SQL CREATE TABLE tab1 USING org.apache.spark.sql.jdbc OPTIONS ( url 'jdbc:h2:mem:testdb0;user=testUser;password=testPass', dbtable 'TEST.PEOPLE', user 'testUser', password '$password') DESC FORMATTED tab1 DESC EXTENDED tab1 ``` Before the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |jdbc:h2:mem:testdb0;user=testUser;password=testPass | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |testPass | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 23:00:10 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=jdbc:h2:mem:testdb0;user=testUser;password=testPass, dbtable=TEST.PEOPLE, user=testUser, password=testPass]))| | ``` After the fix, - The output of SQL statement EXPLAIN ``` == Physical Plan == ExecutedCommand +- CreateDataSourceTableCommand CatalogTable( Table: `tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Provider: org.apache.spark.sql.jdbc Storage(Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###])), false ``` - The output of `DESC FORMATTED` ``` ... |Storage Desc Parameters: | | | | url |### | | | dbtable |TEST.PEOPLE | | | user |testUser | | | password |### | | +----------------------------+------------------------------------------------------------------+-------+ ``` - The output of `DESC EXTENDED` ``` |# Detailed Table Information|CatalogTable( Table: `default`.`tab1` Created: Wed Nov 16 22:43:49 PST 2016 Last Access: Wed Dec 31 15:59:59 PST 1969 Type: MANAGED Schema: [StructField(NAME,StringType,false), StructField(THEID,IntegerType,false)] Provider: org.apache.spark.sql.jdbc Storage(Location: file:/Users/xiaoli/IdeaProjects/sparkDelivery/spark-warehouse/tab1, Properties: [url=###, dbtable=TEST.PEOPLE, user=testUser, password=###]))| | ``` ### How was this patch tested? Added test cases Author: gatorsmile <gatorsmile@gmail.com> Closes apache#15358 from gatorsmile/maskCredentials.

fix.

d3cc470

gatorsmile changed the title ~~Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC~~ [SPARK-17783] [SQL] Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC Oct 5, 2016

rxin reviewed Oct 5, 2016

View reviewed changes

Merge remote-tracking branch 'upstream/master' into maskCredentials

aa1de2c

gatorsmile added 2 commits November 16, 2016 14:52

Merge remote-tracking branch 'upstream/master' into maskCredentials

86e6b5c

address comments.

bc9a508

hvanhovell reviewed Nov 24, 2016

View reviewed changes

gatorsmile added 2 commits November 25, 2016 15:01

Merge remote-tracking branch 'upstream/master' into maskCredentials

08dd522

address comments.

45e0ee3

asfgit closed this in 9f273c5 Nov 28, 2016

gatorsmile mentioned this pull request Nov 28, 2016

[SPARK-17783] [SQL] [BACKPORT-2.0] Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC #16047

Closed

[SPARK-17783] [SQL] Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC #15358

[SPARK-17783] [SQL] Hide Credentials in CREATE and DESC FORMATTED/EXTENDED a PERSISTENT/TEMP Table for JDBC #15358

Uh oh!

Conversation

gatorsmile commented Oct 5, 2016 • 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!

SparkQA commented Oct 5, 2016

Uh oh!

rxin commented Oct 5, 2016

Uh oh!

rxin Oct 5, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Oct 5, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Oct 10, 2016

Choose a reason for hiding this comment

Uh oh!

rxin Oct 5, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gatorsmile commented Oct 5, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hvanhovell commented Nov 16, 2016

Uh oh!

hvanhovell commented Nov 16, 2016

Uh oh!

gatorsmile commented Nov 16, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

gatorsmile commented Nov 17, 2016

Uh oh!

gatorsmile commented Nov 17, 2016

Uh oh!

SparkQA commented Nov 17, 2016

Uh oh!

gatorsmile commented Nov 18, 2016

Uh oh!

gatorsmile commented Nov 24, 2016

Uh oh!

SparkQA commented Nov 24, 2016

Uh oh!

gatorsmile commented Nov 24, 2016

Uh oh!

SparkQA commented Nov 24, 2016

Uh oh!

hvanhovell left a comment

Choose a reason for hiding this comment

Uh oh!

hvanhovell Nov 24, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovell Nov 24, 2016

Choose a reason for hiding this comment

Uh oh!

hvanhovell Nov 24, 2016

Choose a reason for hiding this comment

Uh oh!

gatorsmile Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Nov 26, 2016

Uh oh!

hvanhovell commented Nov 28, 2016

Uh oh!

hvanhovell commented Nov 28, 2016

Uh oh!

gatorsmile commented Nov 28, 2016

Uh oh!

gatorsmile commented Oct 5, 2016 •

edited

Loading

gatorsmile Oct 5, 2016 •

edited

Loading

gatorsmile commented Oct 5, 2016 •

edited

Loading

gatorsmile Nov 25, 2016 •

edited

Loading