i/builtin: add auditd-support interface

[olivercalder]

Add an interface which grants the audit_control capability, along with some required paths for auditd to function properly.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-34214

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.22%. Comparing base (24a0034) to head (af2beeb).
Report is 79 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14811      +/-   ##
==========================================
+ Coverage   78.20%   78.22%   +0.01%     
==========================================
  Files        1151     1157       +6     
  Lines      151396   152850    +1454     
==========================================
+ Hits       118402   119560    +1158     
- Misses      25662    25905     +243     
- Partials     7332     7385      +53     

Flag	Coverage Δ
unittests	78.22% <100.00%> (+0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[olivercalder]

The current capsh calls require CAP_SETPCAP (I think), so setting the CAP_AUDIT_CONTROL capability doesn't actually exercise the CAP_AUDIT_CONTROL capability itself. Instead, we need to attempt to use the CAP_AUDIT_CONTROL. The simplest way I can think to do this is to actually create a real snap containing the auditd package, and then attempt to use the auditctl command to do something like auditctl -r 0.

This requires root and CAP_AUDIT_CONTROL, so this should be just what we need. The auditd service will not be enabled, so we'll get an error message, but the exit code will still be 0:

root@auditctl-testing:~# auditctl -r 0
The audit system is disabled
root@capsh-test:~# echo $?
0

Thus, so this should be an excellent way of probing whether we have access to CAP_AUDIT_CONTROL without actually affecting the system.

The only hard part is that we need auditd to be in a snap. And we can't upload a snap to the store without this interface in place, so we probably need to locally build a snap, then unpack it and include the unpacked squashfs file in the test directory, replacing test-snapd-audit-control.

[olivercalder]

Shellcheck is flipping out about all the horribleness in the auditd dependencies ☠️ I just want it to run the spread test please....

[olivercalder]

It may be helpful to add something which runs /sbin/audisp-syslog as part of the test snap. That may be more mediated by the netlink-audit interface, though, as that one permits CAP_AUDIT_READ and CAP_AUDIT_WRITE.

[olivercalder]

The netlink socket and network added in fd5b03b is needed for auditd to function, as shown here:

Dec 06 13:28:33 autopkgtest kernel: audit: type=1400 audit(1733513313.748:155): apparmor="DENIED" operation="create" class="net" profile="snap.test-snapd-audit-control.audit-rate" pid=15715 comm="auditctl" family="netlink" sock_type="raw" protocol=9 requested="create" denied="create"

[olivercalder]

Once we settle on precisely what needs to be tested (perhaps audisp-syslog in addition to auditctl), we can prune down the files in test-snapd-audit-control to just those actually required by the test apps. There's a bunch of config and systemd service-related stuff at the moment.

[olivercalder]

Spread test is failing because apparently reading /proc/1/loginuid and /proc/1/sessionid doesn't require any special permissions (including any snapd interface).

[alfonsosanchezbeato]

Thanks for the spread test, that is great. However, I do not think we should have the content of the snap with all its binaries, it would be better to simply have snapcraft.yaml and files needed by the recipe in the repo and publish in the store the built snap, possibly registered by the [email protected] user.

Otherwise I have a couple of minor comments.

[alfonsosanchezbeato]

Maybe you could consider using a bash array here

[olivercalder]

I tried this, but it didn't work (shellcheck was mad about snapd-testing-tools). Do you see what would be wrong?

diff --git a/run-checks b/run-checks
index c6b2c9d1e4..5b87c01778 100755
--- a/run-checks
+++ b/run-checks
@@ -242,9 +242,7 @@ if [ "$STATIC" = 1 ]; then
 
     if command -v shellcheck >/dev/null; then
         exclude_tools_path=tests/lib/external/snapd-testing-tools
-        exclude_test_snapd_audit_control_auditd=tests/main/interfaces-audit-control/test-snapd-audit-control/etc/init.d/auditd
-        exclude_test_snapd_audit_control_augenrules=tests/main/interfaces-audit-control/test-snapd-audit-control/sbin/augenrules
-        excluded="$exclude_tools_path $exclude_test_snapd_audit_control_auditd $exclude_test_snapd_audit_control_augenrules"
+        excluded=("$exclude_tools_path" "tests/main/interfaces-audit-control/test-snapd-audit-control/etc/init.d/auditd" "tests/main/interfaces-audit-control/test-snapd-audit-control/sbin/augenrules")
         echo "Checking shell scripts..."
         if [ -n "$CHANGED_FILES" ]; then
             echo "Checking just the changed bash files"
@@ -259,13 +257,7 @@ if [ "$STATIC" = 1 ]; then
         echo "Filtering files"
         FILTERED_FILES=
         for file in $INITIAL_FILES; do
-            include_file=true
-            for excl in $excluded; do
-                if echo "$file" | grep -q "$excl" ; then
-                    include_file=false
-                fi
-            done
-            if [ "$include_file" = true ] ; then
+            if [[ ! "${excluded[*]}" =~ ${file} ]] ; then
                 FILTERED_FILES="$FILTERED_FILES $file"
             fi
         done

[alfonsosanchezbeato]

You will still need the inner for loop, something like this would work:

for excl in "${excluded[@]}"; do
    if [ "$excl" = "$file" ]
    then include_file=false
    fi
done

I mentioned using [[...]] as it supports regex but I think that here the match is actually exact.

[alfonsosanchezbeato]

and compare here using globbing and [[ ... ]]

[olivercalder]

I do not think we should have the content of the snap with all its binaries, it would be better to simply have snapcraft.yaml and files needed by the recipe in the repo and publish in the store the built snap, possibly registered by the [email protected] user.

This sounds good to me, though is it possible to publish a snap to the store which plugs an interface which doesn't yet exist in snapd (or review tools)? My thought is I could strip out everything not strictly required for the minimal spread test (e.g. just keep the auditctl binary and any libraries it loads, strip out all the config and services etc.) and land the interface, then once the interface exists and has been added to review-tools, we could replace the minimal snap with a proper snap in the store which is built for amd64+aarch64, and get better spread coverage.

If there's a way to get a snap in the store which plugs the audit-control interface before it lands in snapd though, that would be great!

[olivercalder]

I suppose an alternative would be for us to build the snap inside the spread test, for now, but I think that's probably worse.

[olivercalder]

I fixed the spread test, by the way. It now tries to read loginuid and sessionid for all processes. This succeeds when the interface is connected and fails when it's not.

[olivercalder]

The plan is to upload test-snapd-audit-control to the snap store so it may be easily installed in spread tests, rather than hard-coding the binary contents here or building the snap dynamically using snapcraft during the spread test.

[olivercalder]

I've requested ownership of the test-snapd-audit-control snap name. Hopefully that can be granted, and the snap can be uploaded to the store. snapcraft happily built the snap with the audit-control interface, even though it doesn't exist yet :) I suspect we may need an override of review-tools when the time comes, for that reason.

