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

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

Draft
wants to merge 46 commits into
base: feature/resource-permissions
Choose a base branch
from
Draft

[DRAFT] [Feature] Introduces resource sharing and access-control #4746

Show file tree
Hide file tree
Changes from 40 commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
831dca3
Add a base setup for resource access evaluation
DarshitChanpura Aug 30, 2024
1c65eff
Adds handler and other access management components for resource sharing
DarshitChanpura Sep 6, 2024
118cb07
Adds sample resource plugin
DarshitChanpura Sep 6, 2024
c41b67d
Removes node_modules entry from gitingore
DarshitChanpura Sep 6, 2024
45d4fa5
Handles changes related to scope
DarshitChanpura Oct 4, 2024
ae2464d
Updates sample plugin to implement a custom scope
DarshitChanpura Oct 4, 2024
aea2253
Fixes Checkstyle and spotless issues
DarshitChanpura Oct 4, 2024
1e17dc0
Fixes initialization error
DarshitChanpura Oct 4, 2024
84746e8
Renames sample resource plugin and adds a logger statement
DarshitChanpura Oct 4, 2024
83e4da0
Changes package name for sample plugin
DarshitChanpura Oct 4, 2024
4b9b9b1
Re-organizes and renames sample plugin files
DarshitChanpura Oct 4, 2024
81216f1
Updates method references to conform to core
DarshitChanpura Oct 4, 2024
1e33dad
Fixes compile errors
DarshitChanpura Oct 4, 2024
a671cc1
Fixes some names and method implementations
DarshitChanpura Oct 14, 2024
47b73da
Adds few concrete method implementations in security plugin
DarshitChanpura Oct 14, 2024
8942a80
Adds capability to introduce index listeners for all resource plugins
DarshitChanpura Oct 15, 2024
2b06603
Removes sampleplugin to be added in a separate PR
DarshitChanpura Nov 11, 2024
6f42bf1
Updates settings.gradle
DarshitChanpura Nov 11, 2024
45b002e
Fixes imports
DarshitChanpura Nov 20, 2024
a30be57
Adds concrete implementations of remainder methods
DarshitChanpura Nov 27, 2024
58003f6
Fixes spotless errors
DarshitChanpura Nov 27, 2024
078a976
Fixes log statement
DarshitChanpura Nov 27, 2024
8e44cf3
Renames ResourceManagement repository and add keyword to search query…
DarshitChanpura Dec 4, 2024
16a0ba6
Fixes delete method
DarshitChanpura Dec 4, 2024
ac53c8f
Fixes updateByQuery painless script
DarshitChanpura Dec 5, 2024
9e6ae85
Updates revoke handler to use painless script
DarshitChanpura Dec 5, 2024
0fe9779
Convert sets to lists
DarshitChanpura Dec 5, 2024
6d7f4c0
Explicitly casts painless entries to set to avoid duplicates
DarshitChanpura Dec 6, 2024
9d4ca1e
Fixes revoke access script
DarshitChanpura Dec 6, 2024
b4b22d6
Fixes revoke script to handle duplicates
DarshitChanpura Dec 6, 2024
e87bb80
Updates logger statement
DarshitChanpura Dec 7, 2024
5ad813b
Adds validation for resource ownership when granting and revoking access
DarshitChanpura Dec 7, 2024
c08a992
Adds super-admin bypass
DarshitChanpura Dec 10, 2024
8e3d41c
Updates method names and adds missing java doc
DarshitChanpura Dec 11, 2024
cabbcd6
Updates methods to return actual resources instead of resource ids
DarshitChanpura Dec 12, 2024
dc964ac
Stash context to fetch resources from a system index
DarshitChanpura Dec 13, 2024
cc973c6
Optimize call to fetch resources
DarshitChanpura Dec 13, 2024
3ce3d92
Updates javadoc
DarshitChanpura Dec 13, 2024
428e11e
Adds input validation
DarshitChanpura Dec 13, 2024
a55ac22
Adds auditlog capability and conform to changes in core
DarshitChanpura Dec 20, 2024
1c62d36
Conforms to type bounding change introduced in core
DarshitChanpura Dec 20, 2024
6a23f46
Merge remote-tracking branch 'upstream/main' into resource-access-con…
DarshitChanpura Dec 20, 2024
c24323c
Stashes context while fetching resource sharing record
DarshitChanpura Dec 20, 2024
f514859
Fixes accessDeclaredMembers error caused in AuditConfig class
DarshitChanpura Dec 20, 2024
193e846
Changes log levels and improves log statements
DarshitChanpura Dec 20, 2024
13fdb81
Bring user notion to security plugin
DarshitChanpura Dec 31, 2024
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
4 changes: 0 additions & 4 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,3 @@ out/
build/
gradle-build/
.gradle/

# nodejs
node_modules/
package-lock.json
Comment on lines -48 to -49
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

it was automatically added. But we don't add node_modules and package-lock since we don't have any front-end components in this project.

114 changes: 103 additions & 11 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand All @@ -68,6 +69,10 @@
import org.opensearch.OpenSearchSecurityException;
import org.opensearch.SpecialPermission;
import org.opensearch.Version;
import org.opensearch.accesscontrol.resources.EntityType;
import org.opensearch.accesscontrol.resources.ResourceService;
import org.opensearch.accesscontrol.resources.ResourceSharing;
import org.opensearch.accesscontrol.resources.ShareWith;
import org.opensearch.action.ActionRequest;
import org.opensearch.action.search.PitService;
import org.opensearch.action.search.SearchScrollAction;
Expand Down Expand Up @@ -120,6 +125,8 @@
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.MapperPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.plugins.ResourceAccessControlPlugin;
import org.opensearch.plugins.ResourcePlugin;
import org.opensearch.plugins.SecureHttpTransportSettingsProvider;
import org.opensearch.plugins.SecureSettingsFactory;
import org.opensearch.plugins.SecureTransportSettingsProvider;
Expand Down Expand Up @@ -173,6 +180,10 @@
import org.opensearch.security.privileges.RestLayerPrivilegesEvaluator;
import org.opensearch.security.privileges.dlsfls.DlsFlsBaseContext;
import org.opensearch.security.resolver.IndexResolverReplacer;
import org.opensearch.security.resources.ResourceAccessHandler;
import org.opensearch.security.resources.ResourceSharingIndexHandler;
import org.opensearch.security.resources.ResourceSharingIndexListener;
import org.opensearch.security.resources.ResourceSharingIndexManagementRepository;
import org.opensearch.security.rest.DashboardsInfoAction;
import org.opensearch.security.rest.SecurityConfigUpdateAction;
import org.opensearch.security.rest.SecurityHealthAction;
Expand Down Expand Up @@ -230,9 +241,10 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
implements
ClusterPlugin,
MapperPlugin,
IdentityPlugin,
ResourceAccessControlPlugin,
// CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
ExtensionAwarePlugin,
IdentityPlugin
ExtensionAwarePlugin
// CS-ENFORCE-SINGLE

{
Expand Down Expand Up @@ -268,6 +280,9 @@ public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
private volatile OpensearchDynamicSetting<Boolean> transportPassiveAuthSetting;
private volatile PasswordHasher passwordHasher;
private volatile DlsFlsBaseContext dlsFlsBaseContext;
private ResourceSharingIndexManagementRepository rmr;
private ResourceAccessHandler resourceAccessHandler;
private final Set<String> indicesToListen = new HashSet<>();

public static boolean isActionTraceEnabled() {

Expand Down Expand Up @@ -708,6 +723,14 @@ public void onIndexModule(IndexModule indexModule) {
dlsFlsBaseContext
)
);

if (this.indicesToListen.contains(indexModule.getIndex().getName())) {
ResourceSharingIndexListener resourceSharingIndexListener = ResourceSharingIndexListener.getInstance();
resourceSharingIndexListener.initialize(threadPool, localClient, auditLog);
indexModule.addIndexOperationListener(resourceSharingIndexListener);
log.warn("Security plugin started listening to operations on index {}", indexModule.getIndex().getName());
}

indexModule.forceQueryCacheProvider((indexSettings, nodeCache) -> new QueryCache() {

@Override
Expand Down Expand Up @@ -1183,14 +1206,25 @@ public Collection<Object> createComponents(

// NOTE: We need to create DefaultInterClusterRequestEvaluator before creating ConfigurationRepository since the latter requires
// security index to be accessible which means
// communciation with other nodes is already up. However for the communication to be up, there needs to be trusted nodes_dn. Hence
// communication with other nodes is already up. However for the communication to be up, there needs to be trusted nodes_dn. Hence
// the base values from opensearch.yml
// is used to first establish trust between same cluster nodes and there after dynamic config is loaded if enabled.
if (DEFAULT_INTERCLUSTER_REQUEST_EVALUATOR_CLASS.equals(className)) {
DefaultInterClusterRequestEvaluator e = (DefaultInterClusterRequestEvaluator) interClusterRequestEvaluator;
e.subscribeForChanges(dcf);
}

final var resourceSharingIndex = ConfigConstants.OPENSEARCH_RESOURCE_SHARING_INDEX;
ResourceSharingIndexHandler rsIndexHandler = new ResourceSharingIndexHandler(
resourceSharingIndex,
localClient,
threadPool,
auditLog
);
resourceAccessHandler = new ResourceAccessHandler(threadPool, rsIndexHandler, adminDns);

rmr = ResourceSharingIndexManagementRepository.create(rsIndexHandler);

components.add(adminDns);
components.add(cr);
components.add(xffResolver);
Expand Down Expand Up @@ -1367,7 +1401,7 @@ public List<Setting<?>> getSettings() {

settings.add(Setting.simpleString(ConfigConstants.SECURITY_CONFIG_INDEX_NAME, Property.NodeScope, Property.Filtered));
settings.add(Setting.groupSetting(ConfigConstants.SECURITY_AUTHCZ_IMPERSONATION_DN + ".", Property.NodeScope)); // not filtered
// here
// here

settings.add(Setting.simpleString(ConfigConstants.SECURITY_CERT_OID, Property.NodeScope, Property.Filtered));

Expand All @@ -1383,8 +1417,8 @@ public List<Setting<?>> getSettings() {
);// not filtered here

settings.add(Setting.boolSetting(ConfigConstants.SECURITY_NODES_DN_DYNAMIC_CONFIG_ENABLED, false, Property.NodeScope));// not
// filtered
// here
// filtered
// here

settings.add(
Setting.boolSetting(
Expand Down Expand Up @@ -1428,8 +1462,8 @@ public List<Setting<?>> getSettings() {
Setting.boolSetting(ConfigConstants.SECURITY_DFM_EMPTY_OVERRIDES_ALL, false, Property.NodeScope, Property.Filtered)
);
settings.add(Setting.groupSetting(ConfigConstants.SECURITY_AUTHCZ_REST_IMPERSONATION_USERS + ".", Property.NodeScope)); // not
// filtered
// here
// filtered
// here

settings.add(Setting.simpleString(ConfigConstants.SECURITY_ROLES_MAPPING_RESOLUTION, Property.NodeScope, Property.Filtered));
settings.add(
Expand Down Expand Up @@ -2065,6 +2099,17 @@ public void onNodeStarted(DiscoveryNode localNode) {
if (!SSLConfig.isSslOnlyMode() && !client && !disabled && !useClusterStateToInitSecurityConfig(settings)) {
cr.initOnNodeStart();
}

// create resource sharing index if absent
rmr.createResourceSharingIndexIfAbsent();

for (ResourcePlugin resourcePlugin : OpenSearchSecurityPlugin.GuiceHolder.getResourceService().listResourcePlugins()) {
String resourceIndex = resourcePlugin.getResourceIndex();

this.indicesToListen.add(resourceIndex);
log.info("Preparing to listen to index: {} of plugin: {}", resourceIndex, resourcePlugin);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, debug?

Copy link
Member Author

@DarshitChanpura DarshitChanpura Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Changed it to warn. Good catch.

}

final Set<ModuleInfo> securityModules = ReflectionHelper.getModulesLoaded();
log.info("{} OpenSearch Security modules loaded so far: {}", securityModules.size(), securityModules);
}
Expand All @@ -2082,6 +2127,7 @@ public Collection<Class<? extends LifecycleComponent>> getGuiceServiceClasses()

final List<Class<? extends LifecycleComponent>> services = new ArrayList<>(1);
services.add(GuiceHolder.class);
log.info("Guice service classes loaded");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to change this to something lower like debug?

Copy link
Member Author

@DarshitChanpura DarshitChanpura Dec 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Good catch. I will remove it.

return services;
}

Expand Down Expand Up @@ -2109,8 +2155,12 @@ public Collection<SystemIndexDescriptor> getSystemIndexDescriptors(Settings sett
ConfigConstants.SECURITY_CONFIG_INDEX_NAME,
ConfigConstants.OPENDISTRO_SECURITY_DEFAULT_CONFIG_INDEX
);
final SystemIndexDescriptor systemIndexDescriptor = new SystemIndexDescriptor(indexPattern, "Security index");
return Collections.singletonList(systemIndexDescriptor);
final SystemIndexDescriptor securityIndexDescriptor = new SystemIndexDescriptor(indexPattern, "Security index");
final SystemIndexDescriptor resourceSharingIndexDescriptor = new SystemIndexDescriptor(
ConfigConstants.OPENSEARCH_RESOURCE_SHARING_INDEX,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Do we want to add a TODO to support custom resource sharing names?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not right now no. Maybe in the next iteration if there is a request for it.

"Resource Sharing index"
);
return List.of(securityIndexDescriptor, resourceSharingIndexDescriptor);
}

@Override
Expand Down Expand Up @@ -2166,12 +2216,48 @@ private void tryAddSecurityProvider() {
});
}

@Override
public <T> Set<T> getAccessibleResourcesForCurrentUser(String systemIndexName, Class<T> clazz) {
return this.resourceAccessHandler.getAccessibleResourcesForCurrentUser(systemIndexName, clazz);
}

@Override
public boolean hasPermission(String resourceId, String systemIndexName, String scope) {
return this.resourceAccessHandler.hasPermission(resourceId, systemIndexName, scope);
}

@Override
public ResourceSharing shareWith(String resourceId, String systemIndexName, ShareWith shareWith) {
return this.resourceAccessHandler.shareWith(resourceId, systemIndexName, shareWith);
}

@Override
public ResourceSharing revokeAccess(
String resourceId,
String systemIndexName,
Map<EntityType, Set<String>> entities,
Set<String> scopes
) {
return this.resourceAccessHandler.revokeAccess(resourceId, systemIndexName, entities, scopes);
}

@Override
public boolean deleteResourceSharingRecord(String resourceId, String systemIndexName) {
return this.resourceAccessHandler.deleteResourceSharingRecord(resourceId, systemIndexName);
}

@Override
public boolean deleteAllResourceSharingRecordsForCurrentUser() {
return this.resourceAccessHandler.deleteAllResourceSharingRecordsForCurrentUser();
}

public static class GuiceHolder implements LifecycleComponent {

private static RepositoriesService repositoriesService;
private static RemoteClusterService remoteClusterService;
private static IndicesService indicesService;
private static PitService pitService;
private static ResourceService resourceService;

// CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions
private static ExtensionsManager extensionsManager;
Expand All @@ -2182,13 +2268,15 @@ public GuiceHolder(
final TransportService remoteClusterService,
IndicesService indicesService,
PitService pitService,
ExtensionsManager extensionsManager
ExtensionsManager extensionsManager,
ResourceService resourceService
) {
GuiceHolder.repositoriesService = repositoriesService;
GuiceHolder.remoteClusterService = remoteClusterService.getRemoteClusterService();
GuiceHolder.indicesService = indicesService;
GuiceHolder.pitService = pitService;
GuiceHolder.extensionsManager = extensionsManager;
GuiceHolder.resourceService = resourceService;
}
// CS-ENFORCE-SINGLE

Expand All @@ -2214,6 +2302,10 @@ public static ExtensionsManager getExtensionsManager() {
}
// CS-ENFORCE-SINGLE

public static ResourceService getResourceService() {
return resourceService;
}

@Override
public void close() {}

Expand Down
4 changes: 4 additions & 0 deletions src/main/java/org/opensearch/security/auth/BackendRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
if (adminDns.isAdminDN(sslPrincipal)) {
// PKI authenticated REST call
threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User(sslPrincipal));
threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, new User(sslPrincipal));
auditLog.logSucceededLogin(sslPrincipal, true, null, request);
return true;
}
Expand Down Expand Up @@ -389,6 +390,8 @@ public boolean authenticate(final SecurityRequestChannel request) {
final User impersonatedUser = impersonate(request, authenticatedUser);
threadPool.getThreadContext()
.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, impersonatedUser == null ? authenticatedUser : impersonatedUser);
threadPool.getThreadContext()
.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, impersonatedUser == null ? authenticatedUser : impersonatedUser);
auditLog.logSucceededLogin(
(impersonatedUser == null ? authenticatedUser : impersonatedUser).getName(),
false,
Expand Down Expand Up @@ -422,6 +425,7 @@ public boolean authenticate(final SecurityRequestChannel request) {
anonymousUser.setRequestedTenant(tenant);

threadPool.getThreadContext().putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser);
threadPool.getThreadContext().putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, anonymousUser);
auditLog.logSucceededLogin(anonymousUser.getName(), false, null, request);
if (isDebugEnabled) {
log.debug("Anonymous User is authenticated");
Expand Down
1 change: 1 addition & 0 deletions src/main/java/org/opensearch/security/filter/SecurityFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,7 @@ private <Request extends ActionRequest, Response extends ActionResponse> void ap
log.info("Transport auth in passive mode and no user found. Injecting default user");
user = User.DEFAULT_TRANSPORT_USER;
threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, user);
threadContext.putPersistent(ConfigConstants.OPENDISTRO_SECURITY_USER, user);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed a few of these putPersistent being added. I do not know the full context, but is there any security implications with putting it in persistently? For example maybe stashing the context wouldn't work if the user is persisted into the threadcontext. Any ideas on this usage @cwperks ?

Copy link
Member

Choose a reason for hiding this comment

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

I want to add the same code in #4896. I added the notion of persistent TC headers in the core a while ago with the intent that these headers are not stashable and I'd like to move to the authenticated user as a non-stashable header.

Its added here because Security is adding an IndexOperationListener on the resourceIndex and the ThreadContext has already been stashed by the sample plugin so that it can write to the resourceIndex. By adding this call here, @DarshitChanpura is guaranteed that he can read the userinfo from the TC inside the IndexOperationListener.

I believe the postIndex hook is running within the same block as the IndexRequest to the resourceIndex.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 to @cwperks answer. User names are particularly required when creating or listing resources. This would ensure that a user who is current context-user is always present and thus not result in null value. At some point in future, the putTransient calls will be removed completely.

} else {
log.error(
"No user found for "
Expand Down
Loading
Loading