750 log directory permissions

[carlwgeorge]

I'm working on packaging recap for Fedora/EPEL. Rpmlint complains about the 750 permissions on the log directories.

recap.noarch: E: non-standard-dir-perm /var/log/recap 750
recap.noarch: E: non-standard-dir-perm /var/log/recap/backups 750
recap.noarch: E: non-standard-dir-perm /var/log/recap/snapshots 750

These were set this way in 2012. Are these permissions actually necessary? Is any recap output actually sensitive?

References:

• https://github.com/rackerlabs/recap/blob/1.0.0/Makefile#L42-L44
• https://github.com/rackerlabs/recap/blob/1.0.0/src/recap#L88-L89
• https://github.com/rackerlabs/recap/blob/1.0.0/CHANGELOG.md#092---2012-11-01

[tonyskapunk]

@carlwgeorge noted the same while working on #94, I was initially to make the changes in there but don't want to hold for those changes, here the PR #99, although, I'm doing the changes to development as I want to implement gitflow to this repo, as soon as I can perform the fix to #94 I'll release a new version to master.

[piersc]

recap can record things that wouldn't otherwise be visible to a non-superuser, e.g. the process associated with a connection in netstat output or the MySQL process list. For this reason the logs probably shouldn't be visible to non-root.

[tonyskapunk]

@piersc that's a good point, in any case we can drop those from /var/log/recap/{backups,snapshots} but leave them on /var/log/recap. @carlwgeorge how mariadbNNNu handles these perms for /var/log/mysql that would be a similar case... if that's handled via rpm scripts then what would be a good compromise?

[carlwgeorge]

Yes, it's enforced in the %files server section of the spec file.

%attr(0750,mysql,mysql) %dir %{_localstatedir}/log/mariadb

[tonyskapunk]

I changed back the perms(b9150b6) on /var/log/recap/ (BASEDIR).

I'd think this should still complain on the perms(?). But I guess that can be "fixed" with:
%attr{0750} %dir %{_localstatedir}/log/recap

btw, I ran rpmlint with the current code and the suggestion above and all I got is a warning:

$ rpmlint util/packaging/rpm/recap.spec 
util/packaging/rpm/recap.spec: W: no-%build-section
0 packages and 1 specfiles checked; 0 errors, 1 warnings.

Using:
$ rpmlint --version
rpmlint version 1.9 Copyright (C) 1999-2007 Frederic Lepied, Mandriva

[carlwgeorge]

rpmlint complains on the RPM itself (recap.noarch in the original comment), not the spec file (which would have shown as recap.spec). It is not required to eliminate all rpmlint warnings. In the package review it was just requested to start the discussion here and hopefully get the justification documented somewhere.

[tonyskapunk]

Yeah, I agree with @piersc we need to retain 0750 for the /var/log/recap (default) dir. Due to information that would be obtained only under elevated privileges.

Out of curiosity, I tried rpmlint on mariadb101u and despite the spec file showing similar attrs I don't see a warning in there.

$ rpmlint mariadb101u-server-utils-10.1.24-1.ius.centos7.x86_64.rpm 2>&1 |
    grep -c dir-perm
0

With that being said, does anybody have any other comment about the changes? if not, I will merge #99

[carlwgeorge]

That dir is owned by -server, not -server-utils.

# rpmlint mariadb101u-server-10.1.24-1.ius.centos7.x86_64.rpm |& grep dir-perm
mariadb101u-server.x86_64: E: non-standard-dir-perm /var/log/mariadb 750

Regardless, it isn't a blocker. The perms can stay they way they are. What is the point of merging #99?

[tonyskapunk]

That dir is owned by -server, not -server-utils.

Ah, that's why, thanks!

What is the point of merging #99?

The only effective changes in there are the perms in directories inside /var/log/recap (snapshots,backups) from 750 to 755, and removing the hard-coded perms in recap, perms are only set in Makefile

[carlwgeorge]

Whoops, I didnt' scroll to the end to see the changes in src/recap, for some reason I thought it was only changes to the Makefile. I also noticed that it changes the script permissions from 755 to 750. I don't think this is necessary, since the script already checks if it is running as root. Changing /var/log/recap/{snapshots,backups} to 750 is fine but also not necessary. My opinion is we should just remove the OUTPUTDIR_MODE stuff from the script and call it good.

[tonyskapunk]

I'm OK with the change on 755 to the directories inside /var/log/recap the PR already includes those, thus is less effort to go with those changes than change them back, going to squash the commits into a single one and we can go with those changes then.

[tonyskapunk]

Closing #99 has been merged.