Re: [pve-devel] [PATCH v2 master ceph 01/11] debian: add patch to fix ceph crash dir permissions in postinst hook

[Max Carrara <m.carrara@proxmox.com>]

On 2/12/24 14:32, Fabian Grünbichler wrote:
> On February 5, 2024 6:54 pm, Max Carrara wrote:
>> Ceph has a postinst hook that sets the ownership of '/var/lib/ceph/*'
>> to ceph:ceph (in our case), but misses out on '/var/lib/ceph/crash/posted'.
>>
>> This patch therefore also updates the permissions of '/var/lib/ceph/*/*'.
>>
>> Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> ---
>> Changes v1 --> v2:
>>   * use `find` instead of for-loop
>>
>>  ...rmissions-of-subdirectories-of-var-l.patch | 50 +++++++++++++++++++
>>  patches/series                                |  1 +
>>  2 files changed, 51 insertions(+)
>>  create mode 100644 patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>>
>> diff --git a/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> new file mode 100644
>> index 000000000..7445f3945
>> --- /dev/null
>> +++ b/patches/0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> @@ -0,0 +1,50 @@
>> +From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
>> +From: Max Carrara <m.carrara@proxmox.com>
>> +Date: Thu, 1 Feb 2024 18:43:36 +0100
>> +Subject: [PATCH] debian: adjust permissions of subdirectories of /var/lib/ceph
>> +
>> +A rather recent PR made ceph-crash run as "ceph" user instead of
>> +root [0]. However, because /var/lib/ceph/crash/posted belongs to root,
>> +ceph-crash cannot actually post any crash logs now.
>> +
>> +This commit fixes this by also updating the permissions of
>> +/var/lib/ceph/*/* - the subdirectories and files of the directories in
>> +/var/lib/ceph - by using `find` instead of a loop over a glob pattern.
>> +
>> +[0]: https://github.com/ceph/ceph/pull/48713
>> +
>> +Signed-off-by: Max Carrara <m.carrara@proxmox.com>
>> +---
>> + debian/ceph-base.postinst | 16 +++++++++-------
>> + 1 file changed, 9 insertions(+), 7 deletions(-)
>> +
>> +diff --git a/debian/ceph-base.postinst b/debian/ceph-base.postinst
>> +index 75eeb59c624..70d07977f82 100644
>> +--- a/debian/ceph-base.postinst
>> ++++ b/debian/ceph-base.postinst
>> +@@ -33,13 +33,15 @@ case "$1" in
>> + 	rm -f /etc/init/ceph.conf
>> + 	[ -x /sbin/start ] && start ceph-all || :
>> + 
>> +-        # adjust file and directory permissions
>> +-	for DIR in /var/lib/ceph/* ; do
>> +-	    if ! dpkg-statoverride --list $DIR >/dev/null
>> +-	    then
>> +-		chown $SERVER_USER:$SERVER_GROUP $DIR
>> +-	    fi
>> +-	done
>> ++	PERM_COMMAND="dpkg-statoverride --list {} > /dev/null || chown ${SERVER_USER}:${SERVER_GROUP} {}"
> 
> this doesn't quote {} properly, files with spaces or other things in
> them can cause issues including shell command injection as root when
> this is executed by dpkg..

Noted, will be corrected in v3!

> 
>> ++
>> ++	# adjust directory permissions
>> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type d -print0 \
>> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> ++
>> ++	# adjust file permissions
>> ++	find /var/lib/ceph -mindepth 1 -maxdepth 2 -type f -print0 \
>> ++	    | xargs -0 -I '{}' sh -c "${PERM_COMMAND}"
>> +     ;;
> 
> do we need the depth stuff or the split by type? we want to make
> everything under /var/lib/ceph owned by ceph:ceph, unless the admin has
> specifically overriden a particular path.

I added `-mindepth 1` because the '/var/lib/ceph' dir would be included in
`find`'s results otherwise, which isn't the case in the original code.
`-maxdepth 2` is to limit it to the depth that I had originally intended in
v1 - files and subdirectories, as well as sub-subdirectories and files in
those.

I agree that everything (in those two levels?) could just be `chown`ed
instead though - would make the above a little less verbose and only
need one invocation.

> 
> a simple 
> 
> find /var/lib/ceph -print0 | ...
> 
> should work and be much simpler (or if we want to limit to dirs and
> files, that can also be simply done in one go by ORing the two checks)

ORing the two checks didn't work in my case - turns out I just held `find`
wrongly.

Will correct all the above in v3 - thanks for your feedback!

> 
>> +     abort-upgrade|abort-remove|abort-deconfigure)
>> + 	:
>> +-- 
>> +2.39.2
>> +
>> diff --git a/patches/series b/patches/series
>> index 865caf23d..cf8f1ea31 100644
>> --- a/patches/series
>> +++ b/patches/series
>> @@ -12,3 +12,4 @@
>>  0012-backport-mgr-dashboard-simplify-authentication-proto.patch
>>  0013-mgr-dashboard-remove-ability-to-create-and-check-TLS.patch
>>  0014-rocksb-inherit-parent-cmake-cxx-flags.patch
>> +0015-debian-adjust-permissions-of-subdirectories-of-var-l.patch
>> -- 
>> 2.39.2
>>
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>>
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>