Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -78,22 +78,22 @@ private boolean checkIfUserIsOwnerOfApiKeys(Authentication authentication, Strin
* TODO bizybot we need to think on how we can propagate appropriate error message to the end user when username, realm name
* is missing. This is similar to the problem of propagating right error messages in case of access denied.
*/
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
// API key cannot own any other API key so deny access
return false;
} else if (ownedByAuthenticatedUser) {
return true;
} else if (Strings.hasText(username) && Strings.hasText(realmName)) {
final String authenticatedUserPrincipal = authentication.getUser().principal();
final String authenticatedUserRealm = authentication.getAuthenticatedBy().getName();
final String authenticatedUserPrincipal = authentication.getUser().authenticatedUser().principal();
final String authenticatedUserRealm = authentication.getSourceRealm().getName();
return username.equals(authenticatedUserPrincipal) && realmName.equals(authenticatedUserRealm);
}
}
return false;
}

private boolean isCurrentAuthenticationUsingSameApiKeyIdFromRequest(Authentication authentication, String apiKeyId) {
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
if (authentication.getSourceRealm().getType().equals(API_KEY_REALM_TYPE)) {
// API key id from authentication must match the id from request
final String authenticatedApiKeyId = (String) authentication.getMetadata().get(API_KEY_ID_KEY);
if (Strings.hasText(apiKeyId)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -96,15 +96,47 @@ public void testAuthenticationWithUserDeniesAccessToApiKeyActionsWhenItIsNotOwne
assertFalse(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
}

public void testGetAndInvalidateApiKeyWillRespectRunAsUser() {
final ClusterPermission clusterPermission =
ManageOwnApiKeyClusterPrivilege.INSTANCE.buildPermission(ClusterPermission.builder()).build();

final Authentication authentication = createMockRunAsAuthentication(
"user_a", "realm_a", "realm_a_type",
"user_b", "realm_b", "realm_b_type");

final TransportRequest getApiKeyRequest = GetApiKeyRequest.usingRealmAndUserName("realm_b", "user_b");
final TransportRequest invalidateApiKeyRequest = InvalidateApiKeyRequest.usingRealmAndUserName("realm_b", "user_b");

assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/get", getApiKeyRequest, authentication));
assertTrue(clusterPermission.check("cluster:admin/xpack/security/api_key/invalidate", invalidateApiKeyRequest, authentication));
}

private Authentication createMockAuthentication(String username, String realmName, String realmType, Map<String, Object> metadata) {
final User user = new User(username);
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.getSourceRealm()).thenReturn(authenticatedBy);
when(authenticatedBy.getName()).thenReturn(realmName);
when(authenticatedBy.getType()).thenReturn(realmType);
when(authentication.getMetadata()).thenReturn(metadata);
return authentication;
}

private Authentication createMockRunAsAuthentication(String username, String realmName, String realmType,
String runAsUsername, String runAsRealmName, String runAsRealmType) {
final Authentication.RealmRef authenticatedBy = mock(Authentication.RealmRef.class);
when(authenticatedBy.getName()).thenReturn(realmName);
when(authenticatedBy.getType()).thenReturn(realmType);
final Authentication.RealmRef lookedUpBy = mock(Authentication.RealmRef.class);
when(lookedUpBy.getName()).thenReturn(runAsRealmName);
when(lookedUpBy.getType()).thenReturn(runAsRealmType);
final User user = new User(username, new String[0], new User(runAsUsername));
final Authentication authentication = mock(Authentication.class);
when(authentication.getUser()).thenReturn(user);
when(authentication.getAuthenticatedBy()).thenReturn(authenticatedBy);
when(authentication.getSourceRealm()).thenReturn(lookedUpBy);
when(authentication.getMetadata()).thenReturn(Map.of());
return authentication;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -884,7 +884,22 @@ public static String getCreatorRealmName(final Authentication authentication) {
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_NAME);
} else {
return authentication.getAuthenticatedBy().getName();
return authentication.getSourceRealm().getName();
}
}

/**
* Returns realm type for the authenticated user.
* If the user is authenticated by realm type {@value API_KEY_REALM_TYPE}
* then it will return the realm name of user who created this API key.
* @param authentication {@link Authentication}
* @return realm type
*/
public static String getCreatorRealmType(final Authentication authentication) {
if (authentication.getAuthenticatedBy().getType().equals(API_KEY_REALM_TYPE)) {
return (String) authentication.getMetadata().get(API_KEY_CREATOR_REALM_TYPE);
} else {
return authentication.getSourceRealm().getType();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,17 +114,11 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
final Map<String, Object> realmField =
existingRealmField instanceof Map ? (Map<String, Object>) existingRealmField : new HashMap<>();

final Object realmName, realmType;
if (Authentication.AuthenticationType.API_KEY == authentication.getAuthenticationType()) {
realmName = authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM_NAME);
realmType = authentication.getMetadata().get(ApiKeyService.API_KEY_CREATOR_REALM_TYPE);
} else {
realmName = authentication.getSourceRealm().getName();
realmType = authentication.getSourceRealm().getType();
}
final Object realmName = ApiKeyService.getCreatorRealmName(authentication);
if (realmName != null) {
realmField.put("name", realmName);
}
final Object realmType = ApiKeyService.getCreatorRealmType(authentication);
if (realmType != null) {
realmField.put("type", realmType);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,9 @@ public String configRoles() {
"manage_api_key_role:\n" +
" cluster: [\"manage_api_key\"]\n" +
"manage_own_api_key_role:\n" +
" cluster: [\"manage_own_api_key\"]\n";
" cluster: [\"manage_own_api_key\"]\n" +
"run_as_role:\n" +
" run_as: [\"user_with_manage_own_api_key_role\"]\n";
}

@Override
Expand All @@ -114,15 +116,17 @@ public String configUsers() {
return super.configUsers() +
"user_with_no_api_key_role:" + usersPasswdHashed + "\n" +
"user_with_manage_api_key_role:" + usersPasswdHashed + "\n" +
"user_with_manage_own_api_key_role:" + usersPasswdHashed + "\n";
"user_with_manage_own_api_key_role:" + usersPasswdHashed + "\n" +
"user_with_run_as_role:" + usersPasswdHashed + "\n";
}

@Override
public String configUsersRoles() {
return super.configUsersRoles() +
"no_api_key_role:user_with_no_api_key_role\n" +
"manage_api_key_role:user_with_manage_api_key_role\n" +
"manage_own_api_key_role:user_with_manage_own_api_key_role\n";
"manage_own_api_key_role:user_with_manage_own_api_key_role\n" +
"run_as_role:user_with_run_as_role\n";
}

private void awaitApiKeysRemoverCompletion() throws Exception {
Expand Down Expand Up @@ -540,6 +544,25 @@ public void testGetApiKeysOwnedByCurrentAuthenticatedUser() throws InterruptedEx
response, userWithManageApiKeyRoleApiKeys.stream().map(o -> o.getId()).collect(Collectors.toSet()), null);
}

public void testGetApiKeysOwnedByRunAsUser() throws ExecutionException, InterruptedException {
int noOfSuperuserApiKeys = randomIntBetween(3, 5);
int noOfApiKeysForUserWithManageApiKeyRole = randomIntBetween(3, 5);
createApiKeys(noOfSuperuserApiKeys, null);
final String userWithManageOwnApiKeyRole = "user_with_manage_own_api_key_role";
final String userWithRunAsRole = "user_with_run_as_role";
List<CreateApiKeyResponse> userWithManageOwnApiKeyRoleApiKeys = createApiKeys(userWithManageOwnApiKeyRole,
userWithRunAsRole, noOfApiKeysForUserWithManageApiKeyRole, null, "monitor");
final Client client = client().filterWithHeader(Map.of("Authorization", UsernamePasswordToken
.basicAuthHeaderValue(userWithRunAsRole, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING),
"es-security-runas-user", userWithManageOwnApiKeyRole));

PlainActionFuture<GetApiKeyResponse> listener = new PlainActionFuture<>();
client.execute(GetApiKeyAction.INSTANCE, GetApiKeyRequest.forOwnedApiKeys(), listener);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a test when the owned flag is not set, but the username and realm are.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was meant do test without the "owner" flag since the updated logic won't get executed with its presense. Somehow I ended up forgetting it. Will update. Thanks

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add tests for get and invalidate API key without "owner=true"

GetApiKeyResponse response = listener.get();
verifyGetResponse(userWithManageOwnApiKeyRole, noOfApiKeysForUserWithManageApiKeyRole, userWithManageOwnApiKeyRoleApiKeys,
response, userWithManageOwnApiKeyRoleApiKeys.stream().map(o -> o.getId()).collect(Collectors.toSet()), null);
}

public void testGetAllApiKeys() throws InterruptedException, ExecutionException {
int noOfSuperuserApiKeys = randomIntBetween(3, 5);
int noOfApiKeysForUserWithManageApiKeyRole = randomIntBetween(3, 5);
Expand Down Expand Up @@ -600,6 +623,25 @@ public void testInvalidateApiKeysOwnedByCurrentAuthenticatedUser() throws Interr
verifyInvalidateResponse(noOfApiKeysForUserWithManageApiKeyRole, userWithManageApiKeyRoleApiKeys, invalidateResponse);
}

public void testInvalidateApiKeysOwnedByRunAsUser() throws InterruptedException, ExecutionException {
int noOfSuperuserApiKeys = randomIntBetween(3, 5);
int noOfApiKeysForUserWithManageApiKeyRole = randomIntBetween(3, 5);
createApiKeys(noOfSuperuserApiKeys, null);
final String userWithManageOwnApiKeyRole = "user_with_manage_own_api_key_role";
final String userWithRunAsRole = "user_with_run_as_role";
List<CreateApiKeyResponse> userWithManageApiKeyRoleApiKeys = createApiKeys(userWithManageOwnApiKeyRole,
userWithRunAsRole, noOfApiKeysForUserWithManageApiKeyRole, null, "monitor");
final Client client = client().filterWithHeader(Map.of("Authorization", UsernamePasswordToken
.basicAuthHeaderValue(userWithRunAsRole, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING),
"es-security-runas-user", userWithManageOwnApiKeyRole));

PlainActionFuture<InvalidateApiKeyResponse> listener = new PlainActionFuture<>();
client.execute(InvalidateApiKeyAction.INSTANCE, InvalidateApiKeyRequest.forOwnedApiKeys(), listener);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above, we should also test without the owner flag toggled but with the concrete username and realm set.

I would also add a negative test, but this is usually better to do in unit tests, but I sometimes sneak in a negative test in integ tests as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added negative tests for calling get and invalidate API key with mismatching username and/or realm name.

InvalidateApiKeyResponse invalidateResponse = listener.get();

verifyInvalidateResponse(noOfApiKeysForUserWithManageApiKeyRole, userWithManageApiKeyRoleApiKeys, invalidateResponse);
}

public void testApiKeyAuthorizationApiKeyMustBeAbleToRetrieveItsOwnInformationButNotAnyOtherKeysCreatedBySameOwner()
throws InterruptedException, ExecutionException {
List<CreateApiKeyResponse> responses = createApiKeys(SecuritySettingsSource.TEST_SUPERUSER,2, null, (String[]) null);
Expand Down Expand Up @@ -699,14 +741,28 @@ private List<CreateApiKeyResponse> createApiKeys(int noOfApiKeys, TimeValue expi
}

private List<CreateApiKeyResponse> createApiKeys(String user, int noOfApiKeys, TimeValue expiration, String... clusterPrivileges) {
final Map<String, String> headers = Collections.singletonMap(
"Authorization", UsernamePasswordToken.basicAuthHeaderValue(user, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING));
return createApiKeys(headers, noOfApiKeys, expiration, clusterPrivileges);
}

private List<CreateApiKeyResponse> createApiKeys(String user, String runAsUser,
int noOfApiKeys, TimeValue expiration, String... clusterPrivileges) {
final Map<String, String> headers = Map.of("Authorization",
UsernamePasswordToken.basicAuthHeaderValue(runAsUser, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING),
"es-security-runas-user", user);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem backwards to me. You authenticate as runAsUser but run-as user.
I assume the parameters are just strangely named, but it probably makes sense to have authenticatingUser and owningUser or something like that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. It does read awkward. Updated.

return createApiKeys(headers, noOfApiKeys, expiration, clusterPrivileges);
}

private List<CreateApiKeyResponse> createApiKeys(Map<String, String> headers,
int noOfApiKeys, TimeValue expiration, String... clusterPrivileges) {
List<CreateApiKeyResponse> responses = new ArrayList<>();
for (int i = 0; i < noOfApiKeys; i++) {
final RoleDescriptor descriptor = new RoleDescriptor("role", clusterPrivileges, null, null);
Client client = client().filterWithHeader(Collections.singletonMap("Authorization", UsernamePasswordToken
.basicAuthHeaderValue(user, SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)));
Client client = client().filterWithHeader(headers);
final CreateApiKeyResponse response = new CreateApiKeyRequestBuilder(client)
.setName("test-key-" + randomAlphaOfLengthBetween(5, 9) + i).setExpiration(expiration)
.setRoleDescriptors(Collections.singletonList(descriptor)).get();
.setName("test-key-" + randomAlphaOfLengthBetween(5, 9) + i).setExpiration(expiration)
.setRoleDescriptors(Collections.singletonList(descriptor)).get();
assertNotNull(response.getId());
assertNotNull(response.getKey());
responses.add(response);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,12 +566,20 @@ public void testApiKeyCacheDisabled() {
assertNull(cachedApiKeyHashResult);
}

public void testWillAlwaysGetAuthenticationRealmName() {
public void testWillGetLookedUpByRealmNameIfExists() {
final Authentication.RealmRef authenticatedBy = new Authentication.RealmRef("auth_by", "auth_by_type", "node");
final Authentication.RealmRef lookedUpBy = new Authentication.RealmRef("lookup_by", "lookup_by_type", "node");
final Authentication.RealmRef lookedUpBy = new Authentication.RealmRef("looked_up_by", "looked_up_by_type", "node");
final Authentication authentication = new Authentication(
new User("user"), authenticatedBy, lookedUpBy);
assertEquals("auth_by", ApiKeyService.getCreatorRealmName(authentication));
assertEquals("looked_up_by", ApiKeyService.getCreatorRealmName(authentication));
}

public void testWillGetLookedUpByRealmTypeIfExists() {
final Authentication.RealmRef authenticatedBy = new Authentication.RealmRef("auth_by", "auth_by_type", "node");
final Authentication.RealmRef lookedUpBy = new Authentication.RealmRef("looked_up_by", "looked_up_by_type", "node");
final Authentication authentication = new Authentication(
new User("user"), authenticatedBy, lookedUpBy);
assertEquals("looked_up_by_type", ApiKeyService.getCreatorRealmType(authentication));
}

private ApiKeyService createApiKeyService(Settings baseSettings) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ public void testProcessorWithEmptyUserData() throws Exception {
User user = Mockito.mock(User.class);
Authentication authentication = Mockito.mock(Authentication.class);
Mockito.when(authentication.getUser()).thenReturn(user);
Mockito.when(authentication.getSourceRealm()).thenReturn(new Authentication.RealmRef("_name", "_type", "_node_name"));
final Authentication.RealmRef authByRealm = new Authentication.RealmRef("_name", "_type", "_node_name");
Mockito.when(authentication.getSourceRealm()).thenReturn(authByRealm);
Mockito.when(authentication.getAuthenticatedBy()).thenReturn(authByRealm);
Mockito.when(authentication.getAuthenticationType()).thenReturn(AuthenticationType.REALM);
Mockito.when(authentication.encode()).thenReturn(randomAlphaOfLength(24)); // don't care as long as it's not null
new AuthenticationContextSerializer().writeToContext(authentication, threadContext);
Expand Down