Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not load SSLService in plugin contructor #49667

Merged
merged 3 commits into from
Dec 17, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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(); }

Expand Down Expand Up @@ -225,14 +232,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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -293,32 +293,27 @@ 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;
// 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<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);

Expand Down Expand Up @@ -358,6 +353,17 @@ Collection<Object> 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: want -> wait

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()));
Expand Down Expand Up @@ -646,7 +652,7 @@ public List<String> getSettingsFilter() {

@Override
public List<BootstrapCheck> getBootstrapChecks() {
return bootstrapChecks;
return bootstrapChecks.get();
}

@Override
Expand Down