Support for SSL Configuration within JerseyTest

[Hakky54]

This PR should resolve the following issue: #4572

On stackoverflow, https://stackoverflow.com/q/63924572/6777695 , someone has asked a question to configure the server with ssl and while helping the person I discovered this feature was missing within the JerseyTest. This PR should enable the developer to create a JerseyTest with a custom SSL/TLS configuration.

It can for example be enabled with the following method override:

// SSLContext sslContext;
// boolean clientMode;
// boolean needClientAuth;
// boolean wantClientAuth;

@Override
protected Optional<SSLEngineConfigurator> getSslEngineConfigurator() {
        return Optional.of(new SSLEngineConfigurator(
                    sslContext,
                    clientMode,
                    needClientAuth,
                    wantClientAuth
            ));
}

This pull request will close the following issue: #4572

[jansupol]

At first glance, the PR looks wrong. The added hard dependency on grizzly in the independent Jersey test module cannot be accepted; while grizzly might be the most common backend for testing, there are many who use other backend.

[Hakky54]

Thank you for your quick reply and feedback! The main reason why I added grizzly-framework is to make the SSLEngineConfigurator available within JerseyTest. The two backends GrizzlyTestContainerFactory and GrizzlyWebTestContainerFactory can use that object directly. But I wasn't aware that end-users also are able to use other backends and that would indeed make this option not usable for their needs and makes this PR indeed wrong...

What do you think of adding support for SSLContext within the DeploymentContext instead of the SSLEngineConfigurator. In that way the backend can have his own adapter to translate it. GrizzlyTestContainerFactory and GrizzlyWebTestContainerFactory can use the SSLContext and create an instance of SSLEngineConfigurator and supply it to the Grizzley Server and other backend will have their own implementation.

[Hakky54]

Hi @jansupol

I tried to resolve your concerns. I removed the hard dependency on grizzly and instead of supplying the ssl configuration with SSLEngineConfigurator it is now working with SSLContext and SSLParameters. The enduser needs to override two methods to enable the ssl configuration:

@Override
protected Optional<SSLContext> getSslContext() {
    SSLContext sslContext = ...; // initialized SSLContext
    return Optional.of(sslContext);
}

@Override
protected Optional<SSLParameters> getSslParameters() {
    SSLParameters sslParameters = new SSLParameters();
    sslParameters.setNeedClientAuth(true);
    return Optional.of(sslParameters);
}

Although not all properties are being picked up from the sslParameters while configuring for example grizzly it can if it will support additional configuration in the future. If such changes happens to support for example custom list of ciphers or protocols the JerseyTest doesn't needs to be adjusted as it can already hold these properties with SSLParameters object.

What do you think of this adjusted pull request, is it more according to what you would apply or do you think we should drop it for now?

[Hakky54]

@senivam or @jansupol I was wondering what the status of this pull request is. Would you like me to adjust something else or add something what you think is still missing?

[jansupol]

Sorry for the delay. We have been focused on other urgent tasks. This PR looks much better than the original one. Would you mind to update the copyright years to 2020? Thank you.

[Hakky54]

Yes, just added it. Let me know if something else needs to be done @jansupol

[jansupol]

Ooops, only the files you actually changed were supposed to have copyright year adjusted. Not all of them...

[Hakky54]

Aha, thats pity. Let me quickly revert it and only change those specific files

[Hakky54]

@jansupol fixed!

[jansupol]

Looks good, thank you

[Hakky54]

Hi @jbescos Would you also like to review this pull request, what is your opinion on it?

[jbescos]

Could you create one unit test where you can test this change?.

[Hakky54]

Hi @jbescos I added some unit tests to cover happy as well as unhappy flows. The tests passes, could you have another look at it?

I also added my own library as a test library to create a SSLContext for the server and client and the main reason was to easily create the ssl material without the need of introducing a-lot additional code/complexity into the code base. I was not quite sure if would be okay with that, so what do you think about it?

[jbescos]

@Hakky54 looks good to me, but I am not sure we can introduce third party libs without some sort of discussion.
As it is in test scope maybe it is not necessary, but honestly I don't know. I wanted to drop the attention of @jansupol and @senivam to check that.

[jansupol]

Every 3rd party dependency needs to go through a legal review. The legal reviews every file whether it contains the license header. Since this lib does not contain the license header in the source files, I am not sure whether the lib would be approved, the process might get complicated. Is it a big deal not to use the lib?

[Hakky54]

I didn't know that every 3rd party dependency would have to go through a legal review, but I can understand that would be time consuming. The library is not a must have for the unit test to be working, but a nice to have to prevent additional custom code into the code base. And I was very enthusiastic to add my own library into it 😄

I have removed the library and added a very minimalistic SslUtils to build the SSLContext. The unit test passes with the latest changes. Could you guys review it again? Anything what should be adjusted or done differently in your opinion?