[apparmor] [PATCH 3/8] utils: Add confdir env variable to aa.py for in-tree testing

Christian Boltz apparmor at cboltz.de
Thu Feb 16 00:29:55 UTC 2017


Hello,

Am Mittwoch, 15. Februar 2017, 12:21:05 CET schrieb Tyler Hicks:
> On 02/12/2017 12:55 PM, Christian Boltz wrote:
> > Am Mittwoch, 8. Februar 2017, 22:01:40 CET schrieb Tyler Hicks:
> >> Instead of hard-coding the location of logprof.conf and other utils
> >> related configuration files to /etc/apparmor/, this patch looks for
> >> the "APPARMOR_PY_CONFDIR" environment variable and, when set, uses
> >> its value for the configuration directory.
> >> 
> >> This allows for the make check target to use the in-tree config
> >> file,
> >> profiles, and parser by default. To override this behavior, the
> >> 
> >> USE_SYSTEM make variable needs to be set like so:
> >>   $ make USE_SYSTEM=1 -C utils check
> >> 
> >> The APPARMOR_PY_CONFDIR should be considered somewhat user-facing,
> >> although undocumented at this time.
> > 
> > I'm not the biggest fan of env variables, so I'm a bit undecided on
> > this patch.
> 
> Agreed. I don't like env vars for stuff like this.
> 
> > Pro:
> > - the env variable is an easy solution
> > - It makes it easy to test aa-logprof, aa-complain etc. with a
> > different> 
> >   config dir. Otherwise we'd have to add commandline parameters
> >   similar
> >   to what 5/8 does in aa-easyprof.
> 
> I think the most important pro is that it doesn't break existing users
> of aa.py.

Silly question - are you aware of any external users of aa.py?

I changed aa.py quite a lot, and never got any complaints IIRC. 
So either we have no external users, or I was very lucky and only 
touched code we only use internally ;-)  Given my luck in triggering 
each and every bug, I'd consider this highly unlikely ;-)

Oh, and IIRC we never gave any guarantee about stable interfaces. This 
doesn't mean we should break it without a good reason, but avoiding env 
variables would be a good reason IMHO ;-)

I'm quite sure that apparmor/rule/*.py will have a stable interface 
(unless something unexpected happens) - but I won't guarantee that for 
aa.py because it contains too much code that is more or less on my 
"needs a rewrite" list.

I even thought about making parts of aa.py (the part that handles and 
stores profiles) a class, which would avoid some workarounds in aa-
mergeprof. This won't happen in the near future, but maybe on the long 
term. And there are some other things that need to be changed first ;-)

Side note: If someone needs stable interfaces for something, please 
speak up. Ideally submit a test-external-interface.py which tests all 
the function calls etc. you need - this is the fastest way to make 
breakage visible ;-)

> > Cons:
> > - undocumented env vars are not nice[tm]
> > - env variables are less "visible" than commandline parameters, so
> > if
> > 
> >   someone "accidently" has them set, it will make debugging more
> >   funny.
> > 
> > An alternative solution would be to move the global code in aa.py
> > into> 
> >     def init_aa( <various optional parameters> )
> > 
> > This would mean every user of aa.py has to call that function (at
> > least if using a function that depends on the configfile), but it
> > would also prevent executing some code (like reading the config
> > file) when "just" importing apparmor.aa.
> 
> I like this idea but would hate to see it deployed for this use case.
> AFAICT, this confdir change is really only useful for our internal
> test suite and 

This is exactly why I don't like the env variable - if someone has it 
set accidentially, funny things[tm] might happen. Especially the fact 
that we only need it for internal usage (unit tests) means that exposing 
it to production code can only hurt.

[Insert usual note about global variables (which are not too different 
from env variables) and the "fun" you can have with them here]

> breaking existing, external code for such a reason
> would be a bad thing for us to do.

We should split off a 2.11 branch before changing the interface - but 
besides that, I see no problem with requiring an init_aa() call.

> Do we have any other good reasons, which benefit external users of
> aa.py, to introduce a requirement to call init_aa()?

Well, actually _not_ calling init_aa() could be a reason ;-)
Not all functions in aa.py depend on the config file etc., so for example 
some of our test cases could work without it. (About half the functions 
don't use one of the variables initialized in the global area of aa.py - 
but I have to admit I didn't check if they depend on other functions.)

The only code that will/should set a non-default config dir are unit 
tests. 

Everything else (aka "production code") would just call init_aa() 
without any parameters.

Oh, and if we really want, we could introduce a --confdir option in the 
aa-* tools. Again, this would probably be most interesting for 
development and testing.


BTW: The biggest problem with the current global code in aa.py are 
probably these two lines:

    raise AppArmorException('Can\'t find AppArmor profiles')

and

    raise AppArmorException('Can\'t find apparmor_parser')

If you find a way _not_ to raise those exceptions, while still ensuring 
parser and profile dir exist (before any attemp to access them), that 
would be the best solution.

An easy way might be to replace the global parser and profile_dir 
variables with functions that return their value (after checking they 
are set and exist), but that might come with a performance impact.

Doing it for the parser should be cheap (in comparison to actually 
running the parser), but profile_dir is used more often.

Oh, and while we are on it - currently, apparmor_parser calls are spread 
over aa.py, easyprof.py, sandbox.py and tools.py. I always wanted to 
have all parser calls in a separate module ;-)  with functions like 
load_profile(), unload_profile() etc.  - and probably an init_parser() 
that sets the parser path.

If we get rid of raising the exceptions, injecting special paths when 
running the unit tests should be easy.


Regards,

Christian Boltz
-- 
Linux just isn't user-friendly when it comes to viruses. You have to
work to find and run them. It doesn't happen automatically as it does
with Windows. The GNU/Linux folks really should improve this glaring
discrepancy.
[http://os.newsforge.com/article.pl?sid=05/01/25/1430222&from=rss]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20170216/3977746e/attachment.pgp>


More information about the AppArmor mailing list