From 6b7555bf2fab598119a3392d4ba08925e615e193 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Thu, 28 Nov 2019 14:00:11 +1100 Subject: [PATCH 1/2] Do not load SSLService in plugin contructor 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: #44536 --- .../elasticsearch/xpack/core/XPackPlugin.java | 19 +++++++++---- .../xpack/core/ssl/SSLService.java | 8 ++++++ .../xpack/security/Security.java | 28 +++++++++++-------- 3 files changed, 39 insertions(+), 16 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 1b20ceae9233e..f14bf94d3b56f 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 @@ -134,10 +134,10 @@ 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; - Environment env = new Environment(settings, configPath); - setSslService(new SSLService(settings, env)); setLicenseState(new XPackLicenseState(settings)); this.licensing = new Licensing(settings); @@ -154,7 +154,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(); } @@ -225,14 +232,16 @@ public Collection createComponents(Client client, ClusterService cluster 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 803f5fb856476..a788090c831d9 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 @@ -122,6 +122,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 fe8ff8f5903b7..787cf6eb4913c 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 @@ -293,7 +293,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) { @@ -301,24 +301,19 @@ public Security(Settings settings, final Path configPath) { } 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; + // TODO this is wrong, we should only use the environment that is provided to createComponents this.env = new Environment(settings, configPath); this.enabled = XPackSettings.SECURITY_ENABLED.get(settings); if (enabled) { 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); @@ -358,6 +353,17 @@ Collection createComponents(Client client, ThreadPool threadPool, Cluste 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. + 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())); @@ -646,7 +652,7 @@ public List getSettingsFilter() { @Override public List getBootstrapChecks() { - return bootstrapChecks; + return bootstrapChecks.get(); } @Override From 37d4996ec777007a9a4c44a290b4f5df513fbd48 Mon Sep 17 00:00:00 2001 From: Tim Vernum Date: Tue, 17 Dec 2019 17:16:03 +1100 Subject: [PATCH 2/2] Fix typo --- .../main/java/org/elasticsearch/xpack/security/Security.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 787cf6eb4913c..3b24042f47efb 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 @@ -354,7 +354,7 @@ Collection createComponents(Client client, ThreadPool threadPool, Cluste } // 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. + // If we wait until #getBoostrapChecks the secure settings will have been cleared/closed. final List checks = new ArrayList<>(); checks.addAll(Arrays.asList( new ApiKeySSLBootstrapCheck(),