zookeeper-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From anmolnar <...@git.apache.org>
Subject [GitHub] zookeeper issue #679: ZOOKEEPER-3172: Quorum TLS - fix port unification to a...
Date Fri, 23 Nov 2018 11:38:59 GMT
Github user anmolnar commented on the issue:

    https://github.com/apache/zookeeper/pull/679
  
    @ivmaykov I did another cycle of review the unit tests, sorry I still not see value in
denial-of-service tests, but maybe I don't see something important. Let's put it this way:
    
    Use case of `UnifiedServerSocket` is the following:
    > Extend standard Socket class to catch all read events in order to trigger TLS/Plaintext
mode detection in a lazy fashion (catching the last chance of doing so).
    
    In order to do that it captures calls to the underlying input/output streams and initiates
detection of channel type on the first read/write operation. So far so good.
    
    The important bit here is that `UnifiedServerSocket` **doesn't contain anything related
to threading**. As a consequence if we write purely unit tests for this class we won't have
to verify anything which is related to threading.
    
    DOS test comment says:
    > This test makes sure that UnifiedServerSocket used properly (a single thread accept()-ing
connections and handing the resulting sockets to other threads for processing) is not vulnerable
to a simple denial-of-service attack in which a client connects and never writes any bytes.
**This should not block the accepting thread, since the read to determine if the client is
sending a TLS handshake or not happens in the processing thread.**
    
    And here's the thing: this test is testing the proper implementation of the server (e.g.
dealing with threads in the right way), which is implemented in the test itself: `UnifiedServerThread`.
    
    My 2 cents here is that if you think you haven't covered a user scenario or an edge with
the existing tests, you need to rewrite these tests to be more strict and test that-and-only-that
particular case which is missing.
    
    I think the coverage is acceptable (what you mentioned in your latest comment has already
been covered), but there's no harm in adding more. I just don't want tests which don't add
value **or** adds value in an unreasonably cumbersome way.
    
    Please correct me if I'm wrong.
    
    
    



---

Mime
View raw message