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.

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

Extensions config for JWT signing/encryption key #2671

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ public static void createRoleMappingFile(File destination) {
copyResourceToFile(resource, destination);
}

public static void createConfigFile(File destination) {
String resource = "config.yml";
copyResourceToFile(resource, destination);
}

public static Path createConfigurationDirectory() {
try {
Path tempDirectory = Files.createTempDirectory("test-security-config");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,17 @@ public int updateRoleMappings(File roleMappingsConfigurationFile) throws Excepti

return SecurityAdmin.execute(commandLineArguments);
}

public int updateConfig(File configFile) throws Exception {
String[] commandLineArguments = {"-cacert", certificates.getRootCertificate().getAbsolutePath(),
"-cert", certificates.getAdminCertificate().getAbsolutePath(),
"-key", certificates.getAdminKey(null).getAbsolutePath(),
"-nhnv",
"-p", String.valueOf(port),
"-f", configFile.getAbsolutePath(),
"-t", "config"
};

return SecurityAdmin.execute(commandLineArguments);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import java.util.Map;

import com.carrotsearch.randomizedtesting.annotations.ThreadLeakScope;
import com.fasterxml.jackson.databind.JsonNode;
import org.awaitility.Awaitility;
import org.junit.BeforeClass;
import org.junit.ClassRule;
Expand Down Expand Up @@ -46,7 +47,7 @@ public class SecurityConfigurationTests {

private static final User USER_ADMIN = new User("admin").roles(ALL_ACCESS);
private static final User LIMITED_USER = new User("limited-user")
.roles(new Role("limited-role").indexPermissions("indices:data/read/search", "indices:data/read/get").on("user-${user.name}"));
.roles(new Role("limited-role").indexPermissions("indices:data/read/search", "indices:data/read/get").on("user-${user.name}"));
public static final String LIMITED_USER_INDEX = "user-" + LIMITED_USER.getName();
public static final String ADDITIONAL_USER_1 = "additional00001";
public static final String ADDITIONAL_PASSWORD_1 = ADDITIONAL_USER_1;
Expand All @@ -61,11 +62,11 @@ public class SecurityConfigurationTests {

@ClassRule
public static LocalCluster cluster = new LocalCluster.Builder()
.clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
.authc(AUTHC_HTTPBASIC_INTERNAL).users(USER_ADMIN, LIMITED_USER).anonymousAuth(false)
.nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() +"__" + ALL_ACCESS.getName()),
SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true))
.build();
.clusterManager(ClusterManager.THREE_CLUSTER_MANAGERS)
.authc(AUTHC_HTTPBASIC_INTERNAL).users(USER_ADMIN, LIMITED_USER).anonymousAuth(false)
.nodeSettings(Map.of(SECURITY_RESTAPI_ROLES_ENABLED, List.of("user_" + USER_ADMIN.getName() +"__" + ALL_ACCESS.getName()),
SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, true))
.build();

@Rule
public TemporaryFolder configurationDirectory = new TemporaryFolder();
Expand All @@ -82,7 +83,7 @@ public static void initData() {
public void shouldCreateUserViaRestApi_success() {
try(TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
HttpResponse httpResponse = client.putJson(INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_1, String.format(CREATE_USER_BODY,
ADDITIONAL_PASSWORD_1));
ADDITIONAL_PASSWORD_1));

assertThat(httpResponse.getStatusCode(), equalTo(201));
}
Expand All @@ -98,7 +99,7 @@ public void shouldCreateUserViaRestApi_success() {
public void shouldCreateUserViaRestApi_failure() {
try(TestRestClient client = cluster.getRestClient(LIMITED_USER)) {
HttpResponse httpResponse = client.putJson(INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_1, String.format(CREATE_USER_BODY,
ADDITIONAL_PASSWORD_1));
ADDITIONAL_PASSWORD_1));

httpResponse.assertStatusCode(403);
}
Expand Down Expand Up @@ -141,7 +142,7 @@ public void shouldCreateUserViaRestApiWhenAdminIsAuthenticatedViaCertificate_pos
try(TestRestClient client = cluster.getRestClient(cluster.getAdminCertificate())) {

HttpResponse httpResponse = client.putJson(INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_2, String.format(CREATE_USER_BODY,
ADDITIONAL_PASSWORD_2));
ADDITIONAL_PASSWORD_2));

