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.

Sign up for GitHub

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

Security: Remove SecurityLifecycleService #30526

Merged
merged 1 commit into from
May 15, 2018
Merged

Security: Remove SecurityLifecycleService #30526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
33 changes: 17 additions & 16 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import org.apache.logging.log4j.Logger;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.ElasticsearchTimeoutException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRequest;
Expand All @@ -16,6 +17,7 @@
import org.elasticsearch.bootstrap.BootstrapCheck;
import org.elasticsearch.client.Client;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.health.ClusterHealthStatus;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.IndexNameExpressionResolver;
import org.elasticsearch.cluster.metadata.IndexTemplateMetaData;
Expand Down Expand Up @@ -232,7 +234,7 @@
import static org.elasticsearch.cluster.metadata.IndexMetaData.INDEX_FORMAT_SETTING;
import static org.elasticsearch.xpack.core.XPackSettings.HTTP_SSL_ENABLED;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_TEMPLATE_NAME;
import static org.elasticsearch.xpack.security.SecurityLifecycleService.SECURITY_INDEX_NAME;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.SECURITY_INDEX_NAME;
import static org.elasticsearch.xpack.security.support.SecurityIndexManager.INTERNAL_INDEX_FORMAT;

public class Security extends Plugin implements ActionPlugin, IngestPlugin, NetworkPlugin, ClusterPlugin,
Expand Down Expand Up @@ -261,6 +263,8 @@ public class Security extends Plugin implements ActionPlugin, IngestPlugin, Netw
private final SetOnce<ThreadContext> threadContext = new SetOnce<>();
private final SetOnce<TokenService> tokenService = new SetOnce<>();
private final SetOnce<SecurityActionFilter> securityActionFilter = new SetOnce<>();
private final SetOnce<SecurityIndexManager> securityIndex = new SetOnce<>();
Copy link
Member

Choose a reason for hiding this comment

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

this is a nit and was there before this change, but maybe we should call this securityIndexManager everywhere instead of securityIndex

Copy link
Member Author

Choose a reason for hiding this comment

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

I actually like securityIndex, since the methods all reflect the state of the index, not the manager itself.

private final SetOnce<IndexAuditTrail> indexAuditTrail = new SetOnce<>();
Copy link
Member

Choose a reason for hiding this comment

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

also why are we using a setonce for these? It looks like we only use them in the createComponents method currently?

Copy link
Member Author

Choose a reason for hiding this comment

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

These will be used in a followup, to wait on their readiness in onNodeStarted()

private final List<BootstrapCheck> bootstrapChecks;
private final List<SecurityExtension> securityExtensions = new ArrayList<>();

Expand Down Expand Up @@ -368,7 +372,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(securityContext.get());

// audit trails construction
IndexAuditTrail indexAuditTrail = null;
Set<AuditTrail> auditTrails = new LinkedHashSet<>();
if (XPackSettings.AUDIT_ENABLED.get(settings)) {
List<String> outputs = AUDIT_OUTPUTS_SETTING.get(settings);
Expand All @@ -383,8 +386,8 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
auditTrails.add(new LoggingAuditTrail(settings, clusterService, threadPool));
break;
case IndexAuditTrail.NAME:
indexAuditTrail = new IndexAuditTrail(settings, client, threadPool, clusterService);
auditTrails.add(indexAuditTrail);
indexAuditTrail.set(new IndexAuditTrail(settings, client, threadPool, clusterService));
auditTrails.add(indexAuditTrail.get());
break;
default:
throw new IllegalArgumentException("Unknown audit trail output [" + output + "]");
Expand All @@ -396,20 +399,20 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(auditTrailService);
this.auditTrailService.set(auditTrailService);

final SecurityLifecycleService securityLifecycleService =
new SecurityLifecycleService(settings, clusterService, threadPool, client, indexAuditTrail);
final TokenService tokenService = new TokenService(settings, Clock.systemUTC(), client, securityLifecycleService, clusterService);
securityIndex.set(new SecurityIndexManager(settings, client, SecurityIndexManager.SECURITY_INDEX_NAME, clusterService));

final TokenService tokenService = new TokenService(settings, Clock.systemUTC(), client, securityIndex.get(), clusterService);
this.tokenService.set(tokenService);
components.add(tokenService);

// realms construction
final NativeUsersStore nativeUsersStore = new NativeUsersStore(settings, client, securityLifecycleService);
final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(settings, client, securityLifecycleService);
final NativeUsersStore nativeUsersStore = new NativeUsersStore(settings, client, securityIndex.get());
final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(settings, client, securityIndex.get());
final AnonymousUser anonymousUser = new AnonymousUser(settings);
final ReservedRealm reservedRealm = new ReservedRealm(env, settings, nativeUsersStore,
anonymousUser, securityLifecycleService, threadPool.getThreadContext());
anonymousUser, securityIndex.get(), threadPool.getThreadContext());
Map<String, Realm.Factory> realmFactories = new HashMap<>(InternalRealms.getFactories(threadPool, resourceWatcherService,
getSslService(), nativeUsersStore, nativeRoleMappingStore, securityLifecycleService));
getSslService(), nativeUsersStore, nativeRoleMappingStore, securityIndex.get()));
for (SecurityExtension extension : securityExtensions) {
Map<String, Realm.Factory> newRealms = extension.getRealms(resourceWatcherService);
for (Map.Entry<String, Realm.Factory> entry : newRealms.entrySet()) {
Expand All @@ -424,7 +427,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(realms);
components.add(reservedRealm);

securityLifecycleService.securityIndex().addIndexStateListener(nativeRoleMappingStore::onSecurityIndexStateChange);
securityIndex.get().addIndexStateListener(nativeRoleMappingStore::onSecurityIndexStateChange);

AuthenticationFailureHandler failureHandler = null;
String extensionName = null;
Expand All @@ -449,15 +452,15 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(authcService.get());

final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService, getLicenseState());
final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, getLicenseState(), securityLifecycleService);
final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, getLicenseState(), securityIndex.get());
final ReservedRolesStore reservedRolesStore = new ReservedRolesStore();
List<BiConsumer<Set<String>, ActionListener<Set<RoleDescriptor>>>> rolesProviders = new ArrayList<>();
for (SecurityExtension extension : securityExtensions) {
rolesProviders.addAll(extension.getRolesProviders(settings, resourceWatcherService));
}
final CompositeRolesStore allRolesStore = new CompositeRolesStore(settings, fileRolesStore, nativeRolesStore,
reservedRolesStore, rolesProviders, threadPool.getThreadContext(), getLicenseState());
securityLifecycleService.securityIndex().addIndexStateListener(allRolesStore::onSecurityIndexStateChange);
securityIndex.get().addIndexStateListener(allRolesStore::onSecurityIndexStateChange);
// to keep things simple, just invalidate all cached entries on license change. this happens so rarely that the impact should be
// minimal
getLicenseState().addListener(allRolesStore::invalidateAll);
Expand All @@ -468,8 +471,6 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
components.add(allRolesStore); // for SecurityFeatureSet and clear roles cache
components.add(authzService);

components.add(securityLifecycleService);

ipFilter.set(new IPFilter(settings, auditTrailService, clusterService.getClusterSettings(), getLicenseState()));
components.add(ipFilter.get());
DestructiveOperations destructiveOperations = new DestructiveOperations(settings, clusterService.getClusterSettings());
Expand Down
129 changes: 0 additions & 129 deletions ...gin/security/src/main/java/org/elasticsearch/xpack/security/SecurityLifecycleService.java
View file
Open in desktop

This file was deleted.

