henrib commented on code in PR #6914:
URL: https://github.com/apache/hive/pull/6914#discussion_r4389938523
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java:
##########
@@ -367,17 +415,25 @@ public static TServerSocket getServerSSLSocket(String
hiveHost, int portNum, Str
includeProtocols));
LOG.info("SSL Server Socket Enabled Protocols: "
+ Arrays.toString(sslServerSocket.getEnabledProtocols()));
+ setClientAuth(sslServerSocket, clientAuth);
}
return thriftServerSocket;
}
+ private static void setClientAuth(SSLServerSocket sslServerSocket,
ClientAuth clientAuth) {
Review Comment:
Could we add automated tests rather than relying on manual verification?
There is already SSL test infrastructure (`HMSTestBase`,
`HMSJsonClientSslTest`, keystores under `data/files`). At minimum, for both
binary and HTTP transports:
- `NEED`: client without cert is rejected, client with cert is accepted
- `WANT`: both are accepted
- `WANT`/`NEED` without `hive.metastore.server.truststore.path` fails at
startup
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1519,6 +1524,32 @@ public enum ConfVars {
"Comma-separated list of SSL protocols to include for the Metastore."),
SSL_INCLUDE_CIPHERSUITES("metastore.include.ciphersuites",
"hive.metastore.include.ciphersuites", "",
"Colon-separated list of cipher suite names to include for the
Metastore."),
+ SSL_CLIENT_AUTH("metastore.ssl.client.auth",
"hive.metastore.ssl.client.auth",
Review Comment:
The client certificate is only used as a transport gate; its identity is
never mapped to a principal or used for authorization. So with `NEED` and no
Kerberos, any holder of a cert signed by a trusted CA can still `set_ugi` as
any user, and `WANT` effectively adds nothing since nothing downstream inspects
the presented cert. Is `WANT` worth exposing? Either way, I think the
description should state these semantics, and recommend that the server
truststore contain only the internal CA (not e.g. `cacerts`).
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ServletSecurity.java:
##########
@@ -522,14 +513,56 @@ static SslContextFactory
createSslContextFactory(Configuration conf) throws IOEx
LOG.info("HTTP Server SSL: SslContextFactory.getExcludeProtocols = {}",
Arrays.toString(factory.getExcludeProtocols()));
}
+ if (includeCipherSuites.length > 0) {
+ factory.setIncludeCipherSuites(includeCipherSuites);
+ }
+ final SecurityUtils.ClientAuth clientAuth = SecurityUtils.getClientAuth(
Review Comment:
`createSslContextFactory` is also used by `ServletServerBuilder`, which
serves the Iceberg REST catalog (`HMSCatalogFactory`,
`IcebergCatalogConfiguration`) and `PropertyServlet`. With `NEED`, REST catalog
clients (Spark/Trino/PyIceberg, typically using OAuth/JWT) would also have to
present client certificates, with no way to opt out. Should the servlet/REST
side get its own setting (or at least an override), and in any case could this
be called out explicitly in the config description?
##########
standalone-metastore/metastore-server/src/main/java/org/apache/hadoop/hive/metastore/ServletSecurity.java:
##########
@@ -522,14 +513,56 @@ static SslContextFactory
createSslContextFactory(Configuration conf) throws IOEx
LOG.info("HTTP Server SSL: SslContextFactory.getExcludeProtocols = {}",
Arrays.toString(factory.getExcludeProtocols()));
}
+ if (includeCipherSuites.length > 0) {
+ factory.setIncludeCipherSuites(includeCipherSuites);
+ }
+ final SecurityUtils.ClientAuth clientAuth = SecurityUtils.getClientAuth(
+ MetastoreConf.getVar(conf, MetastoreConf.ConfVars.SSL_CLIENT_AUTH));
+ if (clientAuth != SecurityUtils.ClientAuth.NONE) {
+ setServerTrustStore(factory, conf);
+ if (clientAuth == SecurityUtils.ClientAuth.NEED) {
+ factory.setNeedClientAuth(true);
+ } else {
+ factory.setWantClientAuth(true);
+ }
+ }
+ LOG.info("HTTP Server SSL: client authentication: {}", clientAuth);
+ return factory;
+ }
+
+ private static void setServerKeyStore(SslContextFactory.Server factory,
Configuration conf) throws IOException {
Review Comment:
This (and `setServerTrustStore` below) duplicates the read/validate/default
logic of `HiveMetaStore.getServerKeyStore`/`getServerTrustStore` without using
`StoreConfig`; similarly `HMSClient` duplicates
`ThriftHiveMetaStoreClient.getClientKeyStore`. Since `SecurityUtils` lives next
to `MetastoreConf`, how about static builders there, e.g.
`SecurityUtils.getServerKeyStore(conf)`, `getServerTrustStore(conf)`,
`getClientKeyStore(conf)`, `getClientTrustStore(conf)`, and have Jetty consume
`ks.path()` / `ks.typeOrDefault()` / `ks.keyManagerAlgorithmOrDefault()`? That
would finish the refactor consistently across all call sites.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
