Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
4 changes: 2 additions & 2 deletions docs/reference/ingest/processors/set-security-user.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@ Sets user-related details (such as `username`, `roles`, `email`, `full_name`,
authenticated user to the current document by pre-processing the ingest.
The `api_key` property is only applicable when the authentication is
through API key. It is an object containing two fields, `id` and `name`.
The `realm` property is also an object with two fields, `name` and `type`.
When using API key authentication, the `realm` property refers to the realm
from which the API key is created. It is also an object with two fields,
`name` and `type`.
from which the API key is created.

IMPORTANT: Requires an authenticated user for the index request.

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,10 @@ public RealmRef getLookedUpBy() {
return lookedUpBy;
}

public RealmRef getNominalRealm() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public RealmRef getNominalRealm() {
public RealmRef getRealm() {

Copy link
Contributor

@tvernum tvernum Feb 5, 2020

Choose a reason for hiding this comment

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

I think getRealm is a bit too general - I worry about someone just deciding to call getRealm() without thinking about the fact that there's 2 possible realms that might be in play and they need to be intentional about which one to use.

Maybe getSourceRealm()?

The better long term fix would actually be to make getLookedUpBy() always return a realm (that is, is uses this implementation) and then have a isUserLookup() if you need to know whether there was a specific (separate) lookup realm.

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 agree getRealm is too general. I am OK with getSourceRealm and will go with it.

Is User#isRunAs functionally equivelent to the isUserLookup method you propose?

Copy link
Contributor

@tvernum tvernum Feb 10, 2020

Choose a reason for hiding this comment

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

Logically equivalent yes, but it's implemented by looking at different fields, so I'd rather have both implementations so that the one in Authentication is specifically about whether lookupRealm is populated.

return lookedUpBy == null ? authenticatedBy : lookedUpBy;
}

public Version getVersion() {
return version;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
*
* * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* * or more contributor license agreements. Licensed under the Elastic License;
* * you may not use this file except in compliance with the Elastic License.
*
*/

package org.elasticsearch.xpack.core.security.authc;

import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.core.security.user.User;

public class AuthenticationTests extends ESTestCase {

public void testWillGetLookupByWhenItExists() {
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 authentication = new Authentication(
new User("user"), authenticatedBy, lookedUpBy);

assertEquals(lookedUpBy, authentication.getNominalRealm());
}

public void testWillGetAuthenticateByWhenLookupIsNull() {
final Authentication.RealmRef authenticatedBy = new Authentication.RealmRef("auth_by", "auth_by_type", "node");
final Authentication authentication = new Authentication(
new User("user"), authenticatedBy, null);

assertEquals(authenticatedBy, authentication.getNominalRealm());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -273,10 +273,8 @@ XContentBuilder newDocument(SecureString apiKey, String name, Authentication aut
.startObject("creator")
.field("principal", authentication.getUser().principal())
.field("metadata", authentication.getUser().metadata())
.field("realm", authentication.getLookedUpBy() == null ?
authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName())
.field("realm_type", authentication.getLookedUpBy() == null ?
authentication.getAuthenticatedBy().getType() : authentication.getLookedUpBy().getType())
.field("realm", authentication.getNominalRealm().getName())
.field("realm_type", authentication.getNominalRealm().getType())
.endObject()
.endObject();

Expand Down Expand Up @@ -886,7 +884,7 @@ 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.getNominalRealm().getName();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to really consider whether this is the change we want.
It's a fairly fundamental change to the ApiKey service, and it affects how the manage_own_api_key privilege would work.
It might be right, I'd need to think about it, but if it is, then we might consider it a breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is my understanding:

In generaly, I feel the changed behaviour is correct, though you are right about it is a breaking change.

The above code is only used in TransportInvalidateApiKeyAction and TransportGetApiKeyAction. The else part will be called when the authentication is not of ApiKey auth type. Since ApiKey auth cannot perform runAs, this means theelse part will only makes a difference when userA runs as userB. In this case, the existing code will return userA realm, while the changes will return userB realm.

I can think of two reasons to support the changed behaviour:

  • When apikey is created, it records the runas user (if there is one) and runas/lookedUpBy realm. It feels correct that the same runas/lookedUpBy info should be checked when retrieve/invalidate the apikey.
  • When you run as someone else, all permissions and stuff should be checked/recorded as someone else.

I don't quite understand how manage_own_api_key work, the document says

All security-related operations on Elasticsearch API keys that are 
owned by the current authenticated user. The operations include 
creating new API keys, retrieving information about API keys, 
and invalidating API keys.

Does this mean you will not be able to create an API key for yourself unless you have this priviledge? If this is the case, I don't see how this is related to the change. Could you please elaborate on it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tim saved the day, I completely missed this 😞 .

@yang I think what you're describing is mostly correct.

Does this mean you will not be able to create an API key for yourself unless you have this priviledge?

Yes

If this is the case, I don't see how this is related to the change. Could you please elaborate on it?

Let's say you run-as and create an API key. The user that you run as only has the manage_own_api_key privilege. The creation would work. But then invalidation (and get), also using the run-as, won't work because the API Key Service would search the APi Key as incorrectly belonging to a user in the realm that authenticated the caller, whereas the API Key is created under the run-as user. (Note, I haven't actually tested this scenario).

In the interest of this PR, I would revert to the previous behavior in the TransportInvalidateApiKeyAction and TransportGetApiKeyAction , and, assuming you agree this is a bug, you should open a bug issue with your finding.

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 tried to simulate the scenario you described with two users foo and bar, where foo is a superuser and bar only has manage_own_api_key for cluster and read for all indices. ES version 7.5.2

It behaves pretty much as you described when requests are made with owner=true. Given foo creates an ApiKey while runas bar, the request
curl -u foo:password -H 'es-security-runas-user: bar' 'localhost:9200/_security/api_key?id=keyId&owner=true' returns empty results.

While I do believe the correct behaviour of above request should be returning the api key, I also think it is better to have a separate PR for this as it is not related to the current issue.

PS: I also found if bar has only manage_own_api_key priviledge, it is not possible to get or delete the api key. I got error says action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [bar]. Once I add manage_api_key to bar, everything works.

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 reverted the logic and created #51975 to track it. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you Yang! But currently I don't think we want to set the authenticating realm in the case of the set security user processor, see my other comment: https://github.com/elastic/elasticsearch/pull/51305/files?file-filters%5B%5D=.java#r375923902

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. Please see my other comment.

}
}

Expand All @@ -901,7 +899,7 @@ 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.getAuthenticatedBy().getType();
return authentication.getNominalRealm().getType();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,31 +87,39 @@ public IngestDocument execute(IngestDocument ingestDocument) throws Exception {
}
break;
case API_KEY:
final HashMap<String, Object> apiKey = new HashMap<>();
final String apiKey = "api_key";
final Object existingApiKeyField = userObject.get(apiKey);
@SuppressWarnings("unchecked")
final Map<String, Object> apiKeyField =
existingApiKeyField instanceof Map ? (Map<String, Object>) existingApiKeyField : new HashMap<>();
Object apiKeyName = authentication.getMetadata().get(ApiKeyService.API_KEY_NAME_KEY);
if (apiKeyName != null) {
apiKey.put("name", apiKeyName);
apiKeyField.put("name", apiKeyName);
}
Object apiKeyId = authentication.getMetadata().get(ApiKeyService.API_KEY_ID_KEY);
if (apiKeyId != null) {
apiKey.put("id", apiKeyId);
apiKeyField.put("id", apiKeyId);
}
if (false == apiKey.isEmpty()) {
userObject.put("api_key", apiKey);
if (false == apiKeyField.isEmpty()) {
userObject.put(apiKey, apiKeyField);
}
break;
case REALM:
final String realmKey = "realm";
final Object existingRealmField = userObject.get(realmKey);
@SuppressWarnings("unchecked")
final Map<String, Object> realmField =
existingRealmField instanceof Map ? (Map<String, Object>) existingRealmField : new HashMap<>();
final Object realmName = ApiKeyService.getCreatorRealmName(authentication);
Copy link
Contributor

@albertzaharovits albertzaharovits Feb 6, 2020

Choose a reason for hiding this comment

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

In case you authenticate without an API Key, this gets you the authentication realm, although it should get the lookup realm, otherwise the set security user processor won't work as expected for run-as users.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a good point. No need to let the existing questionable logic propagate to here. I'll implement the logic locally and this also helps get rid of the other newly added getCreatorRealmType method. 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.

Updated Thanks again for catching this.

final Object realmType = ApiKeyService.getCreatorRealmType(authentication);
final HashMap<String, Object> realm = new HashMap<>();
if (realmName != null) {
realm.put("name", realmName);
realmField.put("name", realmName);
}
final Object realmType = ApiKeyService.getCreatorRealmType(authentication);
if (realmType != null) {
realm.put("type", realmType);
realmField.put("type", realmType);
}
if (false == realm.isEmpty()) {
userObject.put("realm", realm);
if (false == realmField.isEmpty()) {
userObject.put(realmKey, realmField);
}
break;
case AUTHENTICATION_TYPE:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -566,6 +566,22 @@ public void testApiKeyCacheDisabled() {
assertNull(cachedApiKeyHashResult);
}

public void testWillGetLookupRealmNameWhenItExists() {
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 authentication = new Authentication(
new User("user"), authenticatedBy, lookedUpBy);
assertEquals("lookup_by", ApiKeyService.getCreatorRealmName(authentication));
}

public void testWillGetLookupRealmTypeWhenItExists() {
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 authentication = new Authentication(
new User("user"), authenticatedBy, lookedUpBy);
assertEquals("lookup_by_type", ApiKeyService.getCreatorRealmType(authentication));
}

private ApiKeyService createApiKeyService(Settings baseSettings) {
final Settings settings = Settings.builder()
.put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,4 +209,31 @@ public void testApiKeyPopulation() throws Exception {
assertThat(result.get("authentication_type"), equalTo("API_KEY"));
}

public void testWillNotOverwriteExistingApiKeyAndRealm() throws Exception {
User user = new User(null, null, null);
Authentication.RealmRef realmRef = new Authentication.RealmRef(
ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, "_node_name");
ThreadContext threadContext = new ThreadContext(Settings.EMPTY);

threadContext.putTransient(AuthenticationField.AUTHENTICATION_KEY, new Authentication(user, realmRef, null, Version.CURRENT,
Authentication.AuthenticationType.API_KEY,
Map.of(
ApiKeyService.API_KEY_ID_KEY, "api_key_id",
ApiKeyService.API_KEY_NAME_KEY, "api_key_name",
ApiKeyService.API_KEY_CREATOR_REALM_NAME, "creator_realm_name",
ApiKeyService.API_KEY_CREATOR_REALM_TYPE, "creator_realm_type"
)));

IngestDocument ingestDocument = new IngestDocument(IngestDocument.deepCopyMap(Map.of(
"_field", Map.of("api_key", Map.of("version", 42), "realm", Map.of("id", 7))
)), new HashMap<>());
SetSecurityUserProcessor processor = new SetSecurityUserProcessor("_tag", threadContext, "_field", EnumSet.allOf(Property.class));
processor.execute(ingestDocument);

Map<String, Object> result = ingestDocument.getFieldValue("_field", Map.class);
assertThat(result.size(), equalTo(3));
assertThat(((Map) result.get("api_key")).get("version"), equalTo(42));
assertThat(((Map) result.get("realm")).get("id"), equalTo(7));
}

}