[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