Feature/extensions bwc setting #3180

samuelcostae · 2023-08-15T14:58:27Z

@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)));

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?

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)

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?

cwperks · 2023-08-15T15:21:29Z

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?

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.

jwtTokenIncludesBackendRoles ?

@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

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

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

-Original file line number
+Diff line change
@@ Expand Up / @@ -106,6 +106,7 @@ @@
     import org.opensearch.indices.IndicesService;
     import org.opensearch.indices.SystemIndexDescriptor;
     import org.opensearch.plugins.ClusterPlugin;
+    import org.opensearch.plugins.ExtensionAwarePlugin;
     import org.opensearch.plugins.MapperPlugin;
     import org.opensearch.repositories.RepositoriesService;
     import org.opensearch.rest.RestController;
@@ Expand Down Expand Up / @@ -194,7 +195,15 @@ @@
     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,
+            // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
+            ExtensionAwarePlugin
+    // CS-ENFORCE-SINGLE
+    {
         private static final String KEYWORD = ".keyword";
         private static final Logger actionTrace = LogManager.getLogger("opendistro_security_action_trace");
@@ Expand Down Expand Up / @@ -1110,6 +1119,23 @@ public Settings additionalSettings() { @@
             }
             return builder.build();
         }
+        // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
+        @Override
+        public List<Setting<?>> getExtensionSettings() {
+            List<Setting<?>> extensionSettings = new ArrayList<Setting<?>>();
+            extensionSettings.add(
+                Setting.boolSetting(
+                    ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE,
+                    ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT,
+                    Property.ExtensionScope,
+                    Property.Final
+                )
+            );
+            return extensionSettings;
+        }
+        // CS-ENFORCE-SINGLE:
         @Override
         public List<Setting<?>> getSettings() {
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -30,6 +30,7 @@ @@
     import org.apache.logging.log4j.Logger;
     import org.opensearch.common.settings.Settings;
+    import org.opensearch.security.support.ConfigConstants;
     public class JwtVendor {
         private static final Logger logger = LogManager.getLogger(JwtVendor.class);
@@ Expand All / @@ -40,6 +41,7 @@ public class JwtVendor { @@
         private final JsonWebKey signingKey;
         private final JoseJwtProducer jwtProducer;
         private final LongSupplier timeProvider;
+        private final Boolean bwcModeEnabled;
         public JwtVendor(final Settings settings, final Optional<LongSupplier> timeProvider) {
             JoseJwtProducer jwtProducer = new JoseJwtProducer();
@@ Expand All @@
             } else {
                 this.timeProvider = () -> System.currentTimeMillis() / 1000;
             }
+            // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
+            this.bwcModeEnabled = settings.getAsBoolean(
+                ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE,
+                ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT
+            );
+            // CS-ENFORCE-SINGLE
         }
         /*
@@ Expand Down Expand Up / @@ -142,7 +150,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) {
+                String listOfBackendRoles = String.join(",", backendRoles);
+                jwtClaims.setProperty("dbr", listOfBackendRoles);
+            }
             String encodedJwt = jwtProducer.processJwt(jwt);
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -320,6 +320,11 @@ public enum RolesMappingResolution { @@
         public static final String TENANCY_GLOBAL_TENANT_NAME = "global";
         public static final String TENANCY_GLOBAL_TENANT_DEFAULT_NAME = "";
+        // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
+        public static final String EXTENSIONS_BWC_PLUGIN_MODE = "bwcPluginMode";
+        public static final boolean EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT = false;
+        // CS-ENFORCE-SINGLE
         public static Set<String> getSettingAsSet(
             final Settings settings,
             final String key,
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -23,6 +23,7 @@ @@
     import org.junit.Test;
     import org.opensearch.common.settings.Settings;
+    import org.opensearch.security.support.ConfigConstants;
     public class JwtVendorTest {
@@ Expand All / @@ -48,7 +49,7 @@ public void testCreateJwtWithRoles() throws Exception { @@
             String subject = "admin";
             String audience = "audience_0";
             List<String> roles = List.of("IT", "HR");
-            List<String> backendRoles = List.of("Sales");
+            List<String> backendRoles = List.of("Sales", "Support");
             String expectedRoles = "IT,HR";
             Integer expirySeconds = 300;
             LongSupplier currentTime = () -> (int) 100;
@@ Expand All / @@ -71,6 +72,48 @@ public void testCreateJwtWithRoles() throws Exception { @@
             Assert.assertEquals(expectedExp, jwt.getClaim("exp"));
             Assert.assertNotEquals(expectedRoles, jwt.getClaim("er"));
             Assert.assertEquals(expectedRoles, EncryptionDecryptionUtil.decrypt(claimsEncryptionKey, jwt.getClaim("er").toString()));
+            Assert.assertNull(jwt.getClaim("dbr"));
+        }
+        @Test
+        public void testCreateJwtWithBackwardsCompatibilityMode() throws Exception {
+            String issuer = "cluster_0";
+            String subject = "admin";
+            String audience = "audience_0";
+            List<String> roles = List.of("IT", "HR");
+            List<String> backendRoles = List.of("Sales", "Support");
+            String expectedRoles = "IT,HR";
+            String expectedBackendRoles = "Sales,Support";
+            Integer expirySeconds = 300;
+            LongSupplier currentTime = () -> (int) 100;
+            String claimsEncryptionKey = RandomStringUtils.randomAlphanumeric(16);
+            Settings settings = Settings.builder()
+                .put("signing_key", "abc123")
+                .put("encryption_key", claimsEncryptionKey)
+                // CS-SUPPRESS-SINGLE: RegexpSingleline get Extensions Settings
+                .put(ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, true)
+                // CS-ENFORCE-SINGLE
+                .build();
+            Long expectedExp = currentTime.getAsLong() + expirySeconds;
+            JwtVendor jwtVendor = new JwtVendor(settings, Optional.of(currentTime));
+            String encodedJwt = jwtVendor.createJwt(issuer, subject, audience, expirySeconds, roles, backendRoles);
+            JwsJwtCompactConsumer jwtConsumer = new JwsJwtCompactConsumer(encodedJwt);
+            JwtToken jwt = jwtConsumer.getJwtToken();
+            Assert.assertEquals("obo", jwt.getClaim("typ"));
+            Assert.assertEquals("cluster_0", jwt.getClaim("iss"));
+            Assert.assertEquals("admin", jwt.getClaim("sub"));
+            Assert.assertEquals("audience_0", jwt.getClaim("aud"));
+            Assert.assertNotNull(jwt.getClaim("iat"));
+            Assert.assertNotNull(jwt.getClaim("exp"));
+            Assert.assertEquals(expectedExp, jwt.getClaim("exp"));
+            Assert.assertNotEquals(expectedRoles, jwt.getClaim("er"));
+            Assert.assertEquals(expectedRoles, EncryptionDecryptionUtil.decrypt(claimsEncryptionKey, jwt.getClaim("er").toString()));
+            Assert.assertNotNull(jwt.getClaim("dbr"));
+            Assert.assertEquals(expectedBackendRoles, jwt.getClaim("dbr"));
         }
         @Test(expected = Exception.class)
@@ Expand Down @@

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/extensions bwc setting #3180

Uh oh!

Diff view

Diff view

There are no files selected for viewing

samuelcostae Aug 15, 2023

Uh oh!

DarshitChanpura Aug 17, 2023

Uh oh!

samuelcostae Aug 17, 2023

Uh oh!

DarshitChanpura Aug 17, 2023

Uh oh!

cwperks Aug 15, 2023

Uh oh!

RyanL1997 Aug 15, 2023

Uh oh!

samuelcostae Aug 16, 2023

Uh oh!

cwperks Aug 16, 2023

Uh oh!

samuelcostae Aug 17, 2023

Uh oh!

RyanL1997 Aug 18, 2023

Uh oh!

Feature/extensions bwc setting #3180

Uh oh!

Feature/extensions bwc setting #3180

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!