Access SSL contexts using names instead of Settings #30953

tvernum · 2018-05-30T08:51:00Z

Historically we have loaded SSL objects (such as SSLContext,
SSLIOSessionStrategy) by passing in the SSL settings, constructing a
new SSL configuration from those settings and then looking for a
cached object that matches those settings.

The primary issue with this approach is that it requires a fully
configured Settings object to be available any time the SSL context
needs to be loaded. If the Settings include SecureSettings (such as
passwords for keys or keystores) then this is not true, and the cached
SSL object cannot be loaded at runtime.

This commit introduces an alternative approach of naming every cached
ssl configuration, so that it is possible to load the SSL context for
a named configuration (such as "xpack.http.ssl"). This means that the
calling code does not need to have ongoing access to the secure
settings that were used to load the configuration.

Resolves: #30344

Historically we have loaded SSL objects (such as SSLContext, SSLIOSessionStrategy) by passing in the SSL settings, constructing a new SSL configuration from those settings and then looking for a cached object that matches those settings. The primary issue with this approach is that it requires a fully configured Settings object to be available any time the SSL context needs to be loaded. If the Settings include SecureSettings (such as passwords for keys or keystores) then this is not true, and the cached SSL object cannot be loaded at runtime. This commit introduces an alternative approach of naming every cached ssl configuration, so that it is possible to load the SSL context for a named configuration (such as "xpack.http.ssl"). This means that the calling code does not need to have ongoing access to the secure settings that were used to load the configuration.

elasticmachine · 2018-05-30T08:51:02Z

Pinging @elastic/es-security

tvernum · 2018-05-30T08:51:48Z

This is mostly done, but I need to do another pass over it before reviews

jaymode · 2018-06-13T18:37:38Z

This is mostly done, but I need to do another pass over it before reviews

@tvernum how are we looking? Should we go ahead with reviews?

tvernum · 2018-06-15T04:20:33Z

@jaymode It's ready for review now.
I had 1 last test I wanted to write, and that got caught up in various changes to master (broken builds, unused imports, etc).

tvernum · 2018-06-22T07:24:42Z

I'm going to need to think through a couple more issues on this one.
It fails on dynamic settings for monitoring, and I don't have a clear solution in my head yet.

As it stands, the SSLService configures each context name to have a fixed configuration, so updating monitoring SSL settings dynamically has no affect.
I can fix that, but then we run back into the original problem that triggered this.

What I think I need is to be able to apply an update to a named configuration (probably by getting the config, merging the new settings into it, and then putting that config back)

- Prevent dynamic changes to SSL settings that also use secure settings - Dyanmically rebuild SSL context for exporters that don't use secure setting

jaymode · 2018-07-10T19:55:05Z

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

+            contextName = contextName.substring(0, contextName.length() - 1);
+        }
+        final SSLConfiguration configuration = sslConfigurations.get(contextName);
+        if (configuration == null && logger.isTraceEnabled()) {


derp isTraceEnabled means the warn log message only gets logged when trace is enabled

🤦‍♂️

jaymode · 2018-07-10T19:59:27Z

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

     */
+    @Deprecated


can we handle this as part of this change or do you want it to be separate?

I'm planning on tackling it, it's just still being used in a few places & will require some more work.

jaymode · 2018-07-10T20:24:59Z

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

+                storeSslConfiguration(key, configuration);
+                sslContextHolders.computeIfAbsent(configuration, this::createSslContext);
+            }
+        });

        // transport is special because we want to use a auto-generated key when there isn't one


can you do me a favor and delete this comment? It is a left over from me that is no longer relevant

jaymode · 2018-07-10T20:28:43Z

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

-        profileSettings.forEach((profileSetting) ->
-            sslConfigurations.computeIfAbsent(new SSLConfiguration(profileSetting, transportSSLConfiguration), this::createSslContext));
-        return Collections.unmodifiableMap(sslConfigurations);
+        this.sslConfigurations.put("_transport", transportSSLConfiguration);


the use of _ as our prefix to avoid collisions or to make something special concerns me. In the IpFilter we use .http for HTTP to avoid collisions since it is much harder to create a setting/name that would result in that as the key. Maybe we should use . instead of _ here?

Actually I have no idea why I'm naming this one in a special way. It should just be xpack.security.transport.ssl the only special one should be .global

Scrap that, .global should just be xpack.ssl

jaymode · 2018-07-10T20:34:25Z

