-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())); | ||
|
@@ -646,7 +652,7 @@ public List<String> getSettingsFilter() { | |
|
||
@Override | ||
public List<BootstrapCheck> getBootstrapChecks() { | ||
return bootstrapChecks; | ||
return bootstrapChecks.get(); | ||
} | ||
|
||
@Override | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 :(