Details
Description
As pointed out on the mailing list [1], the GetHTTP (and likely PutHTTP) processors use a hard-coded TLS protocol version. PostHTTP also did this and was fixed by NIFI-1688.
The same fix should apply here and unit tests already exist which can be applied to the other processors as well.
For future notice, InvokeHTTP is a better processor for generic HTTP operations and has supported reading the TLS protocol version from the SSLContextService for some time.
Attachments
Issue Links
Activity
Hi alopresto, would like to see this in the 0.x branch. I was about to pick this ticket up when I saw you changed it to In Progress.
Now that 1.0.0 is released I'm not sure what the official policy is, but I think this is small enough and I don't mind backporting it to 0.x.
The official policy is that of the Apache Way. The result of NiFi community discussion on this topic has been documented here https://cwiki.apache.org/confluence/display/NIFI/Git+Branching+and+Release+Line+Management
This is not a change that requires backporting but all changes are eligible for backporting so as long as there is interest and willingness then good to go.
I'll be happy to do the work to cherry-pick whatever alopresto commits to the master branch. I believe this is an important security related change for 0.x.
GitHub user alopresto opened a pull request:
https://github.com/apache/nifi/pull/999
NIFI-2266 Enabled TLSv1.1 and TLSv1.2 protocols for GetHTTP processor
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/alopresto/nifi NIFI-2266
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/nifi/pull/999.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 #999
commit e35495ef1f5fbd1fa3b69c959e88ebef3afda46f
Author: Andy LoPresto <alopresto@apache.org>
Date: 2016-09-10T02:00:49Z
NIFI-2266 Refactored GetHTTP processor to use SSLContext protocol vs. hard-coded TLSv1.
Added unit tests.
Added test resources.
commit afe79031b10aa71dacd1ef45225ca7a21ca19774
Author: Andy LoPresto <alopresto@apache.org>
Date: 2016-09-10T03:08:16Z
NIFI-2266 Converted test handler to return 200 on GET request.
Fixed test assertions for HTTP responses and queued flowfiles.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/999
Hey @alopresto,
I have this unit test failing:
````
Tests run: 7, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.999 sec <<< FAILURE! - in org.apache.nifi.processors.standard.TestGetHTTPGroovy
testGetHTTPShouldConnectToServerWithTLSv1(org.apache.nifi.processors.standard.TestGetHTTPGroovy) Time elapsed: 0.094 sec <<< FAILURE!
java.lang.AssertionError: expected:<1> but was:<2>
at org.junit.Assert.fail(Assert.java:88)
at org.junit.Assert.failNotEquals(Assert.java:834)
at org.junit.Assert.assertEquals(Assert.java:645)
at org.junit.Assert.assertEquals(Assert.java:631)
at org.apache.nifi.util.StandardProcessorTestRunner.assertTransferCount(StandardProcessorTestRunner.java:318)
at org.apache.nifi.util.StandardProcessorTestRunner.assertAllFlowFilesTransferred(StandardProcessorTestRunner.java:313)
at org.apache.nifi.util.TestRunner$assertAllFlowFilesTransferred$5.call(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:133)
at org.apache.nifi.processors.standard.TestGetHTTPGroovy$_testGetHTTPShouldConnectToServerWithTLSv1_closure7.doCall(TestGetHTTPGroovy.groovy:331)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:294)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1019)
at groovy.lang.Closure.call(Closure.java:426)
at groovy.lang.Closure.call(Closure.java:442)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2030)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2015)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2056)
at org.codehaus.groovy.runtime.dgm$162.invoke(Unknown Source)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoMetaMethodSiteNoUnwrapNoCoerce.invoke(PojoMetaMethodSite.java:274)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:56)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
at org.apache.nifi.processors.standard.TestGetHTTPGroovy.testGetHTTPShouldConnectToServerWithTLSv1(TestGetHTTPGroovy.groovy:324)
````
And the logs I have when only running this test in Eclipse:
````
[main] INFO org.eclipse.jetty.util.log - Logging initialized @1147ms
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - Created server with supported protocols: [TLSv1, TLSv1.1, TLSv1.2]
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - JCE unlimited strength installed: false
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - Supported client cipher suites: [...]
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - Created server with supported protocols: [TLSv1]
[main] INFO org.eclipse.jetty.server.Server - jetty-9.3.9.v20160517
[main] INFO org.eclipse.jetty.server.handler.ContextHandler - Started o.e.j.s.ServletContextHandler@1a914089
[main] INFO org.eclipse.jetty.util.ssl.SslContextFactory - x509=X509@2b999ee8(localhost,h=[],w=[]) for SslContextFactory@31ab1e67(file:///.../nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/localhost-ks.jks,null)
[main] INFO org.eclipse.jetty.util.ssl.SslContextFactory - x509=X509@29bbc391(mykey,h=[],w=[]) for SslContextFactory@31ab1e67(file:///.../nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/localhost-ks.jks,null)
[main] INFO org.eclipse.jetty.server.AbstractConnector - Started ServerConnector@5bb8e6fc
[main] INFO org.eclipse.jetty.server.Server - Started @2219ms
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - Set context service protocol to TLSv1
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - GetHTTP supported protocols: TLSv1
[main] INFO org.apache.nifi.processors.standard.TestGetHTTPGroovy - GetHTTP supported cipher suites: [...]
[pool-1-thread-1] WARN org.apache.nifi.processors.standard.GetHTTP - GetHTTP[id=7352e3c6-dd19-4954-bcf6-6b25a8870641] found FlowFile FlowFile[0,2652169680397441.mockFlowFile,22B] in input queue; transferring to success
[qtp1784053627-18] INFO / - Groovy servlet initialized on groovy.util.GroovyScriptEngine@6d2347a2.
[pool-1-thread-1] INFO org.apache.nifi.processors.standard.GetHTTP - GetHTTP[id=7352e3c6-dd19-4954-bcf6-6b25a8870641] Successfully received FlowFile[1,mockFlowfile_1473713477596,0B] from https://localhost:8456/GetHandler.groovy at a rate of 0 bytes/sec; transferred to success
[main] INFO org.eclipse.jetty.server.AbstractConnector - Stopped ServerConnector@5bb8e6fc{SSL,[ssl, http/1.1]} {localhost:8456}
[main] INFO org.eclipse.jetty.server.handler.ContextHandler - Stopped o.e.j.s.ServletContextHandler@1a914089
{/,file:///.../nifi/nifi-nar-bundles/nifi-standard-bundle/nifi-standard-processors/src/test/resources/TestGetHTTP/,UNAVAILABLE}````
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/999
I had the same unit test failure as @pvillard31. Once I resolved that locally, I confirmed that this change works against the URL listed in NIFI-2373. contrib-check passes.
This also cherry-picks cleanly into the 0.x branch, but the testDefaultShouldPreferTLSv1_2() method of TestGetHTTPGroovy.groovy doesn't work with Java 7. It would need the same mods as the corresponding method in TestPostHTTPGroovy.groovy.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/999
@pvillard31 and @mosermw , thanks for noting that. When I re-ran locally, I encountered the same issue. I have resolved it, but not to my complete satisfaction. The test now accepts that n flowfiles were routed to `REL_SUCCESS` instead of 1 explicitly. As the tests are simply concerned with the TLS protocol version used for the communication, this seems a sufficient fix. However, I am confused, because it appears from the logs and debugging that the test runner "finds" a mock flowfile in the queue even if I assert immediately before enqueuing any message that the queue is empty.
I'll continue investigating, but I did want to push this fix so at least the tests pass for anyone reviewing it.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/999
And, of course, as soon as I comment, it hits me – `GetHTTP`, unlike `PostHTTP`, does not require a flowfile input in order to execute. So when I copied the tests I had written for `PostHTTP`, I needed to either remove the `enqueue` action of the message content prior to running, or assert that 2 flowfiles succeeded (the one manually enqueued and the result of the `GET` call). I think the current code is correct and successful, but if desired, I can push my latest changes which remove the manual enqueuing and assert explicitly 1 flowfile succeeded.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/999
@alopresto your last changes are fine to me but it's still not working on my side. If I run the unit test individually it works fine, but when running maven build or the whole tests suite of the class in Eclipse, it is not working:
````
Tests run: 7, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.868 sec <<< FAILURE! - in org.apache.nifi.processors.standard.TestGetHTTPGroovy
testGetHTTPShouldConnectToServerWithTLSv1(org.apache.nifi.processors.standard.TestGetHTTPGroovy) Time elapsed: 0.019 sec <<< FAILURE!
java.lang.AssertionError: null
at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertTrue(Assert.java:52)
at org.apache.nifi.util.StandardProcessorTestRunner.assertQueueEmpty(StandardProcessorTestRunner.java:348)
at org.apache.nifi.util.TestRunner$assertQueueEmpty$7.call(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:117)
at org.apache.nifi.processors.standard.TestGetHTTPGroovy$_testGetHTTPShouldConnectToServerWithTLSv1_closure7.doCall(TestGetHTTPGroovy.groovy:327)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.codehaus.groovy.reflection.CachedMethod.invoke(CachedMethod.java:93)
at groovy.lang.MetaMethod.doMethodInvoke(MetaMethod.java:325)
at org.codehaus.groovy.runtime.metaclass.ClosureMetaClass.invokeMethod(ClosureMetaClass.java:294)
at groovy.lang.MetaClassImpl.invokeMethod(MetaClassImpl.java:1019)
at groovy.lang.Closure.call(Closure.java:426)
at groovy.lang.Closure.call(Closure.java:442)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2030)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2015)
at org.codehaus.groovy.runtime.DefaultGroovyMethods.each(DefaultGroovyMethods.java:2056)
at org.codehaus.groovy.runtime.dgm$162.invoke(Unknown Source)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite$PojoMetaMethodSiteNoUnwrapNoCoerce.invoke(PojoMetaMethodSite.java:274)
at org.codehaus.groovy.runtime.callsite.PojoMetaMethodSite.call(PojoMetaMethodSite.java:56)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCall(CallSiteArray.java:48)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:113)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.call(AbstractCallSite.java:125)
at org.apache.nifi.processors.standard.TestGetHTTPGroovy.testGetHTTPShouldConnectToServerWithTLSv1(TestGetHTTPGroovy.groovy:324)
````
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/999
@pvillard31 I really appreciate your persistence with this one. Not sure why I missed that the last time.
I added a method to clear the flowfile queue in the test runner, because it seems that multiple test cases are causing side-effects. Each should now be completely independent.
(From the latest commit message
```
All tests pass (in test suite when running individually or as a suite, and when running entire project via Maven).
Will raise additional Jira for more full-featured addition of clearQueue mechanics to StandardProcessorTestRunner and related interfaces.```
Please verify on your end.
Commit 639e6d6a76ba92dbefd259861e2efc2ba2247f43 in nifi's branch refs/heads/0.x from alopresto
[ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=639e6d6 ]
NIFI-2266 Refactored GetHTTP processor to use SSLContext protocol vs. hard-coded TLSv1.
This closes #999.
Commit 022f5a506dfb961dce955a2341544de184327b5e in nifi's branch refs/heads/master from alopresto
[ https://git-wip-us.apache.org/repos/asf?p=nifi.git;h=022f5a5 ]
NIFI-2266 Refactored GetHTTP processor to use SSLContext protocol vs. hard-coded TLSv1.
This closes #999.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/999
Thanks @alopresto, everything is OK! I merged it to master. I also followed recommendations from @mosermw to cherry-pick into 0.x with Java 7 compatibility.
Github user alopresto commented on the issue:
https://github.com/apache/nifi/pull/999
Thanks @pvillard31 . Be careful with `StandardProcessorTestRunner` – I think my IDE cleaned up the `<String,String>` typing on line 392 which will cause an issue with Java 7 compatibility.
Github user pvillard31 commented on the issue:
https://github.com/apache/nifi/pull/999
Yes @alopresto, I noticed it, hopefully I've done all the required modifications.
Github user mosermw commented on the issue:
https://github.com/apache/nifi/pull/999
I confirmed that this builds fine on the 0.x branch with Java 7. Thanks @pvillard31 and @alopresto
Removed from 1.0 as I am working on
NIFI-1831andNIFI-1465.