----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83741 -----------------------------------------------------------
checkstyle/import-control.xml <https://reviews.apache.org/r/33620/#comment134792> Had a checkstyle failure because org.apache.kafka.common.config is dependent on org.apache.kafka.common.protocol (SecurityConfig imports SecurityProtocol) clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java <https://reviews.apache.org/r/33620/#comment134787> SunX509 is a JRE-specific algorithm. It would be better to use KeyManagerFactory.getDefaultAlgorithm() and TrustManagerFactory.getDefaultAlgorithm() as defaults so that the code works with any JRE. clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java <https://reviews.apache.org/r/33620/#comment134788> SunX509 is a JRE-specific algorithm. It would be better to use KeyManagerFactory.getDefaultAlgorithm() and TrustManagerFactory.getDefaultAlgorithm() as defaults so that the code works with any JRE. clients/src/main/java/org/apache/kafka/common/network/Authenticator.java <https://reviews.apache.org/r/33620/#comment134793> Don't think authenticator interface should handle selection interestOps. Needs better encapsulation. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134800> Handle BUFFER_OVERFLOW and CLOSED? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134801> Handle BUFFER_OVERFLOW and CLOSED? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134794> I think when delegated tasks are run asynchronously, selection interestOps should be managed within the transport layer. Otherwise, packets may be processed out of order. InterestOps should be set to zero before tasks are executed, and changes to interestOps due to packets being ready for transmission etc. should be cached and set after the delegated tasks are executed. Not sure if the current transport layer interface is sufficient to handle this correctly. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134799> Graceful close needs close handshake messages to be be sent/received to shutdown the SSL engine. See "Shutting Down" section in https://docs.oracle.com/javase/8/docs/technotes/guides/security/jsse/JSSERefGuide.html for an example of a blocking clean shutdown. A non-blocking version of that code is required here. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134798> The man-in-the-middle attack in CVE-2009-3555 has been fixed in Java for quite some time. But since there have been reported DOS vulnerabilities etc., it may be ok to disable re-negotiation for now. There are other scenarios (apart from access to protected resources) where re-negotiation may be useful, for instance to refresh encryption keys. While this may not be strictly necessary for the first version of SSL implementation, it will be good to ensure that the transport layer interfaces are sufficient to incorporate re-negotiation in future. Not sure if the current interface can support re-negotiation and it will be good to get the interfaces sorted now in any case. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment134802> Handle BUFFER_OVERFLOW and CLOSED? clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java <https://reviews.apache.org/r/33620/#comment134804> All these tests were hanging when I ran them from a Windows machine with IBM JRE. The tests did pass for me on Ubuntu with OpenJDK. Will try to figure out why the tests weren't working with IBM JRE. clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java <https://reviews.apache.org/r/33620/#comment134791> SelectorTest should be run with SSL as well as PlainText since all selector functions should be tested with SSL as the transport layer. Simplest way is to parameterize the test. https://reviews.apache.org/r/33133/diff/ has the code for parameterizing the test, which you may be able to reuse with some tweaks. Tried this and testMute() fails with SSL clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java <https://reviews.apache.org/r/33620/#comment134789> SunX509 is a JRE-specific algorithm. It would be better to use KeyManagerFactory.getDefaultAlgorithm(). clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java <https://reviews.apache.org/r/33620/#comment134790> SunX509 is a JRE-specific algorithm. It would be better to use TrustManagerFactory.getDefaultAlgorithm() as defaults so that the code works with any JRE. - Rajini Sivaram On May 12, 2015, 11:20 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 12, 2015, 11:20 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1690 > https://issues.apache.org/jira/browse/KAFKA-1690 > > > Repository: kafka > > > Description > ------- > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. > > > KAFKA-1690. new java producer needs ssl support as a client. SSLFactory tests. > > > KAFKA-1690. new java producer needs ssl support as a client. Added > PrincipalBuilder. > > > Diffs > ----- > > build.gradle fef515b3b2276b1f861e7cc2e33e74c3ce5e405b > checkstyle/checkstyle.xml a215ff36e9252879f1e0be5a86fef9a875bb8f38 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java > cf32e4e7c40738fe6d8adc36ae0cfad459ac5b0b > clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java > bdff518b732105823058e6182f445248b45dc388 > clients/src/main/java/org/apache/kafka/clients/consumer/KafkaConsumer.java > d301be4709f7b112e1f3a39f3c04cfa65f00fa60 > clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java > 8e336a3aa96c73f52beaeb56b931baf4b026cf21 > clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java > 187d0004c8c46b6664ddaffecc6166d4b47351e5 > clients/src/main/java/org/apache/kafka/common/config/AbstractConfig.java > c4fa058692f50abb4f47bd344119d805c60123f5 > clients/src/main/java/org/apache/kafka/common/network/Authenticator.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Channel.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/DefaultAuthenticator.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/network/Selectable.java > b5f8d83e89f9026dc0853e5f92c00b2d7f043e22 > clients/src/main/java/org/apache/kafka/common/network/Selector.java > 57de0585e5e9a53eb9dcd99cac1ab3eb2086a302 > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/protocol/SecurityProtocol.java > dab1a94dd29563688b6ecf4eeb0e180b06049d3f > > clients/src/main/java/org/apache/kafka/common/security/auth/DefaultPrincipalBuilder.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/KafkaPrincipal.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java > PRE-CREATION > clients/src/main/java/org/apache/kafka/common/utils/Utils.java > f73eedb030987f018d8446bb1dcd98d19fa97331 > clients/src/test/java/org/apache/kafka/common/network/EchoServer.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLFactoryTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SSLSelectorTest.java > PRE-CREATION > clients/src/test/java/org/apache/kafka/common/network/SelectorTest.java > d5b306b026e788b4e5479f3419805aa49ae889f3 > clients/src/test/java/org/apache/kafka/common/utils/UtilsTest.java > 2ebe3c21f611dc133a2dbb8c7dfb0845f8c21498 > clients/src/test/java/org/apache/kafka/test/TestSSLUtils.java PRE-CREATION > > Diff: https://reviews.apache.org/r/33620/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
