diff --git a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java b/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java index 2ac1c3b881..3544fb005d 100644 --- a/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java +++ b/src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/validation/AbstractConfigurationValidator.java @@ -188,6 +188,16 @@ public boolean validate() { return false; } + //null element in the values of all the possible keys with DataType as ARRAY + for (Entry allowedKey : allowedKeys.entrySet()) { + JsonNode value = contentAsNode.get(allowedKey.getKey()); + if (value != null) { + if (hasNullArrayElement(value)) { + this.errorType = ErrorType.NULL_ARRAY_ELEMENT; + return false; + } + } + } return valid; } @@ -253,6 +263,10 @@ public XContentBuilder errorsAsXContent(RestChannel channel) { builder.field(entry.getKey(), entry.getValue()); } break; + case NULL_ARRAY_ELEMENT: + builder.field("status", "error"); + builder.field("reason", ErrorType.NULL_ARRAY_ELEMENT.getMessage()); + break; default: builder.field("status", "error"); builder.field("reason", errorType.getMessage()); @@ -281,7 +295,8 @@ public static enum DataType { public static enum ErrorType { NONE("ok"), INVALID_CONFIGURATION("Invalid configuration"), INVALID_PASSWORD("Invalid password"), WRONG_DATATYPE("Wrong datatype"), BODY_NOT_PARSEABLE("Could not parse content of request."), PAYLOAD_NOT_ALLOWED("Request body not allowed for this action."), - PAYLOAD_MANDATORY("Request body required for this action."), SECURITY_NOT_INITIALIZED("Security index not initialized"); + PAYLOAD_MANDATORY("Request body required for this action."), SECURITY_NOT_INITIALIZED("Security index not initialized"), + NULL_ARRAY_ELEMENT("`null` is not allowed as json array element"); private String message; @@ -297,4 +312,19 @@ public String getMessage() { protected final boolean hasParams() { return param != null && param.length > 0; } -} \ No newline at end of file + + private boolean hasNullArrayElement(JsonNode node) { + for (JsonNode element: node) { + if(element.isNull()) { + if (node.isArray()) { + return true; + } + } else if (element.isContainerNode()) { + if (hasNullArrayElement(element)) { + return true; + } + } + } + return false; + } +} diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java index f92b375b1b..d52df102a0 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/ActionGroupsApiTest.java @@ -359,4 +359,17 @@ public void testActionGroupsApiForNonSuperAdmin() throws Exception { } + @Test + public void checkNullElementsInArray() throws Exception{ + setup(); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + + String body = FileHelper.loadFile("restapi/actiongroup_null_array_element.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/actiongroups/CRUD_UT", body, new Header[0]); + Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + } + } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java index 8d167dbf11..8d41ffde3f 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/NodesDnApiTest.java @@ -18,14 +18,18 @@ import com.amazon.opendistroforelasticsearch.security.auditlog.impl.AuditCategory; import com.amazon.opendistroforelasticsearch.security.auditlog.impl.AuditMessage; import com.amazon.opendistroforelasticsearch.security.auditlog.integration.TestAuditlogImpl; +import com.amazon.opendistroforelasticsearch.security.dlic.rest.validation.AbstractConfigurationValidator; import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants; +import com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper; import com.amazon.opendistroforelasticsearch.security.test.helper.rest.RestHelper.HttpResponse; + import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; import org.apache.http.Header; import org.apache.http.HttpStatus; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentType; import org.junit.Assert; import org.junit.Test; @@ -98,6 +102,15 @@ private void testCrudScenarios(final int expectedStatus, final Header... headers assertThat(response.getBody(), response.getStatusCode(), equalTo(expectedStatus)); } + private void checkNullElementsInArray(final Header headers) throws Exception{ + + String body = FileHelper.loadFile("restapi/nodesdn_null_array_element.json"); + HttpResponse response = rh.executePutRequest("_opendistro/_security/api/nodesdn/cluster1", body, headers); + Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + } + @Test public void testNodesDnApiWithDynamicConfigDisabled() throws Exception { setup(); @@ -138,6 +151,12 @@ public void testNodesDnApi() throws Exception { testCrudScenarios(HttpStatus.SC_OK, nonAdminCredsHeader); } + { + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + checkNullElementsInArray(nonAdminCredsHeader); + } + { // any creds, admin certificate, disallowed key - FORBIDDEN rh.keystore = "restapi/kirk-keystore.jks"; @@ -192,4 +211,5 @@ public void testNodesDnApiAuditComplianceLogging() throws Exception { assertThat(actualCategoryCounts, equalTo(expectedCategoryCounts)); } + } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java index 87bee653a6..4f989f3926 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java @@ -20,12 +20,13 @@ import com.amazon.opendistroforelasticsearch.security.DefaultObjectMapper; import org.apache.http.Header; import org.apache.http.HttpStatus; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentType; import org.junit.Assert; import org.junit.Test; import com.fasterxml.jackson.databind.JsonNode; import com.amazon.opendistroforelasticsearch.security.dlic.rest.validation.AbstractConfigurationValidator; -import com.amazon.opendistroforelasticsearch.security.dlic.rest.validation.AbstractConfigurationValidator.ErrorType; import com.amazon.opendistroforelasticsearch.security.support.SecurityJsonNode; import com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper; import com.amazon.opendistroforelasticsearch.security.test.helper.rest.RestHelper.HttpResponse; @@ -346,7 +347,7 @@ public void testRolesApi() throws Exception { Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); settings = DefaultObjectMapper.readTree(response.getBody()); Assert.assertEquals(settings.get("status").asText(), "error"); - Assert.assertEquals(settings.get("reason").asText(), ErrorType.INVALID_CONFIGURATION.getMessage()); + Assert.assertEquals(settings.get("reason").asText(), AbstractConfigurationValidator.ErrorType.INVALID_CONFIGURATION.getMessage()); // -- PATCH // PATCH on non-existing resource @@ -502,4 +503,47 @@ public void testRolesApiForNonSuperAdmin() throws Exception { } + @Test + public void checkNullElementsInArray() throws Exception{ + setup(); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + + String body = FileHelper.loadFile("restapi/roles_null_array_element_cluster_permissions.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/roles_null_array_element_index_permissions.json"); + response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/roles_null_array_element_tenant_permissions.json"); + response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/roles_null_array_element_index_patterns.json"); + response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/roles_null_array_element_masked_fields.json"); + response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/roles_null_array_element_allowed_actions.json"); + response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + } + } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java index 5c6d2d5dbe..f1b38bc6b3 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java @@ -392,4 +392,32 @@ public void testRolesMappingApiForNonSuperAdmin() throws Exception { Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } + + @Test + public void checkNullElementsInArray() throws Exception{ + setup(); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + + String body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_users.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains", + body, new Header[0]); + Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_backend_roles.json"); + response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains", + body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + + body = FileHelper.loadFile("restapi/rolesmapping_null_array_element_hosts.json"); + response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains", + body, new Header[0]); + settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + } } diff --git a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java index d4a1bf47cc..3d5650c28b 100644 --- a/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java +++ b/src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java @@ -603,4 +603,17 @@ public void testUserApiForNonSuperAdmin() throws Exception { } + @Test + public void checkNullElementsInArray() throws Exception{ + setup(); + rh.keystore = "restapi/kirk-keystore.jks"; + rh.sendAdminCertificate = true; + + String body = FileHelper.loadFile("restapi/users_null_array_element.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/internalusers/picard", body, new Header[0]); + Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build(); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + Assert.assertEquals(AbstractConfigurationValidator.ErrorType.NULL_ARRAY_ELEMENT.getMessage(), settings.get("reason")); + } + } diff --git a/src/test/resources/restapi/actiongroup_null_array_element.json b/src/test/resources/restapi/actiongroup_null_array_element.json new file mode 100644 index 0000000000..ff0d34c350 --- /dev/null +++ b/src/test/resources/restapi/actiongroup_null_array_element.json @@ -0,0 +1,4 @@ +{ + "allowed_actions": [null, "OPENDISTRO_SECURITY_WRITE"] +} + diff --git a/src/test/resources/restapi/nodesdn_null_array_element.json b/src/test/resources/restapi/nodesdn_null_array_element.json new file mode 100644 index 0000000000..2998b06760 --- /dev/null +++ b/src/test/resources/restapi/nodesdn_null_array_element.json @@ -0,0 +1,3 @@ +{ + "nodes_dn" : [null] +} diff --git a/src/test/resources/restapi/roles_null_array_element_allowed_actions.json b/src/test/resources/restapi/roles_null_array_element_allowed_actions.json new file mode 100644 index 0000000000..a253062b17 --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_allowed_actions.json @@ -0,0 +1,9 @@ +{ + "cluster_permissions" : ["cluster:monitor1*"], + "index_permissions" : [{ + "index_patterns" : [ "*" ], + "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ], + "allowed_actions" : [ "123", null ] + }], + "tenant_permissions" : [ ] +} diff --git a/src/test/resources/restapi/roles_null_array_element_cluster_permissions.json b/src/test/resources/restapi/roles_null_array_element_cluster_permissions.json new file mode 100644 index 0000000000..95f444566c --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_cluster_permissions.json @@ -0,0 +1,9 @@ +{ + "cluster_permissions" : [null], + "index_permissions" : [ { + "index_patterns" : [ "*" ], + "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ], + "allowed_actions" : [ "123", "indices:data/read/*" ] + } ], + "tenant_permissions" : [ ] +} diff --git a/src/test/resources/restapi/roles_null_array_element_index_patterns.json b/src/test/resources/restapi/roles_null_array_element_index_patterns.json new file mode 100644 index 0000000000..90b9f79b0b --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_index_patterns.json @@ -0,0 +1,9 @@ +{ + "cluster_permissions" : ["cluster:monitor1*"], + "index_permissions" : [{ + "index_patterns" : [null], + "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ], + "allowed_actions" : [ "123", "indices:data/read/*" ] + }], + "tenant_permissions" : [ ] +} diff --git a/src/test/resources/restapi/roles_null_array_element_index_permissions.json b/src/test/resources/restapi/roles_null_array_element_index_permissions.json new file mode 100644 index 0000000000..61b632b041 --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_index_permissions.json @@ -0,0 +1,5 @@ +{ + "cluster_permissions" : ["cluster:monitor1*"], + "index_permissions" : [null], + "tenant_permissions" : [ ] +} diff --git a/src/test/resources/restapi/roles_null_array_element_masked_fields.json b/src/test/resources/restapi/roles_null_array_element_masked_fields.json new file mode 100644 index 0000000000..91a4edfdae --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_masked_fields.json @@ -0,0 +1,9 @@ +{ + "cluster_permissions" : ["cluster:monitor1*"], + "index_permissions" : [{ + "index_patterns" : [ "*" ], + "masked_fields" : [ "abvfg", null, "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ], + "allowed_actions" : [ "123", "indices:data/read/*" ] + }], + "tenant_permissions" : [ ] +} diff --git a/src/test/resources/restapi/roles_null_array_element_tenant_permissions.json b/src/test/resources/restapi/roles_null_array_element_tenant_permissions.json new file mode 100644 index 0000000000..77c0418826 --- /dev/null +++ b/src/test/resources/restapi/roles_null_array_element_tenant_permissions.json @@ -0,0 +1,9 @@ +{ + "cluster_permissions" : ["cluster:monitor1*"], + "index_permissions" : [{ + "index_patterns" : [ "*" ], + "masked_fields" : [ "abvfg", "*ip_dest*::/[0-9]{1,3}$/::XXX", "*ip_source*::/[0-9]{1,3}$/::XXX::/^[0-9]{1,3}/::***", "customer.name::SHA-512", "/[0-9]{1,3}/::SHA-512" ], + "allowed_actions" : [ "123", "indices:data/read/*" ] + }], + "tenant_permissions" : [null] +} diff --git a/src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json b/src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json new file mode 100644 index 0000000000..745526c3f2 --- /dev/null +++ b/src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json @@ -0,0 +1,14 @@ +{ + "users": [ + "user1", + "user2" + ], + "backend_roles": [ + "captains", + null + ], + "hosts": [ + "8.8.8.8", + "8.8.4.4" + ] +} diff --git a/src/test/resources/restapi/rolesmapping_null_array_element_hosts.json b/src/test/resources/restapi/rolesmapping_null_array_element_hosts.json new file mode 100644 index 0000000000..84b8fafd5b --- /dev/null +++ b/src/test/resources/restapi/rolesmapping_null_array_element_hosts.json @@ -0,0 +1,14 @@ +{ + "users": [ + "user1", + "user2" + ], + "backend_roles": [ + "captains", + "role2" + ], + "hosts": [ + null, + "8.8.4.4" + ] +} diff --git a/src/test/resources/restapi/rolesmapping_null_array_element_users.json b/src/test/resources/restapi/rolesmapping_null_array_element_users.json new file mode 100644 index 0000000000..c570fe5f8d --- /dev/null +++ b/src/test/resources/restapi/rolesmapping_null_array_element_users.json @@ -0,0 +1,11 @@ +{ + "users": [ + null + ], + "backend_roles": [ + "captains" + ], + "hosts": [ + "8.8.8.8" + ] +} diff --git a/src/test/resources/restapi/users_null_array_element.json b/src/test/resources/restapi/users_null_array_element.json new file mode 100644 index 0000000000..ac401c0803 --- /dev/null +++ b/src/test/resources/restapi/users_null_array_element.json @@ -0,0 +1,5 @@ +{ + "backend_roles": [ + null + ] +}