Change how disabled OBO is handled

- Added test case to verify when OBO is disabled the correct response is
  recieved
- Added test case where the signing key is rotated
- Patched up a bug where insufficent signing key complexity would only be
  caught when decrypting, but not configuring cluster.

Signed-off-by: Peter Nied <petern@amazon.com>

src/integrationTest/java/org/opensearch/security/http/OnBehalfOfJwtAuthenticationTest.java

@@ -11,10 +11,10 @@
 
 package org.opensearch.security.http;
 
+import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Arrays;
 import java.util.Base64;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.stream.Collectors;
@@ -25,10 +25,12 @@
 import org.apache.hc.core5.http.Header;
 import org.apache.hc.core5.http.HttpStatus;
 import org.apache.hc.core5.http.message.BasicHeader;
+import org.junit.Before;
 import org.junit.ClassRule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
-
+import org.opensearch.common.xcontent.XContentFactory;
+import org.opensearch.core.xcontent.XContentBuilder;
 import org.opensearch.security.authtoken.jwt.EncryptionDecryptionUtil;
 import org.opensearch.test.framework.OnBehalfOfConfig;
 import org.opensearch.test.framework.RolesMapping;
@@ -43,6 +45,7 @@
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.contains;
 import static org.opensearch.security.support.ConfigConstants.SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX;
+import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ADMIN_ENABLED;
 import static org.opensearch.security.support.ConfigConstants.SECURITY_RESTAPI_ROLES_ENABLED;
 import static org.opensearch.test.framework.TestSecurityConfig.AuthcDomain.AUTHC_HTTPBASIC_INTERNAL;
 import static org.opensearch.test.framework.TestSecurityConfig.Role.ALL_ACCESS;
@@ -63,6 +66,9 @@ public class OnBehalfOfJwtAuthenticationTest {
                 StandardCharsets.UTF_8
             )
         );
+    private static final String alternativeSigningKey = Base64.getEncoder()
+        .encodeToString("alternativeSigningKeyalternativeSigningKeyalternativeSigningKeyalternativeSigningKey".getBytes(StandardCharsets.UTF_8));
+
     private static final String encryptionKey = Base64.getEncoder().encodeToString("encryptionKey".getBytes(StandardCharsets.UTF_8));
     public static final String ADMIN_USER_NAME = "admin";
     public static final String OBO_USER_NAME_WITH_PERM = "obo_user";
