Skip to content

Feature/extensions bwc setting #3180

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.

Sign up for GitHub

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

Merged
stephen-crawford merged 8 commits into from
Aug 18, 2023
Merged

Feature/extensions bwc setting #3180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
11d70ba
initial changes
samuelcostae Jul 26, 2023
14d243e
Merge branch 'feature/extensions' into feature/extensions_bwc_setting
samuelcostae Aug 14, 2023
1ec0cf0
Editing JwtVendor.java to add backend roles if BwcMode is active
samuelcostae Aug 15, 2023
771513b
Editing JwtVendor.java to add backend roles if BwcMode is active
samuelcostae Aug 15, 2023
8e5d58c
Merge remote-tracking branch 'origin/feature/extensions_bwc_setting' …
samuelcostae Aug 15, 2023
49ca97b
Editing JwtVendor.java to add backend roles if BwcMode is active
samuelcostae Aug 15, 2023
ce96b6a
Test Added. Making backend roles not encrypted. Suppress 'extensions'…
samuelcostae Aug 17, 2023
752dd9a
Suppress 'extensions' warning
samuelcostae Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 27 additions & 1 deletion src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff 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
13 changes: 12 additions & 1 deletion src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java
View file
Open in desktop
Original file line number Diff 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 @@ -59,6 +61,12 @@ public JwtVendor(final Settings settings, final Optional<LongSupplier> timeProvi
} 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) {
Copy link
Contributor Author

@samuelcostae samuelcostae Aug 15, 2023

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

@DarshitChanpura DarshitChanpura Aug 17, 2023

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

@samuelcostae samuelcostae Aug 17, 2023

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

@DarshitChanpura DarshitChanpura Aug 17, 2023

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("dbr", listOfBackendRoles);
}

String encodedJwt = jwtProducer.processJwt(jwt);

Expand Down
5 changes: 5 additions & 0 deletions src/main/java/org/opensearch/security/support/ConfigConstants.java
View file
Open in desktop
Original file line number Diff 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";
Copy link
Member

@cwperks cwperks Aug 15, 2023

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

@RyanL1997 RyanL1997 Aug 15, 2023

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

@samuelcostae samuelcostae Aug 16, 2023

Choose a reason for hiding this comment

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

jwtTokenIncludesBackendRoles ?

Copy link
Member

@cwperks cwperks Aug 16, 2023

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

@samuelcostae samuelcostae Aug 17, 2023

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

@RyanL1997 RyanL1997 Aug 18, 2023

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;
// CS-ENFORCE-SINGLE

public static Set<String> getSettingAsSet(
final Settings settings,
final String key,
Expand Down
45 changes: 44 additions & 1 deletion src/test/java/org/opensearch/security/authtoken/jwt/JwtVendorTest.java
View file
Open in desktop
Original file line number Diff 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