HBASE-30387: Support SingleEKU certificates by lucakovacs · Pull Request #8681 · apache/hbase

lucakovacs · 2026-09-18T10:48:11Z

No description provided.

PDavid · 2026-09-23T07:26:05Z

The newly added org.apache.hadoop.hbase.rest.TestRESTServerSSL.testClientAuthNeedRejectsClientWithoutCert test seems to fail. Can you please have a look?

PDavid

Many thanks, looks really good. 👍

petersomogyi

Could you extend the TLS documentation?
https://github.com/apache/hbase/blob/master/hbase-website/app/pages/_docs/docs/_mdx/(multi-page)/security/tls.mdx

petersomogyi · 2026-09-24T11:14:25Z

+    String trustStoreLocation = resolveConfig(config, TLS_CONFIG_CLIENT_TRUSTSTORE_LOCATION,
+      TLS_CONFIG_TRUSTSTORE_LOCATION, "");
+    char[] trustStorePassword = resolvePassword(config, TLS_CONFIG_CLIENT_TRUSTSTORE_PASSWORD,
+      TLS_CONFIG_TRUSTSTORE_PASSWORD);
+    String trustStoreType =
+      resolveConfig(config, TLS_CONFIG_CLIENT_TRUSTSTORE_TYPE, TLS_CONFIG_TRUSTSTORE_TYPE, "");


The fallback happens per config key. Setting only hbase.rpc.tls.client.keystore.location means that legacy password and type will be matched with it causing an error. These should not be combined.

petersomogyi · 2026-09-24T11:19:01Z

+      X509Util.ClientAuth clientAuth = X509Util.ClientAuth
+        .fromPropertyValue(c.get(HBASE_UI_SSL_CLIENT_AUTH_MODE, X509Util.ClientAuth.NONE.name()));


It might be an edge case but when the config is in hbase-site.xml with empty value then it will use NEED not NONE when the config is not there at all.

/** * Converts a property value to a ClientAuth enum. If the input string is empty or null, returns * <code>ClientAuth.NEED</code>. * @param prop the property string. * @return the ClientAuth. * @throws IllegalArgumentException if the property value is not "NONE", "WANT", "NEED", or * empty/null. */

petersomogyi · 2026-09-24T11:23:31Z

+      // Truststore is entirely new for Thrift — no legacy fallback because there is no historical
+      // hbase.thrift.ssl.truststore.* configuration. When left unset, no truststore is configured
+      // on the connector and any hbase.thrift.ssl.server.client.auth.mode = WANT/NEED setting
+      // will fail the handshake for lack of a peer-cert trust root.


Opus 5.5 called out this: Jetty falls back to the JVM cacerts, so any publicly-CA-signed client cert is accepted and it will not cause a handshake fail. It should fail at startup if configs are missing or WARN about it to let the operator aware of the configuration issue.

Same problem is present in REST and InfoServer.

petersomogyi · 2026-09-24T11:30:55Z

+  @Test
+  public void testClientAuthModeKeyIsRoleScoped() {
+    // Guard against a "double server.server." regression: the client-auth-mode config key must
+    // resolve to the single role-scoped key, not to a nested/prefixed form.
+    assertEquals("hbase.ui.ssl.server.client.auth.mode", InfoServer.HBASE_UI_SSL_CLIENT_AUTH_MODE);
+  }


What is the point of this test?

petersomogyi · 2026-09-24T13:21:47Z

+    conf.unset(X509Util.TLS_CONFIG_CLIENT_KEYSTORE_LOCATION);
+    conf.unset(X509Util.TLS_CONFIG_CLIENT_KEYSTORE_PASSWORD);
+    conf.unset(X509Util.TLS_CONFIG_CLIENT_KEYSTORE_TYPE);
+    conf.unset(X509Util.TLS_CONFIG_CLIENT_TRUSTSTORE_LOCATION);
+    conf.unset(X509Util.TLS_CONFIG_CLIENT_TRUSTSTORE_PASSWORD);
+    conf.unset(X509Util.TLS_CONFIG_CLIENT_TRUSTSTORE_TYPE);


Isn't this a no-op?
Apart from that this test is the same as testCreateSSLContextWithoutCustomProtocol but that also validates the TLS protocols, not just if it is not empty.

petersomogyi · 2026-09-24T13:22:34Z

