roddhjav / apparmor.d

Full set of AppArmor profiles (~ 1500 profiles)
https://apparmor.pujol.io
GNU General Public License v2.0
395 stars 34 forks source link

feat(profiles-a-f): vim syntax support #380

Closed REmerald closed 2 weeks ago

REmerald commented 3 weeks ago

Add vim modeline instructing the editor to use syntax plugin provided by apparmor.

Continuation of #379 to keep the diff list relatively short.

roddhjav commented 3 weeks ago

I would rather not add any IDE specific syntax. Because they are a lot of IDE...

REmerald commented 3 weeks ago

@roddhjav, I get the logic, but I'm not trying to implement something new, I'm just copying what apparmor itself does, many (around half, 165/365) of the profiles provided by apparmor have this implemented.

$ grep -r --files-with-matches 'vim:' /etc/apparmor.d/ | xargs pacman -Qo
/etc/apparmor.d/tunables/dovecot is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/ubuntu-browsers.d/chromium-browser is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/ubuntu-browsers.d/java is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/ubuntu-browsers.d/kde is owned by apparmor 3.1.7-2
...
/etc/apparmor.d/abstractions/X is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/apache2-common is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/aspell is owned by apparmor 3.1.7-2
...
/etc/apparmor.d/abstractions/xdg-desktop is owned by apparmor 3.1.7-2
/etc/apparmor.d/abstractions/xdg-open is owned by apparmor 3.1.7-2
/etc/apparmor.d/nvidia_modprobe is owned by apparmor 3.1.7-2
/etc/apparmor.d/php-fpm is owned by apparmor 3.1.7-2
/etc/apparmor.d/samba-dcerpcd is owned by apparmor 3.1.7-2
/etc/apparmor.d/samba-rpcd is owned by apparmor 3.1.7-2
/etc/apparmor.d/samba-rpcd-classic is owned by apparmor 3.1.7-2
/etc/apparmor.d/samba-rpcd-spoolss is owned by apparmor 3.1.7-2
/etc/apparmor.d/usr.lib.dovecot.anvil is owned by apparmor 3.1.7-2
/etc/apparmor.d/usr.lib.dovecot.auth is owned by apparmor 3.1.7-2
...
/etc/apparmor.d/usr.lib.dovecot.script-login is owned by apparmor 3.1.7-2
/etc/apparmor.d/usr.lib.dovecot.ssl-params is owned by apparmor 3.1.7-2
/etc/apparmor.d/usr.lib.dovecot.stats is owned by apparmor 3.1.7-2
/etc/apparmor.d/usr.sbin.dovecot is owned by apparmor 3.1.7-2
$ grep -r --files-with-matches 'vim:' /etc/apparmor.d/ | xargs pacman -Qo | grep --invert-match 'apparmor 3.1.7-2'
$ grep -r --files-with-matches 'vim:' /etc/apparmor.d/ | wc -l
117
$ pacman -Qlq apparmor | grep /etc/apparmor.d/ | wc -l
258
$ grep -r --files-with-matches 'vim:' /usr/share/apparmor/extra-profiles/ | wc -l
48
$ pacman -Qlq apparmor | grep /usr/share/apparmor/extra-profiles | wc -l
107

Also, more importantly, apparmor syntax feature isn't actually vim specific, it's provided by apparmor:

$ pacman -Qlq apparmor | grep vim
/usr/share/man/man5/apparmor.vim.5.gz
/usr/share/vim/
/usr/share/vim/vimfiles/
/usr/share/vim/vimfiles/syntax/
/usr/share/vim/vimfiles/syntax/apparmor.vim

Because they are a lot of IDE...

Since apparmor developers implement this feature only for vim (which make sense because vim is the most used editor in terminal), it would not make sense (or even be possible) to implement this for other editors as well.

nobody43 commented 3 weeks ago

I agree, we should preserve upstream style, unless it's outdated.

roddhjav commented 2 weeks ago

While this project aim to be fully compatible with upstream, it redefines the way of writing apparmor profile and literally create a new style. Therefore, the argument of following upstream is not valid on this matter. Not to mention the non functional nature of this syntax.

I am not a fan of having editor dependent comments in every single files... because we have a lot of files. But I suppose it can be useful for some people.

Therefore, I am ok to merge it.

Can you also add it to: https://github.com/roddhjav/apparmor.d/blob/main/docs/development/index.md

REmerald commented 2 weeks ago

@roddhjav, just want to clarify a couple of things.

While this project aim to be fully compatible with upstream, it redefines the way of writing apparmor profile and literally create a new style. Therefore, the argument of following upstream is not valid on this matter.

Ok, now I understand it more clearly.

Not to mention the non functional nature of this syntax.

Not really sure about the exact meaning of this sentence.

I am not a fan of having editor dependent comments in every single files... because we have a lot of files.

Yeah, I agree, and since the change can be implemented gradually, that's why I wanted to split the changes into several (relatively small) pull requests of a specific subfolder, so that you can check and merge one of them per day/week/etc. (whatever you comfortable with, without wasting much of your time).

But I suppose it can be useful for some people.

I think it would be useful for the most people, as most people who want to set up apparmor would be too experienced to use an editor like nano. Even if they use an IDE like vscode or neovim for programming, they wouldn't use it as an editor in terminal to edit something as root.

Can you also add it to: https://github.com/roddhjav/apparmor.d/blob/main/docs/development/index.md

Sure, but is it ok, if I do it in #379? It is the first pull request, so it would be more logical to do it there and merge it first.

P.S. I just noticed that this comment has to be one of the first 5 lines in beginning (or end) of the file (controlled by 'modelines' option which is 5 by default). In most files it's ok, but in some, when there's more than 2 Copyright lines (e.g. example below), this feature doesn't work.

# apparmor.d - Full set of apparmor profiles
# Copyright (C) 2019-2022 Mikhail Morfikov
# Copyright (C) 2022-2024 Alexandre Pujol <alexandre@pujol.io>
# Copyright (C) 2022 Jeroen Rijken
# SPDX-License-Identifier: GPL-2.0-only
# vim:syntax=apparmor

This isn't really a problem, as I can change it with a command like find apparmor.d/ -type f -exec sed -i 's/GPL-2.0-only/GPL-2.0-only\n# vim:syntax=apparmor/' '{}' '+'. Though I'm not sure where in the file you would prefer the comment to be in, in the first line,

# vim:syntax=apparmor
# apparmor.d - Full set of apparmor profiles

second line

# apparmor.d - Full set of apparmor profiles
# vim:syntax=apparmor
# Copyright (C) 2022-2024 Alexandre Pujol <alexandre@pujol.io>

or the last line?

    include if exists <local/mkinitramfs_kmod>
  }

  include if exists <local/mkinitramfs>
}
# vim:syntax=apparmor

I tested all 3 variants and it works in all of them.

roddhjav commented 2 weeks ago

Then, can you add it at the end of the file, i think that it is best to not mess up with the header. Also, please, add a new line before the vim directive:

    include if exists <local/mkinitramfs_kmod>
  }

  include if exists <local/mkinitramfs>
}

# vim:syntax=apparmor

Sure, but is it ok, if I do it in https://github.com/roddhjav/apparmor.d/pull/379? It is the first pull request, so it would be more logical to do it there and merge it first.

Sure, go ahead.

roddhjav commented 2 weeks ago

Thanks, merged