Update "ssl-config" to support X-Pack features by tvernum · Pull Request #74887 · elastic/elasticsearch

tvernum · 2021-07-05T01:44:19Z

This commit upgrades the existing SSPL licensed "ssl-config" library
to include additional features that are supported by the X-Pack SSL
library.

This commit does not make any changes to X-Pack to use these new
features - it introduces them in preparation for their future use by
X-Pack.

The reindex module is updated to reflect API changes in ssl-config

This commit upgrades the existing SSPL licensed "ssl-config" library to include additional features that are supported by the X-Pack SSL library. This commit does not make any changes to X-Pack to use these new features - it introduces them in preparation for their future use by X-Pack. The reindex module is updated to reflect API changes in ssl-config

elasticmachine · 2021-07-05T01:44:23Z

Pinging @elastic/es-security (Team:Security)

tvernum · 2021-07-05T01:46:04Z

I've labelled this as v8.0.0 only, because it's not needed in 7.x (it's only here to support migrating x-pack SSL onto this library).
We can talk about backporting it to 7.x if someone feels strongly, but I didn't make the changes with perfect BWC in mind, I'd need to do a review first.

tvernum · 2021-07-05T01:47:44Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/CompositeTrustConfig.java

+/**
+ * A TrustConfiguration that merges trust anchors from a number of other trust configs to produce a single {@link X509ExtendedTrustManager}.
+ */
+public class CompositeTrustConfig implements SslTrustConfig {


In X-Pack, specifying a keystore, but no truststore gives you a set of trust anchors that support everything in your JDK trust, and all the trust anchors in your keystore.
This class was added so we can also support that behaviour from ssl-config.

tvernum · 2021-07-05T01:50:10Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/DefaultJdkTrustConfig.java


+    @Override
+    public boolean isSystemDefault() {
+        return true;


This method needed for the PKI realm - it will not trust the JDK default issuers.

tvernum · 2021-07-05T01:50:50Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/DerParser.java

 * main/java/org/apache/jmeter/protocol/oauth/sampler/PrivateKeyReader.java
 */
-final class DerParser {
+public final class DerParser {


The RestrictedTrustConfig in X-Pack needs to be able to use this parser.

tvernum · 2021-07-05T01:51:43Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/KeyStoreUtil.java

 * A variety of utility methods for working with or constructing {@link KeyStore} instances.
 */
-final class KeyStoreUtil {
+public final class KeyStoreUtil {


Some of these methods have been made public because X-Pack will need to call them.

tvernum · 2021-07-05T01:52:40Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/KeyStoreUtil.java

+    }
+
+    static Stream<KeyStoreEntry> stream(KeyStore keyStore,
+                                        Function<GeneralSecurityException, ? extends RuntimeException> exceptionHandler) {


This method and the class below were added to support stream-based iteration over keystore entires.

tvernum · 2021-07-05T02:08:17Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemKeyConfig.java

+            throw SslFileUtil.ioException(KEY_FILE_TYPE, List.of(path), e);
        } catch (GeneralSecurityException e) {
-            throw new SslConfigException("cannot load ssl private key file [" + key.toAbsolutePath() + "]", e);
+            throw SslFileUtil.securityException(KEY_FILE_TYPE, List.of(path), e);


Lots of exception handling has been unified. See comments in SslFileUtil

tvernum · 2021-07-05T02:09:00Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslFileUtil.java

+/**
+ * Utility methods for common file handling in SSL configuration
+ */
+final class SslFileUtil {


This is a new class, mostly to handle generating exceptions for particular file related problems.

tvernum · 2021-07-05T02:11:49Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslFileUtil.java

+        return ioException(fileType, paths, cause, null);
+    }
+
+    static SslConfigException ioException(String fileType, List<Path> paths, IOException cause, String detail) {


One of the big things we did to try and make X-Pack SSL setup easier was write clear & explicit error messages for all the common problems that people run into:

File not found

File permissions wrong

File in wrong directory

Missing password

Wrong password

This class replicates those messages into ssl-config and centralises them.
Some of the messages already existed in this library, but the X-Pack ones are encountered more frequently, and have been better turned for readability.

That said, I did try and improve some of them along the way.

tvernum · 2021-07-05T02:13:56Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslKeystoreConfig.java

+import java.util.Objects;
+import java.util.stream.Collectors;
+
+public abstract class SslKeystoreConfig implements SslKeyConfig {


This is a base class for PKCS#11 and File-Keystore (JKS, PKCS#12) support.
The X-Pack model of having 1 class that handled both was really fragile - we often wrote code that assume that the file was non-null.

tvernum · 2021-07-05T02:27:58Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/StoredCertificate.java

+ * Information about a certificate that is locally stored.It includes a reference to the {@link X509Certificate} itself,
+ * as well as information about where it was loaded from.
+ */
+public class StoredCertificate {


This is used for the _ssl/certificates API.
That API uses a CertificateInfo which depends on ToXContent etc, which isn't available inside this library. So this class is a lightweight representation of the information needed for that API, and X-Pack will convert from this class to the X-Pack class.

tvernum · 2021-07-13T01:53:00Z

@elasticmachine update branch

BigPandaToo

I added a couple of nit comments.
LGTM otherwise

BigPandaToo · 2021-07-13T00:32:42Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/KeyStoreUtil.java


    /**
     * Construct an in-memory keystore with multiple trusted cert entries.
+     *


nit: extra line?

I wouldn't say so.
Everything in that file has blank line between the method description & the parameters, so this format is consistent with the rest of the code.

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslKeystoreConfig.java

jkakavas

Thanks for taking the time to add comments for things that would most probably raise questions, that was very helpful. Added some suggestions and comments, mostly nits

jkakavas · 2021-07-13T20:34:41Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemKeyConfig.java

+     * @param keyPassword    Password for the private key (or empty is the key is not encrypted)
+     * @param configBasePath The base directory from which config files should be read (used for diagnostic exceptions)
+     */
+    public PemKeyConfig(String certificate, String key, char[] keyPassword, Path configBasePath) {


nit for readability

Suggested change

public PemKeyConfig(String certificate, String key, char[] keyPassword, Path configBasePath) {

public PemKeyConfig(String certificatePath, String key, char[] keyPassword, Path configBasePath) {

jkakavas · 2021-07-13T20:54:54Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemKeyConfig.java

+        boolean first = true;
+        for (Certificate cert : certificates) {
+            if (cert instanceof X509Certificate) {
+                info.add(new StoredCertificate((X509Certificate) cert, this.certificate, "PEM", null, first));


is it safe to assume that certificate points to a file where if multiple certs are included, the leaf is always the first one?

Probably not - but that's what X-Pack currently does.

It should be - if the certificate path points to a PEM file with multiple certificates, then they should be a chain ordered from leaf to issuer to root - but there's no guarantee that it's true.

However, other things will break if it's not. When we build a keystore we will assume the certificates that we read form a valid chain, and Keystore.setKeyEntry will throw an exception if they're not.

I added a test to show that createKeyManager fails if the chain is in the wrong order. I think that is sufficient for the assumption in this method.

We do have a "private key" for the leaf (first element) certificate. It might be the wrong key (none of the config classes check that in this method), but we have one.
And the core functionality of the config (building a key manager) will fail.

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/KeyStoreUtil.java

jkakavas · 2021-07-14T04:56:15Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslConfigurationLoader.java

-        final Path trustStorePath = resolveSetting(TRUSTSTORE_PATH, basePath::resolve, null);
+    protected SslTrustConfig buildTrustConfig(Path basePath, SslVerificationMode verificationMode, SslKeyConfig keyConfig) {
+        final List<String> certificateAuthorities = resolveListSetting(CERTIFICATE_AUTHORITIES, Function.identity(), null);
+        final String trustStorePath = resolveSetting(TRUSTSTORE_PATH, Function.identity(), null);


jkakavas · 2021-07-14T05:24:18Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslFileUtil.java

+        if (cause.getMessage() != null) {
+            message += " (" + cause.getMessage() + ")";
+        }
+        if (detail != null) {


is it worth adding the hint about UnrecoverableKeyException ? ( maybe only if detail is null )

jkakavas · 2021-07-14T08:21:28Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslKeyConfig.java

+    /**
+     * @return A collection of {@link X509Certificate certificates} used by this config.
+     */
+    Collection<? extends StoredCertificate> getConfiguredCertificates();


Do we foresee exteding StoredCertificate ? Can we make the return type non generic for now ?

jkakavas · 2021-07-14T08:21:49Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslKeyConfig.java

+    List<Tuple<PrivateKey, X509Certificate>> getKeys();
+
+    /**
+     * @return A collection of {@link X509Certificate certificates} used by this config.


nit: update javadoc with corret return type

jkakavas · 2021-07-14T08:23:20Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/PemKeyConfig.java

+        if (certificates.isEmpty()) {
+            return List.of();
+        }
+        final Certificate certificate = certificates.get(0);


nit: rename certificate here for readability ( vs this.certificate)

jkakavas · 2021-07-14T08:28:54Z

libs/ssl-config/src/main/java/org/elasticsearch/common/ssl/SslFileUtil.java

+
+        if (cause.getCause() instanceof UnrecoverableKeyException) {
+            message += " - this is usually caused by an incorrect password";
+        } else if (cause != null && cause.getMessage() != null) {


cause can't be null here, otherwise we'd have thrown an NPE in cause.getCause() above. We can check that cause is not null above and not go into this conditional at all

- Remove PKCS#11 - Improve exception messages - Simplify return types

PemKeyConfig.getConfiguredCertificates relies on the certificate file containing a chain from leaf -> issuer -> .. -> root This commit adds a test to ensure that createKeyManager will fail if that order is not correct, and therefore the assumption in getConfiguredCertificates is acceptable

jkakavas

LGTM

This commit upgrades the existing SSPL licensed "ssl-config" library to include additional features that are supported by the X-Pack SSL library. This commit does not make any changes to X-Pack to use these new features - it introduces them in preparation for their future use by X-Pack. The reindex module is updated to reflect API changes in ssl-config

tvernum added >non-issue :Security/TLS SSL/TLS, Certificates v8.0.0 labels Jul 5, 2021

tvernum requested review from BigPandaToo and jkakavas July 5, 2021 01:44

elasticmachine added the Team:Security Meta label for security team label Jul 5, 2021

tvernum commented Jul 5, 2021

View reviewed changes

tvernum added the :Security/FIPS Running ES in FIPS 140-2 mode label Jul 5, 2021

Merge branch 'master' into feature/merge-ssl-config/common-ssl-base

093e9e4

BigPandaToo approved these changes Jul 13, 2021

View reviewed changes

tvernum added 2 commits July 14, 2021 11:03

Merge branch 'master' into feature/merge-ssl-config/common-ssl-base

d2eb0f6

Improve toString on SslKeystoreConfig

83c53cc

jkakavas reviewed Jul 14, 2021

View reviewed changes

tvernum added 4 commits July 15, 2021 15:45

Address Feedback

03420fd

- Remove PKCS#11 - Improve exception messages - Simplify return types

Merge branch 'master' into feature/merge-ssl-config/common-ssl-base

f15b9d9

Add javadoc on KeyStoreEntry

75bcd1b

tvernum requested a review from jkakavas July 15, 2021 06:46

jkakavas approved these changes Jul 15, 2021

View reviewed changes

tvernum merged commit 940a890 into elastic:master Jul 15, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	public PemKeyConfig(String certificate, String key, char[] keyPassword, Path configBasePath) {
	public PemKeyConfig(String certificatePath, String key, char[] keyPassword, Path configBasePath) {

Conversation

tvernum commented Jul 5, 2021

Uh oh!

elasticmachine commented Jul 5, 2021

Uh oh!

tvernum commented Jul 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum commented Jul 13, 2021

Uh oh!

BigPandaToo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants