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

[John Johansen]

On 03/31/2015 04:28 PM, Christian Boltz wrote:

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

well take a look at v3, I haven't gotten rid of the de-dup yet

>> 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 ;-)
> 
sure, I'm not opposed to removing the de-dup, if it is still an issue
>>>> +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=" ;-)
>
yes and no. In one sense I like the short hand of not using flags=
but, flags= is consistent with the other conditionals in use, and
it would just be cleaner to not have the two different options here.
 
>>>> +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 ;-)
>
hrmmm, ok I'll do that for v4
 
>>>> +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.
> 
true

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

yeah good idea, I can do that to v4

>>>> +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" ;-)
> 
true enough