refactor: run the webserver unprivileged#3666
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sgaist The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Note for reviewers: it's the first time I am taking a shot at fork so I might have implemented things in a wrong fashion. |
0699b71 to
c052012
Compare
|
Hey @sgaist I'm unsure if we have enough time to put this into the upcoming release, but it seems really interesting! So tentatively for Also, I have a side question. While investigating #3664, I had the feeling that when the Prometheus metrics and web server are enabled, the memory consumption slightly increases by simply repeatedly refreshing the metrics web page. However, I can't reliably repro this. So, it would be great if, along with this initiative, we can also investigate more about possible issues with the web server (and the dependent metrics) feature(s). |
|
Since we are likely too late for the upcoming release, moving to |
|
Hey @sgaist , any update on this? |
|
@ekoops sorry, I just realized that your review got lost in my mail box. I'll check that ASAP. |
|
@sgaist any update on this? |
|
I have done some exploration on the IPC side with pipes however I realized that there's no way to send back the information that the webserver was started through this mechanism as by design it enters a blocking loop and there's no callback that could be used to state "started". |
The webserver was running in a thread with the same privileges as the falco process. In order to make running it more secure, use fork rather than thread and then change the user and group id to unprivileged values. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
c052012 to
6eaed3f
Compare
Using a local variable won't work since the server and the main application don't share the same memory space anymore. To check that the server is running, a call to the health endpoint is used. While the start of the server should be pretty fast, a simple exponential backoff has been implemented to allow for room in case the start is slowed for some reason. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
6eaed3f to
23ba06d
Compare
I am wondering whether this is the result of calling the metrics collector snapshot function. |
| } | ||
| if(m_server_thread.joinable()) { | ||
| m_server_thread.join(); | ||
| falco_logger::log(falco_logger::level::INFO, "Webserver: killing fork\n"); |
There was a problem hiding this comment.
What about using "webserver process" instead of "fork"? I mean, both here and in the other places... WDYT?
There was a problem hiding this comment.
I reworded some of the other texts as well to make things nicer.
They were inverted. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
Improve readability Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
| } catch(std::exception &e) { | ||
| falco_logger::log(falco_logger::level::ERR, | ||
| "falco_webserver: " + std::string(e.what()) + "\n"); | ||
| "Webserver: " + std::string(e.what()) + "\n"); |
There was a problem hiding this comment.
My concern is what happens if the m_server->listen(...) call fail. Here, we are catching the exception, and after we print a log line, we just return from the function. Even if the main falco process will recognize that the son is not running, I guess we should avoid letting the web server process to proceed.
There was a problem hiding this comment.
Currently, if the server does not start for some reason, it will be detected by the loop below that tries to connect to the server. However, it's not a nice way to detect that.
I have refactored that part.
| m_webserver_config.m_uid = m_config.get_scalar<uint32_t>("webserver.uid", 1000); | ||
| m_webserver_config.m_gid = m_config.get_scalar<uint32_t>("webserver.gid", 1000); |
There was a problem hiding this comment.
I don't know if 1000 is the right choice here. Maybe we should provide IDs that are not connected to any user... After all, it's not always the case that 1000 is the uid of an user in the system. What do you think?
There was a problem hiding this comment.
Good point, I have currently changed them for the nobody uid/pid.
Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
If starting the server throws for some reason, the result will be instantly shown. Signed-off-by: Samuel Gaist <samuel.gaist@idiap.ch>
cdf6b93 to
fd13688
Compare
|
Hey @sgaist , I guess we broke prometheus metrics: https://github.com/falcosecurity/falco/actions/runs/21926985793?pr=3666 |
|
Oh damn, it seems that my answer got lost in the process. I dug a bit on that part only to realize that the metrics end point activation will happen in the parent process hence the breakage you spotted. |
|
I was able to come back to this and I think it needs a bit more design brainstorming (I was I bit tired when starting this and there are several details that escaped me at that time). I think that the webserver, as a forked process, should not have anything to do with directly accessing the What do you think ? |
4 participants
What type of PR is this?
/kind cleanup
/kind design
/kind feature
Any specific area of the project related to this PR?
What this PR does / why we need it:
The webserver is running in a thread with the same privileges as the falco process. In order to make running it more secure, use fork rather than thread and then change the user and group id to unprivileged values.
Which issue(s) this PR fixes:
Fixes #3485
Special notes for your reviewer:
Does this PR introduce a user-facing change?: