kezhuw commented on code in PR #2269:
URL: https://github.com/apache/zookeeper/pull/2269#discussion_r2173277173
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java:
##########
@@ -1176,12 +1183,98 @@ public void initialize() throws SaslException {
}
authServer = new
SaslQuorumAuthServer(isQuorumServerSaslAuthRequired(),
quorumServerLoginContext, authzHosts);
authLearner = new
SaslQuorumAuthLearner(isQuorumLearnerSaslAuthRequired(),
quorumServicePrincipal, quorumLearnerLoginContext);
+ } else if (isSslQuorum()) {
+ try {
+ authServer = getSslQuorumAuthServer();
+ authLearner = getSslQuorumAuthLearner();
+ } catch (Exception e) {
+ LOG.error(e.getMessage(), e);
+ throw new SaslException(e.getMessage());
+ }
} else {
authServer = new NullQuorumAuthServer();
authLearner = new NullQuorumAuthLearner();
}
}
+ /**
+ * Instantiates and returns the configured SSL QuorumAuthServer
implementation.
+ * <p>
+ * Reads the class name from the {@code sslAuthServerProvider} property. If
+ * no provider is configured, falls back to {@link NullQuorumAuthServer}.
+ * </p>
+ *
+ * @return an instance of {@link QuorumAuthServer}, or {@link
NullQuorumAuthServer}
+ * if no provider is defined
+ * @throws SaslException if the configured class cannot be found,
instantiated,
+ * or does not implement {@link QuorumAuthServer}
+ */
+ private QuorumAuthServer getSslQuorumAuthServer() throws SaslException {
+ if (sslAuthServerProvider == null) {
+ LOG.info("sslAuthServerProvider not defined; using
NullQuorumAuthServer");
+ return new NullQuorumAuthServer();
+ }
+ try {
+ Class<?> cls = Class.forName(sslAuthServerProvider);
+ Object inst = cls.getDeclaredConstructor().newInstance();
+ if (!(inst instanceof QuorumAuthServer)) {
+ throw new SaslException(
+ sslAuthServerProvider + " does not implement
QuorumAuthServer");
Review Comment:
Same for `getSslQuorumAuthLearner`.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java:
##########
@@ -132,7 +133,9 @@ public class QuorumPeer extends ZooKeeperThread implements
QuorumStats.Provider
// of updates; see the implementation comment at
setLastSeenQuorumVerifier().
private AtomicReference<QuorumCnxManager> qcmRef = new AtomicReference<>();
+ /** Class name to instantiate for SSL QuorumAuthServer */
Review Comment:
Sounds like comments to `sslAuthServerProvider` and `sslAuthLearnerProvider`
?
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeerConfig.java:
##########
@@ -390,6 +393,10 @@ public void parseProperties(Properties zkProp) throws
IOException, ConfigExcepti
multiAddressReachabilityCheckEnabled = parseBoolean(key,
value);
} else if (key.equals("oraclePath")) {
oraclePath = value;
+ } else if (key.equals(QuorumAuth.QUORUM_SSL_AUTHPROVIDER)) {
+ sslAuthServerProvider = value;
+ } else if (key.equals(QuorumAuth.QUORUM_SSL_LEARNER_AUTHPROVIDER))
{
Review Comment:
No java system properties ? I found that most other ssl config options
supports both, `sslQuorumReloadCertFiles` is an exception.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/server/quorum/QuorumPeer.java:
##########
@@ -1176,12 +1183,98 @@ public void initialize() throws SaslException {
}
authServer = new
SaslQuorumAuthServer(isQuorumServerSaslAuthRequired(),
quorumServerLoginContext, authzHosts);
authLearner = new
SaslQuorumAuthLearner(isQuorumLearnerSaslAuthRequired(),
quorumServicePrincipal, quorumLearnerLoginContext);
+ } else if (isSslQuorum()) {
+ try {
+ authServer = getSslQuorumAuthServer();
+ authLearner = getSslQuorumAuthLearner();
+ } catch (Exception e) {
+ LOG.error(e.getMessage(), e);
+ throw new SaslException(e.getMessage());
+ }
} else {
authServer = new NullQuorumAuthServer();
authLearner = new NullQuorumAuthLearner();
}
}
+ /**
+ * Instantiates and returns the configured SSL QuorumAuthServer
implementation.
+ * <p>
+ * Reads the class name from the {@code sslAuthServerProvider} property. If
+ * no provider is configured, falls back to {@link NullQuorumAuthServer}.
+ * </p>
+ *
+ * @return an instance of {@link QuorumAuthServer}, or {@link
NullQuorumAuthServer}
+ * if no provider is defined
+ * @throws SaslException if the configured class cannot be found,
instantiated,
+ * or does not implement {@link QuorumAuthServer}
+ */
+ private QuorumAuthServer getSslQuorumAuthServer() throws SaslException {
+ if (sslAuthServerProvider == null) {
+ LOG.info("sslAuthServerProvider not defined; using
NullQuorumAuthServer");
+ return new NullQuorumAuthServer();
+ }
+ try {
+ Class<?> cls = Class.forName(sslAuthServerProvider);
+ Object inst = cls.getDeclaredConstructor().newInstance();
+ if (!(inst instanceof QuorumAuthServer)) {
+ throw new SaslException(
+ sslAuthServerProvider + " does not implement
QuorumAuthServer");
Review Comment:
Is this redundant as we catch `ClassCastException` in catch clauses ?
##########
zookeeper-server/src/test/java/org/apache/zookeeper/server/quorum/auth/MockSslQuorumAuthServer.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.zookeeper.server.quorum.auth;
+
+import static org.junit.Assert.assertEquals;
+import java.io.DataInputStream;
+import java.io.IOException;
+import java.net.Socket;
+import java.security.cert.X509Certificate;
+import org.apache.zookeeper.server.quorum.UnifiedServerSocket;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Test stub implementation of {@link QuorumAuthServer} for SSL quorum
authentication.
+ * Used to verify provider wiring in {@code QuorumPeer}.
+ */
+public class MockSslQuorumAuthServer implements QuorumAuthServer {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(MockSslQuorumAuthServer.class);
+ private static String subjectX509Principal;
+ private final boolean initialized;
+
+ /**
+ * Constructs a new MockSslQuorumAuthServer and reads the expected X.509
principal
+ * from the system property.
+ */
+ public MockSslQuorumAuthServer() {
+ this.initialized = true;
+ subjectX509Principal =
System.getProperty("zookeeper.ssl.quorum.auth.subjectX509Principal");
Review Comment:
I am worry about how capable `QuorumAuthServer` or `QuorumAuthLearner` can
be given that they are constructed without parameters. But given the fact, most
zookeeper config options are opened implicitly through system properties, I
think it is ok for the current situation which `X509AuthenticationProvider`
follows.
--
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: notifications-unsubscr...@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
