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

[John Johansen]

On 03/28/2015 03:49 PM, Christian Boltz wrote:
> Hello,
> 
> 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

> 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
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

>> +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

>> +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

>> +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'

>> +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

^!foo isn't valid