...toring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpSslExporterIT.java

+
+@ESIntegTestCase.ClusterScope(scope = Scope.SUITE,
+    numDataNodes = 1, numClientNodes = 0, transportClientRatio = 0.0, supportsDedicatedMasters = false)
+public class HttpSslExporterIT extends MonitoringIntegTestCase {


this is a nit, but maybe HttpExporterSslIT so that if someone is looking for HttpExporter test classes, they easily find it

I originally named it HttpExporterSslIT but the combination of lower-case l followed up uppercase I offended my eyes, so I changed it. I'll switch it back.

jaymode · 2018-07-10T20:37:11Z

...toring/src/test/java/org/elasticsearch/xpack/monitoring/exporter/http/HttpSslExporterIT.java

+
+    @Override
+    protected Settings nodeSettings(int nodeOrdinal) {
+        if (keystore == null) {


@jkakavas is there something that should be done here for the FIPS work?

Yes, thanks for catching this.

@tvernum can you please add the key and certificate files and use those instead of the keystore when setting up the MockWebServer ?
Using it for the truststore is fine, even in a FIPS-140 JVM so no additional requirements here. When I went through our tests, I opted for using the certificate for certificate_authorities instead of the JKS for truststore (when applicable and made sense ) mainly for consistency, but we don't have to .

Oh FIPS why do you want to make my life complicated?

I need to use the truststore (rather than certificate_authorities) because I need to test passwords, and PEM certs don't have passwords.

There's a few places where the usage is difficult to remove, so I'm leaving them for now and we can tackle them another time.

jaymode

LGTM!

jkakavas · 2018-07-14T05:39:31Z

...plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ldap/LdapTestUtils.java

        if (useGlobalSSL) {
-            connectionSettings = Settings.EMPTY;
+            sslConfiguration = sslService.getSSLConfiguration("_global");


_global -> xpack.ssl

These tests were creating a SSL service that was not aware of the realm that they were trying to test. This no longer works.

Historically we have loaded SSL objects (such as SSLContext, SSLIOSessionStrategy) by passing in the SSL settings, constructing a new SSL configuration from those settings and then looking for a cached object that matches those settings. The primary issue with this approach is that it requires a fully configured Settings object to be available any time the SSL context needs to be loaded. If the Settings include SecureSettings (such as passwords for keys or keystores) then this is not true, and the cached SSL object cannot be loaded at runtime. This commit introduces an alternative approach of naming every cached ssl configuration, so that it is possible to load the SSL context for a named configuration (such as "xpack.http.ssl"). This means that the calling code does not need to have ongoing access to the secure settings that were used to load the configuration. This change also allows monitoring exporters to use SSL passwords from secure settings, however an exporter that uses a secure SSL setting (e.g. truststore.secure_password) may not have its SSL settings updated dynamically (this is prevented by a settings validator). Exporters without secure settings can continue to be defined and updated dynamically. Backport of: - c662565 - edbea73 - 6f2b7dc

* es/6.x: (24 commits) Fix broken backport Switch full-cluster-restart to new style Requests (#32140) Fix multi level nested sort (#32204) MINOR: Remove unused `IndexDynamicSettings` (#32237) (#32248) [Tests] Remove QueryStringQueryBuilderTests#toQuery class assertions (#32236) Switch rolling restart to new style Requests (#32147) Enhance Parent circuit breaker error message (#32056) [ML] Use default request durability for .ml-state index (#32233) Enable testing in FIPS140 JVM (#31666) (#32231) Remove indices stats timeout from monitoring docs TESTS: Check for Netty resource leaks (#31861) (#32225) Rename ranking evaluation response section (#32166) Dependencies: Upgrade to joda time 2.10 (#32160) Backport SSL context names (#30953) to 6.x (#32223) Require Gradle 4.9 as minimum version (#32200) Detect old trial licenses and mimic behaviour (#32209) Painless: Simplify Naming in Lookup Package (#32177) add support for write index resolution when creating/updating documents (#31520) A replica can be promoted and started in one cluster state update (#32042) Rest test - allow for snapshots to take 0 milliseconds ...

tvernum added 10 commits May 7, 2018 16:36

Merge branch 'master' into fix/30344-ssl-context-names

222015b

[WIP] Use named SSL context in LDAP

a360d22

Merge branch 'master' into fix/30344-ssl-context-names

29077e2

Update openldap QA tests for named SSL contexts

9ea7a52

Remove sslSocketFactory(Settings) from SSLService

997a1ec

Deprecate createSSLEngine(Settings)

2999649

Remove createSSLEngine(Settings)

bcde8a5

Reduce use of Settings in SSL

6ce4d32

Merge branch 'master' into fix/30344-ssl-context-names

f9cc028

tvernum added WIP :Security/TLS SSL/TLS, Certificates labels May 30, 2018

tvernum added >bug v7.0.0 >refactoring v6.4.0 labels May 30, 2018

tvernum added 4 commits June 12, 2018 17:14

Merge branch 'master' into fix/30344-ssl-context-names

e0561ee

Improve SSLService tests

bee987c

Small cleanup

7c060d7

Merge branch 'master' into fix/30344-ssl-context-names

2282d01

tvernum added 5 commits June 14, 2018 16:10

Add additional test for named SSL configurations

1aa0c14

Merge branch 'master' into fix/30344-ssl-context-names

39e6629

Merge branch 'master' into fix/30344-ssl-context-names

ef58ff1

Merge branch 'master' into fix/30344-ssl-context-names

d94dd57

Remove unused imports

ee32357

tvernum added the review label Jun 15, 2018

tvernum added 7 commits June 26, 2018 15:17

Merge branch 'master' into fix/30344-ssl-context-names

37cc028

Merge branch 'master' into fix/30344-ssl-context-names

817c565

Fix monitoring to work with dynamic SSL settings

2eb845a

- Prevent dynamic changes to SSL settings that also use secure settings - Dyanmically rebuild SSL context for exporters that don't use secure setting

Merge branch 'master' into fix/30344-ssl-context-names

2f6c87b

Cleanup test static vars

255a42e

Address feedback from @jaymode (round 1)

8ea7e35

Fix import

e8d8299

jaymode reviewed Jul 10, 2018

View reviewed changes

tvernum added 6 commits July 11, 2018 14:45

Rename test & context name

b11d61b

Remove more uses of sslConfiguration from settings

f4bab91

Merge branch 'master' into fix/30344-ssl-context-names

f637dc7

Remove remaining uses of deprecated methods

f774e6d

There's a few places where the usage is difficult to remove, so I'm leaving them for now and we can tackle them another time.

Don't use JKS keystore in test

d1f3147

Fix broken test

44c4cf1

jaymode approved these changes Jul 11, 2018

View reviewed changes

tvernum added 3 commits July 12, 2018 12:01

Merge branch 'master' into fix/30344-ssl-context-names

9a0c026

Fix broken tests

bc3095e

Merge branch 'master' into fix/30344-ssl-context-names

9f6fa6d

tvernum merged commit c662565 into elastic:master Jul 13, 2018

tvernum added backport pending and removed review labels Jul 13, 2018

jkakavas reviewed Jul 14, 2018

View reviewed changes

tvernum added a commit that referenced this pull request Jul 20, 2018

Fix AD / vagrant based tests for #30953

6f2b7dc

These tests were creating a SSL service that was not aware of the realm that they were trying to test. This no longer works.

tvernum removed the backport pending label Jul 23, 2018

tvernum mentioned this pull request Aug 17, 2018

Enable FIPS140LicenseBootstrapCheck #32903

Merged

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access SSL contexts using names instead of Settings #30953

Access SSL contexts using names instead of Settings #30953

tvernum commented May 30, 2018

elasticmachine commented May 30, 2018

tvernum commented May 30, 2018

jaymode commented Jun 13, 2018

tvernum commented Jun 15, 2018

tvernum commented Jun 22, 2018

jaymode Jul 10, 2018

tvernum Jul 11, 2018

jaymode Jul 10, 2018

tvernum Jul 11, 2018

jaymode Jul 10, 2018

jaymode Jul 10, 2018

tvernum Jul 11, 2018

tvernum Jul 11, 2018

jaymode Jul 10, 2018

tvernum Jul 11, 2018

jaymode Jul 10, 2018

jkakavas Jul 11, 2018

tvernum Jul 11, 2018

tvernum Jul 11, 2018

jaymode left a comment

jkakavas Jul 14, 2018

Access SSL contexts using names instead of Settings #30953

Access SSL contexts using names instead of Settings #30953

Conversation

tvernum commented May 30, 2018

elasticmachine commented May 30, 2018

tvernum commented May 30, 2018

jaymode commented Jun 13, 2018

tvernum commented Jun 15, 2018

tvernum commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment