Add spec file

[SpareSimian]

Needs an RPM spec file to maintain file ownership on RPM-based distros. Here's a rudimentary one that I can build on Rocky 8. nmutils.spec.txt

[SpareSimian]

(Since the project lacks a version number, I gave it an arbitrary one of 0.1. It would be useful to give it an official one and tag the project to make packaging a little easier.)

[SpareSimian]

I forgot to make the dispatcher scripts executable in first release. This fixes that. nmutils.spec.txt

[sshambar]

Very nice spec file... I was wondering if when installing with RPM it might be more friendly to:

• place the dispatchers in /usr/lib (so they can be overridden in /etc)
• leave the non-conf files in /etc/nmutils as regular config so that they are replaced on upgrade (but edits are kept as .rpmsave)
• adapt the files to actually use %{_sysconfdir} and %{prefix}/lib as distributed defaults

I guess if since the spec file would call for versioning, I should probably create a release to go with it... Here's a suggested spec file that includes these changes... see if that looks ok to you. nmutils.spec.txt

[SpareSimian]

Consider using libexec for the function libraries in /etc/nmutils. As you say, leave the conf files in /etc/nmutils. https://unix.stackexchange.com/questions/312146/what-is-the-purpose-of-usr-libexec

I wasn't aware of /usr/lib/NetworkManager/dispatcher.d. Is that like the systemd/system directory for units? Where a file in /usr/lib is ignored if one of the same name is found in /etc? If so, that would be the right place to put the stock dispatcher scripts. I do prefer mechanisms that move customizations to separate files that don't get overwritten on updates or need customization edits migrated and it sounds like this accomplishes that.

There are rpm macros for /usr/lib and /usr/libexec: https://docs.fedoraproject.org/en-US/packaging-guidelines/RPMMacros/

[sshambar]

FHS lists libexec as for "internal binaries that are not intended to be executed directly by users or shell scripts." Since the current /etc/nmutils files are just sourced (not executed, ie no #!/bin/bash at the top, or +x perms), they actually might be a better candidate for /usr/share (as they are platform independent, non-executable).

The dispatcher scripts appear to follow the systemd override logic between the same names in etc/usr - the NetworkManager-dispatcher man page isn't clear on the subject, but it works in my tests :).

I checked the rpm macros but couldn't find anything apart from %prefix for the /usr/lib/NetworkManager... as %_libdir is /usr/lib64 on x86_64.

[SpareSimian]

Agreed that /usr/share makes more sense, similar to where web-based packages put their scripts.

Good point about lib64. That's a NM packaging issue. You can see that systemd addressed it with the _unitdir macro. I wonder if anyone has suggested something like an _nm_scriptdir RPM macro?

[sshambar]

My thoughts on /usr/share were to install the "official" script libraries there, and then symlink them to /etc/nmutils where they could (optionally) be replaced/edited by the admin... upgrades would need to handle edits (.rpmsave?) so that new script versions can at least depend on version compatibility. Alternatively, script-library edits in /etc/nmutils could be ignored with the assumption that the admin at least makes an effort to source the /usr/share version :)

[SpareSimian]

I started a thread here on the macros and found the macros used in NetworkManager's own spec file. Perhaps those should be hoisted into the spec file here? https://forums.rockylinux.org/t/rpm-for-networkmanager-utilities-for-gateway-configurations/6132

[sshambar]

I did use %{_prefix} is the previous .spec file uploaded above... not sure it's worth defining a %global for it as NM does as it doesn't get used much :)

Here's the latest version of the spec file with modifications to put libraries in /usr/share, and symlink them to /etc/nmutils on install/upgrade (saving any edits in .rpmsave). I also cleaned up the systemd ddns-onboot@ handling. nmutils.spec.txt - I ran a few upgrade tests and things appear to work as expected (keep in mind %preun runs from the old-package spec when testing!)

[sshambar]

I rewrote the spec file to actually patch the install/config locations so that the symlink hack isn't needed. Installed scripts (/usr/share, /usr/lib/...) will default to using their own library versions, but still use the global /etc/nmutils/conf files. Edits/overrides can be placed in /etc/NetworkManager, and they may of course be modified to pull functions from edited libraries in /etc/nmutils (if desired).

