You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@nifi.apache.org by pvillard31 <gi...@git.apache.org> on 2016/02/17 16:33:45 UTC
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
GitHub user pvillard31 opened a pull request:
https://github.com/apache/nifi/pull/232
NIFI-1521 Allows use of SSL in AMQP Processor
Allows use of SSL in AMQP Processor
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/pvillard31/nifi NIFI-1521
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/232.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #232
----
commit 4ae03bb7ed6b2739351bcda58528d16b497821ff
Author: Pierre Villard <pi...@gmail.com>
Date: 2016-02-16T10:02:10Z
Merge remote-tracking branch 'refs/remotes/apache/master'
commit e3c6b5741b22d22a4c9a3476c843be5390171353
Author: Pierre Villard <pi...@gmail.com>
Date: 2016-02-17T10:39:01Z
Merge remote-tracking branch 'refs/remotes/apache/master'
commit 241c11a914dcea2a01f7e74732e40e511685122c
Author: Pierre Villard <pi...@gmail.com>
Date: 2016-02-17T15:30:09Z
NIFI-1521 Allows use of SSL in AMQP Processor
Allows use of SSL in AMQP Processor
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-194834756
I just re-based the PR. Let me know if something else is needed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/232#discussion_r53195449
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -84,6 +89,72 @@
.allowableValues("0.9.1")
.defaultValue("0.9.1")
.build();
+ public static final PropertyDescriptor USE_SSL_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("Use SSL protocol")
+ .description("Indicates whether or not SSL protocol should be used")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .required(true)
+ .build();
+ public static final PropertyDescriptor VALIDATE_CERTIFICATES = new PropertyDescriptor.Builder()
--- End diff --
@alopresto In fact I added this possibility because this is a use case described in [the RabbitMQ documentation](https://www.rabbitmq.com/ssl.html), and I assumed this is a possible scenario. Regarding the default value, I definitely agree with you, my bad.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-212772606
@pvillard31 I squashed and merged; also backported the changes to 0.x (and I think I got it right this time). Thanks for the contributions.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185265232
Pierre, first of all, huge thanks for the contribution! Quite an important feature especially for enterprise users. Quick question though. One of the initiatives we are going through at the moment is to improve test coverage of our components. I know it's not trivial to the infrastructure tests for things like SSL, but I still wanted to ask if you thought about it and if any type of tests you ran locally could be migrated into unit tests even if mocks has to be used. I can also help with that as well as I am a bit paranoid when it comes to unit testing ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-188803549
I'll see if I can mock the existing Test stubs later on to see if we can have unit tests for it, but given the fact that SSL ControllerService is tested separately I vote +1.
Thank you!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by bbende <gi...@git.apache.org>.
Github user bbende commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185322119
Have we considered using the SSLContextService here rather than specifying all of the keystore/truststore properties in the processor?
Usually the processor will have logic that says "if no SSLContextService was selected then we are doing regular communication, if one is selected then secure communication". This way you can avoid another property to specify if SSL/TLS should be used.
An example of using this service is in InvokeHTTP...
Defining the property:
https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java#L196
Using the service:
https://github.com/apache/nifi/blob/master/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/main/java/org/apache/nifi/processors/standard/InvokeHTTP.java#L479
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185648631
I think you need to have this entry in the nifi-amqp-nar too so that the proper classloading will happen
` <dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-standard-services-api-nar</artifactId>
<type>nar</type>
</dependency>
`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-212008859
@olegz PR updated regarding name/displayName and I also aligned the code to have travis build passing. Let me know if you want me to squash commits.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185598549
Should I consider a modification on the SSL context to let the user choose the client authentication policy?
From what I see in the InvokeHTTP, this is NONE by default:
```java
sslService.createSSLContext(ClientAuth.NONE);
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185708765
@joewitt Thanks! Should be OK now.
I definitely need to set-up a dedicated eclipse for NiFi, the one I am using has auto-formatting rules that conflict with what is expected. Is there an already packaged eclipse with the rules applied on this project?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/232#discussion_r53186223
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -84,6 +89,72 @@
.allowableValues("0.9.1")
.defaultValue("0.9.1")
.build();
+ public static final PropertyDescriptor USE_SSL_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("Use SSL protocol")
+ .description("Indicates whether or not SSL protocol should be used")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .required(true)
+ .build();
+ public static final PropertyDescriptor VALIDATE_CERTIFICATES = new PropertyDescriptor.Builder()
--- End diff --
I am not sure why we would allow certificates _not_ be validated on a secure connection. I recommend removing this option, and I strongly oppose to the default value being `false` if it is allowed to remain.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by alopresto <gi...@git.apache.org>.
Github user alopresto commented on a diff in the pull request:
https://github.com/apache/nifi/pull/232#discussion_r53211145
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -84,6 +89,72 @@
.allowableValues("0.9.1")
.defaultValue("0.9.1")
.build();
+ public static final PropertyDescriptor USE_SSL_PROTOCOL = new PropertyDescriptor.Builder()
+ .name("Use SSL protocol")
+ .description("Indicates whether or not SSL protocol should be used")
+ .allowableValues("true", "false")
+ .defaultValue("false")
+ .required(true)
+ .build();
+ public static final PropertyDescriptor VALIDATE_CERTIFICATES = new PropertyDescriptor.Builder()
--- End diff --
Thanks @pvillard31 . I read the linked documentation, and what they are describing is if the trust chain (the cryptographic link between the presented certificate identifying a resource and a trusted certificate of a known entity proving the relationship) should be verified, specifically for _client certificates_.
> When a web browser connects to an HTTPS web server, the server presents its public certificate, the web browser attempts to establish a chain of trust between the root certificates the browser is aware of and the server's certificate, and all being well, an encrypted communication channel is established. Although not used normally by web browsers and web servers, SSL allows the server to ask the client to present a certificate. In this way the server can verify that the client is who they say they are.
> This policy of whether or not the server asks for a certificate from the client, and whether or not they demand that they are able to trust the certificate, is what the verify and fail_if_no_peer_cert arguments control. Through the {fail_if_no_peer_cert,false} option, we state that we're prepared to accept clients which don't have a certificate to send us, but through the {verify,verify_peer} option, we state that if the client does send us a certificate, we must be able to establish a chain of trust to it.
In this case, I am comfortable with not requiring the client to present a certificate, but I don't think we should provide the option to present an invalid/malformed certificate and successfully connect. If a client certificate is presented, we should require it to validate successfully against a certificate within the provided truststore.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on a diff in the pull request:
https://github.com/apache/nifi/pull/232#discussion_r58389967
--- Diff: nifi-nar-bundles/nifi-amqp-bundle/nifi-amqp-processors/src/main/java/org/apache/nifi/amqp/processors/AbstractAMQPProcessor.java ---
@@ -191,6 +201,14 @@ private Connection createConnection(ProcessContext context) {
if (vHost != null) {
cf.setVirtualHost(vHost);
}
+ // handles TLS/SSL aspects
+ final SSLContextService sslService = context.getProperty(SSL_CONTEXT_SERVICE).asControllerService(SSLContextService.class);
+ final SSLContext sslContext = sslService == null ? null : sslService.createSSLContext();
--- End diff --
Pierre
I was working on merging it but realized that the above no longer compiles. Could you take a look and update?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/nifi/pull/232
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185272014
Ok, let's collaborate. If you can simply describe what you needed to do on both client and server to have SSL, I can definitely mock it on the _TestConnection_ and _TestChannel_.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by joewitt <gi...@git.apache.org>.
Github user joewitt commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185323176
+1 to Bryan's point. We should definitely try to leverage the SSL Context. Oleg is this something you can work with Pierre on with the unit testing?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-205493328
@olegz Following comments on JIRA, I reverted modifications on SSL Context service and added a Client Auth property (as it has been done in Cassandra processors). I will check to externalize the Client Auth policy into the SSL Context service with the related JIRA NIFI-1652.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-211295813
@olegz I just added a unit test even if I don't think it is a vital one! Otherwise it's ready for final review.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by olegz <gi...@git.apache.org>.
Github user olegz commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-211088066
pvillard31 is this still a work in progress of is it ready for final review?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185326467
Yes, you're right... I didn't know about this feature. I will change the processor to use the SSL Context as suggested.
@olegz Regarding unit testing, I am not sure to picture what you want to achieve with a unit test. We could mock up the ConnectionFactory as well but for what purpose? I had a look at the [RabbitMQ unit tests regarding SSL](https://github.com/rabbitmq/rabbitmq-java-client/tree/master/src/test/java/com/rabbitmq/client/test/ssl) part and it seems to be relying on specific set-up/tools.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185642080
I added the use of SSL context and modified the pom file of nifi-amqp-processors to have:
```xml
<dependency>
<groupId>org.apache.nifi</groupId>
<artifactId>nifi-ssl-context-service-api</artifactId>
</dependency>
```
But I must be missing something because when launching NiFi:
```
java.util.ServiceConfigurationError: org.apache.nifi.processor.Processor: Provider org.apache.nifi.amqp.processors.PublishAMQP could not be instantiated
Caused by: java.lang.NoClassDefFoundError: org/apache/nifi/ssl/SSLContextService
Caused by: java.lang.ClassNotFoundException: org.apache.nifi.ssl.SSLContextService
```
Do I need to add something somewhere else to correctly handle this new dependency?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] nifi pull request: NIFI-1521 Allows use of SSL in AMQP Processor
Posted by pvillard31 <gi...@git.apache.org>.
Github user pvillard31 commented on the pull request:
https://github.com/apache/nifi/pull/232#issuecomment-185270244
Well.... I must admit I locally tested the change with a RabbitMQ server, but that's all. I had a look at the existing unit tests for AMQP processors but I am not sure how to mock a SSL context. If you have some hints I'll do my best to add unit tests ;)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---