----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review84235 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java <https://reviews.apache.org/r/33620/#comment135394> On the server, there are actually three settings: - require client authentication/certificate (wherein the handshake will fail if the client doesn't authenticate) - want client authentication/certificate (wherein the server will request client authentication, but allow the handhsake go through if the client doesn't comply) - no client certificate/authentication requested (wherein the server won't even ask) Regrettably, the Java Secure Sockets Extensions represents this with two boolean parameters: `SSLEngine.setWantClientAuth` and `SSLEngine.setNeedClientAuth`. You seem to have adopted their "need client authentication" parameter, but not their "want client authentication" parameter; is this intentional? clients/src/main/java/org/apache/kafka/common/network/SSLChannelBuilder.java <https://reviews.apache.org/r/33620/#comment136154> More a question than an issue: do you want `SSLChannelBuilder` to always build a client channel? clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java <https://reviews.apache.org/r/33620/#comment136157> There's a subtle bug here, and I only know about it because we got burned by the same thing. Here it is: suppose `needClientAuth` is true, and `wantClientAuth` is false. In this case, the code as written will create an `SSLEngine` instance with no client auth desired. Crazy, I know. From the `SSLEngine` javadoc: "An engine's client authentication setting is one of the following: - client authentication required - client authentication requested - no client authentication desired ...Calling this method overrides any previous setting made by this method or setNeedClientAuth(boolean)." IOW, the interface to `SSLEngine` models a parameter with three possible settings using two booleans. This is a great example of why interface design is so important. Anyway, I'd suggest code like this: if (needClientAuth) { sslEngine.setNeedClientAuth(true); } else { sslEngine.setWantClientAuth(wantClientAuth); } This covers the four cases: ``` "wants" True False +-------------+-------------+ True | client auth | client auth | | required | required | "needs" +-------------+-------------+ False| client auth | no client | | requested | auth desired| +-------------+-------------+ ``` Check me, but I think that's what we want here. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136159> Two questions: - is this initialized somewhere? - AFAICT it is a copy of the interest operations in the key... so why does it exist? - clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136209> Does `appReadBuffer` need to be initialized, too? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136163> I'm a bit confused: does line 98 not duplicate the actions of line 96? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136158> Typo: than => then clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136229> Do you ever clear out `appReadBuffer`? As I read through the logic, it seems that you keep accumulating unwrapped messages therein during the handshake? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment136236> Do you want to call `compact` on the source buffer? clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java <https://reviews.apache.org/r/33620/#comment136240> I'm trying to imagine implementing `buildPrincipal` in such a way as to make a Principal from information found in the peer's certifcate: how could I obtain that? I don't see (immediately) a way to navigate from either the `TransportLayer` or the `Authenticator` to the current `SSLSession`... Wow... this has turned into a major piece of code, hasn't it? Thanks for your attention to my comments. I tried to walk through the logic imagining partial reads & partial writes, but I think only found a few corner cases. - Michael Herstine On May 21, 2015, 5:37 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 21, 2015, 5:37 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. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Addressing > reviews. > > > KAFKA-1690. new java producer needs ssl support as a client. Fixed minor > issues with the patch. > > > KAFKA-1690. new java producer needs ssl support as a client. Fixed minor > issues with the patch. > > > Diffs > ----- > > build.gradle cd2aa838fd53e8124f308979b1d70efe0c5725a6 > checkstyle/import-control.xml f2e6cec267e67ce8e261341e373718e14a8e8e03 > clients/src/main/java/org/apache/kafka/clients/ClientUtils.java > 0d68bf1e1e90fe9d5d4397ddf817b9a9af8d9f7a > 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/config/SecurityConfigs.java > PRE-CREATION > 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/ChannelBuilder.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/PlainTextChannelBuilder.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/SSLChannelBuilder.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 > >
