ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

luoxiner · 2024-10-10T14:39:00Z

ClientCnxn::pingRwServer uses raw socket to issue "isro" 4lw command. This results in unsuccessful handshake to tls server. Use SSLSocket when zookeeper.client.secure is set to true.

associated jira issue ZOOKEEPER-4819

kezhuw · 2024-10-12T05:18:45Z

Great! I think we need tests.

luoxiner · 2024-10-13T04:03:07Z

Great! I think we need tests.

Hi @kezhuw, I added a test case based on the ReadOnlyModeTest. Please take a look.

kezhuw

Hi @luoxiner, thank you for your contribution!

Given the fact that pingRwServer uses four letter word command "isro", I propose to use FourLetterWordMain.send4LetterWord directly. Beware that send4LetterWord appends \n to the response.

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

zookeeper-server/src/test/java/org/apache/zookeeper/test/ReadOnlyModeWithSSLTest.java

eolivelli

+1 as soon as @kezhuw's commenta are addressed

Nice work! Thanks

… readonly server ClientCnxn::pingRwServer uses raw socket to issue "isro" 4lw command. This results in unsuccessful handshake to tls server. Use SSLSocket when zookeeper.client.secure is set to true.

luoxiner · 2024-10-14T14:37:43Z

@kezhuw @eolivelli Thank you for your reply. I have changed my code based on the suggestions. Please take a look.

kezhuw

I left comments regarding the interference between ZKConfig and system properties. I think we should build dedicated client side ZKConfig in test so we can test pingRwServer correctly.

kezhuw · 2024-10-15T02:26:43Z

zookeeper-server/src/test/java/org/apache/zookeeper/test/ReadOnlyModeWithSSLTest.java

+        System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, "org.apache.zookeeper.server.NettyServerCnxnFactory");
+        System.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET, "org.apache.zookeeper.ClientCnxnSocketNetty");
+        System.setProperty(ZKClientConfig.SECURE_CLIENT, "true");
+        System.setProperty(clientX509Util.getSslKeystoreLocationProperty(), testDataPath + "/ssl/testKeyStore.jks");


These properties are mixed between client and server. I think we should build a ZKClientConfig for client usage prior to set them up for server(there is ZOOKEEPER-2139 counterpart in server) as ZKClientConfig will inherit properties from system.

zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKConfig.java

Lines 95 to 136 in 75884ec

/**

* Now onwards client code will use properties from this class but older

* clients still be setting properties through system properties. So to make

* this change backward compatible we should set old system properties in

* this configuration.

*/

protected void handleBackwardCompatibility() {

properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));

properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));

properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));

try (ClientX509Util clientX509Util = new ClientX509Util()) {

putSSLProperties(clientX509Util);

properties.put(clientX509Util.getSslAuthProviderProperty(), System.getProperty(clientX509Util.getSslAuthProviderProperty()));

properties.put(clientX509Util.getSslProviderProperty(), System.getProperty(clientX509Util.getSslProviderProperty()));

}

try (X509Util x509Util = new QuorumX509Util()) {

putSSLProperties(x509Util);

}

}

