Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing [SNAP-2653] #1368

Merged
merged 6 commits into from
Jul 27, 2019
Merged

Fixing [SNAP-2653] #1368

merged 6 commits into from
Jul 27, 2019

Conversation

paresh-p11
Copy link
Contributor

@paresh-p11 paresh-p11 commented Jul 23, 2019

  • Mask credentials (in case of s3 URI) in Describe extended/formatted output.
  • Mask credentials in case of s3 on UI for external tables.
  • Disallow access non-admin user to the tables in SNAPPY_HIVE_METASTORE.
  • Added tests for the fixes.

Changes proposed in this pull request

(Fill in the changes here)

Patch testing

Precheckin is in progress

ReleaseNotes.txt changes

(Does this change require an entry in ReleaseNotes.txt? If yes, has it been added to it?)

Other PRs

(Does this change require changes in other projects- store, spark, spark-jobserver, aqp? Add the links of PR of the other subprojects that are related to this change)

* Mask credentials (in case of s3 URI) in Describe extended/formatted output.
* Mask credentials in case of s3 on UI for external tables.
* Disallow access non-admin user to the tables in SNAPPY_HIVE_METASTORE.
* Added tests for the fixes.
Copy link
Contributor

@sumwale sumwale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have not looked at the test code. One comment about moving the change to datasourcePath.

new SnappyExternalTableStats(table.entityName, table.tableType, table.schema,
table.shortProvider, table.externalStore, table.dataSourcePath, table.driverClass)
table.shortProvider, table.externalStore, maskedSrcPath, table.driverClass)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be better to change in StoreHiveCatalog.getDataSourcePath so that both here and sys.hivetables will mask the path. Also will handle masking passwords in sourcePath, if any. Patch to mask for all cases:

diff --git a/core/src/main/scala/io/snappydata/sql/catalog/impl/StoreHiveCatalog.scala b/core/src/main/scala/io/snappydata/sql/catalog/impl/StoreHiveCatalog.scala
index 4921e6df9..00b8214ee 100644
--- a/core/src/main/scala/io/snappydata/sql/catalog/impl/StoreHiveCatalog.scala
+++ b/core/src/main/scala/io/snappydata/sql/catalog/impl/StoreHiveCatalog.scala
@@ -444,16 +447,19 @@ class StoreHiveCatalog extends ExternalCatalog with Logging {
       }
     }

+    private def maskPassword(s: String): String =
+      SnappyExternalCatalog.PASSWORD_MATCH.replaceAllIn(s, "xxx")
+
     private def getDataSourcePath(properties: scala.collection.Map[String, String],
         storage: CatalogStorageFormat): String = {
       properties.get("path") match {
-        case Some(p) if !p.isEmpty => p
+        case Some(p) if !p.isEmpty => maskPassword(p)
         case _ => properties.get("region.path") match { // for external GemFire connector
-          case Some(p) if !p.isEmpty => p
+          case Some(p) if !p.isEmpty => maskPassword(p)
           case _ => properties.get("url") match { // jdbc
             case Some(p) if !p.isEmpty =>
               // mask the password if present
-              val url = SnappyExternalCatalog.PASSWORD_MATCH.replaceAllIn(p, "xxx")
+              val url = maskPassword(p)
               // add dbtable if present
               properties.get(SnappyExternalCatalog.DBTABLE_PROPERTY) match {
                 case Some(d) if !d.isEmpty => s"$url; ${SnappyExternalCatalog.DBTABLE_PROPERTY}=$d"

* Moving the masking logic here takes care of all the places where ExternalTableMetaData is used.
* Masking password in properties as per review comments.
Copy link
Contributor

@sumwale sumwale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See some comments

@paresh-p11 paresh-p11 merged commit a2cf856 into master Jul 27, 2019
@sumwale sumwale deleted the SNAP-2653 branch July 27, 2019 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants