Replace X-Pack SSL config with libs/ssl-config #76636

tvernum · 2021-08-18T06:54:52Z

This is the final step in the removal of the X-Pack specific SSL
configuration code (replaced by libs/ssl-config)

For some time we have had two implementations of SSL Configuration
code. One was in org.elasticsearch.xpack.core.ssl, the other in
org.elasticsearch.common.ssl

These two implementations had essentially the same functionality:

Handle settings such as *.ssl.certificate, *.ssl.key etc
Load certificates and keys from PEM files and Keystores
Build and configure Java class such as SSLContext, KeyManager, etc
based on the configuration and certificates.

As of this common the X-Pack version is no more, and all SSL
configuration in Elasticsearch is handled by the libs/ssl-config
version instead.

Resolves: #68719

This is the final step in the removal of the X-Pack specific SSL configuration code (replaced by libs/ssl-config) For some time we have had two implementations of SSL Configuration code. One was in org.elasticsearch.xpack.core.ssl, the other in org.elasticsearch.common.ssl These two implementations had essentially the same functionality: - Handle settings such as '*.ssl.certificate`, `*.ssl.key` etc - Load certificates and keys from PEM files and Keystores - Build and configure Java class such as SSLContext, KeyManager, etc based on the configuration and certificates. As of this common the X-Pack version is no more, and all SSL configuration in Elasticsearch is handled by the libs/ssl-config version instead.

elasticmachine · 2021-08-18T06:54:55Z

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

tvernum · 2021-08-18T07:08:51Z

...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java

-            assertThat(c.getSerialNumber(), Matchers.equalTo("b8b96c37e332cccb"));
            assertThat(c.getPath(), Matchers.equalTo("testnode.crt"));
            assertThat(c.getFormat(), Matchers.equalTo("PEM"));
+            assertThat(c.getSerialNumber(), Matchers.equalTo("b8b96c37e332cccb"));


The changes in this test are due to two other changes.

X-Pack had a behaviour (which has been replicated into libs/ssl-config) that would have special case behaviour when you configured a keystore, but did not configure any trust anchors.
In that case it would look for any trusted certs in the keystore, and trust them along with the JDK default truststore.
However, there was a bug that meant it did not include those trusted certs in the certificates API.
ssl-config does not have this bug, so there are now more certs shown in some cases.

There was also an issue that the order of the listed certs was undefined, but consistent based on the underlying keystore. So tests that ran against a fix config would produce a consistent (ordered) output, but not based on any intentional login, just based on random(ish) behaviour.
That's been changed now (in a prior PR) so ssl-config now orders the certs consistently.

This test has been updated to reflect that new behaviour.

tvernum · 2021-08-18T07:09:41Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java

     */
    private SSLService createSSLService(Environment environment, ResourceWatcherService resourceWatcherService) {
-        final Map<String, SSLConfiguration> sslConfigurations = SSLService.getSSLConfigurations(environment.settings());
+        final Map<String, SslConfiguration> sslConfigurations = SSLService.getSSLConfigurations(environment);


This capitalisation change is actually the core of the PR.

SSLConfiguration is the old X-Pack version

SslConfiguration is the new ssl-config version

tvernum · 2021-08-18T07:11:09Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java

+        final Map<String, SslConfiguration> sslConfigurations = SSLService.getSSLConfigurations(environment);
        final SSLConfigurationReloader reloader =
-            new SSLConfigurationReloader(environment, resourceWatcherService, sslConfigurations.values());
+            new SSLConfigurationReloader(resourceWatcherService, sslConfigurations.values());


The ssl-config code takes an environment (actually Path basePath) in the constructor, rather than having it passed to each method.
A lot of the changes in this PR are about removing environment where it isn't needed anymore.

tvernum · 2021-08-18T07:51:25Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/RestrictedTrustConfig.java

 * underlying certificate validation.
 */
-public final class RestrictedTrustConfig extends TrustConfig {
+public final class RestrictedTrustConfig implements SslTrustConfig {


I didn't put RestrictedTrustConfig into libs/ssl-config because it has extra dependencies, and we don't need/want to support it for SSPL code like reindex.

This just converts the X-Pack RestrictedTrustConfig to implement the ssl-config interface (SslTrustConfig) instead of extending the X-Pack TrustConfig base class (which is now deleted).

tvernum · 2021-08-18T07:52:54Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

     * For example "xpack.http.ssl" may exist as a name in this map and have the global ssl configuration as a value
     */
-    private final Map<String, SSLConfiguration> sslConfigurations;
+    private final Map<String, SslConfiguration> sslConfigurations;


Most of the lines-of-code changes in this file are s/SSLConfiguration/SslConfiguration/, dropping Environment parameters or dealing with the slightly different method names in the new interfaces (e.g. s/supportedProtocols/getSupportedProtocols/)

I'll comment on the interesting changes.

tvernum · 2021-08-18T07:54:52Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

-        return sslConfiguration.keyConfig() != KeyConfig.NONE;
+    public boolean isConfigurationValidForServerUsage(SslConfiguration sslConfiguration) {
+        Objects.requireNonNull(sslConfiguration, "SslConfiguration cannot be null");
+        return sslConfiguration.getKeyConfig().hasKeyMaterial();


Rather than have a special NONE instance like X-Pack did, and making code check for it, ssl-config has a real method instead.

tvernum · 2021-08-18T07:56:47Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

+                        name = "(shared)";
+                        break;
                }
+                return name + " (with trust configuration: " + configuration.getTrustConfig() + ")";


This is new functionality.
I realised we can supplement the name of the context with a description of the trust config.

The benefit of that is that when we have a (shared) context, we don't need to guess which shared context it might be, we can see the exact configuration.

tvernum · 2021-08-18T07:59:40Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

            }
-            sslConfigurationMap.put(key, new SSLConfiguration(sslSettings));
+            try {
+                sslConfigurationMap.put(key, SslSettingsLoader.load(sslSettings, null, env, getKeyStoreFilter(key)));


This is a big bit of the change (even though it's a small change).

In X-Pack, SSLConfiguration knew how to build itself from Settings. ssl-config doesn't do that. It has a separate loader concept. So this is where we've switch from the X-Pack loading logic to the ssl-config one.

SslSettingsLoader is an X-Pack specific implementation of the abstract SslConfigurationLoader from ssl-config to handle a few X-pack details.
One of which is this new getKeyStoreFilter (see below).

tvernum · 2021-08-18T08:01:16Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

        return Collections.unmodifiableMap(sslConfigurationMap);
    }

+    private static Function<KeyStore, KeyStore> getKeyStoreFilter(String sslContext) {


This implements the logic that @BigPandaToo & I discussed.
It filters out CAs cert/keys from any keystore where there are both CAs and non-CA keypairs.

Because there was a version of this already in X-Pack, I needed to replicate it or the enrollment tests would start to fail. Rather than replicate logic that we knew wasn't quite what we wanted, I implemented something that was closer to our desired end state.

This should resolve #75097

Should we add a note in breaking changes for this ? I know this is rather improbable but there can be keystores in use right now that have 2 PrivateKeyEntry objects, one with CA key/cert and one with a non-CA , where the CA entry is used by the JDK code because its alias is returned first. This change will make it so the non-CA entry will be used and that will break clients connecting to it.

I think it's worth adding a deprecation log.
It's a scenario we can detect (using the same logic here) and issue a deprecation(style) warning.

That's probably more useful than an item breaking changes which everyone needs to read, but no one will care about.

I opened #76710

tvernum · 2021-08-18T08:05:17Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

+                cert.getPath(), cert.getFormat(), cert.getAlias(), cert.hasPrivateKey(), cert.getCertificate()
+            ))
+
+            .collect(Sets.toUnmodifiableSortedSet());


This is needed because CertificateInfo implements ToXContentObject and Writeable which aren't available in ssl-config.

In the old world, SSLConfiguration.getDefinedCertificates returned a Collection of CertificateInfo
In the new world, SslConfiguration.getConfiguredCertificates returns a Collection of StoredCertificate and X-Pack needs to convert it to CertificateInfo (and sorts it, per my comment in SecurityDocumentationIT)

tvernum · 2021-08-18T08:06:11Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SslSettingsLoader.java

            return trustConfig;
        }
-        throw new IllegalArgumentException("SSL trust_restrictions are not currently supported");
+        return new RestrictedTrustConfig(trustRestrictions, trustConfig);


99% of this class was done & merged, but I couldn't add this line until RestrictedTrustConfig switched to implementing the new interface.

BigPandaToo

I added one nit suggestion and a note. LGTM otherwise

BigPandaToo · 2021-08-18T18:32:56Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLConfigurationReloader.java

-                                    ResourceWatcherService resourceWatcherService,
-                                    Collection<SSLConfiguration> sslConfigurations) {
-        startWatching(environment, reloadConsumer(sslServiceFuture), resourceWatcherService, sslConfigurations);
+    public SSLConfigurationReloader(ResourceWatcherService resourceWatcherService,


This is nit... Since we changing SSLConfiguration to SslConfiguration, should we rename the corresponding classes and methods?

We probably should, but my PR is big enough as it is so I resisted the urge to do extra renaming.

BigPandaToo · 2021-08-18T18:55:21Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java

        return Collections.unmodifiableMap(sslConfigurationMap);
    }

+    private static Function<KeyStore, KeyStore> getKeyStoreFilter(String sslContext) {


This should resolve #75097

jkakavas

LGTM

tvernum · 2021-08-19T14:04:34Z

@elasticmachine update branch

This change elastic#76636 actually resolved the elastic#75097 by only filtering out the CAs certs/keys from xpack.security.http.ssl keystore This change re-enables the tests previously affected by elastic#75097 Resolves: elastic#75097

This change #76636 actually resolved the #75097 by only filtering out the CAs certs/keys from xpack.security.http.ssl keystore This change re-enables the tests previously affected by #75097 Resolves: #75097 Co-authored-by: Elastic Machine <[email protected]>

tvernum added >refactoring :Security/TLS SSL/TLS, Certificates v8.0.0 labels Aug 18, 2021

tvernum requested review from BigPandaToo and jkakavas August 18, 2021 06:54

elasticmachine added the Team:Security Meta label for security team label Aug 18, 2021

tvernum commented Aug 18, 2021

View reviewed changes

Style fixes

e050e00

BigPandaToo approved these changes Aug 18, 2021

View reviewed changes

jkakavas approved these changes Aug 19, 2021

View reviewed changes

tvernum mentioned this pull request Aug 19, 2021

Add deprecation for CAs in HTTP Keystore #76710

Open

Merge branch 'master' into feature/merge-ssl-config/final

04f3ef0

tvernum merged commit 76a684a into elastic:master Aug 20, 2021

BigPandaToo mentioned this pull request Aug 23, 2021

Re-enable tests affected by #75097 #76814

Merged

jakelandis removed the v8.0.0 label Sep 15, 2021

jakelandis added the v8.0.0-alpha2 label Sep 15, 2021

Replace X-Pack SSL config with libs/ssl-config #76636

Replace X-Pack SSL config with libs/ssl-config #76636

Uh oh!

Conversation

tvernum commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Aug 18, 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!

tvernum Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

Choose a reason for hiding this comment

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

tvernum commented Aug 19, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tvernum commented Aug 18, 2021 •

edited

Loading

tvernum Aug 18, 2021 •

edited

Loading