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

add non-null to store non-default values when patching security config #1444

Merged
merged 5 commits into from
Jan 7, 2022
Merged

add non-null to store non-default values when patching security config #1444

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f4572d5
add non-null to store non-default values when patching security config
Sep 14, 2021
b6987b8
fixed issue due to which configv6 and configv7 tests failed
Sep 14, 2021
1e3a70b
added newline at EOF securityconfig_nondefault.json
Sep 24, 2021
0536a2f
adding non-null annotation only to properties in ConfigV6 and ConfigV…
nsri19 Sep 29, 2021
53c066e
Rename unit test to testSecurityConfigForHTTPPatch() as per review co…
nsri19 Nov 9, 2021
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
30 changes: 20 additions & 10 deletions src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.fasterxml.jackson.annotation.JsonAnyGetter;
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
Copy link
Member

@cliu123 cliu123 Sep 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How are the changes applied to ConfigV6 tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not able to test changes applied to ConfigV6. Keeping the changes to maintain consistency with ConfigV7.

Copy link
Member

Choose a reason for hiding this comment

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

please create 6x domain to test those changes.

Copy link
Member

Choose a reason for hiding this comment

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

As discussed, this is mainly for 6.x to 7.x migration and will not impact 6.x directly. But lets test migration case as discussed offline.

