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

Add optional state file permissions #2238

Merged
merged 1 commit into from May 7, 2020

Conversation

@sthirugn
Copy link
Contributor

sthirugn commented Apr 27, 2020

Description

Before this commit, it was possible that the puma.state file would be world-readable which may not be desirable in production environments. This introduces a new optional configuration option to set desired state file permissions.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.
# bundle exec rake test:all
<snip>
# Running:
....................................................................................................................................S.....................S.....S.....S..........................................................................................................................................................S........

Fabulous run in 115.014845s, 2.8692 runs/s, 7.3643 assertions/s.

330 runs, 847 assertions, 0 failures, 0 errors, 5 skips

You have skipped tests. Run with --verbose for details.

------------------------------------------------------------ Debugging Info
TestIntegrationCluster#test_term_closes_listeners_tcp
    10 successes, 5 resets, 25 refused, failures 0
TestIntegrationCluster#test_term_closes_listeners_unix
    10 successes, 0 resets, 30 refused, failures 0
---------------------------------------------------------------------------

ruby test/shell/run.rb
curl: (7) Failed to connect to localhost port 10102: Connection refused
Puma starting in single mode...
* Version 4.3.3 (ruby 2.7.0-p0), codename: Mysterious Traveller
* Min threads: 0, max threads: 5
* Environment: development
* Listening on tcp://0.0.0.0:10102
Use Ctrl-C to stop
Hello Worldcurl: (7) Failed to connect to localhost port 10103: Connection refused
Puma starting in single mode...
* Version 4.3.3 (ruby 2.7.0-p0), codename: Mysterious Traveller
* Min threads: 0, max threads: 5
* Environment: development
* Listening on tcp://0.0.0.0:10103
Use Ctrl-C to stop
Hello WorldCommand stop sent success
[440153] Puma starting in cluster mode...
[440153] * Version 4.3.3 (ruby 2.7.0-p0), codename: Mysterious Traveller
[440153] * Min threads: 0, max threads: 5
[440153] * Environment: development
[440153] * Process workers: 3
[440153] * Phased restart available
[440153] * Listening on tcp://0.0.0.0:10102
[440153] Use Ctrl-C to stop
[440153] - Worker 0 (pid: 440155) booted, phase: 0
[440153] - Worker 1 (pid: 440156) booted, phase: 0
[440153] - Worker 2 (pid: 440158) booted, phase: 0
[440153] - Worker 2 (pid: 440178) booted, phase: 0
[440153] - Gracefully shutting down workers...
</snip>
@dentarg
Copy link
Contributor

dentarg commented Apr 27, 2020

Should we add a test for this?

History.md Outdated Show resolved Hide resolved
@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch from 824dfd1 to 0402537 Apr 27, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 27, 2020

Should we add a test for this?

I wanted to write a test and searched for tests related to state_path so I can extend them for permission. But I don't find any tests for state_path. If you know, can you point me to state_path tests so I can extend it for the permissions?

@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch from 0402537 to 10758b5 Apr 27, 2020
lib/puma/dsl.rb Outdated Show resolved Hide resolved
History.md Outdated
@@ -49,6 +49,9 @@
* JSON parse cluster worker stats instead of regex (#2124)
* Support parallel tests in verbose progress reporting (#2223)

* Security
* New configuration option to set state file permissions (#2238)

This comment has been minimized.

Copy link
@nateberkopec

nateberkopec Apr 27, 2020

Member

I think this can be added under "features" rather than a new section

@nateberkopec
Copy link
Member

nateberkopec commented Apr 27, 2020

It's possible there are no tests for state_path. You don't have to write tests for that, but we do need tests for this.

What's the current file permissions of a created statefile today?

What information is in the statefile that would need to protected from read or write?

@ekohl
Copy link

ekohl commented Apr 27, 2020

What's the current file permissions of a created statefile today?

I guess it depends on the umask, but 644 in most conditions. See theforeman/foreman#7578 (comment)

@nateberkopec
Copy link
Member

nateberkopec commented Apr 27, 2020

Do you think we should change the default here?

@ekohl
Copy link

ekohl commented Apr 27, 2020

I think a default of 0640 is a more sane default but may break existing deployments. Given the security nature of it, I think that's fine though as long as there's a clear note in the changelog.

@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch 2 times, most recently from b8f0423 to 9cd118b Apr 28, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 28, 2020

It's possible there are no tests for state_path. You don't have to write tests for that, but we do need tests for this.

I will look into this.

What information is in the statefile that would need to protected from read or write?

control_url, control_auth_token - these will let any user control the puma server.

@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 29, 2020

It's possible there are no tests for state_path. You don't have to write tests for that, but we do need tests for this.

I will look into this.

Tests are added now.

@nateberkopec
Copy link
Member

nateberkopec commented Apr 29, 2020

I'm not comfortable with changing people's file permissions w/o them setting it themselves, so I'll be merging this as-is.

Thank you so much for adding tests btw 🙇

@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 29, 2020

I'm not comfortable with changing people's file permissions w/o them setting it themselves, so I'll be merging this as-is.

Yes, that was my thought too. Lets get this optional for now and make it default in future if needed.

Thank you so much for adding tests btw bow

Absolutely!

@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch 3 times, most recently from eeda1b9 to ad8a4c4 Apr 29, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 29, 2020

Squashed commits and rebased with master.

@sthirugn
Copy link
Contributor Author

sthirugn commented Apr 29, 2020

no idea why File.unlink is failing in windows. I don't have experience or access to windows machines to troubleshoot this. Does anyone else have any insights into this?

Copy link

ekohl left a comment

Alternative:

require 'tmpdir'
Dir.mktmpdir do |dir|
  tmp_file = File.join(tmp_file, 'puma.state')

  # ... rest of the code
end

This means you don't need to think about cleanup. An additional benefit is that you're sure that the file doesn't exist beforehand. Thinking about that also triggered another review comment.

File.write path, YAML.dump(@options)
File.chmod(permission, path) if permission
Comment on lines 12 to 13

This comment has been minimized.

Copy link
@ekohl

ekohl Apr 29, 2020

There's a race condition here: the file is created possibly insecure and then fixed. A more secure way is to ensure permissions before writing. For example (untested):

Suggested change
File.write path, YAML.dump(@options)
File.chmod(permission, path) if permission
if permission
FileUtils.touch(path)
File.chmod(permission, path)
end
File.write path, YAML.dump(@options)

This comment has been minimized.

Copy link
@ehelms

ehelms May 1, 2020

I believe you can do this in a single transaction block that might be a bit more clean:

File.open(path) do |file|
  file.chmod(permission) if permission
  file.write(YAML.dump(@options))
end
@nateberkopec
Copy link
Member

nateberkopec commented Apr 30, 2020

Appreciate your thoughtful reviews @ekohl 🙏

test/test_launcher.rb Outdated Show resolved Hide resolved
@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch 2 times, most recently from 4c09b5c to 84a28bd May 1, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 1, 2020

Review comments updated, commits squashed.

@sthirugn
Copy link
Contributor Author

sthirugn commented May 1, 2020

It looks 2 of the windows tests are failing, I will check

@MSP-Greg
Copy link
Contributor

MSP-Greg commented May 1, 2020

@sthirugn

JFYI, Windows testing of Ruby was non-existent with Ruby 2.2 and 2.3. The issue is whatever is creating the paths like:
d:/a/_temp/d:/a/_temp/d20200501-5716-u567s/puma-state20200501-5716-ze77jb

See:
https://github.com/puma/puma/pull/2238/checks?check_run_id=636932635#step:7:87

Not sure where it's happening...

@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch from 58287b2 to df8984e May 1, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 1, 2020

I spent a good amount of time looking into the windows 2.2/2.3 failures and figured Dir.mktmpdir was not playing well with it. So I reverted the Dir.mktmpdir changes and went back to the old method of creating temp files which seems to work fine now.

Now all the commits are squashed and PR rebased. Let me know if any questions.

Before this commit, it was possible that the puma.state file would be world readable which may not be desirable in production environments. This introduces a new optional configuration option to set desired state file permissions.
@sthirugn sthirugn force-pushed the sthirugn:state_file_perm_update branch from df8984e to cc05732 May 1, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 1, 2020

it doesn't look like the mac 2.7 failure is related to this PR.

@sthirugn sthirugn requested a review from nateberkopec May 4, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 4, 2020

I don't have permissions to remove waiting-for-changes label.

@sthirugn
Copy link
Contributor Author

sthirugn commented May 5, 2020

@nateberkopec @ekohl let me know what you think about the recent changes. Our foreman PR is blocked on this. Any help is appreciated to get this going and get merged. Thanks

@ekohl
ekohl approved these changes May 5, 2020
Copy link

ekohl left a comment

Looks good to me, but I'm not a maintainer on this project so I might be missing things.

@ehelms
ehelms approved these changes May 6, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 6, 2020

thank you @ekohl @ehelms.

@nateberkopec let me know if you need anything before merging this.

@nateberkopec
Copy link
Member

nateberkopec commented May 7, 2020

Thanks for the PR and thanks for the reviews @ekohl and @ehelms

@nateberkopec nateberkopec reopened this May 7, 2020
@nateberkopec nateberkopec merged commit 3060a75 into puma:master May 7, 2020
26 of 27 checks passed
26 of 27 checks passed
ubuntu 2.2
Details
build
Details
ubuntu 2.3
Details
ubuntu 2.4
Details
ubuntu 2.5
Details
ubuntu 2.6
Details
ubuntu 2.7
Details
ubuntu head
Details
ubuntu jruby
Details
ubuntu truffleruby-head
Details
macos 2.2
Details
macos 2.3
Details
macos 2.4
Details
macos 2.5
Details
macos 2.6
Details
macos 2.7 macos 2.7
Details
macos head
Details
macos jruby
Details
macos truffleruby-head
Details
windows 2.2
Details
windows 2.3
Details
windows 2.4
Details
windows 2.5
Details
windows 2.6
Details
windows 2.7
Details
windows mingw
Details
ubuntu jruby-head ubuntu jruby-head
Details
@sthirugn sthirugn deleted the sthirugn:state_file_perm_update branch May 7, 2020
@sthirugn
Copy link
Contributor Author

sthirugn commented May 7, 2020

thank you @nateberkopec

@ehelms
Copy link

ehelms commented May 7, 2020

Thanks @nateberkopec ! To help us with planning, when do you think we will see a release with this fix in it?

@ehelms
Copy link

ehelms commented Jun 17, 2020

@nateberkopec Any chance we could get a 4.3.6 with this change in it? the @theforeman project would be grateful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.