I also added the SELinux module with rules to allow NM scripts to spawn dhclient, and dhclient scripts to manage radvd and nsupdate/dig. The spec file also builds nmutils-selinux as a subpackage so it can be conditionally installed if needed.

The rpm build now also runs the test suite (unless rpmbuild --nocheck is used) on the buildroot scripts/libraries to make sure things appear to be installed corrrectly.

Give it a try, and let me know if you have any changes... if it looks good, I'll create a release :)

[sshambar]

Hold on checking... I think there's a bug in the sed script I need to fix first.

[sshambar]

OK, I've updated the scripts to not set NMCONF (general-functions defaults it), and fixed the "sed" in the spec to correctly edit the files with the new paths. Please give it a try!

[SpareSimian]

If I change the version to "master", this works for me:

wget -nd -c https://github.com/sshambar/nmutils/archive/master/nmutils-master.tar.gz
rpmbuild -ba --nocheck ~buildmeister/rpmbuild/BUILD/nmutils/nmutils.spec

If I leave tests enabled, it fails. There's no tag for version 20220603 so wget can't find that version on GitHub.

[sshambar]

Yeah, I haven't created a release... if you want to build a release, just tar up the source in a directory called nmutils-20220603 and you're good to go. If you'd like, I can just create a release and you can try pulling it directly, but I was going to wait until you'd had a chance to test.

[sshambar]

Your checks might have failed because you're using a "patched" spec file (in the BUILD directory)... try pulling the spec directly from the source (and edit the version), and let me know if you still have any problems. If it works for you, I'll publish a release!

[SpareSimian]

Changing the tarball name and version back didn't help. Here's my build log showing the test failures. Could I be missing a prerequisite that the tests depend on? https://pastebin.com/S6xbBG79

[sshambar]

Interesting... I think the "strict mode" errors might be related to the bash version in the path, could you add a "bash --version" to the test/Makefile (perhaps general: rule) and see which version is actually being used? I can then try to reproduce the error with that version...

[SpareSimian]

GNU bash, version 4.4.20(1)-release (x86_64-redhat-linux-gnu)

This seems to be the latest for RHEL/CentOS/Rocky 8.

[sshambar]

OK, I'll build a Rocky 8 VM and see if I can't get to the bottom of this :)

[sshambar]

OK, I had a bug in the testing's "strict function whitelisting" - but fixing that exposed a bug in bash4's variable expansion in "=~" comparisons... so I updated shtest_setup to work around it.

Also found a few edge-case problems with assoc array comparisons (bash4 ordering was not the same as bash3/5), so I've updated shtest_setup to strictly order the array indexes in comparisons. I also sped up the array_copy function...

With those fixes, I was able to run the rpmbuild on Rocky 8 with tests, so give it a try and see if you still have any problems :)

[SpareSimian]

I'm now able to run "rpmbuild -bb" to completion with no test errors! The RPMs (main and selinux) install fine. My next challenge will be to configure and test the interfaces, but I think this makes the spec file ready for a general audience.

[frankcrawford]

Okay, I'll join this party too. :-)

I've pulled down the latest version and tried it out. I've update the spec file to pull commits, rather than final versions (do you want me to do a PR for it)? The details are here https://docs.fedoraproject.org/en-US/packaging-guidelines/SourceURL/.

Anyway, built it on Fedora 36, it fails with F45f F46f F48 F48f. Any ideas what may be going on there, before I investigate it further.

[sshambar]