import com.fasterxml.jackson.core.JsonProcessingException;
import org.opensearch.security.DefaultObjectMapper;
import org.opensearch.security.auth.internal.InternalAuthenticationBackend;
Expand Down Expand Up @@ -83,6 +84,7 @@ public String toString() {

public static class Kibana {

@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean multitenancy_enabled = true;
public String server_username = "kibanaserver";
public String opendistro_role = null;
Expand All @@ -97,7 +99,7 @@ public String toString() {


}

public static class Http {
public boolean anonymous_auth_enabled = false;
public Xff xff = new Xff();
Expand All @@ -108,7 +110,7 @@ public String toString() {


}

public static class AuthFailureListeners {
@JsonIgnore
private final Map<String, AuthFailureListener> listeners = new HashMap<>();
Expand All @@ -125,7 +127,7 @@ public Map<String, AuthFailureListener> getListeners() {


}

public static class AuthFailureListener {
public String type;
public String authentication_backend;
Expand All @@ -148,8 +150,9 @@ public String asJson() {
}
}
}

public static class Xff {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean enabled = true;
public String internalProxies = Pattern.compile(
"10\\.\\d{1,3}\\.\\d{1,3}\\.\\d{1,3}|" +
Expand All @@ -170,7 +173,7 @@ public String toString() {


}

public static class Authc {

@JsonIgnore
Expand All @@ -193,10 +196,13 @@ public String toString() {


}

public static class AuthcDomain {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean http_enabled= true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean transport_enabled= true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean enabled= true;
public int order = 0;
public HttpAuthenticator http_authenticator = new HttpAuthenticator();
Expand All @@ -211,6 +217,7 @@ public String toString() {
}

public static class HttpAuthenticator {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean challenge = true;
public String type;
public Map<String, Object> config = Collections.emptyMap();
Expand All @@ -231,7 +238,7 @@ public String toString() {


}

public static class AuthzBackend {
public String type = "noop";
public Map<String, Object> config = Collections.emptyMap();
Expand All @@ -252,7 +259,7 @@ public String toString() {


}

public static class AuthcBackend {
public String type = InternalAuthenticationBackend.class.getName();
public Map<String, Object> config = Collections.emptyMap();
Expand All @@ -273,7 +280,7 @@ public String toString() {


}

public static class Authz {
@JsonIgnore
private final Map<String, AuthzDomain> domains = new HashMap<>();
Expand All @@ -295,10 +302,13 @@ public String toString() {


}

public static class AuthzDomain {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean http_enabled = true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean transport_enabled = true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean enabled = true;
public AuthzBackend authorization_backend = new AuthzBackend();
@Override
Expand Down
28 changes: 18 additions & 10 deletions src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
import com.fasterxml.jackson.annotation.JsonAnyGetter;
import com.fasterxml.jackson.annotation.JsonAnySetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.core.JsonProcessingException;
import org.opensearch.security.DefaultObjectMapper;
import org.opensearch.security.auth.internal.InternalAuthenticationBackend;
Expand Down Expand Up @@ -120,6 +121,7 @@ public static class Dynamic {
public Authz authz = new Authz();
public AuthFailureListeners auth_failure_listeners = new AuthFailureListeners();
public boolean do_not_fail_on_forbidden;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean multi_rolespan_enabled = true;
public String hosts_resolver_mode = "ip-only";
public String transport_userrname_attribute;
Expand All @@ -134,6 +136,7 @@ public String toString() {

public static class Kibana {

@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean multitenancy_enabled = true;
public String server_username = "kibanaserver";
public String opendistro_role = null;
Expand All @@ -147,7 +150,7 @@ public String toString() {


}

public static class Http {
public boolean anonymous_auth_enabled = false;
public Xff xff = new Xff();
Expand All @@ -158,7 +161,7 @@ public String toString() {


}

public static class AuthFailureListeners {
@JsonIgnore
private final Map<String, AuthFailureListener> listeners = new HashMap<>();
Expand All @@ -175,7 +178,7 @@ public Map<String, AuthFailureListener> getListeners() {


}

public static class AuthFailureListener {
public String type;
public String authentication_backend;
Expand Down Expand Up @@ -211,7 +214,7 @@ public String asJson() {
}
}
}

public static class Xff {
public boolean enabled = false;
public String internalProxies = Pattern.compile(
Expand All @@ -230,7 +233,7 @@ public String toString() {


}

public static class Authc {

@JsonIgnore
Expand All @@ -254,10 +257,12 @@ public String toString() {


}

public static class AuthcDomain {

@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean http_enabled= true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean transport_enabled= true;
//public boolean enabled= true;
public int order = 0;
Expand Down Expand Up @@ -294,6 +299,7 @@ public String toString() {
}

public static class HttpAuthenticator {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean challenge = true;
public String type;
public Map<String, Object> config = Collections.emptyMap();
Expand Down Expand Up @@ -327,7 +333,7 @@ public String toString() {


}

public static class AuthzBackend {
public String type = "noop";
public Map<String, Object> config = Collections.emptyMap();
Expand Down Expand Up @@ -365,7 +371,7 @@ public String toString() {


}

public static class AuthcBackend {
public String type = InternalAuthenticationBackend.class.getName();
public Map<String, Object> config = Collections.emptyMap();
Expand Down Expand Up @@ -403,7 +409,7 @@ public String toString() {


}

public static class Authz {
@JsonIgnore
private final Map<String, AuthzDomain> domains = new HashMap<>();
Expand All @@ -425,9 +431,11 @@ public String toString() {


}

public static class AuthzDomain {
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean http_enabled = true;
@JsonInclude(JsonInclude.Include.NON_NULL)
public boolean transport_enabled = true;
public AuthzBackend authorization_backend = new AuthzBackend();
public String description;
Expand Down
33 changes: 33 additions & 0 deletions src/test/java/org/opensearch/security/dlic/rest/api/SecurityConfigApiTest.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.runner.RunWith;
import org.junit.runners.Parameterized;

import org.opensearch.security.DefaultObjectMapper;
import org.opensearch.security.support.ConfigConstants;
import org.opensearch.security.test.helper.file.FileHelper;
import org.opensearch.security.test.helper.rest.RestHelper.HttpResponse;
Expand Down Expand Up @@ -108,4 +109,36 @@ public void testSecurityConfigApiWrite() throws Exception {
Assert.assertEquals(HttpStatus.SC_METHOD_NOT_ALLOWED, response.getStatusCode());

}

@Test
public void testSecurityConfigForHTTPPatch() throws Exception {

Settings settings = Settings.builder().put(ConfigConstants.SECURITY_UNSUPPORTED_RESTAPI_ALLOW_SECURITYCONFIG_MODIFICATION, true).build();
setup(settings);

rh.keystore = "restapi/kirk-keystore.jks";
rh.sendAdminCertificate = true;

//non-default config
String updatedConfig = FileHelper.loadFile("restapi/securityconfig_nondefault.json");

//update config
HttpResponse response = rh.executePutRequest(ENDPOINT + "/securityconfig/config", updatedConfig, new Header[0]);
Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());

//make patch request
response = rh.executePatchRequest(ENDPOINT + "/securityconfig", "[{\"op\": \"add\",\"path\": \"/config/dynamic/do_not_fail_on_forbidden\",\"value\": \"false\"}]", new Header[0]);
Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());

//get config
response = rh.executeGetRequest(ENDPOINT + "/securityconfig", new Header[0]);
Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());

// verify configs are same
Assert.assertEquals(DefaultObjectMapper.readTree(updatedConfig), DefaultObjectMapper.readTree(response.getBody()).get("config"));


}
}


14 changes: 2 additions & 12 deletions src/test/java/org/opensearch/security/securityconf/impl/v6/ConfigV6Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ public static Iterable<Boolean> omitDefaults() {
}

public void assertEquals(ConfigV6.Kibana expected, JsonNode node) {
if (omitDefaults && !expected.multitenancy_enabled) {
// false (default) is not persisted
Assert.assertNull(node.get("multitenancy_enabled"));
} else {
Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
}
Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
if (expected.server_username == null) {
Assert.assertNull(node.get("server_username"));
} else {
Expand All @@ -51,12 +46,7 @@ public void assertEquals(ConfigV6.Kibana expected, JsonNode node) {
}

private void assertEquals(ConfigV6.Kibana expected, ConfigV6.Kibana actual) {
if (omitDefaults && !expected.multitenancy_enabled) {
// BUG: false is omitted and is restored to default (which is true) instead of false
Assert.assertTrue(actual.multitenancy_enabled);
} else {
Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
}
Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
if (expected.server_username == null) {
// null is restored to default instead of null
Assert.assertEquals(new ConfigV6.Kibana().server_username, actual.server_username);
Expand Down
14 changes: 2 additions & 12 deletions src/test/java/org/opensearch/security/securityconf/impl/v7/ConfigV7Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,7 @@ public static Iterable<Boolean> omitDefaults() {
}

public void assertEquals(ConfigV7.Kibana expected, JsonNode node) {
if (omitDefaults && !expected.multitenancy_enabled) {
// false (default) is not persisted
Assert.assertNull(node.get("multitenancy_enabled"));
} else {
Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
}
Assert.assertEquals(expected.multitenancy_enabled, node.get("multitenancy_enabled").asBoolean());
if (expected.server_username == null) {
Assert.assertNull(node.get("server_username"));
} else {
Expand All @@ -45,12 +40,7 @@ public void assertEquals(ConfigV7.Kibana expected, JsonNode node) {
}

private void assertEquals(ConfigV7.Kibana expected, ConfigV7.Kibana actual) {
if (omitDefaults && !expected.multitenancy_enabled) {
// BUG: false is omitted and is restored to default (which is true) instead of false
Assert.assertTrue(actual.multitenancy_enabled);
} else {
Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
}
Assert.assertEquals(expected.multitenancy_enabled, actual.multitenancy_enabled);
if (expected.server_username == null) {
// null is restored to default instead of null
Assert.assertEquals(new ConfigV7.Kibana().server_username, actual.server_username);
Expand Down
Loading