private void putSSLProperties(X509Util x509Util) {

properties.put(x509Util.getSslProtocolProperty(), System.getProperty(x509Util.getSslProtocolProperty()));

properties.put(x509Util.getSslEnabledProtocolsProperty(), System.getProperty(x509Util.getSslEnabledProtocolsProperty()));

properties.put(x509Util.getSslCipherSuitesProperty(), System.getProperty(x509Util.getSslCipherSuitesProperty()));

properties.put(x509Util.getSslKeystoreLocationProperty(), System.getProperty(x509Util.getSslKeystoreLocationProperty()));

properties.put(x509Util.getSslKeystorePasswdProperty(), System.getProperty(x509Util.getSslKeystorePasswdProperty()));

properties.put(x509Util.getSslKeystorePasswdPathProperty(), System.getProperty(x509Util.getSslKeystorePasswdPathProperty()));

properties.put(x509Util.getSslKeystoreTypeProperty(), System.getProperty(x509Util.getSslKeystoreTypeProperty()));

properties.put(x509Util.getSslTruststoreLocationProperty(), System.getProperty(x509Util.getSslTruststoreLocationProperty()));

properties.put(x509Util.getSslTruststorePasswdProperty(), System.getProperty(x509Util.getSslTruststorePasswdProperty()));

properties.put(x509Util.getSslTruststorePasswdPathProperty(), System.getProperty(x509Util.getSslTruststorePasswdPathProperty()));

properties.put(x509Util.getSslTruststoreTypeProperty(), System.getProperty(x509Util.getSslTruststoreTypeProperty()));

properties.put(x509Util.getSslContextSupplierClassProperty(), System.getProperty(x509Util.getSslContextSupplierClassProperty()));

properties.put(x509Util.getSslHostnameVerificationEnabledProperty(), System.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));

properties.put(x509Util.getSslCrlEnabledProperty(), System.getProperty(x509Util.getSslCrlEnabledProperty()));

properties.put(x509Util.getSslOcspEnabledProperty(), System.getProperty(x509Util.getSslOcspEnabledProperty()));

properties.put(x509Util.getSslClientAuthProperty(), System.getProperty(x509Util.getSslClientAuthProperty()));

properties.put(x509Util.getSslHandshakeDetectionTimeoutMillisProperty(), System.getProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty()));

properties.put(x509Util.getFipsModeProperty(), System.getProperty(x509Util.getFipsModeProperty()));

}

The test passed since send4LetterWord will inherit these properties in setUp for server side. It would be really good to not populate any properties for this test. But it is not possible currently. I created ZOOKEEPER-4875 instead.

Is it preferable to prioritize the use of ClientConfig over system properties？ If the properties are not already set in the ClientConfig, then we can retrieve them from the system properties. We can also maintain backward compatibility and ensure that properties in ClientConfig remain isolated in this manner.

Is it preferable to prioritize the use of ClientConfig over system properties？

Client should use ZKClientConfig instead system properties after constructed. If not, it is a bug. So, 33c19e3#diff-9657d4a14708c9ec1df56fd01581a442e10ef70cae39c7223d4f979b0ae54263R1307 is buggy as it does not use the one in constructor but the new ZkConfig(). These two are not necessary to have same properties. It should use ClientCnxn.clientConfig which means b13e196#diff-9657d4a14708c9ec1df56fd01581a442e10ef70cae39c7223d4f979b0ae54263R1359 is correct.

zookeeper/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java

Line 1099 in b997145

this.clientConfig = clientConfig != null ? clientConfig : new ZKClientConfig();

If the properties are not already set in the ClientConfig, then we can retrieve them from the system properties.

This is already done in construction of a ZKConfig.

Client should use ZKClientConfig instead system properties after constructed. If not, it is a bug.

This is the whole point of ZOOKEEPER-2139.

Yes, I realize that ZKConfig#init is only called when we construct ZKConfig with empty parameters. I had missed this before. I will fix it based on your comments later.

We can revise here once ZOOKEEPER-4875 delivered.

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

kezhuw

Looks good! I left two minor comments.

zookeeper-server/src/main/java/org/apache/zookeeper/client/FourLetterWordMain.java

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java

kezhuw · 2024-10-17T07:42:14Z

zookeeper-server/src/test/java/org/apache/zookeeper/test/ReadOnlyModeWithSSLTest.java

+        System.setProperty(ServerCnxnFactory.ZOOKEEPER_SERVER_CNXN_FACTORY, "org.apache.zookeeper.server.NettyServerCnxnFactory");
+        System.setProperty(ZKClientConfig.ZOOKEEPER_CLIENT_CNXN_SOCKET, "org.apache.zookeeper.ClientCnxnSocketNetty");
+        System.setProperty(ZKClientConfig.SECURE_CLIENT, "true");
+        System.setProperty(clientX509Util.getSslKeystoreLocationProperty(), testDataPath + "/ssl/testKeyStore.jks");


We can revise here once ZOOKEEPER-4875 delivered.

zookeeper-server/src/main/java/org/apache/zookeeper/client/FourLetterWordMain.java

…rLetterWordMain.java

kezhuw · 2024-10-23T05:00:24Z

Looks good from my side.

@eolivelli Would you want take another look ?

luoxiner force-pushed the ZOOKEEPER-4819 branch from d60b2f1 to b8c90c2 Compare October 13, 2024 04:00

luoxiner force-pushed the ZOOKEEPER-4819 branch from b8c90c2 to b13e196 Compare October 13, 2024 05:09

kezhuw reviewed Oct 13, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/ClientCnxn.java Outdated Show resolved Hide resolved

zookeeper-server/src/test/java/org/apache/zookeeper/test/ReadOnlyModeWithSSLTest.java Outdated Show resolved Hide resolved

eolivelli reviewed Oct 13, 2024

View reviewed changes

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to…

33c19e3

… readonly server ClientCnxn::pingRwServer uses raw socket to issue "isro" 4lw command. This results in unsuccessful handshake to tls server. Use SSLSocket when zookeeper.client.secure is set to true.

luoxiner force-pushed the ZOOKEEPER-4819 branch from b13e196 to 33c19e3 Compare October 14, 2024 14:32

kezhuw reviewed Oct 15, 2024

View reviewed changes

ZOOKEEPER-4819 Optimizing send4LetterWord with specific clientConfig

4c26a3f

luoxiner force-pushed the ZOOKEEPER-4819 branch from 3e0cdd5 to 4c26a3f Compare October 17, 2024 03:43

kezhuw approved these changes Oct 17, 2024

View reviewed changes

kezhuw reviewed Oct 21, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/client/FourLetterWordMain.java Outdated Show resolved Hide resolved

ZOOKEEPER-4819 Optimaze tcp options when calling FourLetterWordMain

26adb57

luoxiner force-pushed the ZOOKEEPER-4819 branch from 36fc60d to 26adb57 Compare October 22, 2024 08:22

kezhuw reviewed Oct 23, 2024

View reviewed changes

zookeeper-server/src/main/java/org/apache/zookeeper/client/FourLetterWordMain.java Outdated Show resolved Hide resolved

Update zookeeper-server/src/main/java/org/apache/zookeeper/client/Fou…

1575749

…rLetterWordMain.java

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

luoxiner commented Oct 10, 2024

kezhuw commented Oct 12, 2024

luoxiner commented Oct 13, 2024

kezhuw left a comment

eolivelli left a comment

luoxiner commented Oct 14, 2024

kezhuw left a comment

kezhuw Oct 15, 2024

kezhuw Oct 15, 2024 •

edited

Loading

luoxiner Oct 15, 2024

kezhuw Oct 15, 2024 •

edited

Loading

kezhuw Oct 15, 2024

luoxiner Oct 17, 2024

kezhuw Oct 17, 2024

kezhuw left a comment

kezhuw Oct 17, 2024

