Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve role cache efficiency for API key roles #58156

Merged
merged 39 commits into from
Jul 13, 2020
Merged

Improve role cache efficiency for API key roles #58156

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
390c499
Only partially deserialise api key doc during authentication
ywangd Jun 16, 2020
c353aef
Improve role cache efficiency and add bwc for API key auth metadata
ywangd Jun 16, 2020
46cf4fd
Fix tests
ywangd Jun 16, 2020
1319909
Update bwc version
ywangd Jun 16, 2020
b1688f9
checkstyle
ywangd Jun 16, 2020
0f9532b
Fix more tests
ywangd Jun 16, 2020
f1e1285
Add bwc for outgoing requests
ywangd Jun 16, 2020
9832f50
Add more tests for bwc and role cache reuse
ywangd Jun 17, 2020
d2560d8
Add mixed cluster bwc test
ywangd Jun 17, 2020
ca698d7
Apply suggestions from code review
ywangd Jul 1, 2020
cf785c9
fix variable name
ywangd Jul 1, 2020
abcd485
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 1, 2020
d62d8a2
Avoid using deprecated API
ywangd Jul 2, 2020
8148050
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd Jul 6, 2020
1440bfe
Address feedback
ywangd Jul 6, 2020
cfca9cc
Fix tests
ywangd Jul 6, 2020
50f1706
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 6, 2020
8fb443e
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
ywangd Jul 6, 2020
797c54b
Update method name change
ywangd Jul 6, 2020
552e7b3
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 6, 2020
3cf31f3
Address feedback
ywangd Jul 6, 2020
1300c4d
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 6, 2020
3531a52
Fix tests
ywangd Jul 7, 2020
9e7996d
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 7, 2020
27be381
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 7, 2020
a032325
A challenging merge
ywangd Jul 7, 2020
9395a83
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 8, 2020
0f08a3c
Address feedback
ywangd Jul 8, 2020
5b75e1d
Simplify creator parsing of ApiKeyDoc
ywangd Jul 12, 2020
bedb46f
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 12, 2020
52698f4
Fix merge
ywangd Jul 12, 2020
8543c18
Address feedback
ywangd Jul 13, 2020
c1fba5a
Address more feedback
ywangd Jul 13, 2020
0c991a6
Merge remote-tracking branch 'origin/master' into es-53939-api-key-ro…
ywangd Jul 13, 2020
0ac2a84
Address feedback
ywangd Jul 13, 2020
70307e7
checkstyle
ywangd Jul 13, 2020
8b1153c
Use new version constants where it is applicable
ywangd Jul 13, 2020
121134b
more checkstyle
ywangd Jul 13, 2020
2b20b48
Add one test for ApiKeyDoc.fromXContent
ywangd Jul 13, 2020
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
6 changes: 6 additions & 0 deletions libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ public void declareLong(BiConsumer<Value, Long> consumer, ParseField field) {
declareField(consumer, p -> p.longValue(), field, ValueType.LONG);
}

public void declareLongOrNull(BiConsumer<Value, Long> consumer, long nullValue, ParseField field) {
// Using a method reference here angers some compilers
declareField(consumer, p -> p.currentToken() == XContentParser.Token.VALUE_NULL ? nullValue : p.longValue(),
field, ValueType.LONG_OR_NULL);
}

public void declareInt(BiConsumer<Value, Integer> consumer, ParseField field) {
// Using a method reference here angers some compilers
declareField(consumer, p -> p.intValue(), field, ValueType.INT);
Expand Down
26 changes: 25 additions & 1 deletion x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/SecurityContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.Version;
import org.elasticsearch.common.Nullable;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.util.concurrent.ThreadContext.StoredContext;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.node.Node;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.Authentication.AuthenticationType;
Expand All @@ -23,10 +25,15 @@
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.function.Consumer;
import java.util.function.Function;

import static org.elasticsearch.xpack.core.security.authc.AuthenticationField.API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY;
import static org.elasticsearch.xpack.core.security.authc.AuthenticationField.API_KEY_ROLE_DESCRIPTORS_KEY;

/**
* A lightweight utility that can find the current user and authentication information for the local thread.
*/
Expand Down Expand Up @@ -149,8 +156,25 @@ public void executeAfterRewritingAuthentication(Consumer<StoredContext> consumer
final Authentication authentication = getAuthentication();
try (ThreadContext.StoredContext ignore = threadContext.stashContext()) {
setAuthentication(new Authentication(authentication.getUser(), authentication.getAuthenticatedBy(),
authentication.getLookedUpBy(), version, authentication.getAuthenticationType(), authentication.getMetadata()));
authentication.getLookedUpBy(), version, authentication.getAuthenticationType(),
rewriteMetadataIfNecessary(version, authentication)));
consumer.accept(original);
}
}

private Map<String, Object> rewriteMetadataIfNecessary(Version version, Authentication authentication) {
ywangd marked this conversation as resolved.
Show resolved Hide resolved
Map<String, Object> metadata = authentication.getMetadata();
if (authentication.getAuthenticationType() == AuthenticationType.API_KEY
&& authentication.getVersion().onOrAfter(Version.V_7_9_0)
&& version.before(Version.V_7_9_0)) {
metadata = new HashMap<>(metadata);
metadata.put(
API_KEY_ROLE_DESCRIPTORS_KEY,
XContentHelper.convertToMap((BytesReference) metadata.get(API_KEY_ROLE_DESCRIPTORS_KEY), false).v2());
ywangd marked this conversation as resolved.
Show resolved Hide resolved
metadata.put(
API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY,
XContentHelper.convertToMap((BytesReference) metadata.get(API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY), false).v2());
}
return metadata;
}
}
2 changes: 2 additions & 0 deletions ...n/core/src/main/java/org/elasticsearch/xpack/core/security/authc/AuthenticationField.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
public final class AuthenticationField {

public static final String AUTHENTICATION_KEY = "_xpack_security_authentication";
public static final String API_KEY_ROLE_DESCRIPTORS_KEY = "_security_api_key_role_descriptors";
public static final String API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY = "_security_api_key_limited_by_role_descriptors";

private AuthenticationField() {}
}
171 changes: 136 additions & 35 deletions ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Open in desktop

Large diffs are not rendered by default.