+  }
+
+  @TestTemplate
+  public void testCreateSSLContextForServerFallsBackToLegacyKeystore() throws Exception {


Same comment as testCreateSSLContextForClientFallsBackToLegacyKeystore

petersomogyi · 2026-09-24T13:39:15Z

+  public void testClientAuthNeedRejectsClientWithoutCert() throws Exception {
+    conf.set(Constants.REST_SSL_CLIENT_AUTH_MODE, "NEED");
+    startRESTServerWithDefaultKeystoreType();
+    assertThrows(SSLException.class, () -> sslClient.get("/version"));


This should be changed to IOException like in TestThriftServerSSLMutualAuth.

lucakovacs · 2026-09-28T11:59:22Z

I made the following changes according to @petersomogyi's review:  

Changed the SSLException to IOException in the TestRESTServerSSL.java file 
Deleted the two tests in TestX509Util.java since after the no-op code was removed the remaining parts are covered in other tests. 
Deleted testClientAuthModeKeyIsRoleScoped() in TestInfoServerTLSConfig.java since it’s not testing any functional behaviour  
Added a helper method validateClientAuthTrustStore to avoid Jetty’s fallback mechanism in Thrift/InfoServer/REST when no truststore is set. Also added tests to test the validation 
Added inline expressions for Thrift/REST/Infoserver when resolving client auth mode to avoid whitespace resolved as NEED 
Added a config validator method for checking mixed role configuration

If acceptable, I would to the documentation on an other PR or JIRA if needed

PDavid · 2026-09-29T11:57:47Z

+  // 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,


Many thanks for adding this method which checks the consistency. 👍 Would it maybe make sense to add unit test for this?

petersomogyi · 2026-09-30T09:48:16Z

+  // 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,


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"

petersomogyi · 2026-09-30T11:33:50Z

+          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");


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.

@lucakovacs this part wasn't touched in your last commit.

petersomogyi · 2026-10-02T08:44:16Z

+  private static void logOnce(Configuration config, String rolePrefix, String legacyPrefix,
+    Keys keys) {
+    StringBuilder unused = new StringBuilder();
+    for (String postfix : keys.all()) {
+      if (config.get(legacyPrefix + postfix) != null) {
+        unused.append(unused.length() == 0 ? "" : ", ").append(legacyPrefix).append(postfix);
+      }
+    }
+    if (unused.length() > 0 && LOGGED_STORES.add(rolePrefix + keys.location)) {
+      LOG.warn("{} supplies this store, so these keys are unused: {}. Remove them once the"
+        + " migration is complete.", rolePrefix, unused);
+    }
+  }


This part looks ugly. String.join is exactly for this behavior. Also, the LOGGED_STORES could be evaluate prior so the for loop/stream is not executed when it was already logged.

if (!LOGGED_STORES.add(rolePrefix + keys.location)) { return; } List<String> unused = Arrays.stream(keys.all()) .map(postfix -> legacyPrefix + postfix) .filter(key -> config.get(key) != null) .collect(Collectors.toList()); if (!unused.isEmpty()) { LOG.warn("{} supplies this store, so these keys are unused: {}. Remove them once the" + " migration is complete.", rolePrefix, String.join(", ", unused)); }

petersomogyi · 2026-10-02T10:35:39Z

+          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");


@lucakovacs this part wasn't touched in your last commit.

lucakovacs force-pushed the singleEKU-support branch from 2122bf1 to b26c894 Compare September 21, 2026 19:11

lucakovacs added 2 commits September 21, 2026 21:16

HBASE-30387: Support SingleEKU certificates

188f1ca

Fixing spotless errors

08557a2

lucakovacs force-pushed the singleEKU-support branch from b26c894 to 08557a2 Compare September 21, 2026 19:16

Fixing unit test failrues

dfb2990

lucakovacs marked this pull request as ready for review September 23, 2026 14:30

PDavid approved these changes Sep 24, 2026

View reviewed changes

petersomogyi requested changes Sep 24, 2026

View reviewed changes

petersomogyi reviewed Sep 24, 2026

View reviewed changes

Addressing code review feedback

0557e49

lucakovacs requested a review from petersomogyi September 28, 2026 11:59

PDavid reviewed Sep 29, 2026

View reviewed changes

Added unit tests for config consistency checker function

6517b42

petersomogyi reviewed Sep 30, 2026

View reviewed changes

lucakovacs added 2 commits October 1, 2026 23:15

Added more unit tests to cover config consistency checking

0710c3f

Refactoring: Handling key/truststore as units

14059a2

petersomogyi reviewed Oct 2, 2026

View reviewed changes

		X509Util.ClientAuth clientAuth = X509Util.ClientAuth
		.fromPropertyValue(c.get(HBASE_UI_SSL_CLIENT_AUTH_MODE, X509Util.ClientAuth.NONE.name()));

Conversation

lucakovacs commented Sep 18, 2026

Uh oh!

PDavid commented Sep 23, 2026

Uh oh!

PDavid left a comment

Choose a reason for hiding this comment

Uh oh!

petersomogyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucakovacs commented Sep 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants