> On May 13, 2015, 3:40 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java, > > lines 61-112 > > <https://reviews.apache.org/r/33620/diff/4/?file=955770#file955770line61> > > > > Those properties will be used by broker config as well, right? Should > > we put them in a separate SslConfig so that it can be reused in both client > > and server?
Thanks lot for the review Jun. In my previous patches I kept these as part of SecurityConfig file and producer, consumer refer to this as a file. Joel thought it would be better if they are part of singel config file like producer.properties or consumer.properties. > On May 13, 2015, 3:40 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java, > > line 98 > > <https://reviews.apache.org/r/33620/diff/5/?file=957068#file957068line98> > > > > This seems to be ssl specific. Is this needed in the interface? It > > doesn't seem to be used. This is added to expose SSLSession for the principal builder, as there are users(linkedin) who wants to read peer's X509Certificate. I am throwing UnsupporterOperation for plaintext and returns a SSLSession if the transportLayer happens to be SSLTransportLayer. > On May 13, 2015, 3:40 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java, > > line 321 > > <https://reviews.apache.org/r/33620/diff/5/?file=957065#file957065line321> > > > > So, we want to support rehandshake? If so, shouldn't we handle other > > handshakeStatus too? Could we reuse the logic in handshake()? In general, > > should we deal with the handshakeStatus before the status or the reverse? I think we don't need support rehandshake. SSL renegotiation or rehandshake helpful if you want to present the identity after the ssl session established or you want to change the encryption keys. In kafka broker case a channel is handshaked and session established and we start to exchange application level data (kafka protocols). In this case rehandshake won't come into play i.e clients once the ssl established begins to transfer data and I don't see a place where client or broker without disconnecting want to send ssl handshake data. > On May 13, 2015, 3:40 p.m., Jun Rao wrote: > > clients/src/main/java/org/apache/kafka/common/network/Selector.java, lines > > 286-291 > > <https://reviews.apache.org/r/33620/diff/5/?file=957067#file957067line286> > > > > I am a bit confused how this works during the handshake. During the > > handshake, we will need to read and write data from the socket. However, > > the read/write part will be routed to connect() and then > > SSLTransportLayer.handshake(), not SSLTransportLayer.read/write. So how > > would the handshake complete? Channel.connect is the new method I added this is not similar to SocketChannel.connect. Inside Channel.connect we go through transportLayer.Handshake. Incase of SSLTransportLayer handshake it will go through read/write from underlying socketChannel to complete the handshake. Once the handshake is done every read/write will go through transportLayer.read/write. - Sriharsha ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83322 ----------------------------------------------------------- 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 > >
