----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33620/#review83807 -----------------------------------------------------------
clients/src/main/java/org/apache/kafka/clients/CommonClientConfigs.java <https://reviews.apache.org/r/33620/#comment134861> "Need client auth" is generally a _server_-side setting. Should this appear in a file named CommonClientConfigs.java? If it should, are you not using "want client auth" as well as "require client auth"? clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java <https://reviews.apache.org/r/33620/#comment135025> Pursuant to my last comment-- what exactly does this config item do? In SSL, the server always presents a certificate; it is the _client_ cert which is optional. So I don't understand what a config item "client require cert" means. clients/src/main/java/org/apache/kafka/common/network/PlainTextTransportLayer.java <https://reviews.apache.org/r/33620/#comment135029> Just as a general aside: when you can't implement a method in a concrete subclass, and are reduced to throwing an exception to so indicate, you're violating the Liskov Substitution Principle-- the idea that a subclass can be used anywhere a superclass is. To put this another way, this is likely to surprise client code written in terms of interface `TransportLayer`. clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java <https://reviews.apache.org/r/33620/#comment135030> Thanks for making these configurable; I think a lot of sites will have strong opinions on what protocol versions & cipher suites they'll want to enable. clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java <https://reviews.apache.org/r/33620/#comment135031> This is only applicable on the server-side. clients/src/main/java/org/apache/kafka/common/network/SSLFactory.java <https://reviews.apache.org/r/33620/#comment135032> This is interesting; I don't see a corresponding `createSSLSocketFactory`-- did I miss it? Besides, I thought Kafka used NIO (in which case you wouldn't be using `SSLServerSocketFactory`, which IIRC is blocking. Who calls this? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135033> Do you want to make your nomenclature more uniform? The network buffers are referred to in terms of "in" & "out" while the application buffers are referred to as "read" & "write". On a related note, do you need read & write for each? At any given point in time, you're doing one or the other, so why not just have a "network" buffer & an "application" buffer? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135034> I'm confused on the difference between `startHandshake` & `handshake` (inherited from interface `TransportLayer`). clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135038> Do you want to set your buffers' limits to 0, here? Is not netOutBuffer.clear() netInBuffer.clear() what you really want (i.e. current position at 0, limit at capacity, indicating that the buffers are available for write up to their capacities) clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135039> Sorry, but I'm a little confused here. Why would the caller be passing `read` & `write`? I mean, you own the underlying socket, so you know that state it's in. And you also have the handshake status, so you know what you're expecting, right? Update: I've stepped through your logic, and it seems to me that you're figuring out at the end of each pass whether you want to read or write, returning that information to the caller, and asking the caller to pass it back in the next invocation of `handshake`. Why? Why not just record your state in a member variable? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135045> I don't know if this is right. Suppose we return from `handshakeWrap` with data still to be written from `netOutBuffer` to the socket. The status will still be NEEDS_UNWRAP, because as far as SSLEngine is concerned, it's wrapped everything & ready for input. In this case, you'll fall through without flushing everything from `netOutBuffer`. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135042> Did you remember to flip `appWriteBuffer` before calling `wrap`? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135046> So if `doRead` is false here, you won't have any data in `netInBuffer` when you do the unwrap. I suppose that you're depending on there being data left over in the buffer, but can you check that yourself instead of depending on `doRead`? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135041> I'm not familiar with Kafka's threading model, but I know throughput is very important to you. Do you want to be blocking execution on this thread while you carry out these tasks? clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135048> Is this right? IIRC, status `BUFFER_UNDERFLOW` signals that SSLEngine didn't have enough data to decrypt, and so there won't be anything in `appReadBuffer` (which I suppose is harmless). clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135023> This is likely to be unhelpful in many cases; according to the javadoc for class X500Principal, the name will be derived from the Subject Name in the peer certificate. Typically, the Subject Name is the hostname of the cert to permit peer validation when tools like curl or a browser are used to hit the endpoint (for debugging purposes, say). At LinkedIn, we use the Subject Alternative Names field to embed service identity. clients/src/main/java/org/apache/kafka/common/network/SSLTransportLayer.java <https://reviews.apache.org/r/33620/#comment135047> So you're transferring bytes from `appReadBuffer` to `dst`, right? Does not `ByteBuffer.put` do this for you? clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java <https://reviews.apache.org/r/33620/#comment135027> This is probably just my ignorance of the Kafka codebase, but what does this interface represent? It looks a lot like a socket to me... clients/src/main/java/org/apache/kafka/common/network/TransportLayer.java <https://reviews.apache.org/r/33620/#comment135026> Agree w/Jun. I'm just starting to figure out the code structure, but "peer principal" and "handshake" look, at first blush, like SSL- & SASL-specific ideas. clients/src/main/java/org/apache/kafka/common/security/auth/PrincipalBuilder.java <https://reviews.apache.org/r/33620/#comment135049> Will it be "pluggable"; i.e. can individual sites provide their own implementations? Thanks Srisharsha-- this is a big job, and non-blocking SSL is quite complex. - Michael Herstine On May 15, 2015, 2:18 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33620/ > ----------------------------------------------------------- > > (Updated May 15, 2015, 2:18 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. > > > 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/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 > >