The repo for the snap is (for now) here: https://github.com/olivercalder/test-snapd-audit-control

[olivercalder]

Rebased due to testing backend changes on master.

[olivercalder]

Publishing of test-snapd-audit-control snap is currently blocked on store approval of the snap name registration. After that succeeds, I plan to upload the snap to the edge channel, then use that snap directly from the store during the spread test, rather than installing from a local unsquashed snap.

[alfonsosanchezbeato]

Thanks for the changes! Looks good, my only comment is that I would really prefer to have sources for test-snapd-audit-control under tests/lib/snaps/, it is really just 5/6 text files and we would have more control this way, even if we use the one from the store.

[olivercalder]

Thanks for the changes! Looks good, my only comment is that I would really prefer to have sources for test-snapd-audit-control under tests/lib/snaps/, it is really just 5/6 text files and we would have more control this way, even if we use the one from the store.

Do you mean have a copy of the test-snapd-audit-control repo (without VC), so snap/snapcraft.yaml plus the scripts in tests/lib/snaps?? Just for documentation purposes? Because the snap does depend on the auditd package which has loads of extra binaries and config files.

[alfonsosanchezbeato]

Thanks for the changes! Looks good, my only comment is that I would really prefer to have sources for test-snapd-audit-control under tests/lib/snaps/, it is really just 5/6 text files and we would have more control this way, even if we use the one from the store.

Do you mean have a copy of the test-snapd-audit-control repo (without VC), so snap/snapcraft.yaml plus the scripts in tests/lib/snaps?? Just for documentation purposes? Because the snap does depend on the auditd package which has loads of extra binaries and config files.

Yes, I refer to the file in the test-snapd-audit-control repo only, the deb sources/binary packages are already in the Ubuntu archive. I'm trying to avoid having things in personal repos.

[olivercalder]

Thanks for the changes! Looks good, my only comment is that I would really prefer to have sources for test-snapd-audit-control under tests/lib/snaps/, it is really just 5/6 text files and we would have more control this way, even if we use the one from the store.

Do you mean have a copy of the test-snapd-audit-control repo (without VC), so snap/snapcraft.yaml plus the scripts in tests/lib/snaps?? Just for documentation purposes? Because the snap does depend on the auditd package which has loads of extra binaries and config files.

Yes, I refer to the file in the test-snapd-audit-control repo only, the deb sources/binary packages are already in the Ubuntu archive. I'm trying to avoid having things in personal repos.

The plan is to transfer repo ownership to the Canonical org and then move snap ownership as well. Do you think there should be no external repo at all, and instead just have the snapcraft sources in the snapd repo?

Also, I see most snaps which have snapcraft.yaml are tests/lib/snaps/store/test-snapd-*, rather than directly in tests/lib/snaps/test-snapd-* (the latter seem to be mostly local snaps with meta/snap.yaml instead of snap/snapcraft.yaml. Which directory would be more appropriate for this test snap?

[olivercalder]

Rebased to pull in test fixes.

[olivercalder]

This PR is ready for re-review.