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

Feature/extensions bwc setting #3180

Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,15 @@
import org.opensearch.http.HttpServerTransport;
import org.opensearch.http.HttpServerTransport.Dispatcher;
import org.opensearch.core.index.Index;
import org.opensearch.identity.Subject;
import org.opensearch.identity.tokens.TokenManager;
import org.opensearch.index.IndexModule;
import org.opensearch.index.cache.query.QueryCache;
import org.opensearch.indices.IndicesService;
import org.opensearch.indices.SystemIndexDescriptor;
import org.opensearch.plugins.ClusterPlugin;
import org.opensearch.plugins.ExtensionAwarePlugin;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.MapperPlugin;
import org.opensearch.repositories.RepositoriesService;
import org.opensearch.rest.RestController;
Expand All @@ -128,6 +132,7 @@
import org.opensearch.security.auditlog.config.AuditConfig.Filter.FilterEntries;
import org.opensearch.security.auditlog.impl.AuditLogImpl;
import org.opensearch.security.auth.BackendRegistry;
import org.opensearch.security.auth.SecurityTokenManager;
import org.opensearch.security.compliance.ComplianceIndexingOperationListener;
import org.opensearch.security.compliance.ComplianceIndexingOperationListenerImpl;
import org.opensearch.security.configuration.AdminDNs;
Expand Down Expand Up @@ -194,7 +199,12 @@
import org.opensearch.watcher.ResourceWatcherService;
// CS-ENFORCE-SINGLE

public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin implements ClusterPlugin, MapperPlugin {
public final class OpenSearchSecurityPlugin extends OpenSearchSecuritySSLPlugin
implements
ClusterPlugin,
MapperPlugin,
ExtensionAwarePlugin,
IdentityPlugin {

private static final String KEYWORD = ".keyword";
private static final Logger actionTrace = LogManager.getLogger("opendistro_security_action_trace");
Expand Down Expand Up @@ -1111,6 +1121,21 @@ public Settings additionalSettings() {
return builder.build();
}

@Override
public List<Setting<?>> getExtensionSettings() {
List<Setting<?>> extentionSettings = new ArrayList<Setting<?>>();
samuelcostae marked this conversation as resolved.
Show resolved Hide resolved

extentionSettings.add(
Setting.boolSetting(
ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE,
ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT,
Property.ExtensionScope,
Property.Final
)
);
return extentionSettings;
}

@Override
public List<Setting<?>> getSettings() {
List<Setting<?>> settings = new ArrayList<Setting<?>>();
Expand Down Expand Up @@ -1890,6 +1915,30 @@ private static String handleKeyword(final String field) {
return field;
}

public static DiscoveryNode getLocalNode() {
return localNode;
}

public static void setLocalNode(DiscoveryNode node) {
localNode = node;
}


@Override
public Subject getSubject() {
return null;
}

@Override
public TokenManager getTokenManager() {
return new SecurityTokenManager(
threadPool,
new XFFResolver(threadPool),
auditLog,
settings
);
}

public static class GuiceHolder implements LifecycleComponent {

private static RepositoriesService repositoriesService;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import java.time.Instant;
import java.util.List;
import java.util.Optional;
import java.util.StringJoiner;
import java.util.function.LongSupplier;

import com.google.common.base.Strings;
Expand All @@ -30,6 +31,9 @@
import org.apache.logging.log4j.Logger;

import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.set.Sets;
import org.opensearch.identity.tokens.BasicAuthToken;
import org.opensearch.security.support.ConfigConstants;

public class JwtVendor {
private static final Logger logger = LogManager.getLogger(JwtVendor.class);
Expand Down Expand Up @@ -103,7 +107,8 @@ public String createJwt(
String audience,
Integer expirySeconds,
List<String> roles,
List<String> backendRoles
List<String> backendRoles,
Boolean bwcModeEnabled
) throws Exception {
String tokenIdentifier = "obo";
long timeMillis = timeProvider.getAsLong();
Expand Down Expand Up @@ -142,7 +147,10 @@ public String createJwt(
throw new Exception("Roles cannot be null");
}

/* TODO: If the backendRoles is not null and the BWC Mode is on, put them into the "dbr" claim */
if (bwcModeEnabled && backendRoles != null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@DarshitChanpura i wonder if just keeping the format used a few lines above would be ok, or if for the backwards compatibility to work we should keep the exact format currently used when adding to the Context:

StringJoiner joiner = new StringJoiner("|"); joiner.add(user.getName()); joiner.add(String.join(",", user.getRoles())); joiner.add(String.join(",", Sets.union(user.getSecurityRoles(), mappedRoles)));

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea behind this is to send backend roles unencrypted if its in backwards compatibility mode. Can you please elaborate on keep the exact same format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code snipped above that i pasted in the above post is how the back end roles are added to the context (joinined with " | " symbol and adding the user's name at the begininng ).

My question if we should populate using the same format as the plugins might be expecting/parsing that exact format and could fail if the joiner character is different.

(Im not aware if this is the case or not)

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, this is only for token parsing so we should be good with the format you have in this PR. @RyanL1997 Can you confirm?

String listOfBackendRoles = String.join(",", backendRoles);
jwtClaims.setProperty("br", EncryptionDecryptionUtil.encrypt(claimsEncryptionKey, listOfBackendRoles));
Copy link
Member

Choose a reason for hiding this comment

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

If this setting is enabled then send the backend roles unencrypted. This setting will determine whether backend roles (br) is included or excluded as a claim in the token.

Copy link
Member

Choose a reason for hiding this comment

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

+1. Based on this TODO comment:

/* TODO: If the backendRoles is not null and the BWC Mode is on, put them into the "dbr" claim */

@samuelcostae Please update it to reflect non-encrypted backend role dbr

}

String encodedJwt = jwtProducer.processJwt(jwt);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -320,6 +320,10 @@ public enum RolesMappingResolution {
public static final String TENANCY_GLOBAL_TENANT_NAME = "global";
public static final String TENANCY_GLOBAL_TENANT_DEFAULT_NAME = "";

public static final String EXTENSIONS_BWC_PLUGIN_MODE = "bwcPluginMode";
Copy link
Member

Choose a reason for hiding this comment

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

BWC_PLUGIN_MODE may not be the best name to capture what this setting does. In an early draft of security for extensions it was called this because it was not fully known yet what it would take for an extension to be backward compatible with plugins.

wdyt about calling this setting EXTENSIONS_INCLUDE_BACKEND_ROLES?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just initiated the run of CI. I doubt the naming including 'EXTENSIONS' gonna pass the lint task, since we do have restrictions of using it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jwtTokenIncludesBackendRoles ?

Copy link
Member

Choose a reason for hiding this comment

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

@samuelcostae You can suppress the enforcement of that check like this: https://github.com/opensearch-project/security/blob/5e8f12ce5afe95f2f510cddf2a5b2cf50c076a66/src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1931C1-L1935

// CS-SUPPRESS-SINGLE: RegexpSingleline Extensions manager used to allow/disallow TLS connections to extensions
public static ExtensionsManager getExtensionsManager() {
    return extensionsManager;
}
// CS-ENFORCE-SINGLE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've included the supression comments, but shouldn't rename it anyway?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that should be fine since this mode is targeting for the usage of the extension.

public static final boolean EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT = false;


public static Set<String> getSettingAsSet(
final Settings settings,
final String key,
Expand Down