[apparmor] [PATCH 3/8] add optional allow prefix to the language v2

[Steve Beattie]

On Tue, Sep 17, 2013 at 02:01:44PM +0200, Christian Boltz wrote:
> Hello,
> 
> Am Montag, 16. September 2013 schrieb Steve Beattie:
> >       - fix a bug in apparmor.vim to let it recognize multiple
> > 	capability entries in a single line.
> 
> I didn't know this is possible ;-)

See, for example, the last few lines in
parser/tst/simple_tests/capability/ok1.sd. (It makes for a good test
case for whether apparmor.vim is handling multiple entries properly.)

> @Kshitij: you'll also need to allow this in the py tools.

> > Index: b/parser/tst/simple_tests/capability/ok_allow1.sd
> > ===================================================================
> > --- /dev/null
> > +++ b/parser/tst/simple_tests/capability/ok_allow1.sd
> > @@ -0,0 +1,41 @@
> > +#
> > +#=DESCRIPTION validate uses of allow/capabilities.
> > +#=EXRESULT PASS
> > +# vim:syntax=subdomain
> 
> new files should use
>     # vim:syntax=apparmor
> (and when you are bored, you should also change all existing profiles ;-)
> In other words: please s/syntax=subdomain/syntax=apparmor/ in the patch ;-)

Okay, I can do that.

> > +++ b/parser/tst/simple_tests/file/allow/ok_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_3.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_append_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_carat_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_carat_2.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_comma_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_comma_2.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_embedded_spaces_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_embedded_spaces_2.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_embedded_spaces_3.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_inv_char_class.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_lock_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_mmap_1.sd
> 
> > +++ b/parser/tst/simple_tests/file/allow/ok_mmap_2.sd
> 
> All those files don't have a vim: header. Maybe you can add it?

Sure. Though, while syntax highlighting with apparmor.vim is a
pretty important tool for me when examining profiles, I don't enable
modelines in vim because it's a bit dangerous to give random files
you open access to vim settings. But it's still a useful hint that
syntax highlighting for apparmor profiles is available, but should
give you an idea why I'm not careful about getting them correct.

> > Index: b/utils/vim/create-apparmor.vim.py
> > ===================================================================
> > --- a/utils/vim/create-apparmor.vim.py
> > +++ b/utils/vim/create-apparmor.vim.py
> 
> > -    'FILE':             r'\v^\s*(audit\s+)?(deny\s+)?(owner\s+)?' + filename + r'\s+', # Start of a file rule
> > +    'FILE':             r'\v^\s*(audit\s+)?((deny|allow)\s+)?(owner\s+)?' + filename + r'\s+', # Start of a file rule
> 
> vi has a restriction on how many (...) you use per line (IIRC 9 - I hit 
> the limit in the past), so we shouldn't add (...) if not really needed.
> 
> Please use   (deny\s+|allow\s+)   instead of   ((deny|allow)\s+)
> (like you did for 'auditdenyowner' and 'auditdeny')

Yes, I hit that parentheses limit when I tried to use
'((deny|allow)\s+)?' in  'auditdenyowner' and 'auditdeny'. I didn't go
back and adjust FILE because when I looked, I couldn't see anything in
create-apparmor.vim.py or apparmor.vim.in that used FILE. Everything
seems to use FILENAME instead.

> > Index: b/utils/vim/apparmor.vim.in
> > ===================================================================
> > --- a/utils/vim/apparmor.vim.in
> > +++ b/utils/vim/apparmor.vim.in
> > @@ -132,7 +132,7 @@ syn keyword  sdCapKey          @@sdKapKe
> 
> > -syn match  sdCap /\v^\s*@@auditdeny@@capability\s+(@@sdKapKeyRegex@@)                         @@EOL@@ ...
> > +syn match  sdCap /\v^\s*@@auditdeny@@capability\s+((@@sdKapKeyRegex@@)\s+)*(@@sdKapKeyRegex@@)@@EOL@@ ...
>
> (lines shortened for readability)
> 
> Looks good, even if it adds two more (...) ;-)

Yeah, it doesn't seem to go over vim's limit. The other way to do
it, which would add only a single of parens, would be to create a
second capability list regex that in included the '\s+' trailing
spaces requirement with each capability name, to be used to match
all the capability names other than the last one that occurs in the
line. This way seemed cleaner.

As an aside, I probably should have submitted this patch separately as
it's an independent fix from adding support for the 'access' keyword,
and it's easily cherry-pickable for the 2.8 branch.

> The more interesting question is: how do we color the line if it contains
> "dangerous" and "normal" capabilities?

The way things work currently is that dangerous capabilities get
highlighted differently (foreground color red) when they occur,
whether they are the only capability in a rule or mixed with
less dangerous ones. They stand out pretty well against the dark
blue foreground color of the rest of the capability rule. Again,
parser/tst/simple_tests/capability/ok1.sd makes for a useful example.

Thanks for the feedback.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130917/3cb8fcc1/attachment.pgp>