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

Make http.enabled: true the default? #12918

Closed
ycombinator opened this issue Jul 15, 2019 · 23 comments
Closed

Make http.enabled: true the default? #12918

ycombinator opened this issue Jul 15, 2019 · 23 comments
Labels
discuss Issue needs further discussion. enhancement Feature:Stack Monitoring libbeat needs_team Indicates that the issue/PR needs a Team:* label Stalled

Comments

@ycombinator
Copy link
Contributor

ycombinator commented Jul 15, 2019

Describe the enhancement:

Currently any Beat (or APM server) starts with http.enabled set to false as the default behavior.

With the introduction of the beat Metricbeat module, should we now default to having http.enabled set to true instead? The module works by periodically calling various Beats APIs.

Describe a specific use case for the enhancement or feature:

Stack Monitoring users will soon be using Metricbeat's beat module to collect Beats' (and APM server) monitoring data for visualizing in the Stack Monitoring app in Kibana. In order to use this module, users will first need to be told to set http.enabled to true in their Beat or APM server instances that need to be monitored. By defaulting http.enabled to true going forward, we can reduce this extra setup step for the user.

FWIW, Logstash nodes start up with their HTTP server enabled by default.

@ycombinator ycombinator added enhancement discuss Issue needs further discussion. libbeat labels Jul 15, 2019
@ycombinator
Copy link
Contributor Author

Pinging @elastic/beats and @elastic/apm-server.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/stack-monitoring

@graphaelli
Copy link
Member

graphaelli commented Jul 15, 2019

It's too bad we (apm) didn't lobby to get this moved under monitoring pre-7.0, as this option is confusing given that apm-server is primarily an http server.

I don't see a problem enabling this by default given the default listener is only on localhost. Did you plan to update the official docker image to expose this port as well? I see making the default http.host use all interfaces (0.0.0.0) under docker but I'm not sure about publishing by default.

@cachedout
Copy link
Contributor

In playing with this, I have a few concerns:

  1. I don't see any authentication necessary to connect to localhost:5066 with this option enabled. Are we comfortable with this?

  2. I poked around a little and it looks like at least a few of the Beats all using port 5066 as the default. What happens with multiple beats on the same machine?

  3. Do we have a recommendation about which user should run beats? From a security perspective, quite honestly, this concerns me. This gives all processes on a machine access to a webserver that might be running as root (yikes!) and to a process which might have access to a secrets store or, even worse, to a production cluster.

TBH, I'm very hesitant about this from a security perspective. While I appreciate that this would make the setup experience easier, I believe that running a webserver locally should be something that is explicitly enabled by users and not the default behavior.

@ycombinator
Copy link
Contributor Author

I don't see any authentication necessary to connect to localhost:5066 with this option enabled. Are we comfortable with this?

What sort of authentication are you thinking of here? Also, what is the concern with calling these non-side-effecting APIs unauthenticated?

I poked around a little and it looks like at least a few of the Beats all using port 5066 as the default. What happens with multiple beats on the same machine?

The user would need to change the http.port setting to a non-default value.

Do we have a recommendation about which user should run beats? From a security perspective, quite honestly, this concerns me. This gives all processes on a machine access to a webserver that might be running as root (yikes!) and to a process which might have access to a secrets store or, even worse, to a production cluster.

It looks like we suggest running Beats as root. So I can understand your concern here.

Given the security concerns, I'm okay with not enabling the HTTP server by default. It would need to be an additional and explicit step performed by the user.

@ph
Copy link
Contributor

ph commented Sep 5, 2019

@ycombinator @cachedout Did you consider allowing beats to listen over a unix socket (I believe windows has name pipe under windows?) For the agent team, this looks more and more to be a must-have. Because let's say that agent starts X number of beats, it means that it also needs to open X number of ports.

Since unix socket is a file we can set permission on it to limit who can access the monitoring information.

cc @michalpristas

@ycombinator
Copy link
Contributor Author

ycombinator commented Sep 5, 2019

I'm good with the unix socket / windows named pipe approach. I'd like to test the Metricbeat beat module with a unix socket address for the Beats HTTP APIs but I don't expect this to be a problem.

@cachedout WDYT? Would this address the security concerns you raised?

@cachedout
Copy link
Contributor

A UNIX socket would address my concerns. That's a 👍 from me.

@graphaelli
Copy link
Member

graphaelli commented Sep 5, 2019

I don't follow the security issue raised here. The stats endpoint uses the stdlib's default mux with some straightforward handlers. The usability tradeoff against a bug in the standard library/these handlers doesn't seem worth it, especially considering that not all beats run as root by default, like apm-server.

@cachedout
Copy link
Contributor

I don't follow the security issue raised here.

One thing that concerns me is that the server is labeled as EXPERIMENTAL when it starts. Here's the exact message the Metricbeat emits when http.server: true is set:

2019-09-06T09:54:59.592+0200 WARN [cfgwarn] api/server.go:34 EXPERIMENTAL: Metrics endpoint is enabled.

I don't feel particularly comfortable setting a default which produces a warning message about an experimental feature listening on a TCP port. Do we know when and if Beats HTTP servers are scheduled to move from being an experimental feature to being GA?