@@ -106,18 +112,36 @@ public class OnBehalfOfJwtAuthenticationTest {
     protected final static TestSecurityConfig.User HOST_MAPPING_OBO_USER = new TestSecurityConfig.User(OBO_USER_NAME_WITH_HOST_MAPPING)
         .roles(HOST_MAPPING_ROLE, ROLE_WITH_OBO_PERM);
 
+    private static OnBehalfOfConfig defaultOnBehalfOfConfig() {
+        return new OnBehalfOfConfig().oboEnabled(oboEnabled).signingKey(signingKey).encryptionKey(encryptionKey);
+    }
+
     @ClassRule
     public static final LocalCluster cluster = new LocalCluster.Builder().clusterManager(ClusterManager.SINGLENODE)
         .anonymousAuth(false)
         .users(ADMIN_USER, OBO_USER, OBO_USER_NO_PERM, HOST_MAPPING_OBO_USER)
         .nodeSettings(
-            Map.of(SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX, true, SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_admin__all_access"))
+            Map.of(
+                SECURITY_ALLOW_DEFAULT_INIT_SECURITYINDEX,
+                true,
+                SECURITY_RESTAPI_ROLES_ENABLED,
+                ADMIN_USER.getRoleNames(),
+                SECURITY_RESTAPI_ADMIN_ENABLED,
+                true,
+                "plugins.security.unsupported.restapi.allow_securityconfig_modification",
+                true
+            )
         )
         .authc(AUTHC_HTTPBASIC_INTERNAL)
         .rolesMapping(new RolesMapping(HOST_MAPPING_ROLE).hostIPs(HOST_MAPPING_IP))
-        .onBehalfOf(new OnBehalfOfConfig().oboEnabled(oboEnabled).signingKey(signingKey).encryptionKey(encryptionKey))
+        .onBehalfOf(defaultOnBehalfOfConfig())
         .build();
 
+    @Before
+    public void before() {
+        patchOnBehalfOfConfig(defaultOnBehalfOfConfig());
+    }
+
     @Test
     public void shouldAuthenticateWithOBOTokenEndPoint() {
         String oboToken = generateOboToken(ADMIN_USER_NAME, DEFAULT_PASSWORD);
@@ -208,6 +232,44 @@ public void shouldNotAuthenticateWithInvalidAPIParameter() {
         }
     }
 
+    @Test
+    public void shouldNotAllowTokenWhenOboIsDisabled() {
+        final String oboToken = generateOboToken(OBO_USER_NAME_WITH_PERM, DEFAULT_PASSWORD);
+        final Header oboHeader = new BasicHeader("Authorization", "Bearer " + oboToken);
+        authenticateWithOboToken(oboHeader, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_OK);
+
+        // Disable OBO via config and see that the authenticator doesn't authorize
+        patchOnBehalfOfConfig(defaultOnBehalfOfConfig().oboEnabled(false));
+        authenticateWithOboToken(oboHeader, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_UNAUTHORIZED);
+
+        // Reenable OBO via config and see that the authenticator is working again
+        patchOnBehalfOfConfig(defaultOnBehalfOfConfig().oboEnabled(true));
+        authenticateWithOboToken(oboHeader, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_OK);
+    }
+
+    @Test
+    public void oboSigningCheckChangeIsDetected() {
+        final String oboTokenOrignalKey = generateOboToken(OBO_USER_NAME_WITH_PERM, DEFAULT_PASSWORD);
+        final Header oboHeaderOriginalKey = new BasicHeader("Authorization", "Bearer " + oboTokenOrignalKey);
+        authenticateWithOboToken(oboHeaderOriginalKey, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_OK);
+
+        // Change the signing key
+        patchOnBehalfOfConfig(defaultOnBehalfOfConfig().signingKey(alternativeSigningKey));
+
+        // Original key should no longer work
+        authenticateWithOboToken(oboHeaderOriginalKey, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_UNAUTHORIZED);
+
+        // Generate new key, check that it is valid
+        final String oboTokenOtherKey = generateOboToken(OBO_USER_NAME_WITH_PERM, DEFAULT_PASSWORD);
+        final Header oboHeaderOtherKey = new BasicHeader("Authorization", "Bearer " + oboTokenOtherKey);
+        authenticateWithOboToken(oboHeaderOtherKey, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_OK);
+
+        // Change back to the original signing key and the original key still works, and the new key doesn't
+        patchOnBehalfOfConfig(defaultOnBehalfOfConfig());
+        authenticateWithOboToken(oboHeaderOriginalKey, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_OK);
+        authenticateWithOboToken(oboHeaderOtherKey, OBO_USER_NAME_WITH_PERM, HttpStatus.SC_UNAUTHORIZED);
+    }
+
     private String generateOboToken(String username, String password) {
         try (TestRestClient client = cluster.getRestClient(username, password)) {
             client.confirmCorrectCredentials(username);
@@ -231,4 +293,20 @@ private void authenticateWithOboToken(Header authHeader, String expectedUsername
             }
         }
     }
+
+    private void patchOnBehalfOfConfig(final OnBehalfOfConfig oboConfig) {
+        try (final TestRestClient adminClient = cluster.getRestClient(cluster.getAdminCertificate())) {
+            final XContentBuilder configBuilder = XContentFactory.jsonBuilder();
+            configBuilder.value(oboConfig);
+
+            final String patchBody = "[{ \"op\": \"replace\", \"path\": \"/config/dynamic/on_behalf_of\", \"value\":"
+                + configBuilder.toString()
+                + "}]";
+            System.err.println("&&& Patch Body: \n" + patchBody);
+            final var response = adminClient.patch("_plugins/_security/api/securityconfig", patchBody);
+            response.assertStatusCode(HttpStatus.SC_OK);
+        } catch (final IOException ex) {
+            throw new RuntimeException(ex);
+        }
+    }
 }

src/main/java/org/opensearch/security/action/onbehalf/CreateOnBehalfOfTokenAction.java

@@ -80,10 +80,10 @@ public void accept(final RestChannel channel) throws Exception {
                 final XContentBuilder builder = channel.newBuilder();
                 BytesRestResponse response;
                 try {
-                    if (securityTokenManager == null) {
+                    if (!securityTokenManager.issueOnBehalfOfTokenAllowed()) {
                         channel.sendResponse(
                             new BytesRestResponse(
-                                RestStatus.SERVICE_UNAVAILABLE,
+                                RestStatus.BAD_REQUEST,
                                 "The OnBehalfOf token generating API has been disabled, see {link to doc} for more information on this feature." /* TODO: Update the link to the documentation website */
                             )
                         );

src/main/java/org/opensearch/security/configuration/ConfigurationRepository.java

@@ -461,7 +461,7 @@ public Map<CType, SecurityDynamicConfiguration<?>> getConfigurationsFromIndex(
             throw new OpenSearchException(e);
         }
 
-        if (logComplianceEvent && auditLog.getComplianceConfig().isEnabled()) {
+        if (logComplianceEvent && auditLog.getComplianceConfig() != null && auditLog.getComplianceConfig().isEnabled()) {
             CType configurationType = configTypes.iterator().next();
             Map<String, String> fields = new HashMap<String, String>();
             fields.put(configurationType.toLCString(), Strings.toString(MediaTypeRegistry.JSON, retVal.get(configurationType)));

src/main/java/org/opensearch/security/http/OnBehalfOfAuthenticator.java

@@ -11,24 +11,23 @@
 
 package org.opensearch.security.http;
 
+import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
+import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;
+import static org.opensearch.security.util.AuthTokenUtils.isAccessToRestrictedEndpoints;
+
 import java.security.AccessController;
 import java.security.PrivilegedAction;
 import java.util.Arrays;
 import java.util.List;
-import java.util.Optional;
 import java.util.Map.Entry;
+import java.util.Optional;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
-import io.jsonwebtoken.Claims;
-import io.jsonwebtoken.JwtParser;
-import io.jsonwebtoken.JwtParserBuilder;
-import io.jsonwebtoken.security.WeakKeyException;
 import org.apache.hc.core5.http.HttpHeaders;
 import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
-
 import org.opensearch.OpenSearchException;
 import org.opensearch.OpenSearchSecurityException;
 import org.opensearch.SpecialPermission;
@@ -42,12 +41,14 @@
 import org.opensearch.security.user.AuthCredentials;
 import org.opensearch.security.util.KeyUtils;
 
-import static org.opensearch.security.OpenSearchSecurityPlugin.LEGACY_OPENDISTRO_PREFIX;
-import static org.opensearch.security.OpenSearchSecurityPlugin.PLUGINS_PREFIX;
-import static org.opensearch.security.util.AuthTokenUtils.isAccessToRestrictedEndpoints;
+import io.jsonwebtoken.Claims;
+import io.jsonwebtoken.JwtParser;
+import io.jsonwebtoken.JwtParserBuilder;
+import io.jsonwebtoken.security.WeakKeyException;
 
 public class OnBehalfOfAuthenticator implements HTTPAuthenticator {
 
+    private static final int MINIMUM_SIGNING_KEY_BIT_LENGTH = 512;
     private static final String REGEX_PATH_PREFIX = "/(" + LEGACY_OPENDISTRO_PREFIX + "|" + PLUGINS_PREFIX + ")/" + "(.*)";
     private static final Pattern PATTERN_PATH_PREFIX = Pattern.compile(REGEX_PATH_PREFIX);
 
@@ -79,12 +80,15 @@ public JwtParser run() {
                 return builder.build();
             }
         });
-
         this.clusterName = clusterName;
         this.encryptionUtil = new EncryptionDecryptionUtil(encryptionKey);
     }
 
     private JwtParserBuilder initParserBuilder(final String signingKey) {
+        final int signingKeyLengthBits = signingKey.length() * 8;
+        if (signingKeyLengthBits < MINIMUM_SIGNING_KEY_BIT_LENGTH) {
+            throw new OpenSearchSecurityException("Signing key size was " + signingKeyLengthBits + " bits, which is not secure enough. Please use a signing_key with a size >= " + MINIMUM_SIGNING_KEY_BIT_LENGTH + " bits.");
+        }
         JwtParserBuilder jwtParserBuilder = KeyUtils.createJwtParserBuilderFromSigningKey(signingKey, log);
 
         if (jwtParserBuilder == null) {

src/main/java/org/opensearch/security/identity/SecurityTokenManager.java

@@ -66,6 +66,7 @@ public void onConfigModelChanged(final ConfigModel configModel) {
     public void onDynamicConfigModelChanged(final DynamicConfigModel dcm) {
         final Settings oboSettings = dcm.getDynamicOnBehalfOfSettings();
         final Boolean enabled = oboSettings.getAsBoolean("enabled", false);
+        logger.warn("Detected change in the dynamic configuration, enabled=" + enabled);
         if (enabled) {
             jwtVendor = createJwtVendor(oboSettings);
         } else {
@@ -83,13 +84,13 @@ JwtVendor createJwtVendor(final Settings settings) {
         }
     }
 
-    boolean oboNotSupported() {
-        return jwtVendor == null || configModel == null;
+    public boolean issueOnBehalfOfTokenAllowed() {
+        return jwtVendor != null && configModel != null;
     }
 
     @Override
     public ExpiringBearerAuthToken issueOnBehalfOfToken(final Subject subject, final OnBehalfOfClaims claims) {
-        if (oboNotSupported()) {
+        if (!issueOnBehalfOfTokenAllowed()) {
             // TODO: link that doc!
             throw new OpenSearchSecurityException(
                 "The OnBehalfOf token generation is not enabled, see {link to doc} for more information on this feature."

src/test/java/org/opensearch/security/identity/SecurityTokenManagerTest.java

@@ -85,7 +85,7 @@ public void onConfigModelChanged_oboNotSupported() {
 
         tokenManager.onConfigModelChanged(configModel);
 
-        assertThat(tokenManager.oboNotSupported(), equalTo(true));
+        assertThat(tokenManager.issueOnBehalfOfTokenAllowed(), equalTo(false));
         verifyNoMoreInteractions(configModel);
     }
 
@@ -96,7 +96,7 @@ public void onDynamicConfigModelChanged_JwtVendorEnabled() {
 
         tokenManager.onConfigModelChanged(configModel);
 
-        assertThat(tokenManager.oboNotSupported(), equalTo(false));
+        assertThat(tokenManager.issueOnBehalfOfTokenAllowed(), equalTo(true));
         verify(mockConfigModel).getDynamicOnBehalfOfSettings();
         verifyNoMoreInteractions(configModel);
     }
@@ -108,7 +108,7 @@ public void onDynamicConfigModelChanged_JwtVendorDisabled() {
         when(dcm.getDynamicOnBehalfOfSettings()).thenReturn(settings);
         tokenManager.onDynamicConfigModelChanged(dcm);
 
-        assertThat(tokenManager.oboNotSupported(), equalTo(true));
+        assertThat(tokenManager.issueOnBehalfOfTokenAllowed(), equalTo(false));
         verify(dcm).getDynamicOnBehalfOfSettings();
         verify(tokenManager, never()).createJwtVendor(any());
     }
@@ -164,7 +164,7 @@ public void issueOnBehalfOfToken_notEnabledOnCluster() {
 
     @Test
     public void issueOnBehalfOfToken_unsupportedSubjectType() {
-        doAnswer(invocation -> false).when(tokenManager).oboNotSupported();
+        doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final IllegalArgumentException exception = assertThrows(
             IllegalArgumentException.class,
             () -> tokenManager.issueOnBehalfOfToken(mock(Subject.class), null)
@@ -174,7 +174,7 @@ public void issueOnBehalfOfToken_unsupportedSubjectType() {
 
     @Test
     public void issueOnBehalfOfToken_missingAudience() {
-        doAnswer(invocation -> false).when(tokenManager).oboNotSupported();
+        doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final IllegalArgumentException exception = assertThrows(
             IllegalArgumentException.class,
             () -> tokenManager.issueOnBehalfOfToken(null, new OnBehalfOfClaims(null, 450L))
@@ -184,7 +184,7 @@ public void issueOnBehalfOfToken_missingAudience() {
 
     @Test
     public void issueOnBehalfOfToken_cannotFindUserInThreadContext() {
-        doAnswer(invocation -> false).when(tokenManager).oboNotSupported();
+        doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
         when(threadPool.getThreadContext()).thenReturn(threadContext);
         final OpenSearchSecurityException exception = assertThrows(
@@ -199,7 +199,7 @@ public void issueOnBehalfOfToken_cannotFindUserInThreadContext() {
     @Test
     public void issueOnBehalfOfToken_jwtGenerationFailure() throws Exception {
         doAnswer(invockation -> new ClusterName("cluster17")).when(cs).getClusterName();
-        doAnswer(invocation -> false).when(tokenManager).oboNotSupported();
+        doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
         threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon", List.of(), null));
         when(threadPool.getThreadContext()).thenReturn(threadContext);
@@ -225,7 +225,7 @@ public void issueOnBehalfOfToken_jwtGenerationFailure() throws Exception {
     @Test
     public void issueOnBehalfOfToken_success() throws Exception {
         doAnswer(invockation -> new ClusterName("cluster17")).when(cs).getClusterName();
-        doAnswer(invocation -> false).when(tokenManager).oboNotSupported();
+        doAnswer(invocation -> true).when(tokenManager).issueOnBehalfOfTokenAllowed();
         final ThreadContext threadContext = new ThreadContext(Settings.EMPTY);
         threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_USER, new User("Jon", List.of(), null));
         when(threadPool.getThreadContext()).thenReturn(threadContext);