Do not load SSLService in plugin contructor (#50519)

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

Author: tvernum

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

@@ -135,11 +135,11 @@ public XPackPlugin(
             final Settings settings,
             final Path configPath) {
         super(settings);
+        // FIXME: The settings might be changed after this (e.g. from "additionalSettings" method in other plugins)
+        // We should only depend on the settings from the Environment object passed to createComponents
         this.settings = settings;
         this.transportClientMode = transportClientMode(settings);
-        Environment env = transportClientMode ? null : new Environment(settings, configPath);
 
-        setSslService(new SSLService(settings, env));
         setLicenseState(new XPackLicenseState(settings));
 
         this.licensing = new Licensing(settings);
@@ -156,7 +156,14 @@ protected Clock getClock() {
     protected void setSslService(SSLService sslService) { XPackPlugin.sslService.set(sslService); }
     protected void setLicenseService(LicenseService licenseService) { XPackPlugin.licenseService.set(licenseService); }
     protected void setLicenseState(XPackLicenseState licenseState) { XPackPlugin.licenseState.set(licenseState); }
-    public static SSLService getSharedSslService() { return sslService.get(); }
+
+    public static SSLService getSharedSslService() {
+        final SSLService ssl = XPackPlugin.sslService.get();
+        if (ssl == null) {
+            throw new IllegalStateException("SSL Service is not constructed yet");
+        }
+        return ssl;
+    }
     public static LicenseService getSharedLicenseService() { return licenseService.get(); }
     public static XPackLicenseState getSharedLicenseState() { return licenseState.get(); }
 
@@ -249,14 +256,16 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
                                                NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) {
         List<Object> components = new ArrayList<>();
 
+        final SSLService sslService = new SSLService(environment);
+        setSslService(sslService);
         // just create the reloader as it will pull all of the loaded ssl configurations and start watching them
-        new SSLConfigurationReloader(environment, getSslService(), resourceWatcherService);
+        new SSLConfigurationReloader(environment, sslService, resourceWatcherService);
 
         setLicenseService(new LicenseService(settings, clusterService, getClock(),
                 environment, resourceWatcherService, getLicenseState()));
 
         // It is useful to override these as they are what guice is injecting into actions
-        components.add(getSslService());
+        components.add(sslService);
         components.add(getLicenseService());
         components.add(getLicenseState());
 

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

@@ -128,6 +128,14 @@ public class SSLService {
     private final SetOnce<SSLConfiguration> transportSSLConfiguration = new SetOnce<>();
     private final Environment env;
 
+    /**
+     * Create a new SSLService using the {@code Settings} from {@link Environment#settings()}.
+     * @see #SSLService(Settings, Environment)
+     */
+    public SSLService(Environment environment) {
+        this(environment.settings(), environment);
+    }
+
     /**
      * Create a new SSLService that parses the settings for the ssl contexts that need to be created, creates them, and then caches them
      * for use later

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

@@ -294,33 +294,28 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
     private final SetOnce<SecurityIndexManager> securityIndex = new SetOnce<>();
     private final SetOnce<NioGroupFactory> groupFactory = new SetOnce<>();
     private final SetOnce<DocumentSubsetBitsetCache> dlsBitsetCache = new SetOnce<>();
-    private final List<BootstrapCheck> bootstrapChecks;
+    private final SetOnce<List<BootstrapCheck>> bootstrapChecks = new SetOnce<>();
     private final List<SecurityExtension> securityExtensions = new ArrayList<>();
 
     public Security(Settings settings, final Path configPath) {
         this(settings, configPath, Collections.emptyList());
     }
 
     Security(Settings settings, final Path configPath, List<SecurityExtension> extensions) {
+        // TODO This is wrong. Settings can change after this. We should use the settings from createComponents
         this.settings = settings;
         this.transportClientMode = XPackPlugin.transportClientMode(settings);
+        // TODO this is wrong, we should only use the environment that is provided to createComponents
         this.env = transportClientMode ? null : new Environment(settings, configPath);
         this.enabled = XPackSettings.SECURITY_ENABLED.get(settings);
         if (enabled && transportClientMode == false) {
             runStartupChecks(settings);
             // we load them all here otherwise we can't access secure settings since they are closed once the checks are
             // fetched
-            final List<BootstrapCheck> checks = new ArrayList<>();
-            checks.addAll(Arrays.asList(
-                new ApiKeySSLBootstrapCheck(),
-                new TokenSSLBootstrapCheck(),
-                new PkiRealmBootstrapCheck(getSslService()),
-                new TLSLicenseBootstrapCheck()));
-            checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
-            this.bootstrapChecks = Collections.unmodifiableList(checks);
+
             Automatons.updateConfiguration(settings);
         } else {
-            this.bootstrapChecks = Collections.emptyList();
+            this.bootstrapChecks.set(Collections.emptyList());
         }
         this.securityExtensions.addAll(extensions);
 
@@ -346,7 +341,7 @@ public Collection<Module> createGuiceModules() {
             }
             modules.add(b -> {
                 // for transport client we still must inject these ssl classes with guice
-                b.bind(SSLService.class).toInstance(getSslService());
+                b.bind(SSLService.class).toProvider(this::getSslService);
             });
 
             return modules;
@@ -403,6 +398,17 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
             return Collections.emptyList();
         }
 
+        // We need to construct the checks here while the secure settings are still available.
+        // If we wait until #getBoostrapChecks the secure settings will have been cleared/closed.
+        final List<BootstrapCheck> checks = new ArrayList<>();
+        checks.addAll(Arrays.asList(
+            new ApiKeySSLBootstrapCheck(),
+            new TokenSSLBootstrapCheck(),
+            new PkiRealmBootstrapCheck(getSslService()),
+            new TLSLicenseBootstrapCheck()));
+        checks.addAll(InternalRealms.getBootstrapChecks(settings, env));
+        this.bootstrapChecks.set(Collections.unmodifiableList(checks));
+
         threadContext.set(threadPool.getThreadContext());
         List<Object> components = new ArrayList<>();
         securityContext.set(new SecurityContext(settings, threadPool.getThreadContext()));
@@ -698,7 +704,7 @@ public List<String> getSettingsFilter() {
 
     @Override
     public List<BootstrapCheck> getBootstrapChecks() {
-       return bootstrapChecks;
+       return bootstrapChecks.get();
     }
 
     @Override