@ph
Copy link
Contributor

ph commented Sep 6, 2019

@cachedout, To be honest, the HTTP server in beats is in for a very long time, I believe we could mark it GA. My understanding of the presence of the experimental warnings was to give us the flexibility to change the output format without notice. The metric endpoint on the beats was used in the past for debugging purpose like getting the current rates of events.

So depending on the stability of the API we could change the feature level.

I don't follow the security issue raised here. The stats endpoint uses the stdlib's default mux with some straightforward handlers. The usability tradeoff against a bug in the standard library/these handlers don't seem worth it, especially considering that not all beats run as root by default, like apm-server.

Not sure I understand exactly what you mean here @graphaelli, my proposal of having the option to listen to a UNIX socket was to make sure we don't consume port on the local machine and we don't have to either detect or keep track of used ports. Of course, related to the fact that the agent will starts multiples instances of the beats.

@graphaelli
Copy link
Member

My understanding of the presence of the experimental warnings was to give us the flexibility to change the output format without notice

That's my understanding as well.

having the option to listen to a UNIX socket was to make sure we don't consume port on the local machine and we don't have to either detect or keep track of used ports

We'll still have to track the domain socket paths though. The process also needs perms to create them somewhere which is less simple for non-root processes but not impossible. I think it would be simpler to message that going from local monitoring to remote monitoring is changing localhost to 0.0.0.0 (or similar). I don't have a full understanding about the interaction with agent and think it would be good to discuss on a call, I'll set something up.

@ph
Copy link
Contributor

ph commented Sep 9, 2019

adding a small summary here, I did a 1:1 with @graphaelli concerning how the agent handle monitoring and we discussed the specific cases of APM-Server, where APM-Server doesn't run by default as root.

There are a few things we can do here:

  1. Fallback to ports for APM and let APM-Server find the correct port, (Agent can query that information somehow)
  2. Still use a unix socket, but manage the permission accordingly to make sure that APM-Server can create the unix socket.

I still think a UNIX socket is a preferable way to go since processes are colocated next to each other for the agent.

@cachedout @ycombinator Do you prefer we create specific issues to add support for UNIX socket when configuring the monitoring endpoint? I just to make sure we can track that change on our side.

Another point that @graphaelli pointed out in our discussion is to use the same strategy as APM-Server to identify if I need to answer on a port or a unix socket.

@cachedout
Copy link
Contributor

@cachedout @ycombinator Do you prefer we create specific issues to add support for UNIX socket when configuring the monitoring endpoint?

If we go the UNIX socket route then, yes, I think a separate issue to track that work and discussion would be good.

@ph
Copy link
Contributor

ph commented Sep 10, 2019

@cachedout I've created #13577 and assigned to both of you feel free to adjust that.

@ph
Copy link
Contributor

ph commented Oct 16, 2019

Now that we can listen to a unix socket or a named pipe under windows, what if we enable the options by default.

comment from @ycombinator

So if we turn it on by default, I assume the default would be to listen on the socket. If so, what would the default socket name/path be? Maybe something including the PID so we don't have collisions between multiple beats running on the same host?

I think for the unix socket the following would make sense /var/run/filebeat.sock, concerning running multiple time the same processes other than the agent, I don't think it's a common use-case?

@ycombinator
Copy link
Contributor Author

concerning running multiple time the same processes other than the agent, I don't think it's a common use-case?

It was a concern brought up by @cachedout in #12918 (comment).

@ph
Copy link
Contributor

ph commented Oct 16, 2019

Correct but the concern is more a problem when you are using TCP, where all the beats listen on the same socket with the move to the unix socket the file can be different and use the name of the beat to separate concern.

@ycombinator
Copy link
Contributor Author

ycombinator commented Oct 16, 2019

Ah yes, good point. The use case of running two instances of the same beat on the same host should be fairly uncommon, I assume. One other thought, what about placing the default socket file under path.data? Two Beat instances really shouldn't share the same path.data so this would help reduce the chance of collisions even more?

@ruflin
Copy link
Contributor

ruflin commented Oct 21, 2019

What if we go with the path that @ph proposed but make the path configurable. If two FB instances are started up, the second one will complain because the socket file already exists. @ycombinator Not sure as a user I would look under path.data for the socket file, but would be good to get some more feedback on this.

@ph
Copy link
Contributor

ph commented Oct 21, 2019

I am not sure about using the path.data, to run two instance of the same beats you already need to supply a path.data different, so yes that would solve the problem. But this seems to break unix guidelines.

If we look at the FSF document They seems to recommend /run, previously `/var/run

Note that named pipe have a similar problem.

@botelastic
Copy link

botelastic bot commented Sep 21, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@botelastic botelastic bot added Stalled needs_team Indicates that the issue/PR needs a Team:* label labels Sep 21, 2020
@botelastic
Copy link

botelastic bot commented Sep 21, 2020

This issue doesn't have a Team:<team> label.

@botelastic botelastic bot closed this as completed Oct 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issue needs further discussion. enhancement Feature:Stack Monitoring libbeat needs_team Indicates that the issue/PR needs a Team:* label Stalled
Projects
None yet
Development

No branches or pull requests

6 participants