Skip to content

Commit

Permalink
Remove identity-related feature flagged code from the RestController (o…
Browse files Browse the repository at this point in the history
…pensearch-project#15430)

* Add authenticate to IdentityPlugin interface

Signed-off-by: Craig Perkins <[email protected]>

* Handle null

Signed-off-by: Craig Perkins <[email protected]>

* Fix tests

Signed-off-by: Craig Perkins <[email protected]>

* Fix ActionModuleTests

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

* Add DelegatingRestHandlerTests

Signed-off-by: Craig Perkins <[email protected]>

* Address forbiddenApi

Signed-off-by: Craig Perkins <[email protected]>

* Remove authenticate from IdentityPlugin and keep RestController feature flagged code removed

Signed-off-by: Craig Perkins <[email protected]>

* Move RestTokenExtractor to identity-shiro plugin

Signed-off-by: Craig Perkins <[email protected]>

* Remove change in IdentityService

Signed-off-by: Craig Perkins <[email protected]>

* Remove changes in ActionModuleTests

Signed-off-by: Craig Perkins <[email protected]>

* Add tests for RestTokenExtractor

Signed-off-by: Craig Perkins <[email protected]>

* Remove DelegatingRestHandler

Signed-off-by: Craig Perkins <[email protected]>

* Call super instead of keeping a reference to the delegated restHandler

Signed-off-by: Craig Perkins <[email protected]>

* Address code review comments

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
  • Loading branch information
cwperks authored Sep 19, 2024
1 parent 77ddfd6 commit 1bc81d3
Show file tree
Hide file tree
Showing 18 changed files with 118 additions and 143 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
- [Workload Management] QueryGroup resource cancellation framework changes ([#15651](https://github.com/opensearch-project/OpenSearch/pull/15651))
- Fallback to Remote cluster-state on Term-Version check mismatch - ([#15424](https://github.com/opensearch-project/OpenSearch/pull/15424))
- Implement WithFieldName interface in ValuesSourceAggregationBuilder & FieldSortBuilder ([#15916](https://github.com/opensearch-project/OpenSearch/pull/15916))
- Remove identity-related feature flagged code from the RestController ([#15430](https://github.com/opensearch-project/OpenSearch/pull/15430))

### Dependencies
- Bump `com.azure:azure-identity` from 1.13.0 to 1.13.2 ([#15578](https://github.com/opensearch-project/OpenSearch/pull/15578))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,41 @@
import org.apache.shiro.SecurityUtils;
import org.apache.shiro.mgt.SecurityManager;
import org.opensearch.client.Client;
import org.opensearch.client.node.NodeClient;
import org.opensearch.cluster.metadata.IndexNameExpressionResolver;
import org.opensearch.cluster.service.ClusterService;
import org.opensearch.common.annotation.ExperimentalApi;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.core.common.io.stream.NamedWriteableRegistry;
import org.opensearch.core.rest.RestStatus;
import org.opensearch.core.xcontent.NamedXContentRegistry;
import org.opensearch.env.Environment;
import org.opensearch.env.NodeEnvironment;
import org.opensearch.identity.PluginSubject;
import org.opensearch.identity.Subject;
import org.opensearch.identity.tokens.AuthToken;
import org.opensearch.identity.tokens.TokenManager;
import org.opensearch.plugins.ActionPlugin;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.plugins.Plugin;
import org.opensearch.repositories.RepositoriesService;
import org.opensearch.rest.BytesRestResponse;
import org.opensearch.rest.RestChannel;
import org.opensearch.rest.RestHandler;
import org.opensearch.rest.RestRequest;
import org.opensearch.script.ScriptService;
import org.opensearch.threadpool.ThreadPool;
import org.opensearch.watcher.ResourceWatcherService;

import java.util.Collection;
import java.util.Collections;
import java.util.function.Supplier;
import java.util.function.UnaryOperator;

/**
* Identity implementation with Shiro
*
* @opensearch.experimental
*/
@ExperimentalApi
public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin {
public final class ShiroIdentityPlugin extends Plugin implements IdentityPlugin, ActionPlugin {
private Logger log = LogManager.getLogger(this.getClass());

private final Settings settings;
Expand Down Expand Up @@ -101,6 +107,37 @@ public TokenManager getTokenManager() {
}

@Override
public UnaryOperator<RestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
return AuthcRestHandler::new;
}

class AuthcRestHandler extends RestHandler.Wrapper {

public AuthcRestHandler(RestHandler original) {
super(original);
}

@Override
public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception {
try {
final AuthToken token = ShiroTokenExtractor.extractToken(request);
// If no token was found, continue executing the request
if (token == null) {
// Authentication did not fail so return true. Authorization is handled at the action level.
super.handleRequest(request, channel, client);
return;
}
ShiroSubject shiroSubject = (ShiroSubject) getCurrentSubject();
shiroSubject.authenticate(token);
// Caller was authorized, forward the request to the handler
super.handleRequest(request, channel, client);
} catch (final Exception e) {
final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, e.getMessage());
channel.sendResponse(bytesRestResponse);
}
}
}

public PluginSubject getPluginSubject(Plugin plugin) {
return new ShiroPluginSubject(threadPool);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@

/**
* OpenSearch specific security manager implementation
*
* @opensearch.experimental
*/
public class ShiroSecurityManager extends DefaultSecurityManager {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@

/**
* Subject backed by Shiro
*
* @opensearch.experimental
*/
public class ShiroSubject implements UserSubject {
private final ShiroTokenManager authTokenHandler;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,13 @@
* this file be licensed under the Apache-2.0 license or a
* compatible open source license.
*/
package org.opensearch.identity.tokens;
package org.opensearch.identity.shiro;

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.opensearch.core.common.Strings;
import org.opensearch.identity.tokens.AuthToken;
import org.opensearch.identity.tokens.BasicAuthToken;
import org.opensearch.rest.RestRequest;

import java.util.Collections;
Expand All @@ -18,9 +20,9 @@
/**
* Extracts tokens from RestRequests used for authentication
*/
public class RestTokenExtractor {
public class ShiroTokenExtractor {

private static final Logger logger = LogManager.getLogger(RestTokenExtractor.class);
private static final Logger logger = LogManager.getLogger(ShiroTokenExtractor.class);

public final static String AUTH_HEADER_NAME = "Authorization";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,6 @@

/**
* Extracts Shiro's {@link AuthenticationToken} from different types of auth headers
*
* @opensearch.experimental
*/
class ShiroTokenManager implements TokenManager {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

/**
* Password matcher for BCrypt
*
* @opensearch.experimental
*/
public class BCryptPasswordMatcher implements CredentialsMatcher {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@

/**
* Internal Realm is a custom realm using the internal OpenSearch IdP
*
* @opensearch.experimental
*/
public class OpenSearchRealm extends AuthenticatingRealm {
private static final String DEFAULT_REALM_NAME = "internal";
Expand Down Expand Up @@ -93,7 +91,7 @@ public OpenSearchRealm build() {
public User getInternalUser(final String principalIdentifier) throws UnknownAccountException {
final User userRecord = internalUsers.get(principalIdentifier);
if (userRecord == null) {
throw new UnknownAccountException();
throw new UnknownAccountException("Incorrect credentials");
}
return userRecord;
}
Expand Down Expand Up @@ -131,7 +129,7 @@ protected AuthenticationInfo doGetAuthenticationInfo(final AuthenticationToken t
return sai;
} else {
// Bad password
throw new IncorrectCredentialsException();
throw new IncorrectCredentialsException("Incorrect credentials");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@

/**
* A non-volatile and immutable object in the storage.
*
* @opensearch.experimental
*/
public class User {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,35 @@
import org.opensearch.identity.IdentityService;
import org.opensearch.plugins.IdentityPlugin;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.threadpool.TestThreadPool;
import org.opensearch.threadpool.ThreadPool;

import java.util.List;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.junit.Assert.assertThrows;
import static org.mockito.Mockito.mock;

public class ShiroIdentityPluginTests extends OpenSearchTestCase {

public void testSingleIdentityPluginSucceeds() {
TestThreadPool threadPool = new TestThreadPool(getTestName());
IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
List<IdentityPlugin> pluginList1 = List.of(identityPlugin1);
IdentityService identityService1 = new IdentityService(Settings.EMPTY, threadPool, pluginList1);
IdentityService identityService1 = new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList1);
assertThat(identityService1.getTokenManager(), is(instanceOf(ShiroTokenManager.class)));
terminate(threadPool);
}

public void testMultipleIdentityPluginsFail() {
TestThreadPool threadPool = new TestThreadPool(getTestName());
IdentityPlugin identityPlugin1 = new ShiroIdentityPlugin(Settings.EMPTY);
IdentityPlugin identityPlugin2 = new ShiroIdentityPlugin(Settings.EMPTY);
IdentityPlugin identityPlugin3 = new ShiroIdentityPlugin(Settings.EMPTY);
List<IdentityPlugin> pluginList = List.of(identityPlugin1, identityPlugin2, identityPlugin3);
Exception ex = assertThrows(OpenSearchException.class, () -> new IdentityService(Settings.EMPTY, threadPool, pluginList));
Exception ex = assertThrows(
OpenSearchException.class,
() -> new IdentityService(Settings.EMPTY, mock(ThreadPool.class), pluginList)
);
assert (ex.getMessage().contains("Multiple identity plugins are not supported,"));
terminate(threadPool);
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* 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.identity.shiro;

import org.opensearch.identity.tokens.AuthToken;
import org.opensearch.identity.tokens.BasicAuthToken;
import org.opensearch.rest.RestRequest;
import org.opensearch.test.OpenSearchTestCase;
import org.opensearch.test.rest.FakeRestRequest;

import java.nio.charset.StandardCharsets;
import java.util.Base64;
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

public class ShiroTokenExtractorTests extends OpenSearchTestCase {

public void testAuthorizationHeaderExtractionWithBasicAuthToken() {
String basicAuthHeader = Base64.getEncoder().encodeToString("foo:bar".getBytes(StandardCharsets.UTF_8));
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(
Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(BasicAuthToken.TOKEN_IDENTIFIER + " " + basicAuthHeader))
).build();
AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest);
assertThat(extractedToken, instanceOf(BasicAuthToken.class));
assertThat(extractedToken.asAuthHeaderValue(), equalTo(basicAuthHeader));
}

public void testAuthorizationHeaderExtractionWithUnknownToken() {
String authHeader = "foo";
RestRequest fakeRequest = new FakeRestRequest.Builder(xContentRegistry()).withHeaders(
Map.of(ShiroTokenExtractor.AUTH_HEADER_NAME, List.of(authHeader))
).build();
AuthToken extractedToken = ShiroTokenExtractor.extractToken(fakeRequest);
assertNull(extractedToken);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -579,7 +579,7 @@ public ActionModule(
actionPlugins.stream().flatMap(p -> p.indicesAliasesRequestValidators().stream()).collect(Collectors.toList())
);

restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService, identityService);
restController = new RestController(headers, restWrapper, nodeClient, circuitBreakerService, usageService);
}

public Map<String, ActionHandler<?, ?>> getActions() {
Expand Down
54 changes: 2 additions & 52 deletions server/src/main/java/org/opensearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@
import org.opensearch.common.io.stream.BytesStreamOutput;
import org.opensearch.common.logging.DeprecationLogger;
import org.opensearch.common.path.PathTrie;
import org.opensearch.common.util.FeatureFlags;
import org.opensearch.common.util.concurrent.ThreadContext;
import org.opensearch.common.util.io.Streams;
import org.opensearch.common.xcontent.XContentType;
Expand All @@ -56,11 +55,6 @@
import org.opensearch.core.xcontent.XContentBuilder;
import org.opensearch.http.HttpChunk;
import org.opensearch.http.HttpServerTransport;
import org.opensearch.identity.IdentityService;
import org.opensearch.identity.Subject;
import org.opensearch.identity.UserSubject;
import org.opensearch.identity.tokens.AuthToken;
import org.opensearch.identity.tokens.RestTokenExtractor;
import org.opensearch.usage.UsageService;

import java.io.ByteArrayOutputStream;
Expand Down Expand Up @@ -125,25 +119,23 @@ public class RestController implements HttpServerTransport.Dispatcher {
/** Rest headers that are copied to internal requests made during a rest request. */
private final Set<RestHeaderDefinition> headersToCopy;
private final UsageService usageService;
private final IdentityService identityService;

public RestController(
Set<RestHeaderDefinition> headersToCopy,
UnaryOperator<RestHandler> handlerWrapper,
NodeClient client,
CircuitBreakerService circuitBreakerService,
UsageService usageService,
IdentityService identityService
UsageService usageService
) {
this.headersToCopy = headersToCopy;
this.usageService = usageService;
if (handlerWrapper == null) {
handlerWrapper = h -> h; // passthrough if no wrapper set
}

this.handlerWrapper = handlerWrapper;
this.client = client;
this.circuitBreakerService = circuitBreakerService;
this.identityService = identityService;
registerHandlerNoWrap(
RestRequest.Method.GET,
"/favicon.ico",
Expand Down Expand Up @@ -472,11 +464,6 @@ private void tryAllHandlers(final RestRequest request, final RestChannel channel
return;
}
} else {
if (FeatureFlags.isEnabled(FeatureFlags.IDENTITY)) {
if (!handleAuthenticateUser(request, channel)) {
return;
}
}
dispatchRequest(request, channel, handler);
return;
}
Expand Down Expand Up @@ -587,43 +574,6 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel
}
}

/**
* Attempts to extract auth token and login.
*
* @return false if there was an error and the request should not continue being dispatched
* */
private boolean handleAuthenticateUser(final RestRequest request, final RestChannel channel) {
try {
final AuthToken token = RestTokenExtractor.extractToken(request);
// If no token was found, continue executing the request
if (token == null) {
// Authentication did not fail so return true. Authorization is handled at the action level.
return true;
}
final Subject currentSubject = identityService.getCurrentSubject();
if (currentSubject instanceof UserSubject) {
((UserSubject) currentSubject).authenticate(token);
logger.debug("Logged in as user " + currentSubject);
}
} catch (final Exception e) {
try {
final BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(
channel,
RestStatus.UNAUTHORIZED,
e.getMessage()
);
channel.sendResponse(bytesRestResponse);
} catch (final Exception ex) {
final BytesRestResponse bytesRestResponse = new BytesRestResponse(RestStatus.UNAUTHORIZED, ex.getMessage());
channel.sendResponse(bytesRestResponse);
}
return false;
}

// Authentication did not fail so return true. Authorization is handled at the action level.
return true;
}

/**
* Get the valid set of HTTP methods for a REST request.
*/
Expand Down
Loading

0 comments on commit 1bc81d3

Please sign in to comment.