petersomogyi commented on code in PR #8681:
URL: https://github.com/apache/hbase/pull/8681#discussion_r4164952627
##########
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:
@lucakovacs this part wasn't touched in your last commit.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/io/crypto/tls/TLSStore.java:
##########
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.io.crypto.tls;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * One keystore or truststore, resolved from configuration with every
attribute drawn from a single
+ * key prefix.
+ * <p/>
+ * Each TLS surface exposes two prefixes for the same store: a role-scoped one
added for single-EKU
+ * certificate support (e.g. {@code hbase.rpc.tls.server.}) and the historical
unscoped one (e.g.
+ * {@code hbase.rpc.tls.}). Resolving attribute by attribute would let a
store's location come from
+ * one prefix while its password or type came from the other, which opens
either the wrong file or
+ * the right file with the wrong credentials. {@link #resolve} therefore picks
the prefix once, from
+ * the location, and reads the rest of the store from that same prefix.
+ */
[email protected]
+public final class TLSStore {
+
+ private static final Logger LOG = LoggerFactory.getLogger(TLSStore.class);
+
+ /**
+ * Tracks which prefixes have already been reported, so a JVM logs at most
one line per store
+ * regardless of how many times a context is built.
+ */
+ private static final Set<String> LOGGED_STORES =
ConcurrentHashMap.newKeySet();
+
+ /**
+ * The key postfixes making up one store, by surface. The RPC keys use
{@code .location} while the
+ * servlet surfaces (REST, Thrift) use {@code .store}, and only the latter
have a separate
+ * {@code keypassword}.
+ */
+ public enum Keys {
+ RPC_KEYSTORE("keystore.location", "keystore.password", "keystore.type",
null),
+ RPC_TRUSTSTORE("truststore.location", "truststore.password",
"truststore.type", null),
+ SERVLET_KEYSTORE("keystore.store", "keystore.password", "keystore.type",
+ "keystore.keypassword"),
+ SERVLET_TRUSTSTORE("truststore.store", "truststore.password",
"truststore.type", null);
+
+ private final String location;
+ private final String password;
+ private final String type;
+ private final String keyPassword;
+
+ Keys(String location, String password, String type, String keyPassword) {
+ this.location = location;
+ this.password = password;
+ this.type = type;
+ this.keyPassword = keyPassword;
+ }
+
+ private String[] all() {
+ return keyPassword == null
+ ? new String[] { location, password, type }
+ : new String[] { location, password, type, keyPassword };
+ }
+ }
+
+ private final String location;
+ private final char[] password;
+ private final char[] keyPassword;
+ private final String type;
+
+ private TLSStore(String location, char[] password, char[] keyPassword,
String type) {
+ this.location = location;
+ this.password = password;
+ this.keyPassword = keyPassword;
+ this.type = type;
+ }
+
+ /**
+ * Reads one store from {@code config}. The prefix supplying the location
supplies every other
+ * attribute too, so the two prefixes are never combined within a single
store.
+ * <p/>
+ * Only the location is mandatory under a prefix: a store may have no
password at all, and the
+ * type is auto-detected from the file extension when absent (see
+ * {@link KeyStoreFileType#fromPropertyValueOrFileName}). Keeping the legacy
keys in place while
+ * migrating is valid -- the ones left unused are logged once so they can be
cleaned up.
+ * @param config the configuration to read from
+ * @param rolePrefix role-scoped prefix, e.g. {@code hbase.rpc.tls.server.}
+ * @param legacyPrefix historical unscoped prefix, e.g. {@code
hbase.rpc.tls.}
+ * @param keys which store to read, and under which postfixes
+ * @throws IllegalArgumentException if {@code rolePrefix} supplies any
attribute but not the
+ * location, since the location would then
be taken from
+ * {@code legacyPrefix}
+ */
+ public static TLSStore resolve(Configuration config, String rolePrefix,
String legacyPrefix,
+ Keys keys) throws IOException {
+ String roleLocationKey = rolePrefix + keys.location;
+ boolean useRole = config.get(roleLocationKey) != null;
+ if (!useRole) {
+ for (String postfix : keys.all()) {
+ if (config.get(rolePrefix + postfix) != null) {
+ throw new IllegalArgumentException(
+ rolePrefix + postfix + " is set, but " + roleLocationKey + " is
not. Once any "
+ + rolePrefix + " key is used, that prefix must supply the store
location.");
+ }
+ }
+ }
+ String prefix = useRole ? rolePrefix : legacyPrefix;
+ if (useRole) {
+ logOnce(config, rolePrefix, legacyPrefix, keys);
+ }
+ return new TLSStore(config.get(prefix + keys.location, ""),
+ config.getPassword(prefix + keys.password),
+ keys.keyPassword == null ? null : config.getPassword(prefix +
keys.keyPassword),
+ config.get(prefix + keys.type, ""));
+ }
+
+ 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);
+ }
+ }
Review Comment:
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));
}
```
--
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]
