You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sriharsha Chintalapani <ha...@hortonworks.com> on 2015/03/11 22:36:56 UTC
Review Request 31958: Patch for KAFKA-1684
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31958/
-----------------------------------------------------------
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
Re: Review Request 31958: Patch for KAFKA-1684
Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
> On March 16, 2015, 9:24 p.m., Michael Herstine wrote:
> > core/src/main/scala/kafka/network/SocketServer.scala, line 318
> > <https://reviews.apache.org/r/31958/diff/1/?file=891657#file891657line318>
> >
> > `{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);
> > }
Thanks for pointing it out I'll fix that.
- Sriharsha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31958/#review76640
-----------------------------------------------------------
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
>
>
Re: Review Request 31958: Patch for KAFKA-1684
Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
> On March 16, 2015, 9:24 p.m., Michael Herstine wrote:
> > core/src/main/scala/kafka/network/ssl/SSLChannel.scala, line 137
> > <https://reviews.apache.org/r/31958/diff/1/?file=891658#file891658line137>
> >
> > 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?
Thanks for the review. Ideally it should be fall through to NEEDS_UNWRAP since scala case statements doesn't allow java style follow-through I am looking at alternatives.
- Sriharsha
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31958/#review76640
-----------------------------------------------------------
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
>
>
Re: Review Request 31958: Patch for KAFKA-1684
Posted by Michael Herstine <mh...@linkedin.com>.
-----------------------------------------------------------
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
>
>