[apparmor] [PATCH 2/3] apparmor.d.pod: refactor profile file, profile, subprofile, hat patterns

[Christian Boltz]

Hello,

Am Dienstag, 31. März 2015 schrieb John Johansen:
> On 03/28/2015 03:49 PM, Christian Boltz wrote:
> > Am Mittwoch, 25. März 2015 schrieb John Johansen:
> >> Signed-off-by: John Johansen <john.johansen at canonical.com>
> >> ---
> >> 
> >>  parser/apparmor.d.pod | 46
> >> 
> >> ++++++++++++++++++++++++++++++---------------- 1 file changed, 30
> >> insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/parser/apparmor.d.pod b/parser/apparmor.d.pod
> >> index 3b4e4e9..9d2664d 100644
> >> --- a/parser/apparmor.d.pod
> >> +++ b/parser/apparmor.d.pod
> >> @@ -44,6 +44,12 @@ to the policy; this behaviour is modelled after
> >> cpp(1).
> >> 
> >>  =over 4
> >> 
> >> +B<PROFILE FILE> = I<PREAMBLE> I<PROFILES>
> >> +
> >> +B<PREAMBLE> = ( I<COMMENT> | I<VARIABLE ASSIGNMENT> | I<INCLUDE> 
> >> )*
> >> +
> >> +B<PROFILES> = ( B<PROFILE> )*
> > 
> > Strictly speaking, this is not 100% correct. INCLUDE can also appear
> > between two profiles, not only in the PREAMBLE.
> > 
> > In practise, this is corner-case enough to ignore it here ;-)
> 
> nah easy to fix

We'll see if there's a 100% correct and still understandable definition. 
I tried a bit, but didn't find something that I'd call an improvement 
;-)

AFAIK the only restriction is that variable definitions must be in the 
preamble (above the first profile), so we'd end up with
    B<PROFILES> = ( I<COMMENT> | I<INCLUDE> | B<PROFILE> )*
which is not too different from PREAMBLE (and should not be named 
PROFILES ;-)

The easiest solution is probably
    B<PROFILE FILE> = ( I<VARIABLE ASSIGNMENT> | I<COMMENT> | 
    I<INCLUDE> | B<PROFILE> )*
with a note saying exactly that VARIABLE ASSIGNMENTs (and INCLUDES 
containing them) must be done before the first PROFILE.

> > The same applies for comments between profiles, which is probably
> > used more often - OTOH I'm sure nobody will consult the manpage to
> > find out where adding a comment is allowed ;-)
> > 
> >> -B<PROFILE> = [ I<COMMENT> ... ] [ I<VARIABLE ASSIGNMENT> ... ] (
> >> '"'
> >> I<PROGRAM> '"' | I<PROGRAM> ) [ 'flags=(complain)' ]'{' ( I<RULES>
> >> )*
> >> '}'
> >> +B<PROFILE> = ( I<FILEGLOB> | I<PROFILE NAME> ) [ I<ATTACHMENT
> >> SPECIFICATION> ] [ <PROFILE FLAG CONDS> ] I<BLOCK>
> >> +
> >> +B<PROFILE NAME> = 'profile' I<AARE>
> > 
> > Even with FILEGLOB, the 'profile' keyword is allowed (but optional),
> > so this should be
> > 
> >     B<PROFILE> = ( [ 'profile '] I<FILEGLOB> | I<PROFILE NAME> ) ...
> > 
> > The more readable solution is probably:
> >     B<PROFILE> = I<PROFILE NAME> ...
> >     B<PROFILE NAME> = [ 'profile ' ] I<FILEGLOB> | 'profile' I<AARE>
> > 
> > BTW: I don't like BLOCK too much and would prefer to keep
> > 
> >     '{' ( I<RULES> )* '}'
> > 
> > even if we have to repeat that for hats and subprofiles. In this
> > case, readability should win over de-duplication ;-)
> 
> I'm not going to change that in this patch, I can do it in another but

no problem - it wouldn't be the first time one of my reviews causes a 
follow-up patch ;-)

> it will just get added back in when I do the patch to add the
> documentation on using prefixes on blocks. ie.
> 
>   audit {  /foo r,
>            /bar w,
>         }
> 
> Though I suppose the block bit could be repeated there as well

I'll come up with the same "readability vs. de-duplication" argument 
there ;-)

> >> +B<ATTACHMENT SPECIFICATION> = I<FILEGLOB>
> >> +
> >> +B<PROFILE FLAG CONDS> = 'flags=(' comma or white space separated
> >> list of I<PROFILE FLAGS> ')'
> > 
> > "flags=" is optional ;-)
> 
> sadly yes

Sounds like you have a strong preference for using "flags=" ;-)

> >> +B<PROFILE FLAGS> = 'complain' | 'enforce' | 'mediate_deleted' |
> >> attach_disconnected'
> > 
> > That list is incomplete, see create-apparmor.vim.py ;-)
> > 
> > aa_flags = ['complain',
> > 
> >             'audit',
> >             'attach_disconnected',
> >             'no_attach_disconnected',
> >             'chroot_attach',
> >             'chroot_no_attach',
> >             'chroot_relative',
> >             'namespace_relative',
> >             'mediate_deleted',
> >             'delegate_deleted']
> 
> right so I added in audit, and chroot_relative. I am deliberately
> leaving out the others atm. They are very specialized and not in use
> any where (I hope) atm and would like to keep it that way if
> possible, until I can take another look at the code around this stuff

ok.
(ideally add a comment with the undocumented flags so that we stumble 
over it one day ;-)

> >> +B<BLOCK> = '{' ( I<RULES> )* '}'
> > 
> > See above for my opinion about BLOCK ;-)
> > 
> >> +B<SUBPROFILE> = I<PROFILE NAME> [ I<ATTACHMENT SPECIFICATION> ] [
> >> <PROFILE FLAG CONDS> ] I<BLOCK>
> >> +
> >> +B<HAT> = ('hat' | '^') I<HATNAME> [ <PROFILE FLAG CONDS> ]
> >> I<BLOCK>
> > 
> > Should be a space after the hat keyword? -> 'hat '
> > 
> > (same question for some other keywords like 'profile' ;-)
> 
> yes but generally we have ignored the whitespace separation of tokens
> almost everywhere. Adding that in will take a lot more than just 'hat'
> and 'profile'

^ is special because it is _not_ followed by whitespace.

That said: I understand your point - maybe the solution is to add a note 
that there is no whitespace between ^ and the hat name. Or a simple 
example like
    ^foo   is the same as   hat foo

> >> +B<HATNAME> = '^'  (non-whitespace characters; see aa_change_hat(2)
> > 
> > A quick test shows that "non-whitespace" isn't correct - for
> > example,
> > ^!foo {}  causes a parser error ;-)  Please specify what exactly is
> > allowed.
> 
> yep, though it looks like the parser here is accepting a broader set
> of names than it should

One more reason to specify the allowed name more detailed than 
"non-whitespace" ;-)


Regards,

Christian Boltz
-- 
> CPU&-Register:  die Person (mit Kurzzeitgedaechnis)
Ich darf doch schwer bitten. Wenn ich morgens aufwache, brauche ich
nicht erst Aktenordner durchzulesen. Ich kann mich auch so erinnern.
[> David Haller und Bernd Brodesser in suse-linux]