Skip to content

NIFI-6196 Upgrade version of Jetty#3426

Closed
jtstorck wants to merge 1 commit intoapache:masterfrom
jtstorck:NIFI-6196
Closed

NIFI-6196 Upgrade version of Jetty#3426
jtstorck wants to merge 1 commit intoapache:masterfrom
jtstorck:NIFI-6196

Conversation

@jtstorck
Copy link
Contributor

@jtstorck jtstorck commented Apr 10, 2019

This PR should be merged after #3416 to avoid merge conflicts due to regenerated certs in nifi-standard-processor.

While reviewing code, it is important to verify that server certs are not reused as client certs. This PR updates several tests that reused server certs, but more investigation is needed for other areas of code that reuse them.

To test this PR:

  • Specific components that use Jetty: ListenHTTP, HandleHttpRequest, InvokeHTTP, JettyWebSocketClient, JettyWebSocketServer, TlsCertificateAuthorityService

Each component should be tested in the following scenarios:

  • Verify an unsecured standalone instance of NiFi
  • Verify a secured anonymous standalone instance of NiFi
  • Verify a secured standalone instance of NiFi
  • Verify a secured cluster of NiFi

To test the TlsCertificateAuthorityService, certs for secured NiFi instances should be generated by a running NiFi CA service.

Please see https://issues.apache.org/jira/browse/NIFI-6196 for the list of issues encountered and their resolutions implemented in this PR.

NOTE: Please retain the flows used to verify this PR. They can be reused for verifying functionality when NiFi supports builds on Java 11.


Thank you for submitting a contribution to Apache NiFi.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically master)?

  • Is your initial contribution a single, squashed commit?

For code changes:

  • Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
  • Have you written or updated unit tests to verify your changes?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

@mcgilman
Copy link
Contributor

Happy to review once out of Draft status.

Updated NOTICE with current copyright year for Jetty dependencies
Updated Jetty SSLContextFactory usage, invoke setEndpointIdentificationAlgorithm(null) on server SslContextFactory instances
Updated TestInvokeHttpSSL to provide a separate client keystore, rather than reusing the server's keystore
Regenerated nifi-standard-processors keystore and truststore, added client keystore
Updated ITestHandleHttpRequest, TestInvokeHttpSSL, TestInvokeHttpTwoWaySSL, and TestListenHTTP to use a separate client keystore instead of reusing the server's keystore.  Also updated the tests to separately test one-way and two-way SSL
@jtstorck jtstorck marked this pull request as ready for review April 12, 2019 03:29
@jtstorck
Copy link
Contributor Author

@thenatog Could you review this PR, please?

@mcgilman
Copy link
Contributor

@jtstorck I am a +1 on this PR. Ran through a number of different functional tests including standalone (secured both two way and one way SSL and unsecured). Also verified functionality with several of our processors that utilize embedded Jetty instances.

Once @thenatog signs off, please merge.

@thenatog
Copy link
Contributor

thenatog commented Apr 30, 2019

I have verified the endpointIdentificationAlgorithm settings are at least checking for SANs using a clientSocket and serverSocket with certs that did and didn't contain SANs. For the moment, I recommend we set the clientSocket endpointIdenticationAlgorithm to null as well as this could be considered a breaking change that would require users to regenerate server certs for services external to NiFi. However, I think that it's generally accepted practice to expect SANs are set correctly for certificates. So, in future, we should flip this to require SAN validation in a major version change down the line.

Once you set the algorithm to null for the clientSocket, +1.

@thenatog thenatog closed this in 65c41ab May 9, 2019
@jtstorck jtstorck deleted the NIFI-6196 branch August 19, 2019 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants