[ZOOKEEPER-3905] Race condition causes sessions to be created for clients even though their certificate authentication has failed

• Type: Bug
• Status: Closed
• Priority: Major
• Resolution: Fixed
• Affects Version/s: 3.5.8, 3.6.2
• Fix Version/s: 3.5.9, 3.7.0, 3.6.2
• Assignee: Andor Molnar
• Reporter: Karan Mehta
• Created: 03/Aug/20 17:46
• Updated: 10/Sep/20 10:43
• Resolved: 08/Aug/20 09:53

Description

To reproduce, apply the diff and run ClientSSLTest#testSecureStandaloneServer() test. The logs would show that a valid session was created before connection was rejected and client had to retry

 

diff --git a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
 index aa02145b2..df1bdcc0f 100644
 — a/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
 +++ b/zookeeper-server/src/main/java/org/apache/zookeeper/common/ZKTrustManager.java
 @@ -111,6 +111,7 @@ public void checkServerTrusted(X509Certificate[] chain, String authType, SSLEngi
 @Override
 public void checkClientTrusted(X509Certificate[] chain, String authType) throws CertificateException
{ x509ExtendedTrustManager.checkClientTrusted(chain, authType); + throw new CertificateException(); }
@Override

  
What should have happened:
Server should instantly close the client's connection and NOT process any request.
 
Potential threat:
Malicious clients may be able to put unnecessary load/traffic on the leader by creating these sessions.
 
Race Condition:
In CertificateVerifier#operationComplete(), `addCnxn(cnxn)` method is only called after auth is completed. NettyServerCnxn#close() returns as a no-op here.
 
I see this as an issue. Please assess the risk and let me know if this is a legit behavior or not.
 

Issue Links

• links to: GitHub