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.

Sign up for GitHub

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

Jump to bottom

CALCITE-5671: Add Option to Disable SSL Certificate Validation to ElasticSearch Adapter #3174

Closed
wants to merge 2 commits into from
Closed

CALCITE-5671: Add Option to Disable SSL Certificate Validation to ElasticSearch Adapter #3174

wants to merge 2 commits into from

Conversation

cgivre
Copy link
Contributor

@cgivre cgivre commented Apr 23, 2023

For many development (and sometimes production) instances of ElasticSearch, the instance is configured using a self-signed SSL certificate or otherwise cannot be validated.

This PR adds a configuration option disableSSLVerification which, when set to true, disables SSL verification. The default behavior is not changed.

@cgivre
Copy link
Contributor Author

cgivre commented Apr 23, 2023

@jnturton FYSA

github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 23, 2023
View reviewed changes
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...earch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java Fixed Show resolved Hide resolved
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Apr 23, 2023
View reviewed changes
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Fixed Show fixed Hide fixed
@zinking
Copy link
Contributor

zinking commented Apr 24, 2023

looks good overall, could you actually try to come up with some test?

@tanclary
Copy link
Contributor

For future reference I believe you can run the gradle target "autostyleApply" to hopefully avoid having to manually fix some of the linting errors. Hope this helps!

@cgivre
Copy link
Contributor Author

cgivre commented Apr 24, 2023

For future reference I believe you can run the gradle target "autostyleApply" to hopefully avoid having to manually fix some of the linting errors. Hope this helps!

Thanks @tanclary Still getting used to Calcite and gradle.

@cgivre cgivre mentioned this pull request Apr 24, 2023
Merged
@cgivre
Copy link
Contributor Author

cgivre commented Apr 24, 2023

@libenchao @zinking
Thanks for your comments. I addressed your comments and added a unit test. I'm using this in Drill, so once this is merged, and DRILL-8385 is merged, there will be end-to-end tests for this capability.

@libenchao
Copy link
Member

I'm using this in Drill, so once this is merged, and apache/drill#2795 is merged, there will be end-to-end tests for this capability.

This sounds great! +1 for merging.

jnturton
jnturton reviewed Apr 25, 2023
View reviewed changes
...h/src/main/java/org/apache/calcite/adapter/elasticsearch/UnsafeX509ExtendedTrustManager.java Outdated
* used in production environments.
*/
@SuppressWarnings("java:S4830")
public final class UnsafeX509ExtendedTrustManager extends X509ExtendedTrustManager {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the Calcite code layout well but this trust manager is a generic utility that could easily be useful to other adapters so I'd personally look for a utils package to put it into.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking the same thing, but I wasn't sure where to put it. If anyone has any suggestions, I'm all ears.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cgivre there is an org.apache.calcite.util package in the core module that looks like it would work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jnturton Good call. Done!

Copy link
Member

@libenchao libenchao Apr 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't have a strong preference of this, currently it seems that only ES Adapter needs this, and I'm not sure if there will be more adapters that need this, so putting it in the util of core seems a little over-designed for me, we may could defer it to the next time that another adapter calls for it. (Please note that calcite is highly customizable, and calcite-core is widely used for many projects, it's not only used by calcite adapters)

Of course if somebody strongly supports this, and I'm fine with it. Then the class comment for UnsafeX509ExtendedTrustManager should be adapted to not only mention about ES.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@libenchao Thanks for the comment. I updated the comment to reflect that this could be used for any calcite adapter that makes http calls.

...earch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java Show resolved Hide resolved
...earch/src/main/java/org/apache/calcite/adapter/elasticsearch/ElasticsearchSchemaFactory.java Show resolved Hide resolved
@cgivre
Copy link
Contributor Author

cgivre commented Apr 26, 2023

@libenchao Thanks for the review. This is my first contribution to Calcite, so is there anything left for me to do to get this merged?

@sonarcloud
Copy link

sonarcloud bot commented Apr 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

33.3% 33.3% Coverage
0.0% 0.0% Duplication

libenchao
libenchao approved these changes Apr 28, 2023
View reviewed changes
Copy link
Member

@libenchao libenchao left a comment

Choose a reason for hiding this comment

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

@cgivre Thanks for the PR, it looks good to me now, merging!

@libenchao libenchao closed this in dd8fa24 Apr 28, 2023
@cgivre
Copy link
Contributor Author

cgivre commented Apr 28, 2023

@libenchao Thanks for the review!

@cgivre cgivre changed the title CALCITE-5671: Add Option to Disable SSL Certificate Validation to ES Adapter CALCITE-5671: Add Option to Disable SSL Certificate Validation to ElasticSearch Adapter May 1, 2023
@cgivre
Copy link
Contributor Author

cgivre commented May 1, 2023

@libenchao Will this get merged? I saw the PR was closed. Thanks!

@libenchao
Copy link
Member

@cgivre This has been merged in dd8fa24, you can see it in git history: https://github.com/apache/calcite/commits

@cgivre
Copy link
Contributor Author

cgivre commented May 1, 2023

@cgivre This has been merged in dd8fa24, you can see it in git history: https://github.com/apache/calcite/commits

Thx!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jnturton jnturton jnturton left review comments

@zinking zinking zinking left review comments

@libenchao libenchao libenchao approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@cgivre @zinking @tanclary @libenchao @jnturton