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 SSL Configuration Library #37287

Merged
merged 12 commits into from
Jan 16, 2019
Merged

Conversation

tvernum
Copy link
Contributor

@tvernum tvernum commented Jan 10, 2019

This introduces a new ssl-config library that can parse
and validate SSL/TLS settings and files.

It supports the standard configuration settings as used in the
Elastic Stack such as "ssl.verification_mode" and
"ssl.certificate_authorities" as well as all file formats used
in other parts of Elasticsearch security (such as PEM, JKS,
PKCS#12, PKCS#8, et al).

Relates: #29755


This commit just adds the new library, a follow up change will make use of it in reindex.
At a future point in time I will also switch X-Pack security to use some/all of this library.

Most of this code is based on existing X-Pack code, but has been heavily refactored in the process.
The PEM parsing code has been modified slightly, mostly due to needing to be part of a standalone library.

This introduces a new ssl-config library that can parse
and validate SSL/TLS settings and files.

It supports the standard configuration settings as used in the
Elastic Stack such as "ssl.verification_mode" and
"ssl.certificate_authorities" as well as all file formats used
in other parts of Elasticsearch security (such as PEM, JKS,
PKCS#12, PKCS#8, et al).
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

* Based on https://github.com/groovenauts/jmeter_oauth_plugin/blob/master/jmeter/src/
* main/java/org/apache/jmeter/protocol/oauth/sampler/PrivateKeyReader.java
*/
final class DerParser {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloned from org.elasticsearch.xpack.core.ssl

/**
* A variety of utility methods for working with or constructing {@link KeyStore} instances.
*/
final class KeyStoreUtil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of these methods existed (in some variation) in org.elasticsearch.xpack.core.ssl.CertParsingUtils.
I pulled in just what I needed.

import java.util.Map;
import java.util.function.Supplier;

final class PemUtils {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cloned from org.elasticsearch.xpack.core.ssl with minor variations (mostly due to not being able to use core ES classes) and some additional detail in error messages

/**
* A base exception for problems that occur while trying to configure SSL.
*/
public class SslConfigException extends RuntimeException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A simple base class because code in libs/ can't use ElasticsearchException

@@ -0,0 +1,149 @@
= Keystore Details
This document details the steps used to create the certificate and keystore files in this directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file and all the keys/certs in this directory were cloned from the x-pack pem-utils tests.

We can't control the message, and it varies by JRE release so there's
no good reason to assert on it.
Copy link
Member

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

I did a first pass and it looks good. I didn't make it to the tests but I'll do so tomorrow with a cleaner head. It sure helped that you marked the cloned files 👍

*/
public abstract void configure(SSLParameters sslParameters);

private static final Map<String, SslClientAuthenticationMode> LOOKUP = new LinkedHashMap<>(3);
Copy link
Member

Choose a reason for hiding this comment

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

Not that I see an issue with this, but for my own benefit/curiosity: Is there a specific reason to use an extra LinkedHashMap and not use Enum.valueOf, or a switch statement to get the SslClientAuthenticationMode from its name ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The version I copied from had a switch, but that style has a few weaknesses (IMO)

  1. You can't create reasonable error messages without duplicating the keys. The old code would switch on string constants, but then use Enum.name().toLowerCase() for the error message. It works, but there's no guarantees of consistency between those 2 sets of values.
  2. It ends up longer and more verbose (particularly when you want helpful diagnostic erros) for no real beenfit.

The other option is to simply use Enum.valueOf and Enum.name which ties the configuration strings to the enum constants. There's a minor risk in that changes to the enum names change the accepted settings, but that risk is covered pretty weel by tests.
Personally that's my preference, but it's not what we normally do, so I didn't go that direction.

}

/**
* Picks the best (highest security / most recent standard) SSL/TLS algorithm that is supported by the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Picks the best (highest security / most recent standard) SSL/TLS algorithm that is supported by the
* Picks the best (highest security / most recent standard) SSL/TLS version that is supported by the

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 resolved this with a slightly different wording. Let me know if you still see an issue.

libs/ssl-config/build.gradle Outdated Show resolved Hide resolved
libs/ssl-config/build.gradle Outdated Show resolved Hide resolved
private String contextAlgorithm() {
if (supportedProtocols.isEmpty()) {
// shouldn't happen...
return "TLSv1.2";
Copy link
Member

Choose a reason for hiding this comment

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

if it shouldn't happen, throw an exception?

libs/ssl-config/src/test/resources/certs/README.txt Outdated Show resolved Hide resolved
* under the License.
*/

archivesBaseName = 'elasticsearch-ssl-config'
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be preferable to alter project.name in settings.gradle as that would have effects across the board, including what you are looking for here. Also means that if we were to publish this the correct pom would be generated without additional configuration and better visibility in the IDE.

There are some additional changes needed in settings.gradle to keep Eclipse happy, see the discussion in #36477 for a similar change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atorok I pushed a5d0144 which I think covers what you've recommended.
Can you let me know if there's more we need to do?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @tvernum , looks good! You can now also remove this line archiveBaseName since with the changed project name that's the default now.

Copy link
Member

@jaymode jaymode left a comment

Choose a reason for hiding this comment

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

LGTM

@tvernum
Copy link
Contributor Author

tvernum commented Jan 15, 2019

Hmm, @elasticmachine, let's run the default distro tests again.

@tvernum
Copy link
Contributor Author

tvernum commented Jan 16, 2019

@elasticmachine -- run gradle build tests 1
@elasticmachine -- run gradle build tests 2

@tvernum tvernum merged commit 6d99e79 into elastic:master Jan 16, 2019
cshtdd added a commit to cshtdd/elasticsearch that referenced this pull request Jan 16, 2019
* master:
  Deprecate _type from LeafDocLookup (elastic#37491)
  Allow system privilege to execute proxied actions (elastic#37508)
  Update Put Watch to allow unknown fields (elastic#37494)
  AwaitsFix testAddNewReplicas
  SQL: Add protocol tests and remove jdbc_type from drivers response (elastic#37516)
  SQL: [Docs] Add an ES-SQL column for data types (elastic#37529)
  IndexMetaData#mappingOrDefault doesn't need to take a type argument. (elastic#37480)
  Simplify + Cleanup Dead Code in Settings (elastic#37341)
  Reject all requests that have an unconsumed body (elastic#37504)
  [Ml] Prevent config snapshot failure blocking migration (elastic#37493)
  Fix line length for aliases and remove suppression (elastic#37455)
  Add SSL Configuration Library (elastic#37287)
  SQL: Remove slightly used meta commands (elastic#37506)
  Simplify Snapshot Create Request Handling (elastic#37464)
  Remove the use of AbstracLifecycleComponent constructor elastic#37488 (elastic#37488)
  [ML] log minimum diskspace setting if forecast fails due to insufficient d… (elastic#37486)
@albertzaharovits
Copy link
Contributor

FYI @tvernum for when you backport, I have pushed 633bd09 .

tvernum added a commit that referenced this pull request Jan 25, 2019
This introduces a new ssl-config library that can parse
and validate SSL/TLS settings and files.

It supports the standard configuration settings as used in the
Elastic Stack such as "ssl.verification_mode" and
"ssl.certificate_authorities" as well as all file formats used
in other parts of Elasticsearch security (such as PEM, JKS,
PKCS#12, PKCS#8, et al).
tvernum added a commit that referenced this pull request Jan 31, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Relates: #37287
Resolves: #29755
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 4, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Backport of: elastic#37527
Relates: elastic#37287
Resolves: elastic#29755
tvernum added a commit that referenced this pull request Feb 4, 2019
Adds reindex.ssl.* settings for reindex from remote.

This uses the ssl-config/ internal library to parse and load SSL
configuration and files. This is applied when using the low level
rest client to connect to a remote ES node

Backport of: #37527
Relates: #37287
Resolves: #29755
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Closes: elastic#38944
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Closes: #38944
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Backport of: elastic#39019
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Backport of: elastic#39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
tvernum added a commit to tvernum/elasticsearch that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: elastic#37287, elastic#37527
Closes: elastic#38944

Backport of: elastic#39019
tvernum added a commit that referenced this pull request Feb 18, 2019
This is used by the reindex-client library which is published to maven

Relates: #37287, #37527
Backport of: #39019
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.

7 participants