You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@flume.apache.org by Joey Echeverria <jo...@cloudera.com> on 2013/03/29 13:44:38 UTC
Review Request: FLUME-997: Support secure transport mechanism
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/
-----------------------------------------------------------
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
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/#review21837
-----------------------------------------------------------
Ship it!
+1
Thanks for making the changes Joey and apologies for the long turnaround time. I'm about to commit this.
- Mike Percy
On April 30, 2013, 5:01 p.m., Joey Echeverria wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10190/
> -----------------------------------------------------------
>
> (Updated April 30, 2013, 5:01 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 38f2205
> 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
>
>
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Joey Echeverria <jo...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/
-----------------------------------------------------------
(Updated April 30, 2013, 5:01 p.m.)
Review request for Flume and Mike Percy.
Changes
-------
I updated the patch based on the feedback from the previous review.
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 (updated)
-----
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 38f2205
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
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Joey Echeverria <jo...@cloudera.com>.
> On March 29, 2013, 2:58 p.m., Brock Noland wrote:
> > flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java, line 163
> > <https://reviews.apache.org/r/10190/diff/1/?file=276330#file276330line163>
> >
> > This should not block this item from being committed, but I think we should file a followup jira to ensure the configuration properties align with the file channel encryption: http://flume.apache.org/FlumeUserGuide.html#file-channel
> >
> > Additionally, it might make sense to refactor things like this class https://github.com/apache/flume/blob/trunk/flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/encryption/JCEFileKeyProvider.java so we can use that here.
> >
Unless anyone disagrees, I'll do this in a follow-up JIRA.
- Joey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/#review18517
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Brock Noland <br...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/#review18517
-----------------------------------------------------------
Hey Joey, looks great! I don't have time to do the authoritative review of this, so I'll let someone else do that. I just had a comment about the key store stuff. Since the FC already does it might make sense to refactor that code to be used here as well. This could be handled in a followup jira.
flume-ng-core/src/main/java/org/apache/flume/source/AvroSource.java
<https://reviews.apache.org/r/10190/#comment38830>
This should not block this item from being committed, but I think we should file a followup jira to ensure the configuration properties align with the file channel encryption: http://flume.apache.org/FlumeUserGuide.html#file-channel
Additionally, it might make sense to refactor things like this class https://github.com/apache/flume/blob/trunk/flume-ng-channels/flume-file-channel/src/main/java/org/apache/flume/channel/file/encryption/JCEFileKeyProvider.java so we can use that here.
- Brock Noland
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
>
>
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Joey Echeverria <jo...@cloudera.com>.
> On April 15, 2013, 3:39 a.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java, line 346
> > <https://reviews.apache.org/r/10190/diff/1/?file=276332#file276332line346>
> >
> > This will add it first on the decode and last on the encode, right?
Yes, I found this documentation to be the most helpful on the encode/decode order, but I had to read all of it to fully grok what was going on:
https://docs.jboss.org/netty/3.2/api/org/jboss/netty/channel/ChannelPipeline.html
Should I add a comment to make it more clear?
> On April 15, 2013, 3:39 a.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java, line 357
> > <https://reviews.apache.org/r/10190/diff/1/?file=276332#file276332line357>
> >
> > How is this different than the Permissive Trust Manager? :)
It's not, but the only reason to make PermissiveTrustManager public is for testing. I'm happy to do that if you think it will be cleaner.
> On April 15, 2013, 3:39 a.m., Mike Percy wrote:
> > flume-ng-sdk/src/main/java/org/apache/flume/api/NettyAvroRpcClient.java, line 653
> > <https://reviews.apache.org/r/10190/diff/1/?file=276336#file276336line653>
> >
> > 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?
That's a good point. I think we want an option to not require two-way SSL when you care about encryption but aren't worried about trust. Perhaps a better default is to use the standard Java truststore if one isn't specified and add an explicit config for trusting all certs.
- Joey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/#review19169
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Joey Echeverria <jo...@cloudera.com>.
> On April 15, 2013, 3:39 a.m., Mike Percy wrote:
> > flume-ng-core/src/test/java/org/apache/flume/source/TestAvroSource.java, line 357
> > <https://reviews.apache.org/r/10190/diff/1/?file=276332#file276332line357>
> >
> > How is this different than the Permissive Trust Manager? :)
>
> Joey Echeverria wrote:
> It's not, but the only reason to make PermissiveTrustManager public is for testing. I'm happy to do that if you think it will be cleaner.
I renamed it to match, but I'm keeping the duplicated code since it shouldn't be public and the two implementations are in different packages.
- Joey
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/10190/#review19169
-----------------------------------------------------------
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
>
>
Re: Review Request: FLUME-997: Support secure transport mechanism
Posted by Mike Percy <mp...@apache.org>.
-----------------------------------------------------------
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
>
>