-
Notifications
You must be signed in to change notification settings - Fork 35.7k
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
util: Set safe permissions for data directory and wallets/
subdir
#17127
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
src/util/system.cpp
Outdated
@@ -741,6 +750,10 @@ const fs::path &GetBlocksDir() | |||
|
|||
const fs::path &GetDataDir(bool fNetSpecific) | |||
{ | |||
#ifndef WIN32 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetDataDir, a getter, is not the place to do things such as set the umask. Please don't add these kind of side-effects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe GetDataDir
is not a proper name for this function as it already has an intended side-effect:
Lines 777 to 780 in ddd3cd3
if (fs::create_directories(path)) { | |
// This is the first run, create wallets subdirectory too | |
fs::create_directories(path / "wallets"); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. I kind of want to get rid of that too. But don't add any more
(edit: also I think setting umask
is the worst kind of side effect possible, it's global state of the program over all threads)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Concept ACK on removing |
ddd3cd3
to
b43aa35
Compare
@laanwj |
b43aa35
to
005649f
Compare
Rebased after #17138 has been merged. |
src/util/system.h
Outdated
@@ -35,6 +35,10 @@ | |||
|
|||
#include <boost/thread/condition_variable.hpp> // for boost::thread_interrupted | |||
|
|||
#ifndef WIN32 | |||
void SetDefaultUmask(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't SetupEnvironment
be the right place for this? Or is that too early?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, SetDefaultUmask()
is called just before the first possible disk write.
SetupEnvironment()
seems too broad, as it is called in utilities too. And, yes, I think it is too early. But I could be wrong ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But does that matter? What would be the consequences of setting it too early? Why wouldn't we want to set the umask for the utilties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't
SetupEnvironment
be the right place for this?
Agree.
005649f
to
7213e8c
Compare
@laanwj Thank you for review. Your comment has been addressed. |
@promag Would you mind reviewing this PR? |
7213e8c
to
67b1209
Compare
Rebased 7213e8c -> 67b1209 (pr17127.04 -> pr17127.05) due to the conflict with #15761. |
67b1209
to
297e61c
Compare
Rebased 67b1209 -> 297e61c (pr17127.05 -> pr17127.06) due to the conflict with #16224. |
utACK 297e61c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 9b105d7
The test datadir will have the following files created in it after node shutdown:
banlist.json, debug.log, fee_estimates.dat, mempool.dat, peers.dat, settings.json
Would it be worth adding a permissions test for e.g debug.log to check that the correct umask is also being applied to files?
Sorry, but I didn't quite get your suggestion. Mind elaborating it? |
My fault for not being clearer! You added a test for directory permissions, but not not one for files. As files and folders will be created with different permissions, it might make sense to add this at the same time? For directories the test covers |
This also fixes #22595 |
9b105d7
to
07c496d
Compare
Updated 9b105d7 -> 07c496d (pr17127.11 -> pr17127.12):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 07c496d
Might want to add this to the release notes too?
This change effectively reverts commits from bitcoin#4286. Users, who rely on non-default access permissions, should use `chmod` command.
07c496d
to
15c7105
Compare
Updated 07c496d -> 15c7105 (pr17127.12 -> pr17127.13): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approach ACK 15c7105
I agree that this may warrant an explicit release note considering a command line option is being dropped.
This change makes all filesystem artifacts--files and directories--being created with the default umask.
15c7105
to
c9ba4f9
Compare
Updated 15c7105 -> c9ba4f9 (pr17127.13 -> pr17127.14, diff):
|
ACK c9ba4f9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK c9ba4f9
On master (1e7564e) docs say:
Basing on that, one could expect that running
bitcoind
first time will create data directory andwallets/
subdirectory with safe 0700 permissions.But that is not the case:
Both directories, in fact, are created with system default permissions.
With this PR:
This PR:
make install
creates files/folders with bad permissions if custom umask is used #22595Changes in behavior: removed
-sysperms
command-line argument / configure option. The related discussions are here:If users rely on non-default access permissions, they could use
chmod
.