Skip to content

Conversation

@BigPandaToo
Copy link
Contributor

@BigPandaToo BigPandaToo commented May 31, 2021

Method to be called by the startup process while elasticsearch is in the
enrollment mode to obtain an
enrollment token used to enroll a new node to the cluster or an enrollment
token to configure Kibana to communicate with a secured elasticsearch
cluster

Resolve: #71438
Related: #72129

Method to be called by the startup process while elasticsearch is in the
 enrollment mode to obtain an
enrollment token used to enroll a new node to the cluster.

Resolve: elastic#71438
Related: elastic#72129
@BigPandaToo BigPandaToo added :Security/Security Security issues without another label v8.0.0 labels May 31, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label May 31, 2021
@elasticmachine
Copy link
Collaborator

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

@BigPandaToo BigPandaToo marked this pull request as draft May 31, 2021 20:40
@BigPandaToo BigPandaToo marked this pull request as ready for review June 1, 2021 08:04
@BigPandaToo BigPandaToo marked this pull request as draft June 1, 2021 18:59
enrollment mode to obtain an
enrollment token used to enroll a new node to the cluster.

Resolve: elastic#71438
Related: elastic#72129
@BigPandaToo BigPandaToo marked this pull request as ready for review June 3, 2021 17:08
@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@jkakavas jkakavas self-requested a review June 8, 2021 20:28
@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

Please address my comments, then I think it's good to merge.

Worth noting that the token, as proposed herein, includes addresses for all the cluster nodes.
In practice connection information for the node that generated the token ought be enough, and we're trying to keep the token short. CC @jkakavas you might have opinions, I'm fine either way.

@BigPandaToo
Copy link
Contributor Author

Please address my comments, then I think it's good to merge.

Worth noting that the token, as proposed herein, includes addresses for all the cluster nodes.
In practice connection information for the node that generated the token ought be enough, and we're trying to keep the token short. CC @jkakavas you might have opinions, I'm fine either way.

That's not true. Address for the first node returned by /_nodes/http API is only incuded. We can be more selective and filter local only or master only node...

Copy link
Contributor

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

Please address my comments, then I think it's good to merge.
Worth noting that the token, as proposed herein, includes addresses for all the cluster nodes.
In practice connection information for the node that generated the token ought be enough, and we're trying to keep the token short. CC @jkakavas you might have opinions, I'm fine either way.

That's not true. Address for the first node returned by /_nodes/http API is only incuded. We can be more selective and filter local only or master only node...

Yes, we only want to return local info. The idea is that you get a token from the node that you want to talk to in the enrollment process. I don't think we should depend on the local node being the first in the response. We should limit the response by passing _local in the request


if (httpCode != HttpURLConnection.HTTP_OK) {
logger.error("Error " + httpCode + "when calling GET " + url + ". ResponseBody: " +
(httpResponseApiKey == null ? "" : httpResponseApiKey.getResponseBody()));
Copy link
Contributor

Choose a reason for hiding this comment

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

httpResponseApiKey can't be null, can it?

@BigPandaToo
Copy link
Contributor Author

@elasticmachine update branch

@BigPandaToo BigPandaToo requested a review from jkakavas June 22, 2021 16:21
assertThat(ex.getMessage(), Matchers.containsString("Unexpected response code [400] from calling GET "));
}

public void testFailedRetrieveHttpInfoNoCaInKeystore() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

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

these do not have to do with failing to retrieve http info right and are not similar to testFailedRetrieveHttpInfo. Can we rename this and the following methods?

Copy link
Contributor

@jkakavas jkakavas left a comment

Choose a reason for hiding this comment

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

LGTM, please change the test method names and we're good to merge, thanks for the iterations!

@BigPandaToo BigPandaToo merged commit b706e8a into elastic:master Jun 23, 2021
if (Strings.isNullOrEmpty(apiKey) || Strings.isNullOrEmpty(apiId)) {
throw new IllegalStateException("Could not create an api key.");
}
return Base64.getEncoder().encodeToString((apiId + ":" + apiKey).getBytes(StandardCharsets.UTF_8));
Copy link
Contributor

Choose a reason for hiding this comment

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

Missed that in the review @BigPandaToo . I don;t think there is need to base64 encode the API key before we put it as a value in the token, as the token itself will be Base64 encoded.

I think the length gains are more significant than the additiional effort the consumers of the token need to make to base64 the string before using it in the Authorization header and this behavior is also consistent with the create api key API .

Can you please tackle this change in a short follow up PR ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, forgot that it won't be used directly

BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Jun 23, 2021
No need for base64 encode the API key before we put it as a value in the
 token, as the token itself will be Base64 encoded

A follow up PR for:
elastic#73573
BigPandaToo added a commit to BigPandaToo/elasticsearch that referenced this pull request Jun 23, 2021
BigPandaToo added a commit that referenced this pull request Jun 23, 2021
No need for base64 encode the API key before we put it as a value in the
 token, as the token itself will be Base64 encoded

A follow up PR for:
#73573
BigPandaToo added a commit that referenced this pull request Jun 24, 2021
* Calculate SHA256 fingerprint for enrollment token

A follow up PR for:
#73573

* Adding a test fix

Resolves: #74525
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

>enhancement :Security/Security Security issues without another label Team:Security Meta label for security team v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement create enrollment token API

5 participants