Skip to content
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

Jump to bottom

Feature/extensions bwc setting #3180

Merged
merged 8 commits into from
Aug 18, 2023
Merged

Feature/extensions bwc setting #3180

Show file tree
Hide file tree
Changes from 6 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
22 changes: 21 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,11 @@
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 {

private static final String KEYWORD = ".keyword";
private static final Logger actionTrace = LogManager.getLogger("opendistro_security_action_trace");
Expand Down Expand Up @@ -1111,6 +1116,21 @@
return builder.build();
}

@Override
public List<Setting<?>> getExtensionSettings() {
List<Setting<?>> extentionSettings = new ArrayList<Setting<?>>();

Check warning on line 1121 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1121 

Added line #L1121 was not covered by tests
samuelcostae marked this conversation as resolved.
Show resolved Hide resolved

extentionSettings.add(
Setting.boolSetting(

Check warning on line 1124 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1123-L1124 

Added lines #L1123 - L1124 were not covered by tests
ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE,
ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE_DEFAULT,
Property.ExtensionScope,
Property.Final
)
);
return extentionSettings;

Check warning on line 1131 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L1131 

Added line #L1131 was not covered by tests
}

@Override
public List<Setting<?>> getSettings() {
List<Setting<?>> settings = new ArrayList<Setting<?>>();
Expand Down
8 changes: 7 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,13 +41,14 @@
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();
try {
this.signingKey = createJwkFromSettings(settings);
} catch (Exception e) {
throw new RuntimeException(e);

Check warning on line 51 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L50-L51 

Added lines #L50 - L51 were not covered by tests
}
this.jwtProducer = jwtProducer;
if (settings.get("encryption_key") == null) {
Expand All @@ -59,6 +61,7 @@
} else {
this.timeProvider = () -> System.currentTimeMillis() / 1000;
}
this.bwcModeEnabled = settings.getAsBoolean(ConfigConstants.EXTENSIONS_BWC_PLUGIN_MODE, true);
samuelcostae marked this conversation as resolved.
Show resolved Hide resolved
}

/*
Expand Down Expand Up @@ -87,13 +90,13 @@
throw new Exception("Settings for key is missing. Please specify at least the option signing_key with a shared secret.");
}

JsonWebKey jwk = new JsonWebKey();

Check warning on line 93 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L93 

Added line #L93 was not covered by tests

for (String key : jwkSettings.keySet()) {
jwk.setProperty(key, jwkSettings.get(key));
}

Check warning on line 97 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L96-L97 

Added lines #L96 - L97 were not covered by tests

return jwk;

Check warning on line 99 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L99 

Added line #L99 was not covered by tests
}
}

Expand Down Expand Up @@ -126,8 +129,8 @@
jwtClaims.setNotBefore(timeMillis);

if (expirySeconds == null) {
long expiryTime = timeProvider.getAsLong() + 300;
jwtClaims.setExpiryTime(expiryTime);

Check warning on line 133 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L132-L133 

Added lines #L132 - L133 were not covered by tests
} else if (expirySeconds > 0) {
long expiryTime = timeProvider.getAsLong() + expirySeconds;
jwtClaims.setExpiryTime(expiryTime);
Expand All @@ -142,18 +145,21 @@
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

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

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

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

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("br", EncryptionDecryptionUtil.encrypt(claimsEncryptionKey, listOfBackendRoles));
Copy link
Member

Choose a reason for hiding this comment

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

If this setting is enabled then send the backend roles unencrypted. This setting will determine whether backend roles (br) is included or excluded as a claim in the token.

Copy link
Member

Choose a reason for hiding this comment

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

+1. Based on this TODO comment:

/* TODO: If the backendRoles is not null and the BWC Mode is on, put them into the "dbr" claim */

@samuelcostae Please update it to reflect non-encrypted backend role dbr

}

String encodedJwt = jwtProducer.processJwt(jwt);

if (logger.isDebugEnabled()) {
logger.debug(

Check warning on line 156 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L156 

Added line #L156 was not covered by tests
"Created JWT: "
+ encodedJwt
+ "\n"
+ jsonMapReaderWriter.toJson(jwt.getJwsHeaders())

Check warning on line 160 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L160 

Added line #L160 was not covered by tests
+ "\n"
+ JwtUtils.claimsToJson(jwt.getClaims())

Check warning on line 162 in src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/authtoken/jwt/JwtVendor.java#L162 

Added line #L162 was not covered by tests
);
}

Expand Down
3 changes: 3 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,9 @@ public enum RolesMappingResolution {
public static final String TENANCY_GLOBAL_TENANT_NAME = "global";
public static final String TENANCY_GLOBAL_TENANT_DEFAULT_NAME = "";

public static final String EXTENSIONS_BWC_PLUGIN_MODE = "bwcPluginMode";
Copy link
Member

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

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

Choose a reason for hiding this comment

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

jwtTokenIncludesBackendRoles ?

Copy link
Member

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

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

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;

public static Set<String> getSettingAsSet(
final Settings settings,
final String key,
Expand Down
Loading