35 changes: 34 additions & 1 deletion .../security/src/main/java/org/elasticsearch/xpack/security/audit/index/IndexAuditTrail.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.elasticsearch.client.transport.TransportClient;
import org.elasticsearch.cluster.ClusterChangedEvent;
import org.elasticsearch.cluster.ClusterState;
import org.elasticsearch.cluster.ClusterStateListener;
import org.elasticsearch.cluster.metadata.AliasOrIndex;
import org.elasticsearch.cluster.metadata.IndexMetaData;
import org.elasticsearch.cluster.metadata.MappingMetaData;
Expand All @@ -29,12 +30,14 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.component.AbstractComponent;
import org.elasticsearch.common.component.LifecycleListener;
import org.elasticsearch.common.network.NetworkAddress;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.transport.TransportAddress;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.AbstractRunnable;
import org.elasticsearch.common.util.concurrent.EsExecutors;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.XContentBuilder;
Expand Down Expand Up @@ -110,7 +113,7 @@
/**
* Audit trail implementation that writes events into an index.
*/
public class IndexAuditTrail extends AbstractComponent implements AuditTrail {
public class IndexAuditTrail extends AbstractComponent implements AuditTrail, ClusterStateListener {

public static final String NAME = "index";
public static final String DOC_TYPE = "doc";
Expand Down Expand Up @@ -199,13 +202,43 @@ public IndexAuditTrail(Settings settings, Client client, ThreadPool threadPool,
} else {
this.client = initializeRemoteClient(settings, logger);
}
clusterService.addListener(this);
clusterService.addLifecycleListener(new LifecycleListener() {
@Override
public void beforeStop() {
stop();
}
});

}

public State state() {
return state.get();
}

@Override
public void clusterChanged(ClusterChangedEvent event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Earlier in the SecurityLifeCycleService the cluster changed event was handled in order - first, to handle the event in SecurityIndexManager later it would start the index audit trail. Is there any order defined which does not change the behavior of handling cluster event? Not sure if this would be of any concern.

Copy link
Member Author

Choose a reason for hiding this comment

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

The order should not matter. The services have no interdependencies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @rjernst

try {
if (state() == IndexAuditTrail.State.INITIALIZED && canStart(event)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have removed the condition Security.indexAuditLoggingEnabled(settings), is the check performed somewhere else or not required anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

The IndexAuditTrail object is only created if that method returns true, so it is not necessary to check within the instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, @rjernst.

threadPool.generic().execute(new AbstractRunnable() {

@Override
public void onFailure(Exception throwable) {
logger.error("failed to start index audit trail services", throwable);
assert false : "security lifecycle services startup failed";
}

@Override
public void doRun() {
start();
}
});
}
} catch (Exception e) {
logger.error("failed to start index audit trail", e);
}
}

/**
* This method determines if this service can be started based on the state in the {@link ClusterChangedEvent} and
* if the node is the master or not. When using remote indexing, a call to the remote cluster will be made to retrieve
Expand Down
4 changes: 2 additions & 2 deletions ...in/security/src/main/java/org/elasticsearch/xpack/security/authc/ExpiredTokenRemover.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
import org.elasticsearch.index.reindex.ScrollableHitSource;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.threadpool.ThreadPool.Names;
import org.elasticsearch.xpack.security.SecurityLifecycleService;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import java.time.Instant;
import java.time.temporal.ChronoUnit;
Expand Down Expand Up @@ -50,7 +50,7 @@ final class ExpiredTokenRemover extends AbstractRunnable {

@Override
public void doRun() {
SearchRequest searchRequest = new SearchRequest(SecurityLifecycleService.SECURITY_INDEX_NAME);
SearchRequest searchRequest = new SearchRequest(SecurityIndexManager.SECURITY_INDEX_NAME);
DeleteByQueryRequest expiredDbq = new DeleteByQueryRequest(searchRequest);
if (timeout != TimeValue.MINUS_ONE) {
expiredDbq.setTimeout(timeout);
Expand Down
6 changes: 3 additions & 3 deletions .../plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/InternalRealms.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.elasticsearch.xpack.core.security.authc.pki.PkiRealmSettings;
import org.elasticsearch.xpack.core.security.authc.saml.SamlRealmSettings;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.security.SecurityLifecycleService;
import org.elasticsearch.xpack.security.authc.esnative.NativeRealm;
import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore;
import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm;
Expand All @@ -30,6 +29,7 @@
import org.elasticsearch.xpack.security.authc.saml.SamlRealm;
import org.elasticsearch.xpack.security.authc.support.RoleMappingFileBootstrapCheck;
import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore;
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -90,13 +90,13 @@ static boolean isStandardRealm(String type) {
public static Map<String, Realm.Factory> getFactories(ThreadPool threadPool, ResourceWatcherService resourceWatcherService,
SSLService sslService, NativeUsersStore nativeUsersStore,
NativeRoleMappingStore nativeRoleMappingStore,
SecurityLifecycleService securityLifecycleService) {
SecurityIndexManager securityIndex) {

Map<String, Realm.Factory> map = new HashMap<>();
map.put(FileRealmSettings.TYPE, config -> new FileRealm(config, resourceWatcherService));
map.put(NativeRealmSettings.TYPE, config -> {
final NativeRealm nativeRealm = new NativeRealm(config, nativeUsersStore);
securityLifecycleService.securityIndex().addIndexStateListener(nativeRealm::onSecurityIndexStateChange);
securityIndex.addIndexStateListener(nativeRealm::onSecurityIndexStateChange);
return nativeRealm;
});
map.put(LdapRealmSettings.AD_TYPE, config -> new LdapRealm(LdapRealmSettings.AD_TYPE, config, sslService,
Expand Down
Loading