----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31958/#review76640 -----------------------------------------------------------
core/src/main/scala/kafka/network/SocketServer.scala <https://reviews.apache.org/r/31958/#comment124223> `{want,needs}ClientAuth` can be tricky-- check the javadoc for `SSLEngine.setWantClientAuth`... there are actually only three states: required, requested, not desired, and the last call to `{want,needs}ClientAuth` "wins". So, if "needs" is True and "wants" is false, invoking the methods in this order will actually overwrite the "needs" setting. Recommend something like: if (sslConnectionConfig.needClientAuth) { sslEngine.setNeedClientAuth(true); } else { sslEngine.setNeedClientAuth(false); sslEngine.setWantClientAuth(sslConnectionConfig.wantClientAuth); } core/src/main/scala/kafka/network/ssl/SSLChannel.scala <https://reviews.apache.org/r/31958/#comment124229> Suppose SSLEngine has written the current message (via `wrap`) to `netOutBuffer`, but that the write call in `flush`, when invoked from `handshakeWrap`, didn't write the entire buffer to the underlying socket. Would not `handshakeStatus` as reported from SSLEngine now be `NEEDS_UNWRAP`? And wouldn't that cause us to fall through to the `NEEDS_UNWRAP` case? Or do we not fall through in Scala case statements? core/src/main/scala/kafka/network/ssl/SSLChannel.scala <https://reviews.apache.org/r/31958/#comment124235> Not sure about this, but do we want to update the position & limit of the buffer? We flipped it after the last read, but I can't rememeber if SSLEngine.unwrap will update them if there's an incomplete packet (i.e. in the BUFFER_UNDERFLOW case). Just a few questions on some corner cases... handling all the possibilities when handshaking over NIO is really tough. - Michael Herstine On March 11, 2015, 9:36 p.m., Sriharsha Chintalapani wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/31958/ > ----------------------------------------------------------- > > (Updated March 11, 2015, 9:36 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1684 > https://issues.apache.org/jira/browse/KAFKA-1684 > > > Repository: kafka > > > Description > ------- > > KAFKA-1684. Implement TLS/SSL authentication. > > > Diffs > ----- > > core/src/main/scala/kafka/network/Channel.scala PRE-CREATION > core/src/main/scala/kafka/network/SocketServer.scala > 76ce41aed6e04ac5ba88395c4d5008aca17f9a73 > core/src/main/scala/kafka/network/ssl/SSLChannel.scala PRE-CREATION > core/src/main/scala/kafka/network/ssl/SSLConnectionConfig.scala > PRE-CREATION > core/src/main/scala/kafka/server/KafkaConfig.scala > 48e33626695ad8a28b0018362ac225f11df94973 > core/src/main/scala/kafka/server/KafkaServer.scala > dddef938fabae157ed8644536eb1a2f329fb42b7 > core/src/main/scala/kafka/utils/SSLAuthUtils.scala PRE-CREATION > core/src/test/scala/unit/kafka/network/SocketServerTest.scala > 0af23abf146d99e3d6cf31e5d6b95a9e63318ddb > core/src/test/scala/unit/kafka/server/KafkaConfigConfigDefTest.scala > c124c8df5b5079e5ffbd0c4ea359562a66aaf317 > core/src/test/scala/unit/kafka/utils/TestSSLUtils.scala PRE-CREATION > > Diff: https://reviews.apache.org/r/31958/diff/ > > > Testing > ------- > > > Thanks, > > Sriharsha Chintalapani > >
