Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More Robust aa-status Output Parser #35

Merged
merged 3 commits into from
Jan 24, 2019
Merged

More Robust aa-status Output Parser #35

merged 3 commits into from
Jan 24, 2019

Conversation

shundhammer
Copy link
Contributor

@shundhammer shundhammer commented Jan 23, 2019

Trello

https://trello.com/c/3d3oJxcu/612-1121274-build-20190105-internal-error-when-trying-to-edit-settings-in-yast-apparrmor-module

Bugzilla

https://bugzilla.suse.com/show_bug.cgi?id=1121274

Problem

AppArmor profiles can also have a short name, not just the full path of the binary that is confined in AppArmor.

There was a recent change in Tumbleweed that appears to use that more heavily, and the old parser of this YaST AppArmor module was not quite robust enough to handle that; it would not find the correct profile anymore, and it tried to access a nonexistent hash key in the hash of all profiles, resulting in a nil:NilClass (NoMethodError) exception.

Sample JSON output

sudo aa-status --pretty-json
{
    "version": "1",
    "profiles": {
        "/usr/bin/lessopen.sh": "enforce",
        "/usr/lib/apache2/mpm-prefork/apache2": "enforce",
        "/usr/lib/apache2/mpm-prefork/apache2//DEFAULT_URI": "enforce",
        "/usr/lib/apache2/mpm-prefork/apache2//HANDLING_UNTRUSTED_INPUT": "enforce",
        "/usr/lib/apache2/mpm-prefork/apache2//phpsysinfo": "enforce",
        "/usr/lib/colord": "enforce",
        "/usr/lib/dovecot/anvil": "enforce",
        "/usr/lib/dovecot/auth": "enforce",
        "/usr/lib/dovecot/config": "enforce",
        "/usr/lib/dovecot/deliver": "enforce",
        "/usr/lib/dovecot/dict": "enforce",
        "/usr/lib/dovecot/dovecot-auth": "enforce",
        "/usr/lib/dovecot/dovecot-lda": "enforce",
        "/usr/lib/dovecot/dovecot-lda//sendmail": "enforce",
        "/usr/lib/dovecot/imap": "enforce",
        "/usr/lib/dovecot/imap-login": "enforce",
        "/usr/lib/dovecot/lmtp": "enforce",
        "/usr/lib/dovecot/log": "enforce",
        "/usr/lib/dovecot/managesieve": "enforce",
        "/usr/lib/dovecot/managesieve-login": "enforce",
        "/usr/lib/dovecot/pop3": "enforce",
        "/usr/lib/dovecot/pop3-login": "enforce",
        "/usr/lib/dovecot/ssl-params": "enforce",
        "/usr/lib/dovecot/stats": "enforce",
        "/usr/{bin,sbin}/dnsmasq": "enforce",
        "/usr/{bin,sbin}/dnsmasq//libvirt_leaseshelper": "enforce",
        "apache2": "enforce",
        "apache2//DEFAULT_URI": "enforce",
        "apache2//HANDLING_UNTRUSTED_INPUT": "enforce",
        "apache2//phpsysinfo": "enforce",
        "avahi-daemon": "enforce",
        "dovecot": "enforce",
        "identd": "enforce",
        "klogd": "enforce",
        "mdnsd": "enforce",
        "nmbd": "enforce",
        "nscd": "enforce",
        "ntpd": "enforce",
        "nvidia_modprobe": "enforce",
        "nvidia_modprobe//kmod": "enforce",
        "ping": "enforce",
        "smbd": "enforce",
        "smbldap-useradd": "enforce",
        "smbldap-useradd///etc/init.d/nscd": "enforce",
        "syslog-ng": "enforce",
        "syslogd": "enforce",
        "traceroute": "enforce",
        "winbindd": "enforce"
    },
    "processes": {
        "/usr/sbin/nscd": [
            {
                "profile": "nscd",
                "pid": "805",
                "status": "enforce"
            }
        ],
        "/usr/sbin/avahi-daemon": [
            {
                "profile": "avahi-daemon",
                "pid": "806",
                "status": "enforce"
            }
        ],
        "/usr/lib/colord": [
            {
                "profile": "/usr/lib/colord",
                "pid": "1790",
                "status": "enforce"
            }
        ]
    }
}

Notice the entries for nscd:

{
    "profiles": {
        ...
        "nscd": "enforce",
        ...
    },
    "processes": {
        "/usr/sbin/nscd": [
            {
                "profile": "nscd",
                "pid": "805",
                "status": "enforce"
            }
        ],
        ...
   }
}

