----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/10190/#review19169 -----------------------------------------------------------
Hi Joey, Thanks very much for your contribution! Apologies for taking so long to review it, I didn't have a lot of background on Java-based SSL so I needed to set aside a decent chunk of time to read up on stuff like http://docs.oracle.com/javase/6/docs/technotes/guides/security/jsse/JSSERefGuide.html to understand the possible security implications of various usages of the APIs. I already posted this on the JIRA but the jks truststore and p12 keystore didn't come through on the patch. If you can, please attach those either separately or included in the patch as a binary diff. Besides that, I have a few other questions / comments, please see below: flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java <https://reviews.apache.org/r/10190/#comment39745> Why are we looping 100x? Maybe we should sleep for a few milliseconds between attempts and try fewer times? flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java <https://reviews.apache.org/r/10190/#comment39747> This will add it first on the decode and last on the encode, right? flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java <https://reviews.apache.org/r/10190/#comment39749> How is this different than the Permissive Trust Manager? :) flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java <https://reviews.apache.org/r/10190/#comment39741> I believe this means we do not attempt to verify trust based on a CA or anything else. Why not? What are your thoughts on deploying this in a production environment? - Mike Percy On March 29, 2013, 12:44 p.m., Joey Echeverria wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/10190/ > ----------------------------------------------------------- > > (Updated March 29, 2013, 12:44 p.m.) > > > Review request for Flume and Mike Percy. > > > Description > ------- > > The patch adds support for SSL to AvroSource and AvroSink. The implementation > compliments the recent addition of compression in FLUME-1915. > > > This addresses bug FLUME-997. > https://issues.apache.org/jira/browse/FLUME-997 > > > Diffs > ----- > > flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java 517d545 > flume-ng-core/src/test/java/org/apache/flume/sink/TestAvroSink.java ac47ee9 > flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java > c699241 > flume-ng-core/src/test/resources/server.p12 PRE-CREATION > flume-ng-core/src/test/resources/truststore.jks PRE-CREATION > flume-ng-doc/sphinx/FlumeUserGuide.rst 600a360 > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java > 8285129 > > flume-ng-sdk/src/main/java/org/apache/flume/api/RpcClientConfigurationConstants.java > 34d73a3 > > Diff: https://reviews.apache.org/r/10190/diff/ > > > Testing > ------- > > There are tests for having SSL enabled on both the client and server with > specific tests using a truststore to verify the server certificate. There's > also a test to make sure you can enable both SSL and compression. > > I probably need to add some negative tests: > > 1) SSL server, non-SSL client > 2) SSL server, SSL client with a truststore that doesn't include the server > certificate > > > Thanks, > > Joey Echeverria > >
