On 2015-03-25 23:46:26, Steve Beattie wrote: > On Wed, Mar 25, 2015 at 05:37:17PM -0500, Tyler Hicks wrote: > > The aa_policy_cache_create() function had a name that didn't quite match > > its actions. It doesn't create a new policy cache. It actually requires > > an existing policy cache, with some sort of .features file, to already > > exist. > > > > It unconditionally makes a policy cache "valid" by clearing all of the > > files and creating a new .features file from the current running kernel. > > Given that, perhaps aa_policy_cache_reset() + a lavender bikeshed?
Hmm... I do like reset better for the current implementation but there's
more to the story with this function.
We currently have a cache structure like this:
/etc/apparmor.d/cache/
↳ .features
↳ usr.bin.foo
↳ usr.bin.bar
You'd make these calls to check the validity of the cache and reset it if
the .features file doesn't match the current kernel:
aa_policy_cache_new(&policy_cache, NULL, /etc/apparmor.d/cache, false);
if (!aa_policy_cache_is_valid(policy_cache))
aa_policy_cache_make_valid(policy_cache)
However, we'll soon have multiple policy cache directories for various kernels.
The layout will look something like this (this is not set in stone yet):
/etc/apparmor.d/cache.d/
↳ 60b725f10c9c/
↳ .features
↳ usr.bin.foo
↳ usr.bin.bar
↳ 3b5d5c371295/
↳ .features
↳ usr.bin.foo
↳ usr.bin.bar
↳ 2cd6ee2c70b0/
↳ .features
↳ usr.bin.foo
↳ usr.bin.bar
The directory names underneath the cache.d directory are some sort of hash. The
implementation is private to libapparmor but the inputs may be the text
representation of a kernel's AA features.
Lets say that the current kernel's AA features hash to "e29311f6f1bf",
which doesn't yet have a valid cache directory. The
aa_policy_cache_make_valid() function will be what creates the new cache
subdirectory and the corresponding .features file.
In that situation, "reset" probably makes less sense than "make_valid"
which probably makes less sense than "create". :)
Does anyone have a suggestion for a function name that works equally
well with the current implementation and the future implementation
that'll support multiple cache directories?
> Also, some manpages would be nice to document the purpose of each
> function and any sequencing issues.
I agree. I'm hoping that I can knock out some man pages today.
>
> That said, in the end, either works for me, so
>
> > Signed-off-by: Tyler Hicks <tyhi...@canonical.com>
>
> Acked-by: Steve Beattie <st...@nxnw.org> with your choice on keeping
> 'make_valid' or converting to 'reset' (with my slight preference for
> reset).
I'm going to hold off on committing this until we see if anyone has a
better suggestion after I gave some more info on what this function will
be in the future.
Thanks for the review!
Tyler
>
> Thanks.
>
> > ---
> > libraries/libapparmor/include/sys/apparmor.h | 2 +-
> > libraries/libapparmor/src/libapparmor.map | 2 +-
> > libraries/libapparmor/src/policy_cache.c | 4 ++--
> > parser/parser_main.c | 2 +-
> > tests/regression/apparmor/aa_policy_cache.c | 14 +++++++-------
> > tests/regression/apparmor/aa_policy_cache.sh | 8 ++++----
> > 6 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/libraries/libapparmor/include/sys/apparmor.h
> > b/libraries/libapparmor/include/sys/apparmor.h
> > index 99ce36b..43d6efc 100644
> > --- a/libraries/libapparmor/include/sys/apparmor.h
> > +++ b/libraries/libapparmor/include/sys/apparmor.h
> > @@ -147,7 +147,7 @@ aa_policy_cache *aa_policy_cache_ref(aa_policy_cache
> > *policy_cache);
> > void aa_policy_cache_unref(aa_policy_cache *policy_cache);
> >
> > bool aa_policy_cache_is_valid(aa_policy_cache *policy_cache);
> > -int aa_policy_cache_create(aa_policy_cache *policy_cache);
> > +int aa_policy_cache_make_valid(aa_policy_cache *policy_cache);
> > int aa_policy_cache_remove(const char *path);
> > int aa_policy_cache_replace_all(aa_policy_cache *policy_cache,
> > aa_kernel_interface *kernel_interface);
> > diff --git a/libraries/libapparmor/src/libapparmor.map
> > b/libraries/libapparmor/src/libapparmor.map
> > index 3f43494..2f440f0 100644
> > --- a/libraries/libapparmor/src/libapparmor.map
> > +++ b/libraries/libapparmor/src/libapparmor.map
> > @@ -77,7 +77,7 @@ APPARMOR_2.10 {
> > aa_policy_cache_ref;
> > aa_policy_cache_unref;
> > aa_policy_cache_is_valid;
> > - aa_policy_cache_create;
> > + aa_policy_cache_make_valid;
> > aa_policy_cache_remove;
> > aa_policy_cache_replace_all;
> > local:
> > diff --git a/libraries/libapparmor/src/policy_cache.c
> > b/libraries/libapparmor/src/policy_cache.c
> > index a9e43bb..e438439 100644
> > --- a/libraries/libapparmor/src/policy_cache.c
> > +++ b/libraries/libapparmor/src/policy_cache.c
> > @@ -225,13 +225,13 @@ bool aa_policy_cache_is_valid(aa_policy_cache
> > *policy_cache)
> > }
> >
> > /**
> > - * aa_policy_cache_create - creates a valid policy_cache for the currently
> > running kernel
> > + * aa_policy_cache_make_valid - empties the policy_cache and makes it
> > valid for the currently running kernel
> > * @policy_cache: the policy_cache
> > *
> > * Returns: 0 on success, -1 on error with errno set and features pointing
> > to
> > * NULL
> > */
> > -int aa_policy_cache_create(aa_policy_cache *policy_cache)
> > +int aa_policy_cache_make_valid(aa_policy_cache *policy_cache)
> > {
> > return create_cache(policy_cache, policy_cache->kernel_features);
> > }
> > diff --git a/parser/parser_main.c b/parser/parser_main.c
> > index 8aee148..1dc3088 100644
> > --- a/parser/parser_main.c
> > +++ b/parser/parser_main.c
> > @@ -929,7 +929,7 @@ int main(int argc, char *argv[])
> > skip_read_cache = 0;
> > } else if (!aa_policy_cache_is_valid(policy_cache)) {
> > if (write_cache && cond_clear_cache &&
> > - aa_policy_cache_create(policy_cache)) {
> > + aa_policy_cache_make_valid(policy_cache)) {
> > if (show_cache)
> > PERROR("Cache write disabled: Cannot
> > create cache '%s': %m\n",
> > cacheloc);
> > diff --git a/tests/regression/apparmor/aa_policy_cache.c
> > b/tests/regression/apparmor/aa_policy_cache.c
> > index b08fd1f..cb4bc71 100644
> > --- a/tests/regression/apparmor/aa_policy_cache.c
> > +++ b/tests/regression/apparmor/aa_policy_cache.c
> > @@ -22,7 +22,7 @@
> >
> > #include <sys/apparmor.h>
> >
> > -#define OPT_CREATE "create"
> > +#define OPT_MAKE_VALID "make-valid"
> > #define OPT_IS_VALID "is-valid"
> > #define OPT_NEW "new"
> > #define OPT_NEW_CREATE "new-create"
> > @@ -40,12 +40,12 @@ static void usage(const char *prog)
> > " %s %s <PATH>\n"
> > " %s %s <PROFILE_NAME>\n"
> > " %s %s <PATH>\n",
> > - prog, OPT_CREATE, prog, OPT_IS_VALID, prog, OPT_NEW,
> > + prog, OPT_MAKE_VALID, prog, OPT_IS_VALID, prog, OPT_NEW,
> > prog, OPT_NEW_CREATE, prog, OPT_REMOVE, prog, OPT_REMOVE_POLICY,
> > prog, OPT_REPLACE_ALL);
> > }
> >
> > -static int test_create(const char *path)
> > +static int test_make_valid(const char *path)
> > {
> > aa_features *features = NULL;
> > aa_policy_cache *policy_cache = NULL;
> > @@ -61,8 +61,8 @@ static int test_create(const char *path)
> > goto out;
> > }
> >
> > - if (aa_policy_cache_create(policy_cache)) {
> > - perror("FAIL - aa_policy_cache_create");
> > + if (aa_policy_cache_make_valid(policy_cache)) {
> > + perror("FAIL - aa_policy_cache_make_valid");
> > goto out;
> > }
> >
> > @@ -204,8 +204,8 @@ int main(int argc, char **argv)
> > exit(1);
> > }
> >
> > - if (strcmp(argv[1], OPT_CREATE) == 0) {
> > - rc = test_create(argv[2]);
> > + if (strcmp(argv[1], OPT_MAKE_VALID) == 0) {
> > + rc = test_make_valid(argv[2]);
> > } else if (strcmp(argv[1], OPT_IS_VALID) == 0) {
> > rc = test_is_valid(argv[2]);
> > } else if (strcmp(argv[1], OPT_NEW) == 0) {
> > diff --git a/tests/regression/apparmor/aa_policy_cache.sh
> > b/tests/regression/apparmor/aa_policy_cache.sh
> > index fb9a830..427ddfa 100755
> > --- a/tests/regression/apparmor/aa_policy_cache.sh
> > +++ b/tests/regression/apparmor/aa_policy_cache.sh
> > @@ -117,12 +117,12 @@ runchecktest "AA_POLICY_CACHE is-valid (no cachedir)"
> > fail is-valid "$cachedir"
> >
> > create_cachedir
> > install_bad_features_file
> > -runchecktest "AA_POLICY_CACHE create (bad .features)" pass create
> > "$cachedir"
> > -runchecktest "AA_POLICY_CACHE create (good .features)" pass create
> > "$cachedir"
> > +runchecktest "AA_POLICY_CACHE make-valid (bad .features)" pass make-valid
> > "$cachedir"
> > +runchecktest "AA_POLICY_CACHE make-valid (good .features)" pass make-valid
> > "$cachedir"
> > remove_features_file
> > -runchecktest "AA_POLICY_CACHE create (no .features)" fail create
> > "$cachedir"
> > +runchecktest "AA_POLICY_CACHE make-valid (no .features)" fail make-valid
> > "$cachedir"
> > remove_cachedir
> > -runchecktest "AA_POLICY_CACHE create (no cachedir)" fail create "$cachedir"
> > +runchecktest "AA_POLICY_CACHE make-valid (no cachedir)" fail make-valid
> > "$cachedir"
> >
> > # Make sure that no test policies are already loaded
> > verify_policies_are_not_loaded
>
> --
> Steve Beattie
> <sbeat...@ubuntu.com>
> http://NxNW.org/~steve/
> --
> AppArmor mailing list
> AppArmor@lists.ubuntu.com
> Modify settings or unsubscribe at:
> https://lists.ubuntu.com/mailman/listinfo/apparmor
signature.asc
Description: Digital signature
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
