From a5c63b319f63374c4d105dc6c3dc1290be956209 Mon Sep 17 00:00:00 2001 From: Azar Fazel Date: Fri, 7 May 2021 12:38:18 -0700 Subject: [PATCH] Address more PR comments --- .../AbstractConfigurationValidator.java | 60 +++++++++++-------- .../dlic/rest/api/ActionGroupsApiTest.java | 4 +- .../dlic/rest/api/NodesDnApiTest.java | 5 +- .../security/dlic/rest/api/RolesApiTest.java | 27 +++++++-- .../dlic/rest/api/RolesMappingApiTest.java | 13 +++- .../security/dlic/rest/api/UserApiTest.java | 4 +- .../actiongroup_null_array_element.json | 5 +- .../restapi/nodesdn_null_array_element.json | 2 +- ...es_null_array_element_allowed_actions.json | 9 +++ ...ll_array_element_cluster_permissions.json} | 2 +- ...les_null_array_element_index_patterns.json | 9 +++ ..._null_array_element_index_permissions.json | 5 ++ ...oles_null_array_element_masked_fields.json | 9 +++ ...null_array_element_tenant_permissions.json | 9 +++ ...ping_null_array_element_backend_roles.json | 14 +++++ ...rolesmapping_null_array_element_hosts.json | 14 +++++ ...olesmapping_null_array_element_users.json} | 2 +- .../restapi/users_null_array_element.json | 2 +- 18 files changed, 154 insertions(+), 41 deletions(-) create mode 100644 src/test/resources/restapi/roles_null_array_element_allowed_actions.json rename src/test/resources/restapi/{roles_null_array_element.json => roles_null_array_element_cluster_permissions.json} (99%) create mode 100644 src/test/resources/restapi/roles_null_array_element_index_patterns.json create mode 100644 src/test/resources/restapi/roles_null_array_element_index_permissions.json create mode 100644 src/test/resources/restapi/roles_null_array_element_masked_fields.json create mode 100644 src/test/resources/restapi/roles_null_array_element_tenant_permissions.json create mode 100644 src/test/resources/restapi/rolesmapping_null_array_element_backend_roles.json create mode 100644 src/test/resources/restapi/rolesmapping_null_array_element_hosts.json rename src/test/resources/restapi/{rolesmapping_null_array_element.json => rolesmapping_null_array_element_users.json} (98%) 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 4a19541344..27216f65f8 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 @@ -22,8 +22,9 @@ import java.util.Map; import java.util.Map.Entry; import java.util.Set; -import java.util.Arrays; -import java.io.IOException; +import java.util.ArrayList; +import java.util.List; +import java.util.HashMap; import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants; import com.amazon.opendistroforelasticsearch.security.DefaultObjectMapper; @@ -191,30 +192,41 @@ public boolean validate() { } //null element in the values of all the possible keys with DataType as ARRAY - try { - HashMap contentAsMap = DefaultObjectMapper.readValue(content.utf8ToString(), HashMap.class); - for (String allowedKey : allowedKeys.keySet()) { - DataType dataType = allowedKeys.get(allowedKey); - if (dataType == DataType.ARRAY && contentAsMap.containsKey(allowedKey)) { - Object valuesObject = contentAsMap.get(allowedKey); - try { - if (((java.util.List) valuesObject).contains(null)) { - this.errorType = ErrorType.INVALID_ARRAY_ELEMENT; - return false; - } - } catch (Exception e) { - log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e); - this.errorType = ErrorType.BODY_NOT_PARSEABLE; + for (Entry allowedKey : allowedKeys.entrySet()) { + DataType dataType = allowedKey.getValue(); + JsonNode value = contentAsNode.get(allowedKey.getKey()); + if (dataType == DataType.ARRAY && value != null) { + try { + List contentArray = DefaultObjectMapper.objectMapper.convertValue(value, java.util.List.class); + if (contentArray.contains(null)) { + this.errorType = ErrorType.NULL_ARRAY_ELEMENT; return false; } + else { + for (int i = 0; i < contentArray.size(); i++) { + if (contentArray.get(i) instanceof HashMap) { + for (Object valueList : ((HashMap) contentArray.get(i)).values()) { + if (((List) valueList).contains(null)) { + this.errorType = ErrorType.NULL_ARRAY_ELEMENT; + return false; + } + } + } + else if(contentArray.get(i) instanceof ArrayList){ + if(((ArrayList) contentArray.get(i)).contains(null)){ + this.errorType = ErrorType.NULL_ARRAY_ELEMENT; + return false; + } + } + } + } + } catch (Exception e) { + log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e); + this.errorType = ErrorType.BODY_NOT_PARSEABLE; + return false; } } - } catch(IOException e) { - log.error(ErrorType.BODY_NOT_PARSEABLE.toString(), e); - this.errorType = ErrorType.BODY_NOT_PARSEABLE; - return false; } - return valid; } @@ -280,9 +292,9 @@ public XContentBuilder errorsAsXContent(RestChannel channel) { builder.field(entry.getKey(), entry.getValue()); } break; - case INVALID_ARRAY_ELEMENT: + case NULL_ARRAY_ELEMENT: builder.field("status", "error"); - builder.field("reason", ErrorType.INVALID_ARRAY_ELEMENT.getMessage()); + builder.field("reason", ErrorType.NULL_ARRAY_ELEMENT.getMessage()); break; default: builder.field("status", "error"); @@ -313,7 +325,7 @@ 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"), - INVALID_ARRAY_ELEMENT("Invalid array element"); + NULL_ARRAY_ELEMENT("`null` is not allowed as json array element"); private String message; 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 4897068365..2a5dd7f46e 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 @@ -365,8 +365,8 @@ public void checkNullElementsInArray() throws Exception{ rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/actiongroups/CRUD_UT", - FileHelper.loadFile("restapi/actiongroup_null_array_element.json"), new Header[0]); + String body = FileHelper.loadFile("restapi/actiongroup_null_array_element.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/actiongroups/CRUD_UT", body, new Header[0]); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } 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 9749a77fd4..0e1155cead 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 @@ -20,6 +20,7 @@ import com.amazon.opendistroforelasticsearch.security.auditlog.integration.TestAuditlogImpl; import com.amazon.opendistroforelasticsearch.security.support.ConfigConstants; import com.amazon.opendistroforelasticsearch.security.test.helper.rest.RestHelper.HttpResponse; +import com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper; import com.fasterxml.jackson.databind.JsonNode; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.collect.ImmutableMap; @@ -199,8 +200,8 @@ public void checkNullElementsInArray() throws Exception{ rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/nodesdn", - com.amazon.opendistroforelasticsearch.security.test.helper.file.FileHelper.loadFile("restapi/nodesdn_null_array_element.json"), new Header[0]); + String body = FileHelper.loadFile("restapi/nodesdn_null_array_element.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/nodesdn", body, new Header[0]); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } } 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 e78f1fe97b..2efee5d7a5 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 @@ -471,7 +471,7 @@ public void testRolesApiForNonSuperAdmin() throws Exception { // Put read only roles response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_transport_client", FileHelper.loadFile("restapi/roles_captains.json"), new Header[0]); - Assert.assertEquals(org.apache.http.HttpStatus.SC_FORBIDDEN, response.getStatusCode()); + Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode()); // Patch single read only roles response = rh.executePatchRequest("/_opendistro/_security/api/roles/opendistro_security_transport_client", "[{ \"op\": \"replace\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]); @@ -510,9 +510,28 @@ public void checkNullElementsInArray() throws Exception{ rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/roles/opendistro_security_role_starfleet", - FileHelper.loadFile("restapi/roles_null_array_element.json"), new Header[0]); - System.out.println(response.getBody()); + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } 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 3da9336af4..110da318e7 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 @@ -399,8 +399,19 @@ public void checkNullElementsInArray() throws Exception{ 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", - FileHelper.loadFile("restapi/rolesmapping_null_array_element.json"), new Header[0]); + body, new Header[0]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); + Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); + + 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]); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } } 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 03cae7112a..b7b76aefb3 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 @@ -599,8 +599,8 @@ public void checkNullElementsInArray() throws Exception{ rh.keystore = "restapi/kirk-keystore.jks"; rh.sendAdminCertificate = true; - HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/internalusers/picard", - FileHelper.loadFile("restapi/users_null_array_element.json"), new Header[0]); + String body = FileHelper.loadFile("restapi/users_null_array_element.json"); + HttpResponse response = rh.executePutRequest("/_opendistro/_security/api/internalusers/picard", body, new Header[0]); Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode()); } diff --git a/src/test/resources/restapi/actiongroup_null_array_element.json b/src/test/resources/restapi/actiongroup_null_array_element.json index 8b24ce1530..ff0d34c350 100644 --- a/src/test/resources/restapi/actiongroup_null_array_element.json +++ b/src/test/resources/restapi/actiongroup_null_array_element.json @@ -1,3 +1,4 @@ { - "allowed_actions": [null] -} \ No newline at end of file + "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 index 91bb55827d..2998b06760 100644 --- a/src/test/resources/restapi/nodesdn_null_array_element.json +++ b/src/test/resources/restapi/nodesdn_null_array_element.json @@ -1,3 +1,3 @@ { "nodes_dn" : [null] -} \ No newline at end of file +} 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.json b/src/test/resources/restapi/roles_null_array_element_cluster_permissions.json similarity index 99% rename from src/test/resources/restapi/roles_null_array_element.json rename to src/test/resources/restapi/roles_null_array_element_cluster_permissions.json index beb9ceb48f..95f444566c 100644 --- a/src/test/resources/restapi/roles_null_array_element.json +++ b/src/test/resources/restapi/roles_null_array_element_cluster_permissions.json @@ -6,4 +6,4 @@ "allowed_actions" : [ "123", "indices:data/read/*" ] } ], "tenant_permissions" : [ ] -} \ No newline at end of file +} 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.json b/src/test/resources/restapi/rolesmapping_null_array_element_users.json similarity index 98% rename from src/test/resources/restapi/rolesmapping_null_array_element.json rename to src/test/resources/restapi/rolesmapping_null_array_element_users.json index b958e1827e..c570fe5f8d 100644 --- a/src/test/resources/restapi/rolesmapping_null_array_element.json +++ b/src/test/resources/restapi/rolesmapping_null_array_element_users.json @@ -8,4 +8,4 @@ "hosts": [ "8.8.8.8" ] -} \ No newline at end of file +} diff --git a/src/test/resources/restapi/users_null_array_element.json b/src/test/resources/restapi/users_null_array_element.json index f310394b8f..ac401c0803 100644 --- a/src/test/resources/restapi/users_null_array_element.json +++ b/src/test/resources/restapi/users_null_array_element.json @@ -2,4 +2,4 @@ "backend_roles": [ null ] -} \ No newline at end of file +}