From cad0f6bf281d99a85047580cd0338b307bf8b77f Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Mon, 30 Dec 2019 14:42:32 +1100 Subject: [PATCH] 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 --- .../elasticsearch/xpack/core/XPackPlugin.java | 19 ++++++++---- .../xpack/core/ssl/SSLService.java | 8 +++++ .../xpack/security/Security.java | 30 +++++++++++-------- 3 files changed, 40 insertions(+), 17 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java index 60584de13f6..23424a4ed39 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java @@ -135,11 +135,11 @@ public class XPackPlugin extends XPackClientPlugin implements ExtensiblePlugin, 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 @@ public class XPackPlugin extends XPackClientPlugin implements ExtensiblePlugin, 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 class XPackPlugin extends XPackClientPlugin implements ExtensiblePlugin, NodeEnvironment nodeEnvironment, NamedWriteableRegistry namedWriteableRegistry) { List 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()); diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java index e79583589d1..ce61422d0a0 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ssl/SSLService.java @@ -128,6 +128,14 @@ public class SSLService { private final SetOnce 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 diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java index d9d03dc670f..7b22659c407 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java @@ -294,7 +294,7 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw private final SetOnce securityIndex = new SetOnce<>(); private final SetOnce groupFactory = new SetOnce<>(); private final SetOnce dlsBitsetCache = new SetOnce<>(); - private final List bootstrapChecks; + private final SetOnce> bootstrapChecks = new SetOnce<>(); private final List securityExtensions = new ArrayList<>(); public Security(Settings settings, final Path configPath) { @@ -302,25 +302,20 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw } Security(Settings settings, final Path configPath, List 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 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 class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw } 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 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw 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 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 components = new ArrayList<>(); securityContext.set(new SecurityContext(settings, threadPool.getThreadContext())); @@ -698,7 +704,7 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw @Override public List getBootstrapChecks() { - return bootstrapChecks; + return bootstrapChecks.get(); } @Override