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.

Sign up for GitHub

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

Jump to bottom

Fixes #29507 - Add puma-status for reporting #7578

Closed
wants to merge 2 commits into from
Closed

Fixes #29507 - Add puma-status for reporting #7578

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8eda3e4
Fixes #29507 - Add puma-status for reporting
sthirugn Apr 8, 2020
5e210d5
Add permission configuration for state file
sthirugn May 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions config/puma/production.rb
View file
Open in desktop
@@ -1,3 +1,9 @@
run_dir = Rails.root.join('tmp')

# Store server info state.
state_path File.join(run_dir, 'puma.state')
state_permission 0640

# Configure "min" to be the minimum number of threads to use to answer
# requests and "max" the maximum.
#
Expand All @@ -17,3 +23,6 @@
dynflow = ::Rails.application.dynflow
dynflow.initialize! unless dynflow.config.lazy_initialization
end

# === Puma control rack application ===
activate_control_app "unix://#{run_dir}/sockets/pumactl.sock"
Copy link
Member

Choose a reason for hiding this comment

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

I haven't checked the permissions on this, but I'm assuming it's foreman:foreman and 0644 which should prevent from everyone writing to it. Correct?

Copy link
Contributor Author

@sthirugn sthirugn Apr 22, 2020 •
edited

Choose a reason for hiding this comment

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

This is how it looks now:

# ll -d /run/foreman/ /run/foreman/sockets
drwxr-xr-x. 5 foreman foreman 200 Apr 22 14:30 /run/foreman/
drwxr-xr-x. 2 foreman foreman  60 Apr 22 14:23 /run/foreman/sockets

# ll /run/foreman/sockets
total 0
srwxrwxrwx. 1 foreman foreman 0 Apr 22 14:23 pumactl.sock

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that any user can connect to the socket and restart the server?

Copy link
Member

Choose a reason for hiding this comment

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

Are these determined by packaging?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, looks like a postgres user is able to restart the puma server if right control_url and control_auth_token is known. Any suggestions on how to lock this down further?

# ll /var/run/foreman/puma.state 
-rw-r--r--. 1 foreman foreman 120 Apr 22 15:23 /var/run/foreman/puma.state

# cat /var/run/foreman/puma.state 
---
pid: 1672
control_url: unix:///run/foreman/sockets/pumactl.sock
control_auth_token: 6c445af0312fff70fe0f190a65644cf

# sudo -u postgres scl enable tfm 'pumactl --control-url 'unix:///var/run/foreman/sockets/pumactl.sock' --control-token 6c445af0312fff70fe0f190a65644cf  restart'
Command restart sent success

Copy link
Member

Choose a reason for hiding this comment

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

What are the permissions on viewing the control token?

Copy link
Contributor Author

@sthirugn sthirugn Apr 22, 2020 •
edited

Choose a reason for hiding this comment

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

see the first line of my post above. the control token is present in puma.state file which has permissions 644

Copy link
Member

@ehelms ehelms Apr 22, 2020 •
edited

Choose a reason for hiding this comment

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

All I can think is having something to adjust the permissions after creation but really the answer is to get a change into Puma to allow configuring the state file permissions.

Is it possible to put the state file in a directory that is not globally readable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All I can think is having something to adjust the permissions after creation but really the answer is to get a change into Puma to allow configuring the state file permissions.

I am working to get a fix to the puma project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Submitted patch to puma project puma/puma#2238 to address the state file security issue.

3 changes: 3 additions & 0 deletions script/foreman-puma-status
View file
Open in desktop
@@ -0,0 +1,3 @@
#!/bin/bash
# Intended for a production environment only
puma-status /run/foreman/puma.state
ekohl marked this conversation as resolved.
Show resolved Hide resolved