From c781d87e332a35e343759b661cf743c66f86feae Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 28 Nov 2019 20:02:20 +0200 Subject: [PATCH 1/5] Mark token authN in the authentication object This commit changes that behavior so that we set the authenticatedBy realm reference to _es_token_service in the Authentication object we build and write to the thread context upon successful authentication with an Elasticsearch Token Service access token. This alligns our behavior with the API keys implementation and allows us to make authorization decisions based on the fact that the current request was authenticated by a bearer token ( i.e. disallow change password requests ). The original authentication realm is still available in the token document. --- .../security/authc/AuthenticationService.java | 4 +- .../xpack/security/authc/TokenService.java | 1 + .../authc/AuthenticationServiceTests.java | 2 +- .../test/change_password/11_token.yml | 67 +++++++++++++++++++ 4 files changed, 72 insertions(+), 2 deletions(-) create mode 100644 x-pack/plugin/src/test/resources/rest-api-spec/test/change_password/11_token.yml diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java index 20289c5f09e91..3ba9cb0b75b00 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java @@ -249,7 +249,9 @@ private void authenticateAsync() { } else { tokenService.getAndValidateToken(threadContext, ActionListener.wrap(userToken -> { if (userToken != null) { - writeAuthToContext(userToken.getAuthentication()); + authenticatedBy = new RealmRef(TokenService.TOKEN_PSEUDO_REALM, TokenService.TOKEN_PSEUDO_REALM, nodeName); + writeAuthToContext(new Authentication(userToken.getAuthentication().getUser(), authenticatedBy, null, + Version.CURRENT, AuthenticationType.TOKEN, userToken.getAuthentication().getMetadata())); } else { checkForApiKey(); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java index 689a6db034130..22065e4140bad 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java @@ -181,6 +181,7 @@ public final class TokenService { public static final Setting DELETE_TIMEOUT = Setting.timeSetting("xpack.security.authc.token.delete.timeout", TimeValue.MINUS_ONE, Property.NodeScope); + public static final String TOKEN_PSEUDO_REALM = "_es_token_service"; static final String TOKEN_DOC_TYPE = "token"; private static final int HASHED_TOKEN_LENGTH = 43; // UUIDs are 16 bytes encoded base64 without padding, therefore the length is (16 / 3) * 4 + ((16 % 3) * 8 + 5) / 6 chars diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java index 532b9121c1a00..6157971415e0a 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java @@ -1173,7 +1173,7 @@ public void testAuthenticateWithToken() throws Exception { }, this::logAndFail)); } assertTrue(completed.get()); - verify(auditTrail).authenticationSuccess(anyString(), eq("realm"), eq(user), eq("_action"), same(message)); + verify(auditTrail).authenticationSuccess(anyString(), eq(TokenService.TOKEN_PSEUDO_REALM), eq(user), eq("_action"), same(message)); verifyNoMoreInteractions(auditTrail); } 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 new file mode 100644 index 0000000000000..fcbbb1c257bbe --- /dev/null +++ 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" + } From 5eef8b3c98b6e79fad0be0613450edde1a95a104 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Mon, 2 Dec 2019 16:36:37 +0200 Subject: [PATCH 2/5] Fix test --- .../client/documentation/SecurityDocumentationIT.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index 372be2c2d08a5..6f7310765745e 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -2291,8 +2291,8 @@ public void testDelegatePkiAuthentication() throws Exception { assertThat(user.getUsername(), is("Elasticsearch Test Client")); RealmInfo authnRealm = resp.getAuthenticationRealm(); assertThat(authnRealm, is(notNullValue())); - assertThat(authnRealm.getName(), is("pki1")); - assertThat(authnRealm.getType(), is("pki")); + assertThat(authnRealm.getName(), is("_es_token_service")); + assertThat(authnRealm.getType(), is("_es_token_service")); } { @@ -2334,8 +2334,8 @@ public void onFailure(Exception e) { assertThat(user.getUsername(), is("Elasticsearch Test Client")); RealmInfo authnRealm = resp.getAuthenticationRealm(); assertThat(authnRealm, is(notNullValue())); - assertThat(authnRealm.getName(), is("pki1")); - assertThat(authnRealm.getType(), is("pki")); + assertThat(authnRealm.getName(), is("_es_token_service")); + assertThat(authnRealm.getType(), is("_es_token_service")); } } From d2f4b9701d10288da1188580097e3f67042141ac Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 12 Dec 2019 11:45:14 +0200 Subject: [PATCH 3/5] Disallow based on AuthenticationType Changes the approach for disallowing password changes to check the AuthenticationType instead of injecting a custom RealmRef for tokens as the authenticatedBy Realm Reference --- .../security/authc/AuthenticationService.java | 4 +- .../xpack/security/authc/TokenService.java | 1 - .../xpack/security/authz/RBACEngine.java | 10 ++-- .../authc/AuthenticationServiceTests.java | 2 +- .../xpack/security/authz/RBACEngineTests.java | 53 +++++++++++++++++-- 5 files changed, 59 insertions(+), 11 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java index 3ba9cb0b75b00..20289c5f09e91 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java @@ -249,9 +249,7 @@ private void authenticateAsync() { } else { tokenService.getAndValidateToken(threadContext, ActionListener.wrap(userToken -> { if (userToken != null) { - authenticatedBy = new RealmRef(TokenService.TOKEN_PSEUDO_REALM, TokenService.TOKEN_PSEUDO_REALM, nodeName); - writeAuthToContext(new Authentication(userToken.getAuthentication().getUser(), authenticatedBy, null, - Version.CURRENT, AuthenticationType.TOKEN, userToken.getAuthentication().getMetadata())); + writeAuthToContext(userToken.getAuthentication()); } else { checkForApiKey(); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java index 22065e4140bad..689a6db034130 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java @@ -181,7 +181,6 @@ public final class TokenService { public static final Setting DELETE_TIMEOUT = Setting.timeSetting("xpack.security.authc.token.delete.timeout", TimeValue.MINUS_ONE, Property.NodeScope); - public static final String TOKEN_PSEUDO_REALM = "_es_token_service"; static final String TOKEN_DOC_TYPE = "token"; private static final int HASHED_TOKEN_LENGTH = 43; // UUIDs are 16 bytes encoded base64 without padding, therefore the length is (16 / 3) * 4 + ((16 % 3) * 8 + 5) / 6 chars 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 index 4b0e99d7290fd..ab5d484d689d6 100644 --- 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 @@ -524,9 +524,13 @@ 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.TOKEN) == false + && authType.equals(Authentication.AuthenticationType.API_KEY) == false ) + && (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType)); } static class RBACAuthorizationInfo implements AuthorizationInfo { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java index 6157971415e0a..532b9121c1a00 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java @@ -1173,7 +1173,7 @@ public void testAuthenticateWithToken() throws Exception { }, this::logAndFail)); } assertTrue(completed.get()); - verify(auditTrail).authenticationSuccess(anyString(), eq(TokenService.TOKEN_PSEUDO_REALM), eq(user), eq("_action"), same(message)); + verify(auditTrail).authenticationSuccess(anyString(), eq("realm"), eq(user), eq("_action"), same(message)); verifyNoMoreInteractions(auditTrail); } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java index fd84afea365be..f35dd90afd3ad 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/RBACEngineTests.java @@ -103,6 +103,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()) @@ -126,9 +127,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)); @@ -181,6 +183,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); @@ -199,6 +202,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, @@ -210,6 +214,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); } @@ -221,6 +266,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); @@ -233,6 +279,7 @@ public void testSameUserPermissionDoesNotAllowChangePasswordForLookedUpByOtherRe verify(authentication).getLookedUpBy(); verify(authentication, times(2)).getUser(); verify(lookedUpBy).getType(); + verify(authentication).getAuthenticationType(); verifyNoMoreInteractions(authentication, lookedUpBy, authenticatedBy); } From cc71571c8b53a53f6a07ec04a254529e98d00845 Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Thu, 12 Dec 2019 13:45:22 +0200 Subject: [PATCH 4/5] revert test change --- .../client/documentation/SecurityDocumentationIT.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java index 74362ab71541d..ec2043d3da296 100644 --- a/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java +++ b/client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java @@ -2291,8 +2291,8 @@ public void testDelegatePkiAuthentication() throws Exception { assertThat(user.getUsername(), is("Elasticsearch Test Client")); RealmInfo authnRealm = resp.getAuthenticationRealm(); assertThat(authnRealm, is(notNullValue())); - assertThat(authnRealm.getName(), is("_es_token_service")); - assertThat(authnRealm.getType(), is("_es_token_service")); + assertThat(authnRealm.getName(), is("pki1")); + assertThat(authnRealm.getType(), is("pki")); } { @@ -2334,8 +2334,8 @@ public void onFailure(Exception e) { assertThat(user.getUsername(), is("Elasticsearch Test Client")); RealmInfo authnRealm = resp.getAuthenticationRealm(); assertThat(authnRealm, is(notNullValue())); - assertThat(authnRealm.getName(), is("_es_token_service")); - assertThat(authnRealm.getType(), is("_es_token_service")); + assertThat(authnRealm.getName(), is("pki1")); + assertThat(authnRealm.getType(), is("pki")); } } From 10564d45ae9cc91df65ea693f3eeefac2d2af9ba Mon Sep 17 00:00:00 2001 From: Ioannis Kakavas Date: Wed, 8 Jan 2020 09:46:11 +0200 Subject: [PATCH 5/5] whitelist>blacklist --- .../org/elasticsearch/xpack/security/authz/RBACEngine.java | 5 ++--- 1 file changed, 2 insertions(+), 3 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 index ab5d484d689d6..b61716462637e 100644 --- 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 @@ -528,9 +528,8 @@ private static boolean checkChangePasswordAction(Authentication authentication) // 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.TOKEN) == false - && authType.equals(Authentication.AuthenticationType.API_KEY) == false ) - && (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType)); + return (authType.equals(Authentication.AuthenticationType.REALM) + && (ReservedRealm.TYPE.equals(realmType) || NativeRealmSettings.TYPE.equals(realmType))); } static class RBACAuthorizationInfo implements AuthorizationInfo {