66 changes: 52 additions & 14 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.Version;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.ContextPreservingActionListener;
import org.elasticsearch.common.Nullable;
Expand All @@ -18,6 +19,7 @@
import org.elasticsearch.common.cache.CacheBuilder;
import org.elasticsearch.common.collect.Tuple;
import org.elasticsearch.common.logging.DeprecationLogger;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.settings.Settings;
Expand All @@ -28,6 +30,7 @@
import org.elasticsearch.license.XPackLicenseState.Feature;
import org.elasticsearch.xpack.core.common.IteratingActionListener;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.DocumentSubsetBitsetCache;
Expand Down Expand Up @@ -221,20 +224,40 @@ public void getRoles(User user, Authentication authentication, ActionListener<Ro

final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
if (authType == Authentication.AuthenticationType.API_KEY) {
apiKeyService.getRoleForApiKey(authentication, ActionListener.wrap(apiKeyRoleDescriptors -> {
final List<RoleDescriptor> descriptors = apiKeyRoleDescriptors.getRoleDescriptors();
if (descriptors == null) {
roleActionListener.onFailure(new IllegalStateException("missing role descriptors"));
} else if (apiKeyRoleDescriptors.getLimitedByRoleDescriptors() == null) {
buildAndCacheRoleFromDescriptors(descriptors, apiKeyRoleDescriptors.getApiKeyId() + "_role_desc", roleActionListener);
} else {
buildAndCacheRoleFromDescriptors(descriptors, apiKeyRoleDescriptors.getApiKeyId() + "_role_desc",
ActionListener.wrap(role -> buildAndCacheRoleFromDescriptors(apiKeyRoleDescriptors.getLimitedByRoleDescriptors(),
apiKeyRoleDescriptors.getApiKeyId() + "_limited_role_desc", ActionListener.wrap(
limitedBy -> roleActionListener.onResponse(LimitedRole.createLimitedRole(role, limitedBy)),
roleActionListener::onFailure)), roleActionListener::onFailure));
}
}, roleActionListener::onFailure));
if (authentication.getVersion().onOrAfter(Version.V_7_9_0)) {
getOrBuildRoleForApiKey(authentication, false, ActionListener.wrap(
role -> {
if (role == Role.EMPTY) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When can this be true?

Copy link
Member Author

@ywangd ywangd Jul 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

When a list of role descriptor is empty, the role building method buildRoleFromDescriptors returns Role.EMPTY.

Also when an API key does not have its own role descriptors, its document will have {} for the role_descriptors field, which in turn will be converted to an empty list of RoleDescriptor.

When the two work together, you can get Role.EMPTY.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it! I have to check the code every single time to tell if the role_descriptors field or the limited_by_role_descriptors can be empty, the meanings are reversed in ApiKeyService#getRoleForApiKey.

Since limitedByRole can never be null I would be building that first, for a minor branching simplification:

         final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
         if (authType == Authentication.AuthenticationType.API_KEY) {
             if (authentication.getVersion().onOrAfter(Version.V_7_9_0)) {
-                buildAndCacheRoleForApiKey(authentication, false, ActionListener.wrap(
-                    role -> {
-                        if (role == Role.EMPTY) {
-                            buildAndCacheRoleForApiKey(authentication, true, roleActionListener);
-                        } else {
-                            buildAndCacheRoleForApiKey(authentication, true, ActionListener.wrap(
-                                limitedByRole -> roleActionListener.onResponse(
-                                    limitedByRole == Role.EMPTY ? role : LimitedRole.createLimitedRole(role, limitedByRole)),
-                                roleActionListener::onFailure
+                buildAndCacheRoleForApiKey(authentication, true, ActionListener.wrap(
+                        limitedByRole -> {
+                            buildAndCacheRoleForApiKey(authentication, false, ActionListener.wrap(
+                                    role -> roleActionListener.onResponse(
+                                            role == Role.EMPTY ? limitedByRole : LimitedRole.createLimitedRole(role, limitedByRole)),
+                                    roleActionListener::onFailure
                             ));
-                        }
-                    },
-                    roleActionListener::onFailure
+                        },
+                        roleActionListener::onFailure
                 ));
             } else {
                 apiKeyService.getRoleForApiKey(authentication, ActionListener.wrap(apiKeyRoleDescriptors -> {

Feel free to follow this suggestion or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

You may be right. But I am not entirely sure when it comes to derived API keys. A derived keys will have limited by role descriptors from parent API key's roles. Since we now zero out API key's roles during authentication, we may end up having an empty limited by role descriptors as well. I have not tested, but I feel it is probably safer to keep the code as is for now.

getOrBuildRoleForApiKey(authentication, true, roleActionListener);
} else {
getOrBuildRoleForApiKey(authentication, true, ActionListener.wrap(
limitedByRole -> roleActionListener.onResponse(
limitedByRole == Role.EMPTY ? role : LimitedRole.createLimitedRole(role, limitedByRole)),
roleActionListener::onFailure
));
}
},
roleActionListener::onFailure
));
} else {
apiKeyService.getRoleForApiKey(authentication, ActionListener.wrap(apiKeyRoleDescriptors -> {
final List<RoleDescriptor> descriptors = apiKeyRoleDescriptors.getRoleDescriptors();
if (descriptors == null) {
roleActionListener.onFailure(new IllegalStateException("missing role descriptors"));
} else if (apiKeyRoleDescriptors.getLimitedByRoleDescriptors() == null) {
buildAndCacheRoleFromDescriptors(descriptors,
apiKeyRoleDescriptors.getApiKeyId() + "_role_desc", roleActionListener);
} else {
buildAndCacheRoleFromDescriptors(descriptors, apiKeyRoleDescriptors.getApiKeyId() + "_role_desc",
ActionListener.wrap(
role -> buildAndCacheRoleFromDescriptors(apiKeyRoleDescriptors.getLimitedByRoleDescriptors(),
apiKeyRoleDescriptors.getApiKeyId() + "_limited_role_desc", ActionListener.wrap(
limitedBy -> roleActionListener.onResponse(LimitedRole.createLimitedRole(role, limitedBy)),
roleActionListener::onFailure)), roleActionListener::onFailure));
}
}, roleActionListener::onFailure));
}

} else {
Set<String> roleNames = new HashSet<>(Arrays.asList(user.roles()));
if (isAnonymousEnabled && anonymousUser.equals(user) == false) {
Expand Down Expand Up @@ -295,6 +318,21 @@ private void buildThenMaybeCacheRole(RoleKey roleKey, Collection<RoleDescriptor>
}, listener::onFailure));
}

private void getOrBuildRoleForApiKey(Authentication authentication, boolean limitedBy, ActionListener<Role> roleActionListener) {
final BytesReference roleDescriptorsBytes = apiKeyService.getRoleDescriptorsBytesForApiKey(authentication, limitedBy);
final String roleDescriptorsHash = new String(Hasher.SHA256.hash(new SecureString(roleDescriptorsBytes.toString().toCharArray())));
Copy link
Contributor

Choose a reason for hiding this comment

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

BytesReference.toString() is not guaranteed to be what you want here.
You want either utf8ToString or just use the bytes directly.

I'm not sure whether we really even need to hash these - we can, but it might not be necessary.
I think the options are either:

  1. Don't bother hashing, but that will mean using a separate cache for API key roles (because the cache key is different)
Suggested change
final String roleDescriptorsHash = new String(Hasher.SHA256.hash(new SecureString(roleDescriptorsBytes.toString().toCharArray())));
final BytesKey cacheKey = new BytesKey(BytesReference.toBytes(roleDescriptorsBytes));
  1. Hash using MessageDigest
Suggested change
final String roleDescriptorsHash = new String(Hasher.SHA256.hash(new SecureString(roleDescriptorsBytes.toString().toCharArray())));
MessageDigest digest = MessageDigests.sha256();
digest.update(BytesReference.toBytes(roleDescriptorsBytes));
final String roleDescriptorsHash = MessageDigests.toHexString(digest.digest());

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 really like BytesKey, unfortunately it is not compatible with the current cache and I don't want to add another cache and associated overhead just because of this incompatibility. So I'll prefer to your second suggestion, i.e. MessageDigest.sha256 where raw bytes are used.

Alternatively, we could potential change RoleKey to have either Set<byte[]> names and byte[] source, so it can accomodate both regular roles and API key roles. But I am not sure whether it really gives us much unless we have a real concern for sha256 collision.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm happy with the sha256 option. I also like BytesKey - it's simple and sensible, but I don't think it's the right fit here unless we change the cache to have aSet<?> as the key, or follow your idea of converting a Set<String> into a BytesKey

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool let's keep the sha256 option then. It works and is simpler.

final RoleKey roleKey = new RoleKey(Set.of(roleDescriptorsHash), limitedBy ? "limited_role_desc" : "role_desc");
ywangd marked this conversation as resolved.
Show resolved Hide resolved
final Role existing = roleCache.get(roleKey);
if (existing == null) {
final long invalidationCounter = numInvalidation.get();
final List<RoleDescriptor> roleDescriptors = apiKeyService.getRoleDescriptorsForApiKey(authentication, limitedBy);
buildThenMaybeCacheRole(roleKey, roleDescriptors, Collections.emptySet(),
true, invalidationCounter, roleActionListener);
} else {
roleActionListener.onResponse(existing);
}
}

public void getRoleDescriptors(Set<String> roleNames, ActionListener<Set<RoleDescriptor>> listener) {
roleDescriptors(roleNames, ActionListener.wrap(rolesRetrievalResult -> {
if (rolesRetrievalResult.isSuccess()) {
Expand Down
41 changes: 41 additions & 0 deletions .../plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityContextTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.security;

import org.elasticsearch.Version;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.util.concurrent.ThreadContext.StoredContext;
Expand All @@ -23,8 +24,12 @@
import java.io.EOFException;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.util.List;
import java.util.Map;
import java.util.concurrent.atomic.AtomicReference;

import static org.elasticsearch.xpack.core.security.authc.AuthenticationField.API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY;
import static org.elasticsearch.xpack.core.security.authc.AuthenticationField.API_KEY_ROLE_DESCRIPTORS_KEY;
import static org.hamcrest.Matchers.instanceOf;

public class SecurityContextTests extends ESTestCase {
Expand Down Expand Up @@ -130,4 +135,40 @@ public void testExecuteAfterRewritingAuthentication() throws IOException {
originalContext.restore();
assertEquals(original, securityContext.getAuthentication());
}

public void testExecuteAfterRewritingAuthenticationShouldRewriteApiKeyMetadataForBwc() throws IOException {
User user = new User("test", null, new User("authUser"));
RealmRef authBy = new RealmRef("_es_api_key", "_es_api_key", "node1");
final Map<String, Object> metadata = Map.of(
API_KEY_ROLE_DESCRIPTORS_KEY, new BytesArray("{\"a role\": {\"cluster\": [\"all\"]}}"),
API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY, new BytesArray("{\"limitedBy role\": {\"cluster\": [\"all\"]}}")
);
final Authentication original = new Authentication(user, authBy, authBy, Version.V_8_0_0,
AuthenticationType.API_KEY, metadata);
original.writeToContext(threadContext);

securityContext.executeAfterRewritingAuthentication(originalCtx -> {
Authentication authentication = securityContext.getAuthentication();
assertEquals(Map.of("a role", Map.of("cluster", List.of("all"))),
authentication.getMetadata().get(API_KEY_ROLE_DESCRIPTORS_KEY));
assertEquals(Map.of("limitedBy role", Map.of("cluster", List.of("all"))),
authentication.getMetadata().get(API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY));
}, Version.V_7_8_0);
}

public void testExecuteAfterRewritingAuthenticationShouldNotRewriteApiKeyMetadataForOldAuthenticationObject() throws IOException {
User user = new User("test", null, new User("authUser"));
RealmRef authBy = new RealmRef("_es_api_key", "_es_api_key", "node1");
final Map<String, Object> metadata = Map.of(
API_KEY_ROLE_DESCRIPTORS_KEY, Map.of("a role", Map.of("cluster", List.of("all"))),
API_KEY_LIMITED_ROLE_DESCRIPTORS_KEY, Map.of("limitedBy role", Map.of("cluster", List.of("all")))
);
final Authentication original = new Authentication(user, authBy, authBy, Version.V_7_8_0, AuthenticationType.API_KEY, metadata);
original.writeToContext(threadContext);

securityContext.executeAfterRewritingAuthentication(originalCtx -> {
Authentication authentication = securityContext.getAuthentication();
assertSame(metadata, authentication.getMetadata());
}, randomFrom(Version.V_8_0_0, Version.V_7_8_0));
}
}
Loading