Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow insecure HTTPS/TLS connection for HttpWaitStrategy #4951

Merged
merged 2 commits into from
May 16, 2022

Conversation

reta
Copy link
Contributor

@reta reta commented Jan 24, 2022

Some containers (fe OpenSearch, but there are many other examples) expose the connection endpoints over HTTPS protocol using self-signed certificates. The HttpWaitStrategy uses the default HttpsURLConnection / SSLSocketFactory which perform certificate validation and reject connections to such HTTPS endpoints. It would be great to provide the option to support HTTPS connection to servers that use self-signed certificates (basically, trust all certificate chains, no validation).

To be noted, in case of HttpsURLConnection, there is a way to disable certificates validation on per-JVM level (using HttpsURLConnection::setDefaultSSLSocketFactory) but it would affect every single HTTPS connection out there which is not desirable.

This feature nicely complements testcontainers/testcontainers-go#45

@kiview would love to hear your opinion, it is not directly related to #4782 but popped up while implementing the integration, thank you.

@kiview
Copy link
Member

kiview commented Jan 24, 2022

Hey @reta, thanks for raising this PR, I can definitely see the need for this in certain scenarios. I also agree, setting this at the JVM level is generally a nogo, so it is very desirable to avoid it.

To understand your use case with regards to OpenSearch better, does it provide no way to run via HTTP? And how would you interact with it after container startup, if it uses HTTPS with self-signed certificates?

@reta
Copy link
Contributor Author

reta commented Jan 24, 2022

Hey @reta, thanks for raising this PR, I can definitely see the need for this in certain scenarios. I also agree, setting this at the JVM level is generally a nogo, so it is very desirable to avoid it.

👍 totally agree, thank you

To understand your use case with regards to OpenSearch better, does it provide no way to run via HTTP?

As of now, the default images do HTTPS only but there are discussions on the subject, opensearch-project/OpenSearch#1598 (in case your are interested)

And how would you interact with it after container startup, if it uses HTTPS with self-signed certificates?

To generalize, there are 2 ways right now: using HTTP(S) or TCP (transport level), for my particular case I am intended to use OpenSearch-provided Rest Client, but others do you TCP transport (in this case HTTP/HTTPS endpoints are useful for health / liveness checks).

Thank you!

@kiview
Copy link
Member

kiview commented Jan 24, 2022

So in case of using their provided REST client, can you configure it to allow insecure HTTPS as well? Just to understand if this helps this use case sufficiently, or if you just get stuck at another point (which would mean allowing HTTP upstream in the image could be the better approach).

@reta
Copy link
Contributor Author

reta commented Jan 24, 2022

So in case of using their provided REST client, can you configure it to allow insecure HTTPS as well? Just to understand if this helps this use case sufficiently, or if you just get stuck at another point (which would mean allowing HTTP upstream in the image could be the better approach).

Oh, got it, sorry, OpenSearch Rest Client use CloseableHttpAsyncClient (httpasyncclient) under the hood and it allows to configure SSL context with own trust managers / host verifiers, thanks!

@reta
Copy link
Contributor Author

reta commented Feb 2, 2022

Hey @kiview , my apologies, absolutely no pressure, just wondering if anything is holding this change back from being integrated? Thank you!

@petedmarsh
Copy link

I have a similar use case and I would love for this feature to be merged.

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My apologies for not responding earlier to the PR. Can we please add a (integration) test for the new functionality? Ideally using a minimal Docker image.

@kiview kiview added this to the next milestone May 15, 2022
reta added 2 commits May 16, 2022 10:24
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
…-signed certs

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@reta reta requested a review from a team as a code owner May 16, 2022 15:16
@reta
Copy link
Contributor Author

reta commented May 16, 2022

My apologies for not responding earlier to the PR. Can we please add a (integration) test for the new functionality? Ideally using a minimal Docker image.

Thanks @kiview , the integration test has been added into existing Elasticsearch test suite (cuz it is also applicable there), please take a look at [1]

[1] https://github.com/testcontainers/testcontainers-java/pull/4951/files#diff-a6f3081bf09c2a39c5a6337b2d9754d7d51ed63f6ce655ad0661832163d0926aR288

Copy link
Member

@kiview kiview left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM 🙂

@eddumelendez eddumelendez merged commit c0183f1 into testcontainers:master May 16, 2022
@eddumelendez
Copy link
Member

Thanks for your contribution @reta ! this is now in master branch and will be part of the next version

@reta
Copy link
Contributor Author

reta commented May 16, 2022

Thanks for your contribution @reta ! this is now in master branch and will be part of the next version

Thanks a mill, @eddumelendez and @kiview !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants