util: Set safe permissions for data directory and `wallets/` subdir #17127

hebasto · 2019-10-13T20:07:09Z

On master (1e7564e) docs say:

$ ./src/bitcoind -help | grep -A 3 sysperms
  -sysperms
       Create new files with system default permissions, instead of umask 077
       (only effective with disabled wallet functionality)

Basing on that, one could expect that running bitcoind first time will create data directory and wallets/ subdirectory with safe 0700 permissions.

But that is not the case:

$ stat .bitcoin | grep id
Access: (0775/drwxrwxr-x)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0775/drwxrwxr-x)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)

Both directories, in fact, are created with system default permissions.

With this PR:

$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)
$ stat .bitcoin/wallets | grep id
Access: (0700/drwx------)  Uid: ( 1000/ hebasto)   Gid: ( 1000/ hebasto)

This PR:

Changes 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.

DrahtBot · 2019-10-13T20:43:52Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	john-moffett, willcl-ark
Concept ACK	laanwj, practicalswift
Stale ACK	jonasschnelli

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26863 (test: merge banning test from p2p_disconnect_ban to rpc_setban by brunoerg)

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.

laanwj · 2019-10-14T09:31:38Z

src/util/system.cpp

GetDataDir, a getter, is not the place to do things such as set the umask. Please don't add these kind of side-effects.

Maybe GetDataDir is not a proper name for this function as it already has an intended side-effect:
https://github.com/bitcoin/bitcoin/blob/ddd3cd30cb050fe9b8738778beecba1d8bd50d52/src/util/system.cpp#L777-L780

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)

src/util/system.cpp

laanwj · 2019-10-14T09:32:23Z

Concept ACK on removing -sysperms.

hebasto · 2019-10-14T11:31:15Z

@laanwj
Thank you for your review. All your comments have been addressed.

hebasto · 2019-10-16T16:08:54Z

Rebased after #17138 has been merged.

laanwj · 2019-10-21T15:13:11Z

src/util/system.h

Wouldn't SetupEnvironment be the right place for this? Or is that too early?

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 ;)

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?

Wouldn't SetupEnvironment be the right place for this?

Agree.

hebasto · 2019-10-26T18:40:54Z

@laanwj Thank you for review. Your comment has been addressed.

hebasto · 2019-11-25T20:04:00Z

@promag Would you mind reviewing this PR?

hebasto · 2020-04-20T19:31:03Z

Rebased 7213e8c -> 67b1209 (pr17127.04 -> pr17127.05) due to the conflict with #15761.

hebasto · 2020-05-09T03:52:23Z

Rebased 67b1209 -> 297e61c (pr17127.05 -> pr17127.06) due to the conflict with #16224.

jonasschnelli · 2020-05-29T09:16:46Z

utACK 297e61c

willcl-ark

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?

hebasto · 2022-11-30T15:19:26Z

@willcl-ark

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?

willcl-ark · 2022-11-30T16:11:18Z

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 datadir and walletsdir. For files I was suggesting we could test debug.log.

willcl-ark · 2022-12-09T09:45:53Z

This also fixes #22595

hebasto · 2022-12-09T13:18:13Z

Updated 9b105d7 -> 07c496d (pr17127.11 -> pr17127.12):

rebased
addressed @willcl-ark's comment

willcl-ark

ACK 07c496d

Might want to add this to the release notes too?

src/util/system.cpp

This change effectively reverts commits from bitcoin#4286. Users, who rely on non-default access permissions, should use `chmod` command.

hebasto · 2023-02-05T13:46:11Z

Updated 07c496d -> 15c7105 (pr17127.12 -> pr17127.13):

rebased
addressed @fanquake's comment

john-moffett

Approach ACK 15c7105

I agree that this may warrant an explicit release note considering a command line option is being dropped.

src/i2p.cpp

src/rpc/request.cpp

This change makes all filesystem artifacts--files and directories--being created with the default umask.

hebasto · 2023-02-06T11:11:42Z

Updated 15c7105 -> c9ba4f9 (pr17127.13 -> pr17127.14, diff):

addressed @john-moffett's comments

john-moffett · 2023-02-06T16:49:44Z

ACK c9ba4f9

willcl-ark

ACK c9ba4f9

