[PATCH 1/2] t/init.t: test newly created config permissions

[PATCH 1/2] t/init.t: test newly created config permissions

[Štěpán Němec]

Creating config 0600 disregarding umask breaks scenarios where daemons
run with credentials different from config owner (but need to read the
config).

Fixed in the next commit.
---
 t/init.t | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/init.t b/t/init.t
index 0096ca307962..82a2a4436261 100644
--- a/t/init.t
+++ b/t/init.t
@@ -19,7 +19,11 @@ sub quiet_fail {
 	my $cfgfile = "$ENV{PI_DIR}/config";
 	my $cmd = [ '-init', 'blist', "$tmpdir/blist",
 		   qw(http://example.com/blist blist@example.com) ];
+	my $umask = umask(070) // xbail "umask: $!";
 	ok(run_script($cmd), 'public-inbox-init OK');
+	umask($umask) // xbail "umask: $!";
+	my $mode = (stat($cfgfile))[2];
+	is(sprintf('0%03o', $mode & 0777), '0604', 'config respects umask');
 
 	is(read_indexlevel('blist'), '', 'indexlevel unset by default');
 

base-commit: a036772bdf732d4779f44974095bfdd0b30271ef
prerequisite-patch-id: ae82f21759a714a6b09181e43d7188ce161b0838
prerequisite-patch-id: 2e5b084feebccfac20ec7229d44f752a03378ce1
prerequisite-patch-id: f448c88e715000bd71f3728f4b41cc36966765e6
prerequisite-patch-id: 417d67853ab35049abd2be97e8ce6850397053f2
prerequisite-patch-id: 2b3a925b08e3c5d032536f81d7e899893da6247b
-- 
2.42.0

[PATCH 2/2] public-inbox-init: honor umask when creating config file

[Štěpán Němec]

File::Temp defaults to 0600, which is unsuitable for the
recommended/typical scenario of daemons running unprivileged and with
UID different from $PI_CONFIG owner, as the deamons need to read
$PI_CONFIG.

Respecting umask might end up creating world-unreadable config, too,
but for people who use such umask that's expected behavior.
---
 script/public-inbox-init | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/script/public-inbox-init b/script/public-inbox-init
index 5de4578158fb..b3a16cfbf69d 100755
--- a/script/public-inbox-init
+++ b/script/public-inbox-init
@@ -1,5 +1,5 @@
 #!perl -w
-# Copyright (C) 2014-2021 all contributors <meta@public-inbox.org>
+# Copyright (C) all contributors <meta@public-inbox.org>
 # License: AGPL-3.0+ <https://www.gnu.org/licenses/agpl-3.0.txt>
 use strict;
 use v5.10.1;
@@ -122,7 +122,8 @@ sysopen($lockfh, $lockfile, O_RDWR|O_CREAT|O_EXCL) or do {
 };
 require PublicInbox::OnDestroy;
 my $auto_unlink = PublicInbox::OnDestroy->new($$, sub { unlink $lockfile });
-my ($perm, %seen);
+my $perm = 0644 & ~umask;
+my %seen;
 if (-e $pi_config) {
 	open(my $oh, '<', $pi_config) or die "unable to read $pi_config: $!\n";
 	my @st = stat($oh);
@@ -219,7 +220,7 @@ if (sysopen $fh, $f, O_CREAT|O_EXCL|O_WRONLY) {
 }
 
 # needed for git prior to v2.1.0
-umask(0077) if defined $perm;
+umask(0077);
 
 require PublicInbox::Spawn;
 PublicInbox::Spawn->import(qw(run_die));
@@ -246,10 +247,8 @@ for my $kv (@c_extra) {
 }
 
 # needed for git prior to v2.1.0
-if (defined $perm) {
-	chmod($perm & 07777, $pi_config_tmp) or
-			die "(f)chmod failed on future $pi_config: $!\n";
-}
+chmod($perm & 07777, $pi_config_tmp) or
+	die "(f)chmod failed on future $pi_config: $!\n";
 
 rename $pi_config_tmp, $pi_config or
 	die "failed to rename `$pi_config_tmp' to `$pi_config': $!\n";
-- 
2.42.0

Re: [PATCH 1/2] t/init.t: test newly created config permissions

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> Creating config 0600 disregarding umask breaks scenarios where daemons
> run with credentials different from config owner (but need to read the
> config).

Thanks for noticing.

> Fixed in the next commit.

I strongly prefer tests and fixes to be in the same commit to
simplify `git bisect' usage.  If you really prefer multiple
commits, I think the `TODO' directive of Test::More can help
preserve easy bisectability.  I haven't tried it myself, but I
just confirmed the Test::More man page on CentOS 7.x documents
it.

Can you either resubmit or let me know if you want me to squash
them together on my end?  Thanks again.

Re: [PATCH 1/2] t/init.t: test newly created config permissions

[Štěpán Němec]

On Mon, 28 Aug 2023 21:19:56 +0000
Eric Wong wrote:

> I strongly prefer tests and fixes to be in the same commit to
> simplify `git bisect' usage.  If you really prefer multiple
> commits, I think the `TODO' directive of Test::More can help
> preserve easy bisectability.  I haven't tried it myself, but I
> just confirmed the Test::More man page on CentOS 7.x documents
> it.

Yeah, checking the history I figured as much, but still submitted it
separately for ease of reproduction if nothing else.

> Can you either resubmit or let me know if you want me to squash
> them together on my end?  Thanks again.

Please feel free to squash, let's save some electrons.

Thanks,

  Štěpán

Re: [PATCH 1/2] t/init.t: test newly created config permissions

[Eric Wong]

Štěpán Němec <stepnem@smrk.net> wrote:
> Eric Wong wrote:
> > Can you either resubmit or let me know if you want me to squash
> > them together on my end?  Thanks again.
> 
> Please feel free to squash, let's save some electrons.

Alright, pushed as commit bd0dc6ad0391f811f5248d83538a2eef8f74de95
Thanks again.