Prevent hidden roles from being added via rolesmapping and internalus…

src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/AbstractApiAction.java

@@ -99,7 +99,7 @@ protected AbstractApiAction(final Settings settings, final Path configPath, fina
 		this.auditLog = auditLog;
 	}
 
-	protected abstract AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params);
+    protected abstract AbstractConfigurationValidator getValidator(RestRequest request, BytesReference ref, Object... params);
 
 	protected abstract String getResourceName();
 
@@ -579,4 +579,33 @@ protected boolean isReadOnly(final SecurityDynamicConfiguration<?> existingConfi
 		return isSuperAdmin() ? false: isReserved(existingConfiguration, name);
 	}
 
+	/**
+	 * Checks if it is valid to add role to opendistro_security_roles or rolesmapping.
+	 * Role can be mapped to user if it exists. Only superadmin can add hidden or reserved roles.
+	 *
+	 * @param channel	Rest Channel for response
+	 * @param role		Name of the role
+	 * @return True if role can be mapped
+	 */
+	protected boolean isValidRolesMapping(final RestChannel channel, final String role) {
+		final SecurityDynamicConfiguration<?> rolesConfiguration = load(CType.ROLES, false);
+		final SecurityDynamicConfiguration<?> rolesMappingConfiguration = load(CType.ROLESMAPPING, false);
+
+		if (!rolesConfiguration.exists(role)) {
+			notFound(channel, "Role '"+role+"' is not available for role-mapping.");
+			return false;
+		}
+
+		if (isHidden(rolesConfiguration, role) || isHidden(rolesMappingConfiguration, role)) {
+			notFound(channel, "Role '"+role+"' is not available for role-mapping.");
+			return false;
+		}
+
+		if (isReadOnly(rolesMappingConfiguration, role)) {
+			forbidden(channel, "Role '" + role + "' has read-only role-mapping.");
+			return false;
+		}
+		return true;
+	}
+
 }

src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/InternalUsersApiAction.java