kezhuw commented Oct 23, 2024

	/**
	* Now onwards client code will use properties from this class but older
	* clients still be setting properties through system properties. So to make
	* this change backward compatible we should set old system properties in
	* this configuration.
	*/
	protected void handleBackwardCompatibility() {
	properties.put(JUTE_MAXBUFFER, System.getProperty(JUTE_MAXBUFFER));
	properties.put(KINIT_COMMAND, System.getProperty(KINIT_COMMAND));
	properties.put(JGSS_NATIVE, System.getProperty(JGSS_NATIVE));

	try (ClientX509Util clientX509Util = new ClientX509Util()) {
	putSSLProperties(clientX509Util);
	properties.put(clientX509Util.getSslAuthProviderProperty(), System.getProperty(clientX509Util.getSslAuthProviderProperty()));
	properties.put(clientX509Util.getSslProviderProperty(), System.getProperty(clientX509Util.getSslProviderProperty()));
	}

	try (X509Util x509Util = new QuorumX509Util()) {
	putSSLProperties(x509Util);
	}
	}

	private void putSSLProperties(X509Util x509Util) {
	properties.put(x509Util.getSslProtocolProperty(), System.getProperty(x509Util.getSslProtocolProperty()));
	properties.put(x509Util.getSslEnabledProtocolsProperty(), System.getProperty(x509Util.getSslEnabledProtocolsProperty()));
	properties.put(x509Util.getSslCipherSuitesProperty(), System.getProperty(x509Util.getSslCipherSuitesProperty()));
	properties.put(x509Util.getSslKeystoreLocationProperty(), System.getProperty(x509Util.getSslKeystoreLocationProperty()));
	properties.put(x509Util.getSslKeystorePasswdProperty(), System.getProperty(x509Util.getSslKeystorePasswdProperty()));
	properties.put(x509Util.getSslKeystorePasswdPathProperty(), System.getProperty(x509Util.getSslKeystorePasswdPathProperty()));
	properties.put(x509Util.getSslKeystoreTypeProperty(), System.getProperty(x509Util.getSslKeystoreTypeProperty()));
	properties.put(x509Util.getSslTruststoreLocationProperty(), System.getProperty(x509Util.getSslTruststoreLocationProperty()));
	properties.put(x509Util.getSslTruststorePasswdProperty(), System.getProperty(x509Util.getSslTruststorePasswdProperty()));
	properties.put(x509Util.getSslTruststorePasswdPathProperty(), System.getProperty(x509Util.getSslTruststorePasswdPathProperty()));
	properties.put(x509Util.getSslTruststoreTypeProperty(), System.getProperty(x509Util.getSslTruststoreTypeProperty()));
	properties.put(x509Util.getSslContextSupplierClassProperty(), System.getProperty(x509Util.getSslContextSupplierClassProperty()));
	properties.put(x509Util.getSslHostnameVerificationEnabledProperty(), System.getProperty(x509Util.getSslHostnameVerificationEnabledProperty()));
	properties.put(x509Util.getSslCrlEnabledProperty(), System.getProperty(x509Util.getSslCrlEnabledProperty()));
	properties.put(x509Util.getSslOcspEnabledProperty(), System.getProperty(x509Util.getSslOcspEnabledProperty()));
	properties.put(x509Util.getSslClientAuthProperty(), System.getProperty(x509Util.getSslClientAuthProperty()));
	properties.put(x509Util.getSslHandshakeDetectionTimeoutMillisProperty(), System.getProperty(x509Util.getSslHandshakeDetectionTimeoutMillisProperty()));
	properties.put(x509Util.getFipsModeProperty(), System.getProperty(x509Util.getFipsModeProperty()));
	}

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

Are you sure you want to change the base?

ZOOKEEPER-4819 Fix can't seek for writable tls server if connected to readonly server #2200

Conversation

luoxiner commented Oct 10, 2024

kezhuw commented Oct 12, 2024

luoxiner commented Oct 13, 2024

kezhuw left a comment

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

luoxiner commented Oct 14, 2024

kezhuw left a comment

Choose a reason for hiding this comment

kezhuw Oct 15, 2024

Choose a reason for hiding this comment

kezhuw Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

luoxiner Oct 15, 2024

Choose a reason for hiding this comment

kezhuw Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

kezhuw Oct 15, 2024

Choose a reason for hiding this comment

luoxiner Oct 17, 2024

Choose a reason for hiding this comment

kezhuw Oct 17, 2024

Choose a reason for hiding this comment

kezhuw left a comment

Choose a reason for hiding this comment

kezhuw Oct 17, 2024

Choose a reason for hiding this comment

kezhuw commented Oct 23, 2024

kezhuw Oct 15, 2024 •

edited

Loading

kezhuw Oct 15, 2024 •

edited

Loading