In profiles the name is just nscd. In processes there is an entry for the /usr/sbin/nscd executable, but the corresponding profile name is in profile in the hash in the list of processes.

The profile /etc/apparmor.d/usr.sbin.nscd looks like this:

#include <tunables/global>
profile nscd /usr/{bin,sbin}/nscd {
  ...
}

I.e. there is the (optional!) name nscd and a shell glob pattern for the binaries that are to be confined in AppArmor.

Fix

  • Extended the parser accordingly

  • Modularized the parser

  • Changed the output format from --json to --pretty-json so it is actually human-readable, not only machine-readable

  • Added logging for easier future debugging

Test

On a fresh installation of the most recent Tumbleweed (from 2091-01-21), the problem was easy to reproduce.

With the fix, it's now working fine.

@shundhammer shundhammer changed the title WIP: More robust aa-status output parser WIP: More Robust aa-status Output Parser Jan 23, 2019
@shundhammer
Copy link
Contributor Author

@goldwynr: Care to take a look at this?

@shundhammer shundhammer changed the title WIP: More Robust aa-status Output Parser More Robust aa-status Output Parser Jan 24, 2019
@goldwynr
Copy link
Collaborator

This change does not seem to be backward compatible with Leap 15.0 apparmor. The reason I requested for a apparmor JSON version update.

@shundhammer
Copy link
Contributor Author

@goldwynr: Thank you for checking this.

I tried to find out with what version of AppArmor that change was released, but I can't make sense of that repo; I see the commit and the merge, but I don't see any tags or a change log.

I would have added a "Requires: apparmor-utils > xy" to the spec file of this YaST module, but I'd need a version number for that.

@shundhammer
Copy link
Contributor Author

https://build.opensuse.org/request/show/598829

  • update to AppArmor 2.13
    • add support for multiple cache directories and cache overlays
      (boo#1069906, boo#1074429)
    • add support for conditional includes in policy
    • remove group restrictions from aa-notify (boo#1058787)
    • aa-complain etc.: set flags for profiles represented by a glob
    • aa-status: split profile from exec name
    • several profile and abstraction updates
    • see https://gitlab.com/apparmor/apparmor/wikis/Release_Notes_2.13
      for the detailed upstream changelog

So we need to require apparmor-utils >= 2.13.

@shundhammer
Copy link
Contributor Author

Rather than requiring apparmor-utils >= 2.13, it is more conservative to conflict with apparmor-utils < 2.13; otherwise the package will always drag in apparmor-utils and apparmor and whatnot. This might not be desirable since yast2-apparmor might be part of a some "yast stuff" sub-pattern.

@goldwynr
Copy link
Collaborator

Honestly, I would like apparmor to update the version number conveyed in JSON format, namely in apparmor/utils/apparmor/ui.py where set_json_mode is called. This would allow us to be compatible with both. This is the first communication message we send across when apparmor gets a json output, namely {'dialog': 'apparmor-json-version', 'data': '2.12'}

I agree this would take longer pushing apparmor guys to put a change, but it would make "inter-operability" better.

Let me push that change.

@shundhammer
Copy link
Contributor Author

Sure, that would be the desirable solution. But we need a bugfix now. While this is not quite as elegant, it is the pragmatic approach.

Copy link
Member

@wfeldt wfeldt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@lslezak lslezak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to add an unit test for this? If you already have a sample JSON output it would be nice use it in some tests...

pid = pidmap["pid"]
if @prof.key?(profile_name)
msg = "Active process #{pid} #{executable_name}"
msg += " profile name #{profile_name}" if executable_name != profile_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np: you can use the << operator:

Suggested change
msg += " profile name #{profile_name}" if executable_name != profile_name
msg << " profile name #{profile_name}" if executable_name != profile_name

@shundhammer shundhammer merged commit aadeecf into yast:master Jan 24, 2019
@yast-bot
Copy link
Contributor

✔️ Public Jenkins job #10 successfully finished
✔️ Created OBS submit request #668378

@yast-bot
Copy link
Contributor

✔️ Internal Jenkins job #11 successfully finished
✔️ Created IBS submit request #182578

@shundhammer
Copy link
Contributor Author

shundhammer commented Jan 28, 2019

BTW follow-up problem: https://bugzilla.suse.com/show_bug.cgi?id=1123258

Fix: PR #36

I.e. the new parser is robust enough to also understand the old JSON format.

@shundhammer shundhammer deleted the huha-profile-names branch January 28, 2019 13:49
@dgdavid dgdavid mentioned this pull request Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants