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

n2yen commented at 5:29 am on June 4, 2018: none

When the option -sysperms=false (default) is enabled, the created datadirs doesn’t take into account the default umask setting of ‘077’. The fix moves the umask operation to just after parameters are parsed during AppInit()

laanwj commented at 3:28 pm on June 4, 2018: member

There’s a few problems with this:

-sysperms can no longer be set in the configuration file
by moving it to bitcoind.cpp, it’s no longer heeded for the GUI

Tend toward NACK. Better to set the permissions on the data directory once, manually, it’s not going to touch them after that.

laanwj commented at 4:01 pm on June 4, 2018: member

Or, if this is a big issue, maybe do it in both places?

MarcoFalke commented at 4:12 pm on June 4, 2018: member

Would be nice to know when this regression was introduced. If it never did anything, I guess it can be removed?

If it is not going to be removed, this must come with functional tests that verify the behavior.

n2yen commented at 8:08 pm on June 4, 2018: none

Appreciate the feedback, I will look into these later - please let me know if you have any other suggestion as well. Thank you!

n2yen force-pushed on Jun 5, 2018

n2yen commented at 7:39 am on June 5, 2018: none

Had a look at the bitcoin-qt as suggested - it looks like if there was a regression it could be because it potentially generates datadir on its own, without calling GetDefaultDataDir(), see comments in the commit above. Tested by running bitcoind, and bitcoin-qt and checking datadir. The existing unit tests, don’t appear to rely on either of the above utility function calls - should I still go ahead and add unit tests for this anyway? thanks!

ken2812221 commented at 7:49 am on June 5, 2018: contributor

@n2yen You can add a python script into functional test, it allow you to run a node to test this.

laanwj commented at 1:43 pm on June 5, 2018: member

If it is not going to be removed, this must come with functional tests that verify the behavior.

It definitely works, but not for the initial directory creation. It works for files created after that, and that’s what it’s intended for (block file data sharing, basically).

n2yen commented at 3:48 pm on June 5, 2018: none

@Iaanwj can you clarify if we should also leave the old the old code where it was also (in AppInitBasicSetup )? i’m not sure, thank you for your feedback!

MarcoFalke commented at 3:52 pm on June 5, 2018: member

@n2yen I haven’t looked, but according to the issue #13371 the old code is doing nothing. Having useless code is useless, so please remove if it is useless.

laanwj commented at 6:22 am on June 7, 2018: member

Oh I get the problem now, it’s not -sysperms option that’s broken, but the default umask setting. It isn’t effective yet when the data directory is created, but is by the time when e.g. the wallet is created (as that’s later in the init process). I strongly disagree that it does nothing, though.

OK, yes, it should definitely be moved so that the data directory is also created with that umask too.

I’m still not sure bitcoind.cpp is the right place to move it though, because that wouldn’t make umask setting apply at all to bitcoin-qt.

There’s an inherent chicken-and-egg problem with the -sysperms option, because by the time bitcoin.conf is read, the data directory is already created.

Also agree this needs a test, because apperently this is another initialization issue with strict ordering requirements.

laanwj commented at 6:31 am on June 7, 2018: member

So to show the current code should not naively be removed (nor applied for bitcoind only), this affects wallet default permissions:

0$ src/bitcoind -datadir=/tmp/wallet1 -regtest
1$ stat /tmp/wallet1/regtest/wallets/wallet.dat 
2Access: (0600/-rw-------)

After removing the umask setting code from AppInitBasicSetup:

0$ src/bitcoind -datadir=/tmp/wallet2 -regtest
1$ stat /tmp/wallet2/regtest/wallets/wallet.dat
2Access: (0660/-rw-rw----)

(wallet is group-readable now, the exact result depends on the umask in the user’s environment of course, but this is undesirable)

n2yen commented at 0:07 am on June 8, 2018: none

@Iaanwj ok, so we’re in agreement, that the umask in AppInitBasicSetup() may not be early enough. I agree that the umask cannot just be in bitcoind.cpp, but also in bitcoin-qt or some common code that can be called earlier… I will also be adding tests in the way suggested by @ken2812221.

