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.

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

URL option for BaseRunAsSuperuserCommand #81025

Merged
merged 9 commits into from
Nov 29, 2021

Conversation

jkakavas
Copy link
Member

@jkakavas jkakavas commented Nov 24, 2021

Add a --url option for elasticsearch-reset-password and
elasticsearch-create-enrollment-token CLI Tools ( and any tools
that would extend BaseRunAsSuperuserCommand ).
The tools use CommandLineHttpClient internally, which tries its
best to deduce the URL of the local node based on the configuration
but there are certain cases where it either fails or returns an
unwanted result. Concretely:

  • CommandLineHttpClient#getDefaultURL will always return a URL with
    the port set to 9200, unless otherwise explicitly set in the
    configuration. When running multiple nodes on the same host,
    subsequent nodes get sequential port numbers after 9200 by default
    and this means that the CLI tool will always connect the first of
    n nodes in a given host. Since these tools depend on a file realm
    local user, requests to other nodes would fail
  • When an ES node binds and listens to many addresses, there can
    be the case that not all of the IP addresses are added as SANs in
    the certificate that is used for TLS on the HTTP layer.
    CommandLineHttpClient#getDefaultURL will pick an address based on
    a preference order but that address might not be in the SANs and
    thus all requests to the node would fail due to failed hostname
    verification.

Manually setting --url to an appropriate value allows users to
overcome these edge cases.

Resolves: #80481

Add a --url option for elasticsearch-reset-password and
elasticsearch-create-enrollment-token CLI Tools ( and any tools
that would extend BaseRunAsSuperuserCommand ).
The tools use CommandLineHttpClient internally, which tries its
best to deduce the URL of the local node based on the configuration
but there are certain cases where it either fails or returns an
unwanted result. Concretely:

- CommandLineHttpClient#getDefaultURL will always return a URL with
the port set to 9200, unless otherwise explicitly set in the
configuration. When running multiple nodes on the same host,
subsequent nodes get sequential port numbers after 9200 by default
and this means that the CLI tool will always connect the first of
n nodes in a given host. Since these tools depend on a file realm
local user, requests to other nodes would fail
- When an ES node binds and listens to many addresses, there can
be the case that not all of the IP addresses are added as SANs in
the certificate that is used for TLS on the HTTP layer.
CommandLineHttpClient#getDefaultURL will pick an address based on
a preference order but that address might not be in the SANs and
thus all requests to the node would fail due to failed hostname
verification.

Manually setting `--url` to an appropriate value allows users to
overcome these edge cases.
@jkakavas jkakavas added :Security/Security Security issues without another label >enhancement v8.0.0 labels Nov 24, 2021
@elasticmachine elasticmachine added the Team:Security Meta label for security team label Nov 24, 2021
@elasticmachine
Copy link
Collaborator

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

`--url`:: Specifies the base URL (hostname and port) that the tool uses to submit API
requests to {es}. The default value is determined from the settings in your
`elasticsearch.yml` file. If `xpack.security.http.ssl.enabled` is set to `true`,
you must specify an HTTPS URL.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it would be preferable that we drop the "If xpack.security.http.ssl.enabled is set to true,
you must specify an HTTPS URL" scheme condition, by having the tool check for that itself (like it does for the "default URL")?

Moreover, a URL is more than the scheme and the host and port pair, and using DNS names to refer to the node might not be ideal, as it might not be included in the SAN of the cert.
Do you think it would be preferable that this option be more focused on the ip and port that the local node can be reached at?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think it would be preferable that this option be more focused on the ip and port that the local node can be reached at?

I contemplated the same but in the end settled to a single option as a simpler thing, happy to discuss.

a URL is more than the scheme and the host and port pair

true but what we should be aiming for here is not naming strictness for the sake of it but a name for the option that’s understandable by (or at least explainable for ) the majority of users. I think that url fits the bill.

and using DNS names to refer to the node might not be ideal, as it might not be included in the SAN of the cert.

the idea is that if you ever need to use this parameter you do so because you know that you want to talk to the node at an ip or hostname that is in the SANs but CommandLineHttpClient picks another. So you know what should be in the url (either a hostname or an IP address )

Do you think it would be preferable that we drop the "If xpack.security.http.ssl.enabled is set to true,
you must specify an HTTPS URL" scheme condition, by having the tool check for that itself (like it does for the "default URL")?

If we change this to two parameters then yes it makes sense , we shouldn’t be asking the users for a third parameter (scheme) but I’m not 100% that ee should change.

Do we introduce an ip and a port parameter? Do we mandate both if one is passed or have default values for the port ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we introduce an ip and a port parameter? Do we mandate both if one is passed or have default values for the port ?

I don't have a preference. Having to specify both the ip and the port simultaneously (as is the case with this url parameter), as a single option is OK.

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.

I think it would be an improvement if we convey that the new cmd line argument takes the ip address and port of the local node. LGTM otherwise.

FWIW I am also OK with documenting your workaround of specifying the -Enode.host and -Enode.port as arguments.

@jkakavas
Copy link
Member Author

I think it would be an improvement if we convey that the new cmd line argument takes the ip address and port of the local node.

You mean in the docs ? Something like :

`--url`:: Specifies the base URL (hostname and port of the local node) that
 the tool uses to submit API requests to {es}. The default value is determined 
from the settings in your `elasticsearch.yml` file. 
If `xpack.security.http.ssl.enabled` is set to `true`, you must specify an HTTPS URL.

?

Copy link
Contributor

@lockewritesdocs lockewritesdocs left a comment

Choose a reason for hiding this comment

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

Added a few suggestions, but overall LGTM.

Comment on lines +63 to +64
requests to {es}. The default value is determined from the settings in your
`elasticsearch.yml` file. If `xpack.security.http.ssl.enabled` is set to `true`,
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
requests to {es}. The default value is determined from the settings in your
`elasticsearch.yml` file. If `xpack.security.http.ssl.enabled` is set to `true`,
requests to {es}. The combination of values for `network.host` and `http.port` in your
`elasticsearch.yml` file determine the default URL. If `xpack.security.http.ssl.enabled` is set to `true`,

Copy link
Contributor

Choose a reason for hiding this comment

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

@jkakavas, I think it's worth indicating which settings determine the default value. What do you think?

This change would also apply to the --url description for the create-enrolllment-token tool.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not that simple actually. It can be network.host or http.host , with http.port, let alone advanced options or talking about _site_, _local_, _global_ etc. I don't think this is the place to go through all these in detail :/

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course it's not that simple 😆 , which is probably why you opted for simplicity in your original description. I'm 👍 with keeping your original text -- thank you for the explanation!

Comment on lines 87 to 88
The following example resets the password of a native user with username `user2` to an auto-generated value
prints the new password in the console, while overriding the url where the local elasticsearch node is reachable:
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
The following example resets the password of a native user with username `user2` to an auto-generated value
prints the new password in the console, while overriding the url where the local elasticsearch node is reachable:
The following example resets the password of a native user named `user2` to an auto-generated value,
prints the new password in the console, and overrides the URL where the local {es} node is reachable:

Comment on lines 66 to 67
The following command creates an enrollment token for enrolling a kibana instance into a cluster,
while overriding the url where the local elasticsearch node is reachable:
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
The following command creates an enrollment token for enrolling a kibana instance into a cluster,
while overriding the url where the local elasticsearch node is reachable:
The following command creates an enrollment token for enrolling a {kib} instance into a cluster,
and overrides the URL where the local {es} node is reachable:

Copy link
Member Author

Choose a reason for hiding this comment

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

and feels off here ( and I mean "feels", I can't actually argue about it being wrong :). To me your suggestion reads as if the example command invocation does two things:

  1. It creates an enrollment token
  2. It overrides the URL where the local elasticsearch node is reachable

which is not accurate.
It does only 1. but 2 is something that temporarily happens - and only for the context of this command's execution - so that 1. can succeed.

WDYT @lockewritesdocs ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooo, I see! In that case, let's explore options. Choose whichever one you like better, or let me know neither hits the mark.

This rendition is similar to your original text, but adds "temporarily" to distinguish the time constraint:

The following command creates an enrollment token for enrolling a {kib} instance into a cluster, while temporarily overriding the URL where the local {es} node is reachable:

Another option dismisses the notion of the interaction between 1 and 2 entirely, and instead focuses on what the URL denotes:

The following command creates an enrollment token for enrolling a {kib} instance into a cluster. The specified URL indicates where the local {es} node is reachable:

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the discussion Adam. It actually makes me think that the original "is reachable" is misleading. Setting this option doesn't change at all where elasticsearch is reachable or listens to, but only where the CLI tool will talk to.
In my original version, there was a "while overriding the url where the CLI tool thinks that the local elasticsearch node is reachable" implied there.

With that context, do you have any suggestions on how to frame this ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that context, I think something like the following statement works:

The following command creates an enrollment token for enrolling a {kib} instance into a cluster. The specified URL indicates where the elasticsearch-create-enrollment-token tool attempts to reach the local {es} node:

If we want to indicate more about "where the CLI tool thinks that the local ES node is reachable," then we might need further clarification of what happens if the tool doesn't find the node at the specified URL:

The following command creates an enrollment token for enrolling a {kib} instance into a cluster. The specified URL indicates where the elasticsearch-create-enrollment-token tool attempts to reach the local {es} node. If the tool can't reach {es} at the specified port, it iterates through port numbers sequentially on the indicated host:

Caveat: I'm not certain that the last sentence here ☝️ is true.

Copy link
Member Author

Choose a reason for hiding this comment

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

Caveat: I'm not certain that the last sentence here point_up is true.

It doesn't work like this actually,

The following command creates an enrollment token for enrolling a {kib} instance into a cluster. The specified URL indicates where the elasticsearch-create-enrollment-token tool attempts to reach the local {es} node:

sounds good to me, let's keep this simple

@jkakavas jkakavas merged commit 537f371 into elastic:master Nov 29, 2021
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 30, 2021
* upstream/master: (150 commits)
  Fix ComposableIndexTemplate equals when composed_of is null (elastic#80864)
  Optimize DLS bitset building for matchAll query (elastic#81030)
  URL option for BaseRunAsSuperuserCommand (elastic#81025)
  Less Verbose Serialization of Snapshot Failure in SLM Metadata (elastic#80942)
  Fix shadowed vars pt7 (elastic#80996)
  Fail shards early when we can detect a type missmatch (elastic#79869)
  Delegate Ref Counting to ByteBuf in Netty Transport (elastic#81096)
  Clarify `unassigned.reason` docs (elastic#81017)
  Strip blocks from settings for reindex targets (elastic#80887)
  Split off the values supplier for ScriptDocValues (elastic#80635)
  [ML] Switch message and detail for model snapshot deprecations (elastic#81108)
  [DOCS] Update xrefs for snapshot restore docs (elastic#81023)
  [ML] Updates visiblity of validate API (elastic#81061)
  Track histogram of transport handling times (elastic#80581)
  [ML] Fix datafeed preview with remote indices (elastic#81099)
  [ML] Fix acceptable model snapshot versions in ML deprecation checker (elastic#81060)
  [ML] Add logging for failing PyTorch test (elastic#81044)
  Extending the timeout waiting for snapshot to be ready (elastic#81018)
  [ML] Fix incorrect logging of unexpected model size error (elastic#81089)
  [ML] Make inference timeout test more reliable (elastic#81094)
  ...

# Conflicts:
#	server/src/main/java/org/elasticsearch/index/mapper/NumberFieldMapper.java
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 30, 2021
* upstream/master: (55 commits)
  Fix ComposableIndexTemplate equals when composed_of is null (elastic#80864)
  Optimize DLS bitset building for matchAll query (elastic#81030)
  URL option for BaseRunAsSuperuserCommand (elastic#81025)
  Less Verbose Serialization of Snapshot Failure in SLM Metadata (elastic#80942)
  Fix shadowed vars pt7 (elastic#80996)
  Fail shards early when we can detect a type missmatch (elastic#79869)
  Delegate Ref Counting to ByteBuf in Netty Transport (elastic#81096)
  Clarify `unassigned.reason` docs (elastic#81017)
  Strip blocks from settings for reindex targets (elastic#80887)
  Split off the values supplier for ScriptDocValues (elastic#80635)
  [ML] Switch message and detail for model snapshot deprecations (elastic#81108)
  [DOCS] Update xrefs for snapshot restore docs (elastic#81023)
  [ML] Updates visiblity of validate API (elastic#81061)
  Track histogram of transport handling times (elastic#80581)
  [ML] Fix datafeed preview with remote indices (elastic#81099)
  [ML] Fix acceptable model snapshot versions in ML deprecation checker (elastic#81060)
  [ML] Add logging for failing PyTorch test (elastic#81044)
  Extending the timeout waiting for snapshot to be ready (elastic#81018)
  [ML] Fix incorrect logging of unexpected model size error (elastic#81089)
  [ML] Make inference timeout test more reliable (elastic#81094)
  ...
weizijun added a commit to weizijun/elasticsearch that referenced this pull request Nov 30, 2021
* upstream/master: (55 commits)
  Fix ComposableIndexTemplate equals when composed_of is null (elastic#80864)
  Optimize DLS bitset building for matchAll query (elastic#81030)
  URL option for BaseRunAsSuperuserCommand (elastic#81025)
  Less Verbose Serialization of Snapshot Failure in SLM Metadata (elastic#80942)
  Fix shadowed vars pt7 (elastic#80996)
  Fail shards early when we can detect a type missmatch (elastic#79869)
  Delegate Ref Counting to ByteBuf in Netty Transport (elastic#81096)
  Clarify `unassigned.reason` docs (elastic#81017)
  Strip blocks from settings for reindex targets (elastic#80887)
  Split off the values supplier for ScriptDocValues (elastic#80635)
  [ML] Switch message and detail for model snapshot deprecations (elastic#81108)
  [DOCS] Update xrefs for snapshot restore docs (elastic#81023)
  [ML] Updates visiblity of validate API (elastic#81061)
  Track histogram of transport handling times (elastic#80581)
  [ML] Fix datafeed preview with remote indices (elastic#81099)
  [ML] Fix acceptable model snapshot versions in ML deprecation checker (elastic#81060)
  [ML] Add logging for failing PyTorch test (elastic#81044)
  Extending the timeout waiting for snapshot to be ready (elastic#81018)
  [ML] Fix incorrect logging of unexpected model size error (elastic#81089)
  [ML] Make inference timeout test more reliable (elastic#81094)
  ...
jkakavas added a commit to jkakavas/elasticsearch that referenced this pull request Nov 30, 2021
Add a --url option for elasticsearch-reset-password and
elasticsearch-create-enrollment-token CLI Tools ( and any tools
that would extend BaseRunAsSuperuserCommand ).
The tools use CommandLineHttpClient internally, which tries its
best to deduce the URL of the local node based on the configuration
but there are certain cases where it either fails or returns an
unwanted result. Concretely:

- CommandLineHttpClient#getDefaultURL will always return a URL with
the port set to 9200, unless otherwise explicitly set in the
configuration. When running multiple nodes on the same host,
subsequent nodes get sequential port numbers after 9200 by default
and this means that the CLI tool will always connect the first of
n nodes in a given host. Since these tools depend on a file realm
local user, requests to other nodes would fail
- When an ES node binds and listens to many addresses, there can
be the case that not all of the IP addresses are added as SANs in
the certificate that is used for TLS on the HTTP layer.
CommandLineHttpClient#getDefaultURL will pick an address based on
a preference order but that address might not be in the SANs and
thus all requests to the node would fail due to failed hostname
verification.

Manually setting `--url` to an appropriate value allows users to
overcome these edge cases.
ywangd pushed a commit to ywangd/elasticsearch that referenced this pull request Dec 1, 2021
Add a --url option for elasticsearch-reset-password and
elasticsearch-create-enrollment-token CLI Tools ( and any tools
that would extend BaseRunAsSuperuserCommand ).
The tools use CommandLineHttpClient internally, which tries its
best to deduce the URL of the local node based on the configuration
but there are certain cases where it either fails or returns an
unwanted result. Concretely:

- CommandLineHttpClient#getDefaultURL will always return a URL with
the port set to 9200, unless otherwise explicitly set in the
configuration. When running multiple nodes on the same host,
subsequent nodes get sequential port numbers after 9200 by default
and this means that the CLI tool will always connect the first of
n nodes in a given host. Since these tools depend on a file realm
local user, requests to other nodes would fail
- When an ES node binds and listens to many addresses, there can
be the case that not all of the IP addresses are added as SANs in
the certificate that is used for TLS on the HTTP layer.
CommandLineHttpClient#getDefaultURL will pick an address based on
a preference order but that address might not be in the SANs and
thus all requests to the node would fail due to failed hostname
verification.

Manually setting `--url` to an appropriate value allows users to
overcome these edge cases.
jkakavas added a commit that referenced this pull request Dec 1, 2021
Add a --url option for elasticsearch-reset-password and
elasticsearch-create-enrollment-token CLI Tools ( and any tools
that would extend BaseRunAsSuperuserCommand ).
The tools use CommandLineHttpClient internally, which tries its
best to deduce the URL of the local node based on the configuration
but there are certain cases where it either fails or returns an
unwanted result. Concretely:

- CommandLineHttpClient#getDefaultURL will always return a URL with
the port set to 9200, unless otherwise explicitly set in the
configuration. When running multiple nodes on the same host,
subsequent nodes get sequential port numbers after 9200 by default
and this means that the CLI tool will always connect the first of
n nodes in a given host. Since these tools depend on a file realm
local user, requests to other nodes would fail
- When an ES node binds and listens to many addresses, there can
be the case that not all of the IP addresses are added as SANs in
the certificate that is used for TLS on the HTTP layer.
CommandLineHttpClient#getDefaultURL will pick an address based on
a preference order but that address might not be in the SANs and
thus all requests to the node would fail due to failed hostname
verification.

Manually setting `--url` to an appropriate value allows users to
overcome these edge cases.
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 v8.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CLI tools extending BaseRunAsSuperuserCommand should only connect to the local node
5 participants