Disallow Password Change when authenticated by Token (#49694) (#53614)

Password changes are only allowed when the user is currently authenticated by a realm (that permits the password to be changed) and not when authenticated by a bearer token or an API key.
elastic · Mar 17, 2020 · 23af171 · 23af171
1 parent 7f21ade
commit 23af171
Show file tree

Hide file tree

Showing 3 changed files with 123 additions and 6 deletions.
diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/RBACEngine.java
@@ -539,9 +539,12 @@ private static boolean checkChangePasswordAction(Authentication authentication)
         }
 
         assert realmType != null;
-        // ensure the user was authenticated by a realm that we can change a password for. The native realm is an internal realm and
-        // right now only one can exist in the realm configuration - if this changes we should update this check
-        return ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType);
+        // Ensure that the user is not authenticated with an access token or an API key.
+        // Also ensure that the user was authenticated by a realm that we can change a password for. The native realm is an internal realm
+        // and right now only one can exist in the realm configuration - if this changes we should update this check
+        final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
+        return (authType.equals(Authentication.AuthenticationType.REALM)
+            && (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType)));
     }
 
     static class RBACAuthorizationInfo implements AuthorizationInfo {

diff --git a/...plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/...plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java
@@ -102,6 +102,7 @@ public void testSameUserPermission() {
         final String action = changePasswordRequest ? ChangePasswordAction.NAME : AuthenticateAction.NAME;
         final Authentication authentication = mock(Authentication.class);
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.REALM);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authenticatedBy.getType())
@@ -125,9 +126,10 @@ public void testSameUserPermissionDoesNotAllowNonMatchingUsername() {
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
-        when(authenticatedBy.getType())
-            .thenReturn(changePasswordRequest ? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE) :
-                randomAlphaOfLengthBetween(4, 12));
+        final String authenticationType = changePasswordRequest ? randomFrom(ReservedRealm.TYPE, NativeRealmSettings.TYPE) :
+            randomAlphaOfLengthBetween(4, 12);
+        when(authenticatedBy.getType()).thenReturn(authenticationType);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.REALM);
 
         assertThat(request, instanceOf(UserRequest.class));
         assertFalse(engine.checkSameUserPermissions(action, request, authentication));
@@ -180,6 +182,7 @@ public void testSameUserPermissionRunAsChecksAuthenticatedBy() {
         final Authentication authentication = mock(Authentication.class);
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
         final Authentication.RealmRef lookedUpBy = mock(Authentication.RealmRef.class);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.REALM);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authentication.getLookedUpBy()).thenReturn(lookedUpBy);
@@ -198,6 +201,7 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForOtherRealms() {
         final String action = ChangePasswordAction.NAME;
         final Authentication authentication = mock(Authentication.class);
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.REALM);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authenticatedBy.getType()).thenReturn(randomFrom(LdapRealmSettings.LDAP_TYPE, FileRealmSettings.TYPE,
@@ -209,6 +213,47 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForOtherRealms() {
         verify(authenticatedBy).getType();
         verify(authentication).getAuthenticatedBy();
         verify(authentication, times(2)).getUser();
+        verify(authentication).getAuthenticationType();
+        verifyNoMoreInteractions(authenticatedBy, authentication);
+    }
+
+    public void testSameUserPermissionDoesNotAllowChangePasswordForApiKey() {
+        final User user = new User("joe");
+        final ChangePasswordRequest request = new ChangePasswordRequestBuilder(mock(Client.class)).username(user.principal()).request();
+        final String action = ChangePasswordAction.NAME;
+        final Authentication authentication = mock(Authentication.class);
+        final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
+        when(authentication.getUser()).thenReturn(user);
+        when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.API_KEY);
+        when(authenticatedBy.getType()).thenReturn(ApiKeyService.API_KEY_REALM_TYPE);
+
+        assertThat(request, instanceOf(UserRequest.class));
+        assertFalse(engine.checkSameUserPermissions(action, request, authentication));
+        verify(authenticatedBy).getType();
+        verify(authentication).getAuthenticatedBy();
+        verify(authentication, times(2)).getUser();
+        verify(authentication).getAuthenticationType();
+        verifyNoMoreInteractions(authenticatedBy, authentication);
+    }
+
+    public void testSameUserPermissionDoesNotAllowChangePasswordForAccessToken() {
+        final User user = new User("joe");
+        final ChangePasswordRequest request = new ChangePasswordRequestBuilder(mock(Client.class)).username(user.principal()).request();
+        final String action = ChangePasswordAction.NAME;
+        final Authentication authentication = mock(Authentication.class);
+        final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
+        when(authentication.getUser()).thenReturn(user);
+        when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.TOKEN);
+        when(authenticatedBy.getType()).thenReturn(NativeRealmSettings.TYPE);
+
+        assertThat(request, instanceOf(UserRequest.class));
+        assertFalse(engine.checkSameUserPermissions(action, request, authentication));
+        verify(authenticatedBy).getType();
+        verify(authentication).getAuthenticatedBy();
+        verify(authentication, times(2)).getUser();
+        verify(authentication).getAuthenticationType();
         verifyNoMoreInteractions(authenticatedBy, authentication);
     }
 
@@ -220,6 +265,7 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe
         final Authentication authentication = mock(Authentication.class);
         final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
         final Authentication.RealmRef lookedUpBy = mock(Authentication.RealmRef.class);
+        when(authentication.getAuthenticationType()).thenReturn(Authentication.AuthenticationType.REALM);
         when(authentication.getUser()).thenReturn(user);
         when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
         when(authentication.getLookedUpBy()).thenReturn(lookedUpBy);
@@ -232,6 +278,7 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe
         verify(authentication).getLookedUpBy();
         verify(authentication, times(2)).getUser();
         verify(lookedUpBy).getType();
+        verify(authentication).getAuthenticationType();
         verifyNoMoreInteractions(authentication, lookedUpBy, authenticatedBy);
     }
 

diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/change_password/11_token.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/change_password/11_token.yml
@@ -0,0 +1,67 @@
+---
+setup:
+  - skip:
+      features: headers
+  - do:
+      cluster.health:
+        wait_for_status: yellow
+  - do:
+      security.put_user:
+        username: "token_joe"
+        body:  >
+          {
+            "password": "s3krit",
+            "roles" : [ "token_admin" ]
+          }
+  - do:
+      security.put_role:
+        name: "token_admin"
+        body:  >
+          {
+            "cluster": ["manage_token"],
+            "indices": [
+              {
+                "names": "*",
+                "privileges": ["all"]
+              }
+            ]
+          }
+---
+teardown:
+  - do:
+      security.delete_user:
+        username: "token_joe"
+        ignore: 404
+  - do:
+      security.delete_role:
+        name: "token_admin"
+        ignore: 404
+
+---
+"Test user changing their password authenticating with token not allowed":
+
+  - do:
+      headers:
+        Authorization: "Basic dG9rZW5fam9lOnMza3JpdA=="
+      security.get_token:
+        body:
+          grant_type: "password"
+          username: "token_joe"
+          password: "s3krit"
+
+  - match: { type: "Bearer" }
+  - is_true: access_token
+  - set: { access_token: token }
+  - match: { expires_in: 1200 }
+  - is_false: scope
+
+  - do:
+      headers:
+        Authorization: Bearer ${token}
+      catch: forbidden
+      security.change_password:
+        username: "joe"
+        body:  >
+          {
+            "password" : "s3krit2"
+          }