n2yen force-pushed on Jun 8, 2018

n2yen commented at 1:46 pm on June 9, 2018: none

Updated commit with tests to verify permissions of regtest directory, and umask is set as early as sensible in both bitcoind and bitcoin-qt. @laanwj , @MarcoFalke can I please get your feedback?

DrahtBot commented at 5:02 am on July 26, 2018: member

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

Conflicts

No conflicts as of last run.

DrahtBot commented at 3:16 pm on July 29, 2018: member

DrahtBot closed this on Jul 29, 2018

DrahtBot reopened this on Jul 29, 2018

DrahtBot added the label Needs rebase on Aug 25, 2018

n2yen force-pushed on Sep 11, 2018

n2yen commented at 7:53 am on September 11, 2018: none

Rebased on top of: eb2f1bd276108e70aff0f582a407e9b702eb4dd1 - Merge #14189: qa: Fix silent merge conflict in wallet_importmulti… On sept 10, 2018

DrahtBot removed the label Needs rebase on Sep 11, 2018

in test/functional/feature_nosysperms.py:19 in 3b8551d3a8 outdated

14+        self.setup_clean_chain = True
15+        self.num_nodes = 1
16+
17+    def check_permissions(self, fullpathname):
18+        self.log.info('{}  {}'.format(stat.filemode(os.stat(fullpathname).st_mode), fullpathname))
19+        return (os.stat(fullpathname).st_mode & (stat.S_IRWXO|stat.S_IRWXG)) == 0

practicalswift commented at 4:00 pm on September 14, 2018:

02018-09-14 14:23:42 flake8(pr=13389): test/functional/feature_nosysperms.py:19:62: E227 missing whitespace around bitwise or shift operator

n2yen commented at 4:54 am on September 16, 2018:

Done.

in test/functional/feature_nosysperms.py:30 in 3b8551d3a8 outdated

25+                result.append(self.check_permissions(os.path.join(path, filename)))
26+        return result
27+
28+    def run_test(self):
29+        #self.stop_node(0)
30+        #self.start_node(0, ['-regtest'])

practicalswift commented at 4:01 pm on September 14, 2018:

Remove these two comments :-)

n2yen commented at 4:54 am on September 16, 2018:

Done. Thanks for the feedback!

ken2812221 commented at 5:06 am on September 16, 2018: contributor

Concept ACK. nit: You should skip the test on Windows.

n2yen commented at 2:54 pm on September 16, 2018: none

Hi @ken2812221, Thanks for your feedback. I’ve made the changes, to skip the test on Windows, however I don’t have a windows machine to confirm it skips. Can someone try it? I’ve confirmed the tests will run on Linux.

DrahtBot added the label Needs rebase on Sep 16, 2018

n2yen force-pushed on Sep 17, 2018

ken2812221 commented at 2:25 am on September 17, 2018: contributor

I would prefer to rename to feature_sysperms.py and check if -sysperms works as expected and no -sysperms works as expected. Now you only check for no sysperms, missing the test with -sysperms.

I don’t have a windows machine to confirm it skips. Can someone try it?

#14007 is about to merge, you can check if it works as expected on Appveyor after then.

DrahtBot removed the label Needs rebase on Sep 17, 2018

n2yen commented at 4:15 pm on September 17, 2018: none

Thanks Ken2812221, yup, I’ll look into it.

in test/functional/feature_nosysperms.py:25 in 2e2ed57eb4 outdated

20+        self.log.info('{}  {}'.format(stat.filemode(os.stat(fullpathname).st_mode), fullpathname))
21+        return (os.stat(fullpathname).st_mode & (stat.S_IRWXO | stat.S_IRWXG)) == 0
22+
23+    def check_dir_permissions(self, regtest_dirpath):
24+        result = []
25+        for path, dirs, files in os.walk(regtest_dirpath):

practicalswift commented at 8:22 pm on September 23, 2018:

Nit: for path, _, files in os.walk(regtest_dirpath):

n2yen force-pushed on Sep 30, 2018

n2yen commented at 5:24 pm on September 30, 2018: none

travis-ci appears to have failed due to (from last line in log): “The job exceeded the maximum time limit for jobs, and has been terminated.” @practicalswift , @ken2812221 - can I get your advise? best regards.

practicalswift commented at 5:53 pm on September 30, 2018: contributor

@n2yen Job restarted :-)

n2yen commented at 6:49 pm on September 30, 2018: none

@practicalswift thank you!

n2yen commented at 1:13 am on October 1, 2018: none

@practicalswift timed out again, this time, didn’t even get a chance to run the feature tests!

DrahtBot added the label GUI on Mar 19, 2019

DrahtBot added the label Tests on Mar 19, 2019

in test/functional/feature_sysperms.py:41 in 0394a871db outdated

36+            raise SkipTest("This test does not run on Windows.")
37+
38+        self.restart_node(0)
39+        datadir = self.nodes[0].datadir
40+
41+        self.log.info("Running test without -sysperms option - assuming umask of '0o77'")

hebasto commented at 5:05 pm on April 26, 2019:

'0o77' - typo?

n2yen commented at 2:25 pm on April 27, 2019:

you’re right, should be ‘077’. What did you make of -sysperm option, is it still needed?

hebasto commented at 3:04 pm on April 27, 2019:

What did you make of -sysperm option, is it still needed?

My concerns about -sysperms are in the note here #15902.

n2yen commented at 4:29 pm on April 27, 2019:

does it makes sense to close this PR - perhaps replace it with simply removing the option? i suppose the tests might still be useful…

MarcoFalke commented at 5:54 pm on May 20, 2019: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

n2yen force-pushed on May 21, 2019

DrahtBot added the label Needs rebase on Jul 24, 2019

MarcoFalke deleted a comment on Jul 24, 2019

n2yen force-pushed on Jul 27, 2019

n2yen commented at 4:01 pm on July 27, 2019: none

rebased!

DrahtBot removed the label Needs rebase on Jul 27, 2019

jonasschnelli commented at 8:32 am on October 9, 2019: contributor

utACK 09072f762ae3fe9a746ab447458e95a6fb646ed3

laanwj commented at 8:54 am on October 9, 2019: member

I don’t think my original comments have been addressed (edit: the GUI one is). This still doesn’t allow setting sysperm in the configuration file, only on the command line.

It also complicates the initialization order.

I’m starting to think it would be better to remove the sysperm setting.

jonasschnelli commented at 9:10 am on October 9, 2019: contributor

I underestimated the fact that one can no longer use the sysperm setting in the config file after this. I just retrieved my ACK.

DrahtBot added the label Needs rebase on May 21, 2020

jonasschnelli commented at 7:02 am on May 29, 2020: contributor

@n2yen: what is the status of this PR? Can you have a look in addressing/commenting @laanwj comment?

jonasschnelli added the label Waiting for author on May 29, 2020

n2yen commented at 8:19 pm on May 29, 2020: none

ok, will have a look.

Fix #13371 - call umask earlier and remove it from AppInitBasicSetup() as it is too late there.

When the option -sysperms=false (default) is enabled, the created datadirs does not take into account the default umask setting of '077'.
Fix moves the umask operation as early as possible inside of bitcoind, and bitcoin-qt. This was needed because by the time AppInitBasicSetup()
is called, datadir, has typically already been created. Calling umask earlier ensures any filesystem writes that follows has the correct umask set.

Tests: feature_sysperms.py - validates datadir permissions where -sysperms=false (default)
Notes:
'-sysperms' option requires '-disablewallet'
- add check for windows platform, feature_nosysperms.py will explicitly skip the test if
  'Windows' platform is detected.

if bitcoind is built with NO_WALLET=1. That is -sysperms will fail if
not accompanied with -disablewallet option

6cca05d65d

rebased and move sysperms check after reading of config file 82e2f141c1

n2yen force-pushed on May 31, 2020

DrahtBot removed the label Needs rebase on Jun 1, 2020

jonasschnelli commented at 7:00 am on June 5, 2020: contributor

Closing in favour of #17127 (which is probably superior).

jonasschnelli closed this on Jun 5, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts