Do not load SSLService in plugin contructor #49667

tvernum · 2019-11-28T03:14:45Z

XPackPlugin created an SSLService within the plugin contructor.
This has 2 negative consequences:

The service may be constructed based on a partial view of settings.
Other plugins are free to add setting values via the
additionalSettings() method, but this (necessarily) happens after
plugins have been constructed.
Any exceptions thrown during the plugin construction are handled
differently than exceptions thrown during "createComponents".
Since SSL configurations exceptions are relatively common, it is
far preferable for them to be thrown and handled as part of the
createComponents flow.

This commit moves the creation of the SSLService to
XPackPlugin.createComponents, and alters the sequence of some other
steps to accommodate this change.

Relates: #44536

XPackPlugin created an SSLService within the plugin contructor. This has 2 negative consequences: 1. The service may be constructed based on a partial view of settings. Other plugins are free to add setting values via the additionalSettings() method, but this (necessarily) happens after plugins have been constructed. 2. Any exceptions thrown during the plugin construction are handled differently than exceptions thrown during "createComponents". Since SSL configurations exceptions are relatively common, it is far preferable for them to be thrown and handled as part of the createComponents flow. This commit moves the creation of the SSLService to XPackPlugin.createComponents, and alters the sequence of some other steps to accommodate this change. Relates: elastic#44536

elasticmachine · 2019-11-28T03:14:47Z

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

tvernum · 2019-11-28T03:15:49Z

I added a few FIXME/TODOs in here for other places where we are incorrectly creating components/reading settings during construction.

I will work through fixing those in separate PR.

rjernst

LGTM. To be clear about use of settings in the ctor, it may eventually work when/if we split out setting registration into a separate plugin class that gets loaded before the actual plugin, as we have discussed to allow validation of secure setting names in elasticsearch-keystore, but moving away from them now would be correct for the reason you noted in the TODO comments.

rjernst · 2019-12-05T18:18:27Z

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

+     * Create a new SSLService using the {@code Settings} from {@link Environment#settings()}.
+     * @see #SSLService(Settings, Environment)
+     */
+    public SSLService(Environment environment) {


Why do we still need the other ctor? I only see 2 non test uses of the other ctor.

We don't need it, but there's a lot of test uses that would cause this PR to get quite big. I'm going to remove the old constructor in a follow-up PR.

rjernst · 2019-12-05T18:22:54Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -358,6 +353,17 @@ protected Clock getClock() {
            return Collections.singletonList(new SecurityUsageServices(null, null, null, null));
        }

+        // We need to construct the checks here while the secure settings are still available.
+        // If we want until #getBoostrapChecks the secure settings will have been cleared/closed.


typo: want -> wait

rjernst · 2019-12-05T18:28:17Z

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

@@ -225,14 +232,16 @@ public Settings additionalSettings() {
                                               NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
        List<Object> components = new ArrayList<>();

+        final SSLService sslService = new SSLService(environment);
+        setSslService(sslService);


nit: this looks like the only use of setSslService, can it be removed and just set the setonce member directly?

LocalStateCompositeXPackPlugin does crazy stuff.
This is the only use of the method, but we have multiple implementations :(

albertzaharovits

LGTM, Makes sense to me to initialize the SSLService with the Environment instance most of the other components use (apart from the PlugingService), and because security depends on x-pack-core the SSLService will always be created on the latter's createComponents, before it is referenced in the former's factory methods.

This removes the old `SSLService(Settings, Environment)` constructor and converts all uses cases to the `SSLService(Environment)` constructor that was added in elastic#49667

XPackPlugin created an SSLService within the plugin contructor. This has 2 negative consequences: 1. The service may be constructed based on a partial view of settings. Other plugins are free to add setting values via the additionalSettings() method, but this (necessarily) happens after plugins have been constructed. 2. Any exceptions thrown during the plugin construction are handled differently than exceptions thrown during "createComponents". Since SSL configurations exceptions are relatively common, it is far preferable for them to be thrown and handled as part of the createComponents flow. This commit moves the creation of the SSLService to XPackPlugin.createComponents, and alters the sequence of some other steps to accommodate this change. Relates: elastic#44536 Backport of: elastic#49667

XPackPlugin created an SSLService within the plugin contructor. This has 2 negative consequences: 1. The service may be constructed based on a partial view of settings. Other plugins are free to add setting values via the additionalSettings() method, but this (necessarily) happens after plugins have been constructed. 2. Any exceptions thrown during the plugin construction are handled differently than exceptions thrown during "createComponents". Since SSL configurations exceptions are relatively common, it is far preferable for them to be thrown and handled as part of the createComponents flow. This commit moves the creation of the SSLService to XPackPlugin.createComponents, and alters the sequence of some other steps to accommodate this change. Backport of: #49667

This removes the old `SSLService(Settings, Environment)` constructor and converts all uses cases to the `SSLService(Environment)` constructor that was added in #49667

XPackPlugin created an SSLService within the plugin contructor. This has 2 negative consequences: 1. The service may be constructed based on a partial view of settings. Other plugins are free to add setting values via the additionalSettings() method, but this (necessarily) happens after plugins have been constructed. 2. Any exceptions thrown during the plugin construction are handled differently than exceptions thrown during "createComponents". Since SSL configurations exceptions are relatively common, it is far preferable for them to be thrown and handled as part of the createComponents flow. This commit moves the creation of the SSLService to XPackPlugin.createComponents, and alters the sequence of some other steps to accommodate this change. Relates: elastic#44536

This removes the old `SSLService(Settings, Environment)` constructor and converts all uses cases to the `SSLService(Environment)` constructor that was added in elastic#49667

tvernum added >enhancement :Security/TLS SSL/TLS, Certificates v8.0.0 v7.6.0 labels Nov 28, 2019

tvernum requested review from rjernst and albertzaharovits November 28, 2019 03:14

rjernst approved these changes Dec 5, 2019

View reviewed changes

albertzaharovits approved these changes Dec 6, 2019

View reviewed changes

tvernum added 2 commits December 17, 2019 16:59

Merge branch 'master' into defer-ssl-service-constuct

0699f40

Fix typo

37d4996

tvernum merged commit 046595d into elastic:master Dec 17, 2019

tvernum added the backport pending label Dec 17, 2019

tvernum mentioned this pull request Dec 19, 2019

Remove obsolete constructor from SSLService #50347

Merged

tvernum mentioned this pull request Dec 30, 2019

[Backport 7.x] Do not load SSLService in plugin contructor #50519

Merged

tvernum removed the backport pending label Dec 30, 2019

tvernum mentioned this pull request Jan 30, 2020

Add IDP Configuration settings and related tests #51682

Merged

This was referenced Feb 3, 2020

[meta] 7.6 release elastic/elasticsearch-net#4340

Closed

[meta] 7.6 release elastic/elasticsearch-net#4341

Closed

jkakavas mentioned this pull request Feb 12, 2020

Test adjustments for FIPS 140 in Java 8 #52211

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not load SSLService in plugin contructor #49667

Do not load SSLService in plugin contructor #49667

tvernum commented Nov 28, 2019

elasticmachine commented Nov 28, 2019

tvernum commented Nov 28, 2019

rjernst left a comment

rjernst Dec 5, 2019

tvernum Dec 17, 2019

rjernst Dec 5, 2019

rjernst Dec 5, 2019

tvernum Dec 17, 2019

albertzaharovits left a comment

Do not load SSLService in plugin contructor #49667

Do not load SSLService in plugin contructor #49667

Conversation

tvernum commented Nov 28, 2019

elasticmachine commented Nov 28, 2019

tvernum commented Nov 28, 2019

rjernst left a comment

Choose a reason for hiding this comment

rjernst Dec 5, 2019

Choose a reason for hiding this comment

tvernum Dec 17, 2019

Choose a reason for hiding this comment

rjernst Dec 5, 2019

Choose a reason for hiding this comment

rjernst Dec 5, 2019

Choose a reason for hiding this comment

tvernum Dec 17, 2019

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment