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

Add support for Search Guard in Elasticsearch connector #438

Open
wants to merge 2 commits into
base: master
from

Conversation

5 participants
@kokosing
Copy link
Member

kokosing commented Mar 11, 2019

Add support for Search Guard in Elasticsearch connector

Extracted from: prestodb/presto#12422

@cla-bot cla-bot bot added the cla-signed label Mar 11, 2019

@kokosing kokosing requested a review from zhenxiao Mar 11, 2019

@kokosing kokosing force-pushed the kokosing:origin/master/104 branch from 1fca82b to 9d5e8de Mar 11, 2019

@zhenxiao
Copy link
Member

zhenxiao left a comment

looks good to me

builder = Settings.builder()
.put("client.transport.ignore_cluster_name", true);
}
switch (config.getCertificateFormat()) {

This comment has been minimized.

@dain

dain Mar 11, 2019

Member

Maybe use io.airlift.security.pem.PemReader#isPem(java.lang.String) instead of making users deal with it.

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

This might be tricky given how they split up their configs into pemcert, pemkey, pemtrustedcas. I wonder if users normally have all three files, or if they typically combine at least the first two.

Builder builder;
TransportClient client;
if (clusterName.isPresent()) {
builder = Settings.builder()

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

Nit: move initialization of builder to the variable declaration

Builder builder = Settings.builder();
if (...) {
    builder.put(...);
}
static TransportClient createTransportClient(ElasticsearchConnectorConfig config, TransportAddress address, Optional<String> clusterName)
{
Settings settings;
Builder builder;

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

Nit: don't static import Builder, as the name is too generic.

{
Settings settings;
Builder builder;
TransportClient client;

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

If we return directly below, this variable isn't needed.

return certificateFormat;
}

@Config("searchguard.ssl.transport.certificate_format")

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

Use dashes for configuration properties

private SearchGuardCertificateFormat certificateFormat = NONE;
private File pemcertFilepath = new File("etc/elasticsearch/esnode.pem");
private File pemkeyFilepath = new File("etc/elasticsearch/esnode-key.pem");
private String pemkeyPassword = "";

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

Configs normally use null for unset properties

builder = Settings.builder()
.put("client.transport.ignore_cluster_name", true);
}
switch (config.getCertificateFormat()) {

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

This might be tricky given how they split up their configs into pemcert, pemkey, pemtrustedcas. I wonder if users normally have all three files, or if they typically combine at least the first two.

InetAddress address = InetAddress.getByName(split.getSearchNode());
client = new PreBuiltTransportClient(settings)
.addTransportAddress(new TransportAddress(address, split.getPort()));
address = InetAddress.getByName(split.getSearchNode());
}
catch (UnknownHostException e) {
throw new PrestoException(ELASTICSEARCH_CONNECTION_ERROR, format("Error connecting to search node (%s:%d)", split.getSearchNode(), split.getPort()), e);

This comment has been minimized.

@electrum

electrum Mar 12, 2019

Member

Should this be changed to

"Failed to resolve search node (%s:%d)"
client = new PreBuiltTransportClient(settings, SearchGuardSSLPlugin.class).addTransportAddress(address);
break;
case JKS:
settings = Settings.builder()

This comment has been minimized.

@kokosing

kokosing Mar 12, 2019

Author Member

@zhenxiao Is this intended to drop client.transport.ignore_cluster_name or cluster.name that are set above?

This comment has been minimized.

@zhenxiao

zhenxiao Mar 13, 2019

Member

whoops, I might lose the context, client.transport.ignore_cluster_name is set to true, when cluster.name is not configured. This is for one use case, where it is a virtual ELK cluster with a number of underlying ELK clusters, sharing search nodes. Not able to configure cluster name in this case.

@kokosing kokosing force-pushed the kokosing:origin/master/104 branch from 9d5e8de to c80e5e8 Mar 12, 2019

@cla-bot

This comment has been minimized.

Copy link

cla-bot bot commented Mar 12, 2019

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to cla@prestosql.io. If you are contributing on behalf of someone else (e.g., your employer), the individual CLA may not be sufficient and your employer may need the Corporate CLA signed.

@cla-bot cla-bot bot removed the cla-signed label Mar 12, 2019

@kokosing

This comment has been minimized.

Copy link
Member Author

kokosing commented Mar 12, 2019

Applied comments.

@electrum
Copy link
Member

electrum left a comment

This looks good.

A future enhancement is to split out the configs for the two different key types and bind the configs conditionally based on the configured type. Thus, it would be an error to provide configs for an un-configured type.


static TransportClient createTransportClient(ElasticsearchConnectorConfig config, TransportAddress address, Optional<String> clusterName)
{
Settings settings;

This comment has been minimized.

@electrum

electrum Mar 13, 2019

Member

We could remove this and inline it as builder.build()

@@ -54,6 +65,15 @@ public void testExplicitPropertyMappings()
.put("elasticsearch.request-timeout", "1s")
.put("elasticsearch.max-request-retries", "3")
.put("elasticsearch.max-request-retry-time", "5s")
.put("searchguard.ssl-transport-certificate_format", "PEM")

This comment has been minimized.

@electrum

electrum Mar 13, 2019

Member

Use - for certificate-format

We normally use dots to group related functionality in configs. I would name these like

  • searchguard.ssl-transport.certificate-format
  • searchguard.ssl-transport.pemcert-filepath

Or shorten to

  • searchguard.ssl.certificate-format

Since ssl-transport seems redundant

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.