HIVE-28838: Remove sensitive jdbc properties from explain plan for JdbcStorageHandler tables

[InvisibleProgrammer]

It removes the following sensitive properties from tables created with JdbcStorageHandler:

• hive.sql.dbcp.username
• hive.sql.dbcp.password
• hive.sql.jdbc.url

Why are the changes needed?

Explain extended can cause a security issue if they query sys tables: Sys tables are created with JdbcStorageHandler and so that, the output of explain extended will contain sensitive jdbc connection info.

Does this PR introduce any user-facing change?

Yes, it removes the listed properties from explain extended output when a JdbcStorageHandler table is included in a query.

Is the change a dependency upgrade?

No

How was this patch tested?

QTest added: explain_systest_password.q

[zhangbutao]

I think removing password is enough.

[InvisibleProgrammer]

I would disagree with that. I excluded the username, password, and the JDBC URL.
With the username, we can share some extra information about who is the admin.
And about the JDBC URL, it provides the proper path to the metastore database.

[zratkai]

I agree with you to remove all three.

[zhangbutao]

When users query MySQL tables through Hive jdbc storage handler, users sometimes need to know which MySQL database the table comes from through JDBC URL. Users may execute show create table mysql_jdbc_tbl to check the JDBC URL.

Therefore, I think it is necessary to retain the JDBC URL & username.

[deniskuzZ]

describe formatted / show create table also displays the password in plain text. I don't think it's covered here.

[deniskuzZ]

Therefore, I think it is necessary to retain the JDBC URL & username.

agree, at least it should be available to the table owners/admins

PS: SYS schema is accessible only to admins, while the INFORMATION_SCHEMA is available to end users.

[zhangbutao]

describe formatted / show create table also displays the password in plain text. I don't think it's covered here.

Yes, This PR mainly focuses on removing sensitive information in explain. However, it does not handle the show create table for jdbc storage handler tables. Currently, sensitive information can still be seen in show create table.

For example, if you create a mysql jdbc handler table in Hive, and then show create table to check its TBLPROPERTIES, you will still can see its password "hive.sql.dbcp.password" = "passwd".

CREATE EXTERNAL TABLE jdbc_mysql_table
(
 id int
)
STORED BY 'org.apache.hive.storage.jdbc.JdbcStorageHandler'
TBLPROPERTIES (
                "hive.sql.database.type" = "MYSQL",
                "hive.sql.jdbc.driver" = "com.mysql.jdbc.Driver",
                "hive.sql.jdbc.url" = "jdbc:mysql://127.0.0.1:3306/testdb",
                "hive.sql.dbcp.username" = "root",
                "hive.sql.dbcp.password" = "passwd",
                "hive.sql.table" = "testmysql"
);

show create table is the main use case that I am focusing on. BTW, we will check which MySQL database this table belongs to through the JDBC URL (hive.sql.jdbc.url). This is also the reason why I think the JDBC URL should be retained.

[InvisibleProgrammer]

Hi,

This ticket came up because of an interesting behavior of explain extended in case of sys tables:
System tables are special kind of JdbcStorageHandler tables: they are created with JdbcStorageHandler but they are not containing a jdbc connection info. Instead of that, they are using the existing connection info provided in HMS configuration.

The interesting thing is that explain extended is a query. And for queries, Hive adds the jdbc connection info to the task (I assume because it is required to be able to run the query).
The questionable behavior is that at sys tables, why we share the JDO connection info provided in HMS config?

This PR addresses this issue. It is not about show create table or describe formatted at all. And, for regular JdbcStorageHandler tables, I would expect to see that information in show create table - as table is created with providing this info. And this is not the HMS metastore db connection info at all.

So, I'm open to have a discussion or a follow up ticket to rethink what kind of information we want to share and for what kind of users. But with this PR, I want to address the case when a simple explain statement can share the HMS database connection info.

[InvisibleProgrammer]

I modified the qtest file to demonstrate that the problem doesn't exists for show create table or describe table - as it expected as the connection info is not part of the sys table creation.

[deniskuzZ]

SYS tables could be queried only by privilege users/admins, so I don't see a security risk here.
However, it makes sense to exclude the connection info from the explain plan.

[zhangbutao]

Could we use a more simpler qtest instead?

[InvisibleProgrammer]

What would you prefer by a simpler qtest?

It only selects a constant. And the output is limited to one row.

[zhangbutao]

ok, it is reasonable for the explain case.

If we also need to fix the "show create table" use case, then only a simple SQL command "show create table" is enough.

[zratkai]

LGTM

[deniskuzZ]

Why do we need to define separate var for every connection config?
Can't we pass hive.sql regex to a filter?

[InvisibleProgrammer]

Can't we pass hive.sql regex to a filter?

It is a great question: if we add a regex pattern to filter this out, there is a chance that we can filter out something in the future that is not implemented yet. And the person who implements it, won't even know about it at all. But on the other hand, there are no huge improvements at Hive - I mean, it looks like a complete software - so I see only a little chance that it will happen.

And also, thank you for this comment. I just now realized it is a thrift generated code. So it is definitely a bad idea to put a new constant here - it will be removed as thrift is re-generated. Let me find a proper place to store those constants.

[InvisibleProgrammer]

@zhangbutao , @zratkai , @deniskuzZ , thank you for the reviews so far. Do I have any question that I didn't answer or needs more to discuss about?

Thank you

[zhangbutao]

+1 LGTM

I would suggest optimizing the title, and emphasizing that this PR is for the sensitive information repair related to the explain type tasks of the sys JdbcStorageHandler table.

[deniskuzZ]

@InvisibleProgrammer, what about

  public Map<String, String> getTblPropsExplain() { // only for displaying plan
    return PlanUtils.getPropertiesForExplain(tblProps,
      hive_metastoreConstants.TABLE_IS_CTAS,
      hive_metastoreConstants.TABLE_BUCKETING_VERSION);
  }

it covers explain CTAS query

[InvisibleProgrammer]

For the first read, it looks like this is in the class, CreateTableLikeDesc. So it has a different explain operator then everything else. It is good to know about it. Let me check how it behaves for sys tables.
Thank you for pointing it out.

[sonarqubecloud]

Quality Gate passed

Issues
12 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud