petersomogyi commented on code in PR #8681:
URL: https://github.com/apache/hbase/pull/8681#discussion_r4143237397
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/X509Util.java:
##########
@@ -278,6 +279,44 @@ public static char[] resolvePassword(Configuration config,
String roleKey, Strin
return value;
}
+ // resolveConfig falls back per key, so a store whose location comes from
one prefix and whose
+ // password/type come from the other would open the wrong file, or the right
file with the wrong
+ // credentials. A store is a unit: reject configs that straddle both
prefixes.
+ public static void validateConfigPrefixConsistency(Configuration config,
String rolePrefix,
Review Comment:
While this validation is ok I think it can be problematic when an operator
is migrating to the new configs.
It is not possible to add the new configs (all of the single EKU related)
while the dual-use configs are still in place.
Wouldn't it make sense to fetch all the single EKU configs and if that
misses any of the required key-value pairs then throw an exception?
IMO having all single use and all dual use configs is still a valid setup,
maybe a WARN log can be added to "remove the legacy once migration is completed"
##########
hbase-http/src/main/java/org/apache/hadoop/hbase/http/HttpServer.java:
##########
@@ -475,7 +491,18 @@ public HttpServer build() throws IOException {
HttpConfiguration httpsConfig = new HttpConfiguration(httpConfig);
httpsConfig.addCustomizer(new SecureRequestCustomizer());
SslContextFactory.Server sslCtxFactory = new
SslContextFactory.Server();
+ // Requesting a client certificate without an explicit truststore
would leave Jetty
+ // falling back to the keystore (or the JVM cacerts) as the
client-cert trust anchor,
+ // silently trusting issuers the operator never configured. Fail
fast instead.
+ X509Util.validateClientAuthTrustStore(
+ needsClientAuth
+ ? X509Util.ClientAuth.NEED
+ : (wantsClientAuth ? X509Util.ClientAuth.WANT :
X509Util.ClientAuth.NONE),
+ trustStore, InfoServer.HBASE_UI_SSL_CLIENT_AUTH_MODE,
+ "hbase.ui.ssl.server.truststore.location",
"hbase.ui.ssl.truststore.location",
+ "ssl.server.truststore.location");
Review Comment:
* the last three parameters are only used in the exception message, the
actual check is just "clientAuth != NONE && blank(trustStore) → throw"
* The HttpServer is a generic base class, but those `hbase.ui.ssl.*` key
names belong to `InfoServer`. Could this validation live in InfoServer instead,
right after `clientAuth` is computed? That's also where the resolved truststore
location and the correct key names are already in hand and it matches how
REST/Thrift call `validateClientAuthTrustStore` from their own setup.
* That move also removes the nested ternary here (InfoServer already has the
ClientAuth enum, so it need not reconstruct it from the two booleans). It's
hard to see which arguments are actually passed using this nested ternary
operators.
--
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]