httpResponse.assertStatusCode(201);
}
Expand All @@ -158,7 +159,7 @@ public void shouldCreateUserViaRestApiWhenAdminIsAuthenticatedViaCertificate_neg
TestCertificates testCertificates = cluster.getTestCertificates();
try(TestRestClient client = cluster.getRestClient(testCertificates.createSelfSignedCertificate("CN=attacker"))) {
HttpResponse httpResponse = client.putJson(INTERNAL_USERS_RESOURCE + ADDITIONAL_USER_2, String.format(CREATE_USER_BODY,
ADDITIONAL_PASSWORD_2));
ADDITIONAL_PASSWORD_2));

httpResponse.assertStatusCode(401);
}
Expand Down Expand Up @@ -209,7 +210,28 @@ public void shouldUseSecurityAdminTool() throws Exception {
assertThat(exitCode, equalTo(0));
try(TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
Awaitility.await().alias("Waiting for rolemapping 'readall' availability.")
.until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200));
.until(() -> client.get("_plugins/_security/api/rolesmapping/readall").getStatusCode(), equalTo(200));
}
}

@Test
public void shouldReloadExtensionsConfigurationFromFile() throws Exception {
SecurityAdminLauncher securityAdminLauncher = new SecurityAdminLauncher(cluster.getHttpPort(), cluster.getTestCertificates());
File config = configurationDirectory.newFile("config.yml");
ConfigurationFiles.createConfigFile(config);
int exitCode = securityAdminLauncher.updateConfig(config);
Copy link
Member

@cwperks cwperks Apr 27, 2023

Choose a reason for hiding this comment

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

Nice! This is great that you were able to show the dynamic config settings being updated using the integration test framework. 🚀

assertThat(exitCode, equalTo(0));

try (TestRestClient client = cluster.getRestClient(USER_ADMIN)) {
Awaitility.await()
.until(() ->
{
HttpResponse httpResponse = client.get("_plugins/_security/api/securityconfig");
JsonNode jsonNode = DefaultObjectMapper.objectMapper.readTree(httpResponse.getBody());
return jsonNode.get("config").get("dynamic").get("extensions");

}, jsonNode -> jsonNode.get("encryption_key").asText().equals("encryption key") && jsonNode.get("signing_key").asText().equals("signing key")
);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,12 @@
*/
package org.opensearch.test.framework.matcher;

import java.util.Map;

import org.hamcrest.Description;
import org.hamcrest.TypeSafeDiagnosingMatcher;

import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse;
import org.opensearch.common.collect.ImmutableOpenMap;
import org.opensearch.common.settings.Settings;

import static java.util.Objects.isNull;
Expand All @@ -32,12 +33,12 @@ class GetSettingsResponseContainsIndicesMatcher extends TypeSafeDiagnosingMatche
@Override
protected boolean matchesSafely(GetSettingsResponse response, Description mismatchDescription) {

final ImmutableOpenMap<String, Settings> indexToSettings = response.getIndexToSettings();
final Map<String, Settings> indexToSettings = response.getIndexToSettings();
for (String index : expectedIndices) {
if (!indexToSettings.containsKey(index)) {
mismatchDescription
.appendText("Response contains settings of indices: ")
.appendValue(indexToSettings.keys());
.appendValue(indexToSettings.keySet().toArray());
return false;
}
}
Expand Down
3 changes: 3 additions & 0 deletions src/integrationTest/resources/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,3 +15,6 @@ config:
authentication_backend:
type: "internal"
config: {}
extensions:
signing_key: "signing key"
encryption_key: "encryption key"
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

package org.opensearch.security.authtoken.jwt;

import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Base64;

Expand All @@ -29,7 +30,7 @@ public static String encrypt(final String secret, final String data) {
// rebuild key using SecretKeySpec
SecretKey originalKey = new SecretKeySpec(Arrays.copyOf(decodedKey, 16), "AES");
cipher.init(Cipher.ENCRYPT_MODE, originalKey);
byte[] cipherText = cipher.doFinal(data.getBytes("UTF-8"));
byte[] cipherText = cipher.doFinal(data.getBytes(StandardCharsets.UTF_8));
return Base64.getEncoder().encodeToString(cipherText);
} catch (Exception e) {
throw new RuntimeException(
Expand All @@ -47,7 +48,7 @@ public static String decrypt(final String secret, final String encryptedString)
SecretKey originalKey = new SecretKeySpec(Arrays.copyOf(decodedKey, 16), "AES");
cipher.init(Cipher.DECRYPT_MODE, originalKey);
byte[] cipherText = cipher.doFinal(Base64.getDecoder().decode(encryptedString));
return new String(cipherText);
return new String(cipherText, StandardCharsets.UTF_8);
} catch (Exception e) {
throw new RuntimeException("Error occured while decrypting data", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;

import org.opensearch.common.settings.Settings;
import org.opensearch.security.auth.AuthDomain;
import org.opensearch.security.auth.AuthFailureListener;
import org.opensearch.security.auth.AuthorizationBackend;
Expand Down Expand Up @@ -80,7 +81,7 @@ public abstract class DynamicConfigModel {
public abstract Multimap<String, AuthFailureListener> getAuthBackendFailureListeners();
public abstract List<ClientBlockRegistry<InetAddress>> getIpClientBlockRegistries();
public abstract Multimap<String, ClientBlockRegistry<String>> getAuthBackendClientBlockRegistries();

public abstract Settings getDynamicExtensionsSettings();
protected final Map<String, String> authImplMap = new HashMap<>();

public DynamicConfigModel() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,12 @@ public List<ClientBlockRegistry<InetAddress>> getIpClientBlockRegistries() {
public Multimap<String, ClientBlockRegistry<String>> getAuthBackendClientBlockRegistries() {
return Multimaps.unmodifiableMultimap(authBackendClientBlockRegistries);
}


@Override
public Settings getDynamicExtensionsSettings() {
return Settings.EMPTY;
}

private void buildAAA() {

final SortedSet<AuthDomain> restAuthDomains0 = new TreeSet<>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,13 @@ public List<ClientBlockRegistry<InetAddress>> getIpClientBlockRegistries() {
public Multimap<String, ClientBlockRegistry<String>> getAuthBackendClientBlockRegistries() {
return Multimaps.unmodifiableMultimap(authBackendClientBlockRegistries);
}


@Override
public Settings getDynamicExtensionsSettings() {
return Settings.builder()
.put(Settings.builder().loadFromSource(config.dynamic.extensions.configAsJson(), XContentType.JSON).build())
.build();
}

private void buildAAA() {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;

import org.opensearch.security.DefaultObjectMapper;
Expand Down Expand Up @@ -71,11 +72,12 @@ public static class Dynamic {
public String hosts_resolver_mode = "ip-only";
public String transport_userrname_attribute;
public boolean do_not_fail_on_forbidden_empty;

public Extensions extensions = new Extensions();

@Override
public String toString() {
return "Dynamic [filtered_alias_mode=" + filtered_alias_mode + ", kibana=" + kibana + ", http=" + http + ", authc=" + authc + ", authz="
+ authz + "]";
+ authz + ", extensions=" + extensions + "]";
}
}

Expand Down Expand Up @@ -319,5 +321,32 @@ public String toString() {


}


public static class Extensions {
@JsonProperty("signing_key")
private String signingKey;
@JsonProperty("encryption_key")
private String encryptionKey;

public String getSigningKey() {
return signingKey;
}

public void setSigningKey(String signingKey) {
this.signingKey = signingKey;
}

public String getEncryptionKey() {
return encryptionKey;
}

public void setEncryptionKey(String encryptionKey) {
this.encryptionKey = encryptionKey;
}

@Override
public String toString() {
return "Extensions [signing_key=" + signingKey + ", encryption_key=" + encryptionKey +"]";
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.core.JsonProcessingException;

import org.opensearch.security.DefaultObjectMapper;
Expand Down Expand Up @@ -125,11 +126,12 @@ public static class Dynamic {
public String hosts_resolver_mode = "ip-only";
public String transport_userrname_attribute;
public boolean do_not_fail_on_forbidden_empty;
public Extensions extensions = new Extensions();

@Override
public String toString() {
Copy link
Member

Choose a reason for hiding this comment

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

Can you update this toString and also add toString() to the Extensions class similar to how Kibana is done?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a use case for allowing this part of the configuration to be printed? Since it stores encryption/signing keys I think it would be a good idea to hide it as much as possible.

Copy link
Member

Choose a reason for hiding this comment

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

That's a good point, I'm checking to see if and where it is called.

The HttpAuthenticator does print the config (See https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java#L324-L331) which in the case of a JWT backend does include the signing key or other JWK settings if using a different type of encryption than HS512.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it's ok to include this secret strings in toString() methods. Pushed my changes

return "Dynamic [filtered_alias_mode=" + filtered_alias_mode + ", kibana=" + kibana + ", http=" + http + ", authc=" + authc + ", authz="
+ authz + "]";
+ authz + ", extensions=" + extensions + "]";
}
}

Expand Down Expand Up @@ -461,8 +463,43 @@ public String toString() {
return "AuthzDomain [http_enabled=" + http_enabled + ", transport_enabled=" + transport_enabled
+ ", authorization_backend=" + authorization_backend + ", description=" + description + "]";
}



}

public static class Extensions {
@JsonProperty("signing_key")
private String signingKey;
@JsonProperty("encryption_key")
private String encryptionKey;

@JsonIgnore
public String configAsJson() {
try {
return DefaultObjectMapper.writeValueAsString(this, false);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
}
}

public String getSigningKey() {
return signingKey;
}

public void setSigningKey(String signingKey) {
this.signingKey = signingKey;
}

public String getEncryptionKey() {
return encryptionKey;
}

public void setEncryptionKey(String encryptionKey) {
this.encryptionKey = encryptionKey;
}

@Override
public String toString() {
return "Extensions [signing_key=" + signingKey + ", encryption_key=" + encryptionKey +"]";
}
}

}
3 changes: 3 additions & 0 deletions src/test/resources/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -96,3 +96,6 @@ config:
multi_rolespan_enabled: false
hosts_resolver_mode: "ip-only"
transport_userrname_attribute: null
extensions:
signing_key: "signing key"
encryption_key: "encryption key"
4 changes: 3 additions & 1 deletion src/test/resources/restapi/securityconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,9 @@
"do_not_fail_on_forbidden":false,
"multi_rolespan_enabled":false,
"hosts_resolver_mode":"ip-only",
"do_not_fail_on_forbidden_empty":false
"do_not_fail_on_forbidden_empty":false,
"extensions": {
}
}

}
6 changes: 5 additions & 1 deletion src/test/resources/restapi/securityconfig_nondefault.json
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,10 @@
"do_not_fail_on_forbidden" : false,
"multi_rolespan_enabled" : true,
"hosts_resolver_mode" : "ip-only",
"do_not_fail_on_forbidden_empty" : false
"do_not_fail_on_forbidden_empty" : false,
"extensions": {
"signing_key": "signing key",
"encryption_key": "encryption key"
}
}
}