@@ -115,27 +115,11 @@ protected void handlePut(RestChannel channel, final RestRequest request, final C
         final ObjectNode contentAsNode = (ObjectNode) content;
         final SecurityJsonNode securityJsonNode = new SecurityJsonNode(contentAsNode);
 
-        // Don't allow user to add hidden, reserved or non-existent rolesmapping
+        // Don't allow user to add non-existent role or a role for which role-mapping is hidden or reserved
         final List<String> opendistroSecurityRoles = securityJsonNode.get("opendistro_security_roles").asList();
         if (opendistroSecurityRoles != null) {
-            final SecurityDynamicConfiguration<?> rolesConfiguration = load(CType.ROLES, false);
-            final SecurityDynamicConfiguration<?> rolesmappingConfiguration = load(CType.ROLESMAPPING, false);
             for (final String role: opendistroSecurityRoles) {
-
-                if (rolesConfiguration.getCEntry(role) == null) {
-                    notFound(channel, "Role '"+role+"' is not available.");
-                    return;
-                }
-
-                if (rolesmappingConfiguration.isHidden(role)) {
-                    notFound(channel, "Role '"+role+"' is not available.");
-                    return;
-                }
-
-                if (isReadOnly(rolesmappingConfiguration, role)) {
-                    forbidden(channel, "Role '" + role + "' is read-only.");
-                    return;
-                }
+                if (!isValidRolesMapping(channel, role)) return;
             }
         }
 

src/main/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiAction.java

@@ -15,15 +15,21 @@
 
 package com.amazon.opendistroforelasticsearch.security.dlic.rest.api;
 
+import java.io.IOException;
 import java.nio.file.Path;
 import java.util.List;
 
+import com.amazon.opendistroforelasticsearch.security.DefaultObjectMapper;
+import com.amazon.opendistroforelasticsearch.security.securityconf.impl.SecurityDynamicConfiguration;
+import com.fasterxml.jackson.databind.JsonNode;
 import com.google.common.collect.ImmutableList;
+import org.elasticsearch.action.index.IndexResponse;
 import org.elasticsearch.client.Client;
 import org.elasticsearch.cluster.service.ClusterService;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.inject.Inject;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestController;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.RestRequest.Method;
@@ -55,6 +61,36 @@ public RolesMappingApiAction(final Settings settings, final Path configPath, fin
 		super(settings, configPath, controller, client, adminDNs, cl, cs, principalExtractor, evaluator, threadPool, auditLog);
 	}
 
+	@Override
+	protected void handlePut(RestChannel channel, final RestRequest request, final Client client, final JsonNode content) throws IOException {
+		final String name = request.param("name");
+
+		if (name == null || name.length() == 0) {
+			badRequestResponse(channel, "No " + getResourceName() + " specified.");
+			return;
+		}
+
+		final SecurityDynamicConfiguration<?> rolesMappingConfiguration = load(getConfigName(), false);
+		final boolean rolesMappingExists = rolesMappingConfiguration.exists(name);
+
+		if (!isValidRolesMapping(channel, name)) return;
+
+		rolesMappingConfiguration.putCObject(name, DefaultObjectMapper.readTree(content, rolesMappingConfiguration.getImplementingClass()));
+
+		saveAnUpdateConfigs(client, request, getConfigName(), rolesMappingConfiguration, new OnSucessActionListener<IndexResponse>(channel) {
+
+			@Override
+			public void onResponse(IndexResponse response) {
+				if (rolesMappingExists) {
+					successResponse(channel, "'" + name + "' updated.");
+				} else {
+					createdResponse(channel, "'" + name + "' created.");
+				}
+
+			}
+		});
+	}
+
 	@Override
 	public List<Route> routes() {
 		return routes;

src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesApiTest.java

@@ -55,7 +55,7 @@ public void testPutRole() throws Exception {
     }
 
     @Test
-    public void testAllRolesNotContainMetaHeader() throws Exception {
+    public void testAllRolesForSuperAdmin() throws Exception {
 
         setup();
 
@@ -64,6 +64,9 @@ public void testAllRolesNotContainMetaHeader() throws Exception {
         HttpResponse response = rh.executeGetRequest("_opendistro/_security/api/roles");
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
         Assert.assertFalse(response.getBody().contains("_meta"));
+
+        // Super admin should be able to see all roles including hidden
+        Assert.assertTrue(response.getBody().contains("opendistro_security_hidden"));
     }
 
     @Test
@@ -142,8 +145,8 @@ public void testRolesApi() throws Exception {
         Assert.assertFalse(response.getBody().contains("\"cluster_permissions\":[\"*\"]"));
         Assert.assertTrue(response.getBody().contains("\"cluster_permissions\" : ["));
 
-        // hidden role
-        response = rh.executeGetRequest("/_opendistro/_security/api/roles/opendistro_security_internal", new Header[0]);
+        // Super admin should be able to describe hidden role
+        response = rh.executeGetRequest("/_opendistro/_security/api/roles/opendistro_security_hidden", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
         Assert.assertTrue(response.getBody().contains("\"hidden\":true"));
 

src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/RolesMappingApiTest.java

@@ -47,6 +47,13 @@ public void testRolesMappingApi() throws Exception {
 		Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 		Assert.assertTrue(response.getContentType(), response.isJsonContentType());
 
+		// Superadmin should be able to see hidden rolesmapping
+		Assert.assertTrue(response.getBody().contains("opendistro_security_hidden"));
+
+		// Superadmin should be able to see reserved rolesmapping
+		Assert.assertTrue(response.getBody().contains("opendistro_security_reserved"));
+
+
 		// -- GET
 
 		// GET opendistro_security_role_starfleet, exists
@@ -73,9 +80,9 @@ public void testRolesMappingApi() throws Exception {
 		Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 		Assert.assertTrue(response.getContentType(), response.isJsonContentType());
 
-	        // GET, rolesmapping is hidden, allowed for super admin
-        	response = rh.executeGetRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", new Header[0]);
-        	Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+		// Super admin should be able to describe particular hidden rolemapping
+		response = rh.executeGetRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 		Assert.assertTrue(response.getBody().contains("\"hidden\":true"));
 
 		// create index
@@ -102,10 +109,10 @@ public void testRolesMappingApi() throws Exception {
 		response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_library", new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 
-        	// hidden role
-        	response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", new Header[0]);
-        	Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
-		Assert.assertTrue(response.getBody().contains("'opendistro_security_role_internal' deleted."));
+		// hidden role
+        response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
+		Assert.assertTrue(response.getBody().contains("'opendistro_security_internal' deleted."));
 
 		// remove complete role mapping for opendistro_security_role_starfleet_captains
 		response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_starfleet_captains", new Header[0]);
@@ -189,8 +196,8 @@ public void testRolesMappingApi() throws Exception {
 				FileHelper.loadFile("restapi/rolesmapping_all_access.json"), new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode());
 
-        // hidden role, allowed for super admin
-        response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal",
+		// hidden role, allowed for super admin
+        response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal",
                 FileHelper.loadFile("restapi/rolesmapping_all_access.json"), new Header[0]);
         Assert.assertEquals(HttpStatus.SC_CREATED, response.getStatusCode());
 
@@ -212,7 +219,8 @@ public void testRolesMappingApi() throws Exception {
 
         // PATCH hidden resource, must be not found, can be found by super admin
         rh.sendAdminCertificate = true;
-        response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", "[{ \"op\": \"add\", \"path\": \"/a/b/c\", \"value\": [ \"foo\", \"bar\" ] }]", new Header[0]);
+        response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", "[{ \"op\": \"add\", \"path\": \"/a/b/c\", \"value\": [ " +
+            "\"foo\", \"bar\" ] }]", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
 
         // PATCH value of hidden flag, must fail with validation error
@@ -246,7 +254,7 @@ public void testRolesMappingApi() throws Exception {
 
         // PATCH hidden resource, must be bad request
         rh.sendAdminCertificate = true;
-        response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping", "[{ \"op\": \"add\", \"path\": \"/opendistro_security_role_internal/a\", \"value\": [ \"foo\", \"bar\" ] }]", new Header[0]);
+        response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping", "[{ \"op\": \"add\", \"path\": \"/opendistro_security_internal/a\", \"value\": [ \"foo\", \"bar\" ] }]", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
 
         // PATCH value of hidden flag, must fail with validation error
@@ -312,7 +320,6 @@ private void checkAllSfAllowed() throws Exception {
 		rh.sendAdminCertificate = false;
 		checkReadAccess(HttpStatus.SC_OK, "picard", "picard", "sf", "ships", 1);
 		checkWriteAccess(HttpStatus.SC_OK, "picard", "picard", "sf", "ships", 1);
-
 		// ES7 only supports one doc type, so trying to create a second one leads to 400  BAD REQUEST
 		checkWriteAccess(HttpStatus.SC_BAD_REQUEST, "picard", "picard", "sf", "public", 1);
 	}
@@ -364,24 +371,24 @@ public void testRolesMappingApiForNonSuperAdmin() throws Exception {
 		Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
 
 		// GET, rolesmapping is hidden, allowed for super admin
-		response = rh.executeGetRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", new Header[0]);
+		response = rh.executeGetRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
 		// Delete hidden roles mapping
-		response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal" , new Header[0]);
+		response = rh.executeDeleteRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal" , new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
 		// Put hidden roles mapping
-		response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal",
+		response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal",
 				FileHelper.loadFile("restapi/rolesmapping_all_access.json"), new Header[0]);
-		Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
+		Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
 		// Patch hidden roles mapping
-		response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", "[{ \"op\": \"add\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
+		response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", "[{ \"op\": \"add\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
 
 		// Patch multiple hidden roles mapping
-		response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping", "[{ \"op\": \"add\", \"path\": \"/opendistro_security_role_internal/description\", \"value\": \"foo\" }]", new Header[0]);
+		response = rh.executePatchRequest("/_opendistro/_security/api/rolesmapping", "[{ \"op\": \"add\", \"path\": \"/opendistro_security_internal/description\", \"value\": \"foo\" }]", new Header[0]);
 		Assert.assertEquals(HttpStatus.SC_BAD_REQUEST, response.getStatusCode());
 
 	}

src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/TenantInfoActionTest.java

@@ -54,14 +54,13 @@ public void testTenantInfoAPI() throws Exception {
         rh.sendAdminCertificate = true;
 
         //update security config
-        response = rh.executePatchRequest("/_opendistro/_security/api/securityconfig", "[{\"op\": \"add\",\"path\": \"/config/dynamic/kibana/opendistro_role\",\"value\": \"opendistro_security_role_internal\"}]", new Header[0]);
+        response = rh.executePatchRequest("/_opendistro/_security/api/securityconfig", "[{\"op\": \"add\",\"path\": \"/config/dynamic/kibana/opendistro_role\",\"value\": \"opendistro_security_internal\"}]", new Header[0]);
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 
-        response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_role_internal", payload, new Header[0]);
+        response = rh.executePutRequest("/_opendistro/_security/api/rolesmapping/opendistro_security_internal", payload, new Header[0]);
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 
         rh.sendAdminCertificate = false;
-
         response = rh.executeGetRequest("_opendistro/_security/tenantinfo");
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
     }

src/test/java/com/amazon/opendistroforelasticsearch/security/dlic/rest/api/UserApiTest.java

@@ -121,12 +121,10 @@ public void testUserApi() throws Exception {
         Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
         Assert.assertTrue(response.getBody().contains("\"hidden\":true"));
 
-        // Associating with hidden rolemapping is not allowed
-        response = rh.executePutRequest("/_opendistro/_security/api/internalusers/nagilum", "{ \"opendistro_security_roles\": [\"opendistro_security_hidden\"]}",
-            new Header[0]);
-        Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
-        settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build();
-        Assert.assertEquals(settings.get("message"), "Role 'opendistro_security_hidden' is not available.");
+        // Associating with hidden role is allowed (for superadmin)
+        response = rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"opendistro_security_roles\": " +
+                "[\"opendistro_security_hidden\"]}", new Header[0]);
+        Assert.assertEquals(HttpStatus.SC_OK, response.getStatusCode());
 
         // Associating with reserved role is allowed (for superadmin)
         response = rh.executePutRequest("/_opendistro/_security/api/internalusers/test", "{ \"opendistro_security_roles\": [\"opendistro_security_reserved\"], " +
@@ -139,7 +137,7 @@ public void testUserApi() throws Exception {
             new Header[0]);
         Assert.assertEquals(HttpStatus.SC_NOT_FOUND, response.getStatusCode());
         settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build();
-        Assert.assertEquals(settings.get("message"), "Role 'non_existent' is not available.");
+        Assert.assertEquals(settings.get("message"), "Role 'non_existent' is not available for role-mapping.");
 
         // Wrong config keys
         response = rh.executePutRequest("/_opendistro/_security/api/internalusers/nagilum", "{\"some\": \"thing\", \"other\": \"thing\"}",
@@ -563,7 +561,7 @@ public void testUserApiForNonSuperAdmin() throws Exception {
             new Header[0]);
         Assert.assertEquals(HttpStatus.SC_FORBIDDEN, response.getStatusCode());
         Settings settings = Settings.builder().loadFromSource(response.getBody(), XContentType.JSON).build();
-        Assert.assertEquals(settings.get("message"), "Role 'opendistro_security_reserved' is read-only.");
+        Assert.assertEquals(settings.get("message"), "Role 'opendistro_security_reserved' has read-only role-mapping.");
 
         // Patch single hidden user
         response = rh.executePatchRequest("/_opendistro/_security/api/internalusers/hide", "[{ \"op\": \"add\", \"path\": \"/description\", \"value\": \"foo\" }]", new Header[0]);

src/test/resources/restapi/roles.yml

@@ -17,6 +17,20 @@ opendistro_security_unittest_1:
     allowed_actions:
     - "*"
   tenant_permissions: []
+opendistro_security_role_starfleet_library:
+  reserved: true
+  hidden: false
+  description: "Migrated from v6 (all types mapped)"
+  cluster_permissions: []
+  index_permissions:
+    - index_patterns:
+        - "abc*"
+      dls: null
+      fls: null
+      masked_fields: null
+      allowed_actions:
+        - "ALL"
+  tenant_permissions: []
 
 opendistro_security_hidden:
   reserved: false
@@ -33,7 +47,6 @@ opendistro_security_hidden:
       allowed_actions:
         - "ALL"
   tenant_permissions: []
-
 opendistro_security_reserved:
   reserved: true
   hidden: false
@@ -48,7 +61,6 @@ opendistro_security_reserved:
       masked_fields: null
       allowed_actions:
         - "ALL"
-
 opendistro_security_internal:
   reserved: false
   hidden: true
@@ -57,7 +69,7 @@ opendistro_security_internal:
   - "OPENDISTRO_SECURITY_CLUSTER_ALL"
   index_permissions:
   - index_patterns:
-    - "*"
+    - "abc*"
     dls: null
     fls: null
     masked_fields: null

src/test/resources/restapi/roles_mapping.yml

@@ -87,14 +87,13 @@ opendistro_security_theindex_admin:
   - "theindexadmin"
   and_backend_roles: []
   description: "Migrated from v6"
-opendistro_security_role_internal:
+opendistro_security_internal:
   reserved: false
   hidden: true
-  backend_roles:
-  - "starfleet*"
-  - "ambassador"
+  backend_roles: []
   hosts: []
-  users: []
+  users:
+    - "test"
   and_backend_roles: []
   description: "Migrated from v6"
 opendistro_security_zdummy_all: