Copilot commented on code in PR #6476:
URL: https://github.com/apache/hive/pull/6476#discussion_r3227066923
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java:
##########
@@ -303,14 +319,20 @@ public static TServerSocket getServerSSLSocket(String
hiveHost, int portNum, Str
}
SSLServerSocket sslServerSocket = (SSLServerSocket)
thriftServerSocket.getServerSocket();
List<String> enabledProtocols = new ArrayList<>();
- for (String protocol : sslServerSocket.getEnabledProtocols()) {
+ for (String protocol : sslServerSocket.getSupportedProtocols()) {
if (sslVersionBlacklistLocal.contains(protocol.toLowerCase())) {
LOG.debug("Disabling SSL Protocol: " + protocol);
} else {
enabledProtocols.add(protocol);
}
}
- sslServerSocket.setEnabledProtocols(enabledProtocols.toArray(new
String[0]));
+ if (includeProtocols.length > 0) {
+ Set<String> includeProtocolsLowerCase =
Arrays.stream(includeProtocols).map(String::toLowerCase)
+ .collect(Collectors.toSet());
+ enabledProtocols = enabledProtocols.stream()
+ .filter(protocol ->
includeProtocolsLowerCase.contains(protocol.toLowerCase())).toList();
+ }
+
sslServerSocket.setEnabledProtocols(enabledProtocols.toArray(String[]::new));
Review Comment:
If includeProtocols is configured but none of the requested protocols are
present after applying the blacklist/intersection, enabledProtocols becomes
empty and sslServerSocket.setEnabledProtocols(...) will throw
IllegalArgumentException at runtime. Please validate the intersection and fail
fast with a clear error message (e.g., list requested protocols and
supported/enabled protocols) instead of letting a low-level exception occur.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java:
##########
@@ -319,18 +341,23 @@ public static TServerSocket getServerSSLSocket(String
hiveHost, int portNum, Str
public static TTransport getSSLSocket(String host, int port, int
socketTimeout, int connectionTimeout,
String trustStorePath, String trustStorePassWord, String trustStoreType,
- String trustStoreAlgorithm) throws TTransportException {
- TSSLTransportFactory.TSSLTransportParameters params =
- new TSSLTransportFactory.TSSLTransportParameters();
- String tStoreType = trustStoreType.isEmpty()? KeyStore.getDefaultType() :
trustStoreType;
- String tStoreAlgorithm = trustStoreAlgorithm.isEmpty()?
- TrustManagerFactory.getDefaultAlgorithm() : trustStoreAlgorithm;
- params.setTrustStore(trustStorePath, trustStorePassWord,
- tStoreAlgorithm, tStoreType);
- params.requireClientAuth(true);
+ String trustStoreAlgorithm, String[] includeProtocols, String[]
cipherSuites) throws TTransportException {
+ TSSLTransportFactory.TSSLTransportParameters params =
getSSLTransportParameters(false,
+ trustStorePath, trustStorePassWord,
+ trustStoreAlgorithm.isEmpty() ?
TrustManagerFactory.getDefaultAlgorithm() : trustStoreAlgorithm,
+ trustStoreType.isEmpty() ? KeyStore.getDefaultType() : trustStoreType,
+ cipherSuites);
// The underlying SSLSocket object is bound to host:port with the given
SO_TIMEOUT and
// connection timeout and SSLContext created with the given params
TSocket tSSLSocket = TSSLTransportFactory.getClientSocket(host, port,
socketTimeout, params);
+ if (includeProtocols.length > 0) {
+ SSLSocket sslSocket = (SSLSocket) (tSSLSocket.getSocket());
+ Set<String> includeProtocolsLowerCase =
Arrays.stream(includeProtocols).map(String::toLowerCase)
+ .collect(Collectors.toSet());
+ String[] enabledProtocols =
Arrays.stream(sslSocket.getSupportedProtocols())
+ .filter(protocol ->
includeProtocolsLowerCase.contains(protocol.toLowerCase())).toArray(String[]::new);
Review Comment:
On the client side, when includeProtocols is set but none of the configured
protocols are supported, enabledProtocols becomes an empty array and
sslSocket.setEnabledProtocols(enabledProtocols) will throw
IllegalArgumentException. Add a check for an empty intersection and throw a
more actionable exception (or fall back to defaults) so misconfiguration is
easier to diagnose.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/conf/MetastoreConf.java:
##########
@@ -1477,6 +1477,10 @@ public enum ConfVars {
"Metastore SSL certificate truststore type."),
SSL_TRUSTMANAGERFACTORY_ALGORITHM("metastore.trustmanagerfactory.algorithm",
"hive.metastore.trustmanagerfactory.algorithm", "",
"Metastore SSL certificate truststore algorithm."),
+ SSL_INCLUDE_PROTOCOLS("metastore.include.protocols",
"hive.metastore.include.protocols", "",
+ "List of include SSL protocols separated by comma for Metastore"),
+ SSL_INCLUDE_CIPHERSUITES("metastore.include.ciphersuites",
"hive.metastore.include.ciphersuites", "",
+ "List of include cipher suite names separated by colon for Metastore"),
Review Comment:
These new config descriptions are grammatically unclear. Consider rephrasing
to something like “Comma-separated list of SSL protocols to include …” /
“Colon-separated list of cipher suite names to include …” to make the expected
format unambiguous.
##########
standalone-metastore/metastore-client/src/main/java/org/apache/hadoop/hive/metastore/client/ThriftHiveMetaStoreClient.java:
##########
@@ -575,9 +580,13 @@ private THttpClient createHttpClient(URI store, boolean
useSSL) throws MetaExcep
String trustStorePassword = MetastoreConf.getPassword(conf,
MetastoreConf.ConfVars.SSL_TRUSTSTORE_PASSWORD);
String trustStoreType = MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.SSL_TRUSTSTORE_TYPE).trim();
String trustStoreAlgorithm = MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.SSL_TRUSTMANAGERFACTORY_ALGORITHM).trim();
+ String[] includeProtocols =
Iterables.toArray(Splitter.on(",").trimResults().omitEmptyStrings()
+ .split(MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.SSL_INCLUDE_PROTOCOLS)), String.class);
+ String[] includeCipherSuites =
Iterables.toArray(Splitter.on(":").trimResults().omitEmptyStrings()
+ .split(MetastoreConf.getVar(conf,
MetastoreConf.ConfVars.SSL_INCLUDE_CIPHERSUITES)), String.class);
Review Comment:
The parsing for SSL includeProtocols/includeCipherSuites is duplicated here
and again in createBinaryClient below. Consider extracting a small helper
(e.g., parseIncludedProtocols()/parseIncludedCipherSuites()) to ensure
delimiter/trim/omitEmptyStrings behavior stays consistent across code paths.
##########
standalone-metastore/metastore-common/src/main/java/org/apache/hadoop/hive/metastore/utils/SecurityUtils.java:
##########
@@ -303,14 +319,20 @@ public static TServerSocket getServerSSLSocket(String
hiveHost, int portNum, Str
}
SSLServerSocket sslServerSocket = (SSLServerSocket)
thriftServerSocket.getServerSocket();
List<String> enabledProtocols = new ArrayList<>();
- for (String protocol : sslServerSocket.getEnabledProtocols()) {
+ for (String protocol : sslServerSocket.getSupportedProtocols()) {
Review Comment:
In getServerSSLSocket, building the allowed protocol list from
sslServerSocket.getSupportedProtocols() can accidentally enable protocols that
were not enabled by default by the JVM/JSSE (supported is a superset of
enabled). This can weaken the default security posture if the blacklist is
incomplete. Consider starting from getEnabledProtocols() and then applying the
blacklist/include filtering so you only ever narrow the enabled set.
--
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]
