-
Notifications
You must be signed in to change notification settings - Fork 349
#2616 bwcPlugin extension setting and getTokenManager implementation #2938
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
Changes from all commits
2fc41ed
f051397
f439e3d
4548a29
eb54ee9
a39fcad
d993067
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 |
|---|---|---|
|
|
@@ -99,13 +99,17 @@ | |
| import org.opensearch.extensions.ExtensionsManager; | ||
| import org.opensearch.http.HttpServerTransport; | ||
| import org.opensearch.http.HttpServerTransport.Dispatcher; | ||
| import org.opensearch.identity.Subject; | ||
| import org.opensearch.identity.tokens.TokenManager; | ||
| import org.opensearch.index.Index; | ||
| 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.indices.breaker.CircuitBreakerService; | ||
| 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; | ||
|
|
@@ -127,6 +131,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; | ||
|
|
@@ -193,7 +198,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"); | ||
|
|
@@ -1107,6 +1117,21 @@ public Settings additionalSettings() { | |
| return builder.build(); | ||
| } | ||
|
|
||
| @Override | ||
| public List<Setting<?>> getExtensionSettings() { | ||
| List<Setting<?>> extentionSettings = new ArrayList<Setting<?>>(); | ||
|
|
||
| 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<?>>(); | ||
|
|
@@ -1886,6 +1911,15 @@ private static String handleKeyword(final String field) { | |
| return field; | ||
| } | ||
|
|
||
| @Override | ||
| public Subject getSubject() { | ||
| return null; | ||
|
Member
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. This will also need to be addressed as we implement IdentityPlugin for the security plugin. Subject should never be null and if no subject is present in the current context then its assumed to be unauthenticated. You can see an example implementation of SecuritySubject in this draft PR: https://github.com/opensearch-project/security/pull/2773/files#diff-f21aa0165f9d0633658e7c99a1375b84258d1ef2beecaad8df286cfe5ba2a7d2R1-R49 |
||
| } | ||
|
|
||
| @Override | ||
| public TokenManager getTokenManager() { | ||
| return new SecurityTokenManager(threadPool, new XFFResolver(threadPool), auditLog, settings); | ||
|
|
||
| public static DiscoveryNode getLocalNode() { | ||
| return localNode; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| /* | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| * | ||
| * The OpenSearch Contributors require contributions made to | ||
| * this file be licensed under the Apache-2.0 license or a | ||
| * compatible open source license. | ||
| */ | ||
|
|
||
| package org.opensearch.security.auth; | ||
|
|
||
| import org.greenrobot.eventbus.Subscribe; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.common.transport.TransportAddress; | ||
| import org.opensearch.common.util.set.Sets; | ||
| import org.opensearch.identity.tokens.AuthToken; | ||
| import org.opensearch.identity.tokens.BasicAuthToken; | ||
| import org.opensearch.identity.tokens.TokenManager; | ||
| import org.opensearch.security.auditlog.AuditLog; | ||
| import org.opensearch.security.http.XFFResolver; | ||
| import org.opensearch.security.securityconf.ConfigModel; | ||
| import org.opensearch.security.support.ConfigConstants; | ||
| import org.opensearch.security.user.User; | ||
| import org.opensearch.threadpool.ThreadPool; | ||
|
|
||
| import java.util.Set; | ||
| import java.util.StringJoiner; | ||
|
|
||
| public class SecurityTokenManager implements TokenManager { | ||
|
Member
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. Hey @samuelcostae , as @scrawfor99 pointed out if this PR is limited to the settings then wdyt about making this be a Noop version of the token manager and focus the PR on the settings? Edit: Can this be incorporated into the
Contributor
Author
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. @cwperks Either way is fine by me, I agree that is beyond the scope originally stated in the issue description, but I thought you had previously asked me(In the other PR) to include some usage of the setting. Were you referencing some other part of the implementation? |
||
|
|
||
| Boolean extensionBwcCompatMode; | ||
| User user; | ||
| ConfigModel configModel; | ||
| Set<String> mappedRoles; | ||
| UserInjector userInjector; | ||
|
|
||
| @Subscribe | ||
| public void onConfigModelChanged(ConfigModel configModel) { | ||
| this.configModel = configModel; | ||
| } | ||
|
|
||
| public SecurityTokenManager(ThreadPool threadPool, final XFFResolver xffResolver, AuditLog auditLog, Settings settings) { | ||
| this.userInjector = new UserInjector(settings, threadPool, auditLog, xffResolver); | ||
| this.extensionBwcCompatMode = settings.getAsBoolean( | ||
| ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, | ||
| ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT | ||
| ); | ||
| this.user = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_USER); | ||
| final TransportAddress caller = threadPool.getThreadContext().getTransient(ConfigConstants.OPENDISTRO_SECURITY_REMOTE_ADDRESS); | ||
|
|
||
| if (user == null) { | ||
| user = userInjector.getInjectedUser(); | ||
| } | ||
| this.mappedRoles = configModel.mapSecurityRoles(user, caller); | ||
| } | ||
|
|
||
| @Override | ||
| public AuthToken issueToken(String audience) { | ||
| if (extensionBwcCompatMode) { | ||
| StringJoiner joiner = new StringJoiner("|"); | ||
| joiner.add(user.getName()); | ||
| joiner.add(String.join(",", user.getRoles())); | ||
| joiner.add(String.join(",", Sets.union(user.getSecurityRoles(), mappedRoles))); | ||
|
|
||
| return new BasicAuthToken(joiner + "This is the Token including the encrypted backend roles"); | ||
| } else { | ||
| return new BasicAuthToken("This is standard Token without the roles"); | ||
| } | ||
| } | ||
| } | ||
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.
This looks fine to me, but I'd also like to see the usage of the setting inside the JWTVendor in
feature/extensionsbranch to show how this setting impacts the payload of the JWT created of the obo token for forwarding to an extension.