Security: add create api key transport action by jaymode · Pull Request #34572 · elastic/elasticsearch

jaymode · 2018-10-17T20:30:47Z

In order to support api keys for access to elasticsearch, we need the
ability to generate these api keys. A transport action has been added
along with the request and response objects that allow for the
generation of api keys. The api keys require a name and optionally
allow a role to be specified which defines the amount of access the key
has. Additionally an expiration may also be provided.

This change does not include the restriction that the role needs to be
a subset of the user's permissions, which will be added seperately. As
it exists in this change, the api key is currently not usable which is
another aspect that will come later.

Relates #34383

In order to support api keys for access to elasticsearch, we need the ability to generate these api keys. A transport action has been added along with the request and response objects that allow for the generation of api keys. The api keys require a name and optionally allow a role to be specified which defines the amount of access the key has. Additionally an expiration may also be provided. This change does not include the restriction that the role needs to be a subset of the user's permissions, which will be added seperately. As it exists in this change, the api key is currently not usable which is another aspect that will come later. Relates elastic#34383

elasticmachine · 2018-10-17T20:30:49Z

Pinging @elastic/es-security

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java

...in/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyResponse.java

...e/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRespoonseTests.java

...urity/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/TokenService.java

...security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java

...in/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyResponse.java

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

...urity/src/main/java/org/elasticsearch/xpack/security/rest/action/RestCreateApiKeyAction.java

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

jaymode · 2018-10-31T17:22:28Z

@tvernum @jkakavas this is ready for review again

tvernum

There's 1 issue about still indexing the api key.
The other comments are suggestions that you can take or leave as you wish.

tvernum · 2018-11-01T09:18:14Z

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java

+            validationException = addValidationError("name is required", validationException);
+        } else if (name.length() > 256) {
+            validationException = addValidationError("name may not be more than 256 characters long", validationException);
+        }


Should we have any other restrictions on names?
I feel like starting or ending in whitespace is likely to cause confusion, and perhaps we should reserve names start with _ just in case we need them in the future?

Added those restrictions

...in/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyResponse.java

...urity/src/main/java/org/elasticsearch/xpack/security/action/TransportCreateApiKeyAction.java

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

tvernum · 2018-11-01T09:27:04Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

+                    .field("principal", authentication.getUser().principal())
+                    .field("metadata", authentication.getUser().metadata())
+                    .field("realm", authentication.getLookedUpBy() == null ?
+                        authentication.getAuthenticatedBy().getName() : authentication.getLookedUpBy().getName())


I guess it depends on how this gets used in the future, but we may need to think about how we deal with realm names, and their potential to change over time.
In particular, out-of-the-box we have "default_file" and "default_native", but once someone adds another realm (e.g. LDAP), they tend to not use "default_native" as the name of their native realm (because we don't tell them to).

The reason behind storing the realm is based on the invalidation api and being able to invalidate by realm. It really stinks that we don't have a way to enforce realm name consistency between nodes

...k/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

...gin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

jkakavas

LGTM, thanks for the iterations !

jkakavas · 2018-11-01T17:50:25Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java

+    private static final Logger logger = LogManager.getLogger(ApiKeyService.class);
    private static final String TYPE = "doc";
+    public static final Setting<String> PASSWORD_HASHING_ALGORITHM = new Setting<>(
+        "xpack.security.authc.api_key.hashing.algorithm", "pbkdf2", Function.identity(), (v, s) -> {


nit but maybe xpack.security.authc.api_key_hashing.algorithm makes more sense ( consistent with xpack.security.authc.password_hashing.algorithm ) ? Naming is hard and I don't have strong opinions

Sure I'll be consistent

jaymode · 2018-11-01T19:15:18Z

We definitely need to think about how we expect administrators to handle existing api-keys when a user's privileges are downgraded.

This is tricky. Maybe we provide an API for an administrator to validate permissions against a set of role names for a given user? We don't keep a record of what a users roles are, so I do not think we can do this automatically unless we have shadow users. Maybe something like:

POST /_xpack/security/api_key/_check_permissions
{
        "username": "tvernum",
        "roles": [ ]
}

tvernum

👍

jaymode added >non-issue :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) labels Oct 17, 2018

jaymode requested review from bizybot and tvernum October 17, 2018 20:30

jaymode changed the base branch from master to security_api_keys October 17, 2018 20:33

Merge branch 'master' into create_api_key

7446f3d

tvernum reviewed Oct 19, 2018

View reviewed changes

jaymode added 9 commits October 19, 2018 08:04

Merge branch 'master' into create_api_key

aac38fe

add streaminput super constructors

4dfb7ed

check name when setting

710ab24

fix class name

eacd0da

fail if role retrieval was not a success

28d2e72

add api key service

758abac

fix test

a4c2db8

add setting

a8bccfa

add client action

02bd86d

bizybot reviewed Oct 22, 2018

View reviewed changes

jaymode mentioned this pull request Oct 22, 2018

Responses can use Writeable.Reader interface #34655

Merged

bizybot reviewed Oct 23, 2018

View reviewed changes

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java Outdated Show resolved Hide resolved

jaymode added 5 commits October 23, 2018 08:12

Merge branch 'master' into create_api_key

9a0260e

add super calls for reading

6e74f5a

delete unimplemented rest action

60a06fd

use unmodifiable list

68cce7c

use securestring in response

fddba0c

jaymode requested a review from jkakavas October 24, 2018 15:37

jaymode added 3 commits October 24, 2018 10:15

fix securestring to xcontent

447fb30

Merge branch 'security_api_keys' into create_api_key

6a44c9b

provide id with key, only store hash of key

e6e82c7

Merge branch 'security_api_keys' into create_api_key

54d753f

tvernum requested changes Nov 1, 2018

View reviewed changes

jkakavas requested changes Nov 1, 2018

View reviewed changes

jaymode added 6 commits November 1, 2018 10:01

Merge branch 'security_api_keys' into create_api_key

03a45ea

use writeable.reader for response

a9e9c61

s/LOGGER/logger

a9b7e3f

no key in id

a4d163d

assert no key in id

2a06c0f

allow configurable hash

a818b45

jkakavas approved these changes Nov 1, 2018

View reviewed changes

jaymode added 3 commits November 1, 2018 12:23

rename setting

40aea03

more name restrictions

75ae8d4

Merge branch 'security_api_keys' into create_api_key

68104e2

tvernum approved these changes Nov 2, 2018

View reviewed changes

jaymode merged commit f087b54 into elastic:security_api_keys Nov 2, 2018

jaymode deleted the create_api_key branch November 2, 2018 13:48

Conversation

jaymode commented Oct 17, 2018

Uh oh!

elasticmachine commented Oct 17, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaymode commented Oct 31, 2018

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

tvernum Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

jaymode Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tvernum Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

jaymode Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkakavas left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkakavas Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

jaymode Nov 1, 2018

Choose a reason for hiding this comment

Uh oh!

jaymode commented Nov 1, 2018

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jkakavas left a comment •

edited

Loading