…d `wall…

fanquake added the Utils/log/libs label Oct 13, 2019

laanwj reviewed Oct 14, 2019

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

hebasto force-pushed the 20191013-permissions branch from ddd3cd3 to b43aa35 Compare October 14, 2019 11:29

DrahtBot added the Needs rebase label Oct 15, 2019

hebasto force-pushed the 20191013-permissions branch from b43aa35 to 005649f Compare October 16, 2019 16:07

DrahtBot removed the Needs rebase label Oct 16, 2019

laanwj reviewed Oct 21, 2019

View reviewed changes

hebasto force-pushed the 20191013-permissions branch from 005649f to 7213e8c Compare October 26, 2019 18:37

This was referenced Feb 11, 2020

gui: Bilingual GUI error messages #16224

Merged

Replace -upgradewallet startup option with upgradewallet RPC #15761

Merged

DrahtBot added the Needs rebase label Apr 19, 2020

hebasto force-pushed the 20191013-permissions branch from 7213e8c to 67b1209 Compare April 20, 2020 19:28

DrahtBot removed the Needs rebase label Apr 20, 2020

DrahtBot mentioned this pull request May 6, 2020

rpc: prevent circular deps by extracting into separate include #18871

Closed

DrahtBot added the Needs rebase label May 8, 2020

hebasto force-pushed the 20191013-permissions branch from 67b1209 to 297e61c Compare May 9, 2020 03:50

DrahtBot removed the Needs rebase label May 9, 2020

DrahtBot mentioned this pull request May 14, 2020

doc: noban precludes maxuploadtarget disconnects #18968

Merged

jonasschnelli mentioned this pull request Jun 5, 2020

Utils and libraries: Fix #13371 - move umask operation earlier in AppInit() #13389

Closed

DrahtBot mentioned this pull request Jun 5, 2020

interfaces: Expose settings.json methods to GUI #15936

Merged

DrahtBot mentioned this pull request Nov 17, 2022

init: Evaluate sysperms before config file #26512

Closed

willcl-ark approved these changes Nov 17, 2022

View reviewed changes

hebasto force-pushed the 20191013-permissions branch from 9b105d7 to 07c496d Compare December 9, 2022 13:16

hebasto mentioned this pull request Dec 9, 2022

make install creates files/folders with bad permissions if custom umask is used #22595

Closed

fanquake requested a review from willcl-ark December 20, 2022 16:41

willcl-ark approved these changes Jan 6, 2023

View reviewed changes

fanquake reviewed Jan 6, 2023

View reviewed changes

src/util/system.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Jan 10, 2023

test: merge banning test from p2p_disconnect_ban to rpc_setban #26863

Closed

Remove -sysperms option

8a6219e

This change effectively reverts commits from bitcoin#4286. Users, who rely on non-default access permissions, should use `chmod` command.

hebasto force-pushed the 20191013-permissions branch from 07c496d to 15c7105 Compare February 5, 2023 08:09

fanquake requested review from john-moffett and willcl-ark February 5, 2023 15:15

john-moffett reviewed Feb 5, 2023

View reviewed changes

src/i2p.cpp Outdated Show resolved Hide resolved

src/rpc/request.cpp Outdated Show resolved Hide resolved

hebasto added 2 commits February 6, 2023 11:08

Apply default umask in SetupEnvironment()

581f16e

This change makes all filesystem artifacts--files and directories--being created with the default umask.

test: Add test for file system permissions

c9ba4f9

hebasto force-pushed the 20191013-permissions branch from 15c7105 to c9ba4f9 Compare February 6, 2023 11:09

willcl-ark approved these changes Feb 6, 2023

View reviewed changes

fanquake merged commit 6e08e5c into bitcoin:master Feb 7, 2023

hebasto deleted the 20191013-permissions branch February 7, 2023 11:36

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 7, 2023

Merge bitcoin#17127: util: Set safe permissions for data directory an…

9c8d89e

…d `wall…

willcl-ark mentioned this pull request Mar 15, 2023

Allow groups of accounts to access the RPC cookie file #25270

Closed

willcl-ark mentioned this pull request Dec 7, 2023

/home/bitcoin/.bitcoin permissions vs. /data/bitcoin raspibolt/raspibolt#1347

Closed

bitcoin locked and limited conversation to collaborators Feb 7, 2024

util: Set safe permissions for data directory and wallets/ subdir #17127

util: Set safe permissions for data directory and wallets/ subdir #17127

Uh oh!

Conversation

hebasto commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Oct 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

laanwj Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

laanwj commented Oct 14, 2019

Uh oh!

hebasto commented Oct 14, 2019

Uh oh!

hebasto commented Oct 16, 2019

Uh oh!

laanwj Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 21, 2019

Choose a reason for hiding this comment

Uh oh!

laanwj Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto Oct 26, 2019

Choose a reason for hiding this comment

Uh oh!

hebasto commented Oct 26, 2019

Uh oh!

hebasto commented Nov 25, 2019

Uh oh!

hebasto commented Apr 20, 2020

Uh oh!

hebasto commented May 9, 2020

Uh oh!

jonasschnelli commented May 29, 2020

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Nov 30, 2022

Uh oh!

willcl-ark commented Nov 30, 2022

Uh oh!

willcl-ark commented Dec 9, 2022

Uh oh!

hebasto commented Dec 9, 2022

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hebasto commented Feb 5, 2023

Uh oh!

john-moffett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Feb 6, 2023

Uh oh!

john-moffett commented Feb 6, 2023

Uh oh!

util: Set safe permissions for data directory and `wallets/` subdir #17127

util: Set safe permissions for data directory and `wallets/` subdir #17127

hebasto commented Oct 13, 2019 •

edited

Loading

DrahtBot commented Oct 13, 2019 •

edited

Loading

laanwj Oct 14, 2019 •

edited

Loading