I'm hoping the test failures are just filename ordering issues - I've added LC_ALL=C to the wildcard testing (but with names like xx.match,yy.match I didn't think that was necessary). If you still get test errors, please paste the test failure here :)

I was actually planning on creating a release (so the specfile source can reference it directly), but I wanted to get enough working builds first.

I also added a systemd-rpm-macros build requires as that appears to be necessary on F36 when I mock build it. I didn't get the test failures under mock though, so I am curious what errors you were seeing.

[frankcrawford]

Really interesting, the test issues seem to only occur on NFS, not on local filesystems. On NFS I get the following issue:

 F45  OK   | nmg::foreach_filematch(<2 matches>) - returns true
 F45f FAIL | nmg::foreach_filematch(<2 matches>) - makes callbacks (reg 1)
diff: <expected> <found>
1,2c1
< ./results/xx.match xx
< ./results/yy.match yy
 ---
> 
 F46  OK   | nmg::foreach_filematch(<2 matches>) - returns true
 F46f FAIL | nmg::foreach_filematch(<2 matches>) - makes callbacks (reg 1)
diff: <expected> <found>
1,2c1
< ./results/xx.match xx arg
< ./results/yy.match yy arg
 ---
> 
 F47  OK   | nmg::foreach_filematch(<no-pat>) - returns true
 F47f OK   | nmg::foreach_filematch(<no-pat>) - makes no callbacks
 F48  FAIL | nmg::foreach_filematch(<2 matches, callback fails>) - returns fail code
    expected: 3
       found: 0
 F48f FAIL | nmg::foreach_filematch(<2 matches, callback fails>) - makes 1 callback (reg 1)
    expected: './results/xx.match xx fail'
       found: ''

but the issue does not occur on local (ext4) filesystem.

Of note the failed tests are tose that involve writing 2 empty files and comparing them.

[frankcrawford]

Okay, pulling in your latest changes doesn't seems to fix those errors on NFS, but a new one sometimes appears, ipv6-prefix-nm-test L2F fails, if the others seem to succeed, which happens about 1 in 5 runs, again it seems to be NFS related.

As this looks to be entirely related to NFS caching, and that shouldn't be the case for a normal installation, I'd say just ignore it. I can build on the server, so it doesn't stop me, and I can push it all to COPR anyway.

[frankcrawford]

FYI, I built the latest in github on COPR with no issues for Fedora Rawhide (F37). You can see it here https://copr.fedorainfracloud.org/coprs/frankcrawford/nmutils/

[sshambar]

Really interesting, the test issues seem to only occur on NFS, not on local filesystems. On NFS I get the following issue:

That really is fascinating! I'm curious if it's a quirk with your nfs server, or a bug in my wildcard handling that could affect others (for example) using a nfs mounted root filesystem...

I setup a nfs mount and tried on my fedora server, but couldn't reproduce it... could you edit the general-test file and add the following around line 1120 (just below the printf that creates the empty files):

echo "$TEST_OUT/xx.match"
/bin/ls "$TEST_OUT"
for arg in "$TEST_OUT/"*.match; do /bin/ls -l "$arg"; done

and then run ./general-test +F45*

[frankcrawford]

I've taken this to a new issue #9 as it is independent of the spec file creation.

[frankcrawford]

Back to spec file issues, I've run a build of the current spec file on COPR and while F34, F35, F36, rawhide, EPEL8 and EPEL9 work, EPEL7 fails due to rpmbuild being old, and giving the following error:

error: line 36: Dependency tokens must begin with alpha-numeric, '_' or '/': Requires: (selinux-policy >= %{_selinux_policy_version} if selinux-policy-targeted)

From memory systemd-rpm-macros does not exist in RHEL7/EPEL7, and you need to just add a BuildRequires: systemd in that case.

[sshambar]

OK, I've updated the spec to handle epel7 and also pre-4.13 rpmbuild boolean conditionals, and made selinux a build condition parameter.

In building for epel7, I discovered some old bash3 quoting behavior in bash 4.2 used there, and patched to 95-radvd-gen to handle it.

Let me know if you see any other build issues!

[sshambar]

Well, I was about to label the release... but then tried the scripts on Fedora 36, and of course NetworkManager has decided to change all their SELinux labels, which makes the policy file non-functional... I'm going to play with a couple solutions (either specifically labeling the scripts as they were before, or creating new allow rules). Once I have something that works on F36 as well as older NetworkManagers, I'll push a new update...

[frankcrawford]

That is annoying. I had noticed the SELinux issues, and was going to look at it sometime, but hadn't really got around to reviewing them.

I'll have a little look at them over the weekend and see if I can tell what is going on.

[sshambar]

It's exceedingly annoying, as it totally changes how the scripts transition between domains, and what is allowed to execute them (eg, systemd can no longer run them).

I think I may have to explore creating a dedicated domain for the nmutils scripts so that the permissions are explicit and compatible between old and new policies >sigh<

[sshambar]

I've been having fun with NetworkManager's new SELinux policy... the F36 release runs dispatchers in a very restrictive domain (NetworkManager_dispatcher_custom_t, where before they were in the initrc_t domain, which is basically unrestricted). I played for awhile with creating a new "nmutils_t" domain to run the scripts, but the number of fiddly permissions needed to handle everything from dhclient, ip config, radvd, and systemctl was proving "fragile" -- in the end I fell back on creating a new label for the NM dispatchers (nmutils_exec_t), which transitions to initrc_t (as before), and everything works correctly with the minimum of fuss...

In order to make the labeling work correctly though, I needed to change the interface-dispatcher (now renamed dispatcher_action) so that it is sourced from a minimal dispatcher file, which is named ##-ifd- so that the SELinux policy can correctly label it. The new system should allow the dispatcher to automatically benefit from upgrades too, which is nice.

The latest nmutils.spec file includes all these changes, and uses the new selinux/Makefile too.

I've tested on various Fedoras (and several other EPEL targets) and don't see any SELinux errors, but if you get a chance give the new build a try!

(I'm struggling to get Rocky-8 mock builds to work atm, something to do with the TLS certificates on mirror sites, I'll keep working on testing that though...)

[frankcrawford]

I'm not certain, but I think there is an issue with the post-install script for the SELinux policy. You don't actually set the file context at any point, and more particularly, you don't change it, if they change. You may need something like: restorecon -R /usr/lib/NetworkManager/dispatcher.d || :

I noticed this after building and applying the latest update and it was still logging failures (in permissive mode) until I ran a restorecon across the files. Prior to that they had the NetworkManager_dispatcher_custom_tstill.

[sshambar]

Which target are you building for?

There are standard selinux macros in the pre/posttrans sections called %selinux_relabel_pre/post - if you expand the macros you can see the pre macro saves the file contexts, and the post macro passes them to the fixfiles command to correctly label them after the new semodule has been installed.

You might check and make sure the version you installed are using the macros (try rpm -q --scripts nmutils-selinux)

• if they aren't present, then I'll need to find out what's different on the target you're building for
• if they are present, but not working, then there may be a bug in the distribution's version of the macros

The macros are used in quite a number of other packages (eg mysql-selinux, memcache-selinux) so they should be well tested... but there might be something different on your target...

[frankcrawford]

The version is built on COPR for Fedora F36, and the scripts run are:

preinstall scriptlet (using /bin/sh):

if /usr/sbin/selinuxenabled; then 
  if [ -e /etc/selinux/config ]; then 
    . /etc/selinux/config 
  fi 
  _policytype=targeted 
  if [ -z "${_policytype}" ]; then 
    _policytype="targeted" 
  fi 
  if [ "${SELINUXTYPE}" = "${_policytype}" ]; then 
    [ -f /var/lib/rpm-state/file_contexts.pre ] || cp -f /etc/selinux/${SELINUXTYPE}/contexts/files/file_contexts /var/lib/rpm-state/file_contexts.pre 
  fi 
fi
postinstall scriptlet (using /bin/sh):

if [ -e /etc/selinux/config ]; then 
  . /etc/selinux/config 
fi 
_policytype=targeted 
if [ -z "${_policytype}" ]; then 
  _policytype="targeted" 
fi 
if [ "${SELINUXTYPE}" = "${_policytype}" ]; then 
  /usr/sbin/semodule -n -s ${_policytype} -X 200 -i /usr/share/selinux/packages/targeted/nmutils.pp.bz2 || : || : 
  /usr/sbin/selinuxenabled && /usr/sbin/load_policy || : 
fi
postuninstall scriptlet (using /bin/sh):
if [ $1 -eq 0 ]; then
  # Package removal, not upgrade

if [ -e /etc/selinux/config ]; then 
  . /etc/selinux/config 
fi 
_policytype=targeted 
if [ -z "${_policytype}" ]; then 
  _policytype="targeted" 
fi 
if [ $1 -eq 0 ]; then 
  if [ "${SELINUXTYPE}" = "${_policytype}" ]; then 
    /usr/sbin/semodule -n -X 200 -s ${_policytype} -r nmutils || : &> /dev/null || : 
    /usr/sbin/selinuxenabled && /usr/sbin/load_policy || : 
  fi 
fi 

fi
posttrans scriptlet (using /bin/sh):

if [ -e /etc/selinux/config ]; then 
  . /etc/selinux/config 
fi 
_policytype=targeted 
if [ -z "${_policytype}" ]; then 
  _policytype="targeted" 
fi 
if /usr/sbin/selinuxenabled && [ "${SELINUXTYPE}" = "${_policytype}" ]; then 
   if [ -f /var/lib/rpm-state/file_contexts.pre ]; then 
     /usr/sbin/fixfiles -C /var/lib/rpm-state/file_contexts.pre restore &> /dev/null 
     rm -f /var/lib/rpm-state/file_contexts.pre 
   fi 
fi

If you look, there is nothing actually setting the context on the files after installation. I suspect a following update, that changed the files, or a global setting of contexts would fix it (and that is why I noticed it), but nothing does it at the time of installation.

An old document showing what to add is here https://fedoraproject.org/wiki/PackagingDrafts/SELinux#File_contexts however, it predates most of the macros.

BTW, part of the issue was also probably that I had your earlier SELinux module installed, and the clean up was probably not as clean as you would like.

[SpareSimian]

A couple pages addressing setting file attributes: https://unix.stackexchange.com/questions/469134/what-is-the-proper-way-to-set-selinux-context-in-an-rpm-spec-rhel7-in-2018 https://lukas-vrabec.com/index.php/2017/03/18/using-rpm-macros-in-product-selinux-subpackages/

[sshambar]

Actually, the macros look like they should work correctly...it may be that something in the old selinux package macros messed up the upgrade...

/usr/sbin/fixfiles -C /var/lib/rpm-state/file_contexts.pre restore &> /dev/null

..should have taken the differences between the pre and post install contexts and fixed the changed files (much faster than a full restorecon)... You might try again as both a fresh install and then an upgrade, and see if things don't work as expected (and make sure there's no manually installed override nmutils module (semanage module -l showing nmutils with 400 priority) or conflicting local context changes (semanage fcontext -C -l)

[frankcrawford]

Yes, that is possibly what i would have thought, but they still had the old context, so it didn't seem to set them.

It probably had something to do with a conflict with your previous SELinux attempt. I wonder if there may have been some form of collision between the /var/lib/rpm-state/file_contexts.pre file between the install and then remove scriptlets before the fixfiles ran.

Also, I don't think it removed the previous SELinux module (with priority 400) as I had to do that manually.

So, if you think the current scriptlets match current best practice, then I'm happy to put it down to an issue with the previous version.

[sshambar]

Also, I don't think it removed the previous SELinux module (with priority 400) as I had to do that manually.

Yeah, that pretty much explains eveything -- I'll explain in detail below, feel free to skip if you're not curious :)

Package SELinux modules are installed at priority 200, and are designed to override/mask the base policy (if any) with the same name at priority 100.

However, the admin can manually install their own module with the same name (default priority 400) which will override/mask both base and package modules, and offer the admin options to fix or change those modules (and should not be removed/changed by the package :)

If you had a priority 400 nmutils module installed, it masked the package module (which includes masking the file context added in the new package module), and would have prevented the package scripts from seeing any context changes. Once that 400 module was removed, then the restorecon would have correctly honored the package's file contexts.

In short, it appears the scripts are probably doing their job -- if they appear to be working (no SELinux denials?) and also working, then I think it may be a good time to actually create an "official" release 🥂

[frankcrawford]

Thanks for the explanation.

Yes, now the official scripts look to be working correctly, as I am no longer getting SELinux denials, so it is worth doing a release.

[sshambar]

I've tagged a release, let me know if you have any trouble using it!

[frankcrawford]

Built with no issues on COPR. You can grab the release from https://copr.fedorainfracloud.org/coprs/frankcrawford/nmutils/ for most common RPM based systems.

[sshambar]

Cool... looks good and installed easily on my Fedora 35 test VM... (had to directly specify 20220621-1 though, as you have a "-2" build that installs by default :).

The build did require my VM to upgrade to the latest selinux-policy packages, but after looking at the rpm macros I guess that's just part of the standard "%selinux_requires" behavior, and is expected.

[frankcrawford]

Ahh, sorry about that, I'll clear out the old one, although it is exactly the same except for the spec file, and that it pulled from the repo prior to tagging.

[frankcrawford]

It should now be fixed.

[sshambar]

Works great... any other issues with the spec file, or should I close it?

[frankcrawford]

I'm happy with it now, so feel free to close the issue.

[SpareSimian]

Agreed. We can open a new one if any quirks show up. I can build it from the tarball with "rpmbuild -ta" and install the two resulting RPMs using dnf. I still need to configure and test the actual scripts, but that's a separate matter.