Create enrollment token API #72186

BigPandaToo · 2021-04-23T23:25:11Z

API can be called by the startup process or a user with appropriate
privileges while elasticsearch is in the enrollment mode to obtain an
enrollment token used to enroll a new node to the cluster or configure a
new client to communicate with the cluster.

Resolve: #71438
Related: #72129

As part of the security on be default project, we will be offering an enrollment process that allows new nodes to join a cluster, or clients to bootstrap their configuration to communicate with a cluster that is already running with security enabled. This enrollment process is based on the use of enrollment tokens.

API can be called by the startup process or a user with appropriate privileges while elasticsearch is in the enrollment mode to obtain an enrollment token used to enroll a new node to the cluster or configure a new client to communicate with the cluster. Resolve: elastic#71438 Related: elastic#72129

elasticmachine · 2021-04-23T23:26:22Z

Pinging @elastic/es-security (Team:Security)

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

jkakavas

Added some comments, will take another look on Monday too

server/src/main/java/org/elasticsearch/enrollment/EnrollmentSettings.java

x-pack/docs/en/security/authorization/privileges.asciidoc

x-pack/plugin/core/src/main/java/org/elasticsearch/license/XPackLicenseState.java

...ava/org/elasticsearch/xpack/core/security/action/enrollment/CreateEnrollmentTokenAction.java

docs/reference/cluster/create_enrollment_token.asciidoc

BigPandaToo · 2021-04-26T19:35:51Z

@elasticmachine update branch

BigPandaToo · 2021-04-26T19:59:17Z

@jkakavas Refactored the code according to our discussion.
The PR still pending:

Transport Action test
Some Doc tests are still failing
Rest high level client API (may be a separate PR)

docs/reference/cluster/create-enrollment-token.asciidoc

...ain/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

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

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

BigPandaToo · 2021-05-04T09:21:31Z

@elasticmachine update branch

...vileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java

server/src/main/java/org/elasticsearch/enrollment/EnrollmentSettings.java

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

jkakavas · 2021-05-05T13:48:44Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                        builder.endObject();
+                        final String jsonString = Strings.toString(builder);
+                        final String token = Base64.getEncoder().encodeToString(jsonString.getBytes(StandardCharsets.UTF_8));
+


maybe log the enrollment token pre-encoding in TRACE level ?

@BigPandaToo ^^

Not sure if we should log this material...

@albertzaharovits what's your take on this?

We usually do not log credentials. In this case they are short lived, but on the other hand they are powerful (add a new node). I would not log it.

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

...n/security/src/main/java/org/elasticsearch/xpack/security/authc/support/ApiKeyGenerator.java

server/src/main/java/org/elasticsearch/enrollment/EnrollmentSettings.java

...vileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/Constants.java

x-pack/plugin/core/src/test/resources/org/elasticsearch/xpack/core/action/httpCA.pem

jkakavas · 2021-05-06T08:43:39Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                        builder.endObject();
+                        final String jsonString = Strings.toString(builder);
+                        final String token = Base64.getEncoder().encodeToString(jsonString.getBytes(StandardCharsets.UTF_8));
+


@BigPandaToo ^^

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

x-pack/docs/en/rest-api/security/create-enrollment-token.asciidoc

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

...ain/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java

x-pack/docs/en/security/authorization/privileges.asciidoc

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

jkakavas · 2021-05-12T18:24:41Z

The class is named SecurityDocumentationIT

…

On Wednesday, May 12, 2021, Lyudmila Fokina ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In client/rest-high-level/src/test/java/org/elasticsearch/ client/SecurityIT.java <#72186 (comment)> : > @@ -11,6 +11,7 @@ import org.apache.http.client.methods.HttpDelete; import org.elasticsearch.ElasticsearchStatusException; import org.elasticsearch.client.security.AuthenticateResponse; +import org.elasticsearch.client.security.CreateEnrollmentTokenResponse; Not sure where it is... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#72186 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACOOCKDZETT22CDHMKTMKTLTNLAZJANCNFSM43PPMRRA> .

BigPandaToo · 2021-05-17T10:24:35Z

@elasticmachine update branch

albertzaharovits

I did a very comprehensive review. It is looking good, yet I was a bit opinionated about some parts of it. This API is crucial and sensitive, so it's something we need to take extra care with, IMO.

But this needs tests, most notably integration tests (eg see ApiKeyIntegTests, ~~and maybe SecurityDocumentationIT as Ioannis was saying~~).

EDIT:
I would also propose we "shape" this more for node enrollment. It is likely we'll not give kibana a token that can be used to take over the cluster (ie the node enrollment token), but rather a different token. I won't dwell much on this, we can rectify names and docs later.

EDIT2:
We need to decide if we want to expose this enroll API to "unenrolled" clients (this PR contains code for it). I am of the opinion that we should not, because otherwise we have to think about a flow for the client to get the credentials and the cert; ie it is not sufficient to say "use these superuser credentials" for two reasons:

the client cannot validate the TLS connection (in a sense, it can't because it is not enrolled), which makes it vulnerable to a MITM that can intercept the token.
if the superuser credentials can now add new nodes and generate HTTPS node certificates, it changes the threat model, eg superusers can be easily disabled, but a node participation to a cluster and associated HTTP TLS certificates cannot be revoked (at least it is very hard).

docs/java-rest/high-level/security/create-enrollment-token.asciidoc

albertzaharovits · 2021-05-18T19:50:22Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+    public List<Route> routes() {
+        return List.of(
+            new Route(POST, "/_security/enrollment_token"),
+            new Route(PUT, "/_security/enrollment_token"));


nit: In general I prefer we don't use PUT without a {name} URL part, but I know we're not consistent.
If you're also opinionated about this and don't like the PUT but copied it for consistency, we can remove it.

albertzaharovits · 2021-05-18T19:54:07Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+
+    @Override
+    public String getName() {
+        return "cluster_enrolment_token_action";


Suggested change

return "cluster_enrolment_token_action";

return "security_create_enrollment_token_action";

albertzaharovits · 2021-05-18T19:55:44Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+/**
+ * Rest endpoint to create an enrollment token
+ */
+public class RestCreateEnrollmentTokenAction extends SecurityBaseRestHandler {


nit: make it final.

albertzaharovits · 2021-05-19T07:04:24Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+    @Override
+    protected RestChannelConsumer innerPrepareRequest(final RestRequest request, final NodeClient client) throws IOException {
+        if (Node.ENROLLMENT_ENABLED.get(settings) != true) {
+            throw new IllegalStateException("Enrollment mode is not enabled.");


IllegalStateExceptions are a bit scary and they look unpolished IMO.
I tend to use them like an assert deep inside the internals. It's true that this API can be thought off as internal, but a little user friendliness can't hurt either, who knows how we might expose it.
My suggestion is to use an ElasticsearchException for this case, with a more detailed message, such as: "This node is not configured to issue enrollment tokens which allow clients and new nodes to communicate and join the cluster"
Also, this translates to HTTP 500, which is OKish, but it signals that the server hit an error when it actually followed the configuration. So in the spirit of improving user friendliness, I would opt for one of 501, 403 or 404 (see RestStatus).

I think status code 4xx is more suitable in this scenario than 5xx. As I understand it, we should use 4xx to signal client error which can be rectified by the cilent (either change request parameters or in this case enable enrollment first). I think 400 is suitable for this case even though it is generic. Because this failure is not really about permission (403) or resource being unavailable (404).

Thanks @ywangd , 400 looks good to me too.

albertzaharovits · 2021-05-19T15:42:01Z

x-pack/docs/en/rest-api/security/create-enrollment-token.asciidoc

+
+<1> The enrollment token contains the following information:
+    - IP Address and port number for the interface where the Elasticsearch node is listening for HTTP connections;
+    - The fingerprint of the CA certificate that is used to sign the certificate that the Elasticsearch node presents for TLS on the HTTP layer;


nit: mention that this is the SHA-1 fingerprint of the DER encoding for the certificate.

albertzaharovits · 2021-05-19T15:45:23Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                    ActionListener.wrap(
+                        CreateApiKeyResponse -> {
+                            try {
+                                final String address = nodeService.info(false, false, false, false, false, false,


We should use the same output format that nodes info uses, eg NetworkAddress.format(httpInfo.getAddress().publishAddress() .

albertzaharovits · 2021-05-19T15:48:08Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                return;
+            } else {
+                final TimeValue expiration = TimeValue.timeValueSeconds(ENROLL_API_KEY_EXPIRATION_SEC);
+                final String[] clusterPrivileges = { "enroll" };


Please use ClusterPrivilegeResolver.ENROLL.getName().

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

ywangd

I stil need look closer to the client code and tests. Thanks!

ywangd · 2021-05-20T12:26:18Z

rest-api-spec/src/main/resources/rest-api-spec/api/security.create_enrollment_token.json

+  "security.create_enrollment_token":{
+    "documentation":{
+      "url":"https://www.elastic.co/guide/en/elasticsearch/reference/master/security-create-enrollment-token.html",
+      "description":"Create an enrollment token to allow a new node to enroll in an existing secured elasticsearch cluster, or a client to configure itself to communicate with a secured elasticsearch cluster."


Based on the live discussion, we can remove the "enroll client" bit here for now.

ywangd · 2021-05-20T12:27:00Z

rest-api-spec/src/main/resources/rest-api-spec/api/security.create_enrollment_token.json

+      "url":"https://www.elastic.co/guide/en/elasticsearch/reference/master/security-create-enrollment-token.html",
+      "description":"Create an enrollment token to allow a new node to enroll in an existing secured elasticsearch cluster, or a client to configure itself to communicate with a secured elasticsearch cluster."
+    },
+    "stability":"stable",


Maybe this should be "experimental"?

ywangd · 2021-05-20T12:28:35Z

x-pack/docs/en/rest-api/security/create-enrollment-token.asciidoc

+==== {api-description-title}
+
+The purpose of the create enrollment token API is to generate an enrollment with which user can enroll a new node
+in an existing secured elasticsearch cluster, or a client can configure itself to


Similarly, we can remove "enroll client" here.

ywangd · 2021-05-20T12:42:06Z

...va/org/elasticsearch/xpack/core/security/action/enrollment/CreateEnrollmentTokenRequest.java

+
+    @Override
+    public String toString() {
+        return "get_enrollment_token";


Nit: It is not really necessary to override this method since the request class does not have any field itself and the super class's implementation should be sufficient. But if we are overriding, it's better to have it consistent with the class name, i.e. create_... instead of get_...?

ywangd · 2021-05-20T12:59:52Z

...st/java/org/elasticsearch/xpack/core/security/action/CreateEnrollmentTokenResponseTests.java

+import java.nio.charset.StandardCharsets;
+import java.util.Base64;
+
+public class CreateEnrollmentTokenResponseTests extends AbstractXContentTestCase<CreateEnrollmentTokenResponse> {


The AbstractXContentTestCase is mostly and makes more sense for testing the client side CreateEnrollmentTokenResponse class, where you already have a parser. And you test the client side response class can correctly parse the server side response. For test server side response itself, you can extend AbstractWireSerializingTestCase. But we are not consistent about it.

ywangd · 2021-05-20T16:34:40Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                                builder.startObject();
+                                builder.field("adr", address);
+                                builder.field("fgr", fingerprint);
+                                builder.field("key", CreateApiKeyResponse.getKey().toString());


This is only the "secret" part of an API key, you need the ID part to be returned as part of the response as well.

ywangd · 2021-05-20T16:38:59Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+            new RestBuilderListener<>(channel) {
+                @Override
+                public RestResponse buildResponse(CreateEnrollmentTokenResponse response, XContentBuilder builder) throws Exception {
+                    builder.startObject();
+                    builder.field("enrollment_token", response.getEnrollmentToken());
+                    builder.endObject();
+                    return new BytesRestResponse(RestStatus.OK, builder);
+                }
+            }


Suggested change

new RestBuilderListener<>(channel) {

@Override

public RestResponse buildResponse(CreateEnrollmentTokenResponse response, XContentBuilder builder) throws Exception {

builder.startObject();

builder.field("enrollment_token", response.getEnrollmentToken());

builder.endObject();

return new BytesRestResponse(RestStatus.OK, builder);

}

}

new RestToXContentListener<>(channel)

ywangd · 2021-05-20T16:41:20Z

x-pack/plugin/src/yamlRestTest/resources/rest-api-spec/test/privileges/11_builtin.yml

+---
+"Test get builtin privileges 7.x":
+  - skip:
+      version: "7.99.99 - "


Suggested change

version: "7.99.99 - "

version: "8.0.0 - "

albertzaharovits · 2021-05-27T04:55:33Z

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java

+                generator.generateApiKey(securityContext.getAuthentication(), apiRequest,
+                    ActionListener.wrap(
+                        CreateApiKeyResponse -> {
+                            try {


Nit: this is not required, the exception is caught in ActionListener#doExecute.

albertzaharovits · 2021-05-27T06:41:47Z

After further consideration this PR seems tricky because of the design change discussions that are in progress.

Summarizing the design discussions that are in-progress:

client enrollment needs further refinement, because whoever is going to eg install kibana to connect to ES, should also not be able to add new ES nodes
the API to generate an enrollment token should not have a client interface, as it can be used to extract the secret key material from nodes. We can add it later, but only if absolutely necessary, not just in case. I would go so for as recommending against creating an API in the first place and piecing together the token in the cmd line utility (or the start-up script) that has to run locally.
the manage_enrollment and enroll privileges are effectively the same privilege, ie the privilege to take over the cluster (like a superuser you cannot disable - more than a superuser).
the enrollment token is opaque but it doesn't have to. I think there are benefits for it not being opaque, ie a comma delimited list, chief among them is that the admin can tinker with the hostname and port parts, to account for any proxy in between the client and the node. It would also be shorter because encoding text to base64 expands the number of characters.

albertzaharovits · 2021-05-27T07:55:09Z

@BigPandaToo As we've discussed, I'm going to approve this PR, even if it's not addressing all the points above, considering that it is following the original design. But I will raise blocker issues that we'll need to sort out before we release.

Please resolve all the comments by either fixing or saying that the respective point needs to be discussed further (and fixed in a follow-up).
I'll take another quick look afterwards, and approve.

BigPandaToo · 2021-05-27T15:56:42Z

@elasticmachine update branch

tvernum · 2021-05-28T03:42:12Z

...org/elasticsearch/xpack/security/rest/action/enrollment/RestCreateEnrollmentTokenAction.java

+    @Override
+    public List<Route> routes() {
+        return List.of(
+            new Route(POST, "/_security/enrollment_token"),


I know this is what the design doc proposes, but I think it's a mistake.
I think all the enrollment APIs should be under a common URI path.

That is the doc proposes:

GET /_security/enroll_node POST /_security/enroll_client POST /_security/enrollment_token

Putting aside the HTTP Method question, think the lack of a common prefix is a problem.
I would propose that everything is under /_security/enroll/ or /_security/enrollment/

e.g.

POST /_security/enroll/node POST /_security/enroll/client POST /_security/enroll/token

❤️ /_security/enroll~~ment~~

BigPandaToo · 2021-06-01T08:10:34Z

A new one is posted instead: #73573

BigPandaToo added 3 commits April 23, 2021 20:21

Create enrolment token

f78af77

BigPandaToo added :Security/Security Security issues without another label >enhancement Team:Security Meta label for security team v8.0.0 labels Apr 23, 2021

BigPandaToo requested review from albertzaharovits and jkakavas April 23, 2021 23:26

ywangd reviewed Apr 23, 2021

View reviewed changes

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

jkakavas requested changes Apr 24, 2021

View reviewed changes

Addressing PR feedback

60c0574

elasticmachine and others added 2 commits April 26, 2021 15:35

Merge branch 'master' into Create_ET

906a013

Addressing PR feedback

97b2d32

BigPandaToo requested a review from jkakavas April 26, 2021 19:52

ywangd mentioned this pull request Apr 27, 2021

Enroll node API #72129

Merged

jkakavas reviewed Apr 29, 2021

View reviewed changes

elasticmachine and others added 2 commits May 4, 2021 05:21

Merge branch 'master' into Create_ET

3eedc37

Addressing PR comments

23d2907

jkakavas reviewed May 5, 2021

View reviewed changes

Addressing PR comments

4151e40

BigPandaToo requested a review from jkakavas May 5, 2021 19:39

jkakavas reviewed May 6, 2021

View reviewed changes

...ain/java/org/elasticsearch/xpack/core/security/authz/privilege/ClusterPrivilegeResolver.java Show resolved Hide resolved

jkakavas reviewed May 6, 2021

View reviewed changes

x-pack/docs/en/security/authorization/privileges.asciidoc Outdated Show resolved Hide resolved

jkakavas reviewed May 6, 2021

View reviewed changes

x-pack/docs/en/security/authorization/privileges.asciidoc Show resolved Hide resolved

ywangd reviewed May 12, 2021

View reviewed changes

...org/elasticsearch/xpack/security/action/enrollment/TransportCreateEnrollmentTokenAction.java Outdated Show resolved Hide resolved

Merge branch 'master' into Create_ET

97537f2

Addressing PR comments adding tests

db56545

BigPandaToo requested a review from jkakavas May 12, 2021 19:55

BigPandaToo added 6 commits May 12, 2021 22:03

Addressing PR comments adding tests

77bf9d5

fixing tests and style

626d957

fixing tests

d107633

fixing tests

f4be25b

Adding test

fb7b523

Disabling running enrollment test with Fips

1800069

albertzaharovits mentioned this pull request May 17, 2021

Fixing Test get builtin privileges #73064

Merged

Merge branch 'master' into Create_ET

865eb0a

BigPandaToo requested a review from ywangd May 17, 2021 12:17

albertzaharovits reviewed May 19, 2021

View reviewed changes

ywangd reviewed May 20, 2021

View reviewed changes

albertzaharovits reviewed May 27, 2021

View reviewed changes

Merge branch 'master' into Create_ET

009cc45

tvernum reviewed May 28, 2021

View reviewed changes

tvernum mentioned this pull request May 28, 2021

Add the Enroll Kibana API #72207

Merged

BigPandaToo closed this Jun 1, 2021

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

	return "cluster_enrolment_token_action";
	return "security_create_enrollment_token_action";

Create enrollment token API #72186

Create enrollment token API #72186

Uh oh!

Conversation

BigPandaToo commented Apr 23, 2021

Uh oh!

elasticmachine commented Apr 23, 2021

Uh oh!

Uh oh!

jkakavas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BigPandaToo commented Apr 26, 2021

Uh oh!

BigPandaToo commented Apr 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BigPandaToo commented May 4, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkakavas commented May 12, 2021 via email

Uh oh!

BigPandaToo commented May 17, 2021

Uh oh!

albertzaharovits 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

albertzaharovits left a comment •

edited

Loading