[apparmor] [patch 14/18] parser: add additional language tests to get wider test coverage
Steve Beattie
steve at nxnw.org
Sat Jan 18 07:16:07 UTC 2014
On Fri, Jan 17, 2014 at 01:08:30AM +0100, Christian Boltz wrote:
> Am Donnerstag, 16. Januar 2014 schrieb Steve Beattie:
> > This patch adds several assorted language tests, to exercise various
> > parts of the parser that were not being covered by the language tests
> > previously. Areas lacking were found using the coverage compilation
> > option; coverage from the language tests is still incomplete.
> >
> > Signed-off-by: Steve Beattie <steve at nxnw.org>
>
> Acked-by: Christian Boltz <apparmor at cboltz.de>
> with some comments and questions inline.
Thanks for reviewing.
> > Index: b/parser/tst/simple_tests/file/ok_slashquote_1.sd
> > ===================================================================
> > --- /dev/null
> > +++ b/parser/tst/simple_tests/file/ok_slashquote_1.sd
> > @@ -0,0 +1,8 @@
> > +#
> > +#=DESCRIPTION unnecessary slash quotes are okay
> > +#=EXRESULT PASS
> > +#
> > +
> > +profile blart {
> > + /bingo/bang\o/bongo rw,
> > +}
>
> IIRC this will/should (?) at least cause a parser warning - at least I
> remember a patch to add warnings for superfluous backslashes was
> discussed ;-)
>
> If I'm right, please add a comment saying that there will be a warning.
Yes:
Warning from simple_tests/file/ok_slashquote_1.sd (simple_tests/file/ok_slashquote_1.sd line 9): Character o was quoted unnecessarily, dropped preceding quote ('\') character
Comment added.
>
> > Index: b/parser/tst/simple_tests/file/ok_quoted_1.sd
>
> > +#=DESCRIPTION simple quoted tab expansion
>
> > + "/bin/alpha\tbeta" rix,
>
>
> > Index: b/parser/tst/simple_tests/file/ok_quoted_2.sd
>
> > +#=DESCRIPTION simple quoted newline expansion
>
> > + "/bin/alpha\nbeta" rix,
>
>
> > Index: b/parser/tst/simple_tests/file/ok_quoted_3.sd
>
> > +#=DESCRIPTION simple quoted carriage return expansion
>
> > + "/bin/alpha\rbeta" rix,
>
>
> > Index: b/parser/tst/simple_tests/file/ok_quoted_4.sd
>
> > +#=DESCRIPTION simple quoted quote expansion
>
> > + "/bin/alpha\"beta" rix,
>
>
> > Index: b/parser/tst/simple_tests/file/ok_quoted_5.sd
>
> > +#=DESCRIPTION simple quoted backslash expansion
>
> > + "/bin/alpha\\beta" rix,
>
> That sounds like some corner cases that should get different (inline)
> highlighting in apparmor.vim, maybe similar to variables.
Probably. Want to draw up a patch?
> > Index: b/parser/tst/simple_tests/file/ok_2.sd
> > +#=Description basic uppercase permission file rule (should emit
> > warning)
>
> > + /usr/bin/foo RWM,
>
> Hmm, I'm a bit surprised that this gives "only" a warning.
I suspect support for uppercase RWM has been deprecated long enough and
we can remove it.
> For completeness, you should also add a test for IX ;-)
> (in addition to the iX testcase you already have)
Sure.
> > Index: b/parser/tst/simple_tests/profile/flags/flags_bad14.sd
> > ===================================================================
> > --- /dev/null
> > +++ b/parser/tst/simple_tests/profile/flags/flags_bad14.sd
> > @@ -0,0 +1,12 @@
> > +#
> > +#=DESCRIPTION fail if flags is not speeled correctly
> > +#=EXRESULT FAIL
> > +# vim:syntax=subdomain
>
> That should be syntax=apparmor.
Doh. Fixed.
> > +# Last Modified: Sun Apr 17 19:44:44 2005
> > +#
> > +/does/not/exist flogs=(audit) {
>
> ;-)
>
> > + #include <includes/base>
>
> Did you intentionally use "includes" instead of "abstractions" here?
> That's probably not a good idea because it adds a second possible
> failure (file not found), which means the test will fail even if flogs=
> goes unnoticed.
Yes, "includes/" as the path is intentional; the simple.pl script
sets the parser's include dir to tst/simple_tests, and the include/
directory under there has some sample includes files for testing. The
intent was to try to keep the parsing tests self-contained and not
depend on outside abstractions being installed, so a different include
path was chosen intentionally.
That said, you could probably guess that, given the syntax=subdomain
line, the test profile was copy-n-wasted from elsewhere, and the
include portion is, as you point out, extraneous to the test. So I've
dropped the include rule.
(Oh, and the vim syntax highlighting does detect that the profile
declaration has something wrong :) )
> > Index: b/parser/tst/simple_tests/file/ok_other_1.sd
>
> > +#=DESCRIPTION simple other flag test
>
> > + other /tmp/** rw,
>
> Hmm, support for other seems to be missing in apparmor.vim.
> Nice[tm].
Ah, so it is. Here's a patch that adds it, as well as some additional
test cases around the combinations of audit, allow, deny and other:
Signed-off-by: Steve Beattie <steve at nxnw.org>
---
parser/tst/simple_tests/file/allow/ok_other_1.sd | 7 +++++++
parser/tst/simple_tests/file/allow/ok_other_2.sd | 7 +++++++
parser/tst/simple_tests/file/ok_other_2.sd | 7 +++++++
parser/tst/simple_tests/file/ok_other_3.sd | 7 +++++++
utils/vim/create-apparmor.vim.py | 8 ++++----
5 files changed, 32 insertions(+), 4 deletions(-)
Index: b/utils/vim/create-apparmor.vim.py
===================================================================
--- a/utils/vim/create-apparmor.vim.py
+++ b/utils/vim/create-apparmor.vim.py
@@ -88,11 +88,11 @@ filename=r'(\/|\@\{\S*\})\S*'
aa_regex_map = {
'FILENAME': filename,
- 'FILE': r'\v^\s*(audit\s+)?(deny\s+|allow\s+)?(owner\s+)?' + filename + r'\s+', # Start of a file rule
+ 'FILE': r'\v^\s*(audit\s+)?(deny\s+|allow\s+)?(owner\s+|other\s+)?' + filename + r'\s+', # Start of a file rule
# (whitespace_+_, owner etc. flag_?_, filename pattern, whitespace_+_)
- 'DENYFILE': r'\v^\s*(audit\s+)?deny\s+(owner\s+)?' + filename + r'\s+', # deny, otherwise like FILE
- 'auditdenyowner': r'(audit\s+)?(deny\s+|allow\s+)?(owner\s+)?',
- 'audit_DENY_owner': r'(audit\s+)?deny\s+(owner\s+)?', # must include "deny", otherwise like auditdenyowner
+ 'DENYFILE': r'\v^\s*(audit\s+)?deny\s+(owner\s+|other\s+)?' + filename + r'\s+', # deny, otherwise like FILE
+ 'auditdenyowner': r'(audit\s+)?(deny\s+|allow\s+)?(owner\s+|other\s+)?',
+ 'audit_DENY_owner': r'(audit\s+)?deny\s+(owner\s+|other\s+)?', # must include "deny", otherwise like auditdenyowner
'auditdeny': r'(audit\s+)?(deny\s+|allow\s+)?',
'EOL': r'\s*,(\s*$|(\s*#.*$)\@=)', # End of a line (whitespace_?_, comma, whitespace_?_ comment.*)
'TRANSITION': r'(\s+-\>\s+\S+)?',
Index: b/parser/tst/simple_tests/file/ok_other_2.sd
===================================================================
--- /dev/null
+++ b/parser/tst/simple_tests/file/ok_other_2.sd
@@ -0,0 +1,7 @@
+#
+#=DESCRIPTION simple deny other flag test
+#=EXRESULT PASS
+
+profile test {
+ deny other /tmp/** rw,
+}
Index: b/parser/tst/simple_tests/file/ok_other_3.sd
===================================================================
--- /dev/null
+++ b/parser/tst/simple_tests/file/ok_other_3.sd
@@ -0,0 +1,7 @@
+#
+#=DESCRIPTION simple other flag test
+#=EXRESULT PASS
+
+profile test {
+ audit other /tmp/** rw,
+}
Index: b/parser/tst/simple_tests/file/allow/ok_other_1.sd
===================================================================
--- /dev/null
+++ b/parser/tst/simple_tests/file/allow/ok_other_1.sd
@@ -0,0 +1,7 @@
+#
+#=DESCRIPTION simple allow other flag test
+#=EXRESULT PASS
+
+profile test {
+ allow other /tmp/** rw,
+}
Index: b/parser/tst/simple_tests/file/allow/ok_other_2.sd
===================================================================
--- /dev/null
+++ b/parser/tst/simple_tests/file/allow/ok_other_2.sd
@@ -0,0 +1,7 @@
+#
+#=DESCRIPTION simple audit allow other flag test
+#=EXRESULT PASS
+
+profile test {
+ audit allow other /tmp/** rw,
+}
> That all said - I remember that we discussed an automated way to test
> apparmor.vim (basically by letting vim write the colored file as HTML) -
> but that was loooong ago and I don't remember the details.
>
> Can someone give a pointer or, better, provide a patch for the tests/
> Makefile? ;-)
I don't have time to draw up a patch, but the 2html.vim
syntax plugin looks like it's the right thing to use, based on
http://vim.wikia.com/wiki/Pasting_code_with_syntax_coloring_in_emails ,
if anyone else wants to take this on.
--
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/20140117/ef92232a/attachment.pgp>
More information about the AppArmor
mailing list