[apparmor] [PATCH v2 01/14] libapparmor: Simplify aa_policy_cache API
Tyler Hicks
tyhicks at canonical.com
Fri May 29 23:04:09 UTC 2015
On 2015-05-29 04:31:10, John Johansen wrote:
> On 04/02/2015 08:17 AM, Tyler Hicks wrote:
> > This patch changes the aa_policy_cache_new() prototype and gets rid of
> > aa_policy_cache_is_valid() and aa_policy_cache_create().
> >
> > The create bool of aa_policy_cache_new() is replaced with a 16 bit
> > unsigned int used to specify the maximum number of caches that should be
> > present in the specified cache directory. If the number is exceeded, the
> > old cache directories are reaped. The definition of "old" is private to
> > libapparmor and only 1 cache directory is currently supported. However,
> > that will change in the near future and multiple cache directories will
> > be supported.
> >
> > If 0 is specified for the max_caches parameter, no new caches can be
> > created and only an existing, valid cache can be used. An error is
> > returned if no valid caches exist in that case.
> >
> > If UINT16_MAX is specified, an unlimited amount of caches can be created
> > and reaping is disabled.
> >
> > This means that 0 to (2^16)-2, or infinite, caches will be supported in
> > the future.
> >
> > This change allows for the parser to continue to support the
> > --skip-bad-cache (by passing 0 for max_caches) and the --write-cache
> > option (by passing 1 or more for max_caches) without confusing
> > libapparmor users with the aa_policy_cache_{is_valid,create}()
> > functions.
> this is less confusing than is_valid,create() ?
It removes two calls that libapparmor consumers would need to make and
replaces them with one argument. Seems easier to me but maybe that's not
the case.
>
> the patch changes the semantics around skip-bad-cache/cond_clear_cache
>
> >
> > Signed-off-by: Tyler Hicks <tyhicks at canonical.com>
> > ---
> > libraries/libapparmor/include/sys/apparmor.h | 4 +-
> > libraries/libapparmor/src/libapparmor.map | 2 +-
> > libraries/libapparmor/src/policy_cache.c | 56 ++++++------
> > parser/parser_main.c | 22 ++---
> > tests/regression/apparmor/aa_policy_cache.c | 130 +++++++++------------------
> > tests/regression/apparmor/aa_policy_cache.sh | 22 ++---
> > 6 files changed, 88 insertions(+), 148 deletions(-)
> >
> > diff --git a/libraries/libapparmor/include/sys/apparmor.h b/libraries/libapparmor/include/sys/apparmor.h
> > index 99ce36b..743d967 100644
> > --- a/libraries/libapparmor/include/sys/apparmor.h
> > +++ b/libraries/libapparmor/include/sys/apparmor.h
> > @@ -142,12 +142,10 @@ int aa_kernel_interface_write_policy(int fd, const char *buffer, size_t size);
> > typedef struct aa_policy_cache aa_policy_cache;
> > int aa_policy_cache_new(aa_policy_cache **policy_cache,
> > aa_features *kernel_features, const char *path,
> > - bool create);
> > + uint16_t max_caches);
> > 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_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_is_valid has been left in the map despite being removed
Thanks - I'll remove that.
>
> > - 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..b4391b1 100644
> > --- a/libraries/libapparmor/src/policy_cache.c
> > +++ b/libraries/libapparmor/src/policy_cache.c
> > @@ -85,16 +85,29 @@ error:
> > static int init_cache_features(aa_policy_cache *policy_cache,
> > aa_features *kernel_features, bool create)
> + bool cond_clear_cache
>
> > {
> > + bool call_create_cache = false;
> > +
> > if (aa_features_new(&policy_cache->features,
> > policy_cache->features_path)) {
> > policy_cache->features = NULL;
> > if (!create || errno != ENOENT)
> > return -1;
> >
> > - return create_cache(policy_cache, kernel_features);
> > + /* The cache directory needs to be created */
> > + call_create_cache = true;
> > + } else if (!aa_features_is_equal(policy_cache->features,
> > + kernel_features)) {
> > + if (!create) {
> if (!create || !cond_clear) {
>
> > + errno = ENOENT;
> > + return -1;
> > + }
> > +
> > + /* The cache directory needs to be refreshed */
> > + call_create_cache = true;
> > }
> >
> > - return 0;
> > + return call_create_cache ?
> > + create_cache(policy_cache, kernel_features) : 0;
> > }
> >
> > struct replace_all_cb_data {
> > @@ -131,16 +144,21 @@ static int replace_all_cb(DIR *dir unused, const char *name, struct stat *st,
> > * aa_policy_cache_new object upon success
> > * @kernel_features: features representing the currently running kernel
> > * @path: path to the policy cache
> > - * @create: true if the cache should be created if it doesn't already exist
> > + * @max_caches: The maximum number of policy caches, one for each unique set of
> > + * kernel features, before older caches are auto-reaped. 0 means
> > + * that no new caches should be created (existing, valid caches
> > + * will be used) and auto-reaping is disabled. UINT16_MAX means
> > + * that a cache can be created and auto-reaping is disabled.
> > *
> > * Returns: 0 on success, -1 on error with errno set and *@policy_cache
> > * pointing to NULL
> > */
> > int aa_policy_cache_new(aa_policy_cache **policy_cache,
> > aa_features *kernel_features, const char *path,
> > - bool create)
> > + uint16_t max_caches)
> > {
> > aa_policy_cache *pc;
> > + bool create = max_caches > 0;
> >
> > *policy_cache = NULL;
> >
> > @@ -149,6 +167,11 @@ int aa_policy_cache_new(aa_policy_cache **policy_cache,
> > return -1;
> > }
> >
> > + if (max_caches > 1) {
> > + errno = ENOTSUP;
> > + return -1;
> > + }
> > +
> > pc = calloc(1, sizeof(*pc));
> > if (!pc) {
> > errno = ENOMEM;
> > @@ -212,31 +235,6 @@ void aa_policy_cache_unref(aa_policy_cache *policy_cache)
> > }
> >
> > /**
> > - * aa_policy_cache_is_valid - checks if the policy_cache is valid for the currently running kernel
> > - * @policy_cache: the policy_cache
> > - *
> > - * Returns: true if the policy_cache is valid for the currently running kernel,
> > - * false if not
> > - */
> > -bool aa_policy_cache_is_valid(aa_policy_cache *policy_cache)
> > -{
> > - return aa_features_is_equal(policy_cache->features,
> > - policy_cache->kernel_features);
> > -}
> > -
> > -/**
> > - * aa_policy_cache_create - creates a valid policy_cache 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)
> > -{
> > - return create_cache(policy_cache, policy_cache->kernel_features);
> > -}
> > -
> > -/**
> > * aa_policy_cache_remove - removes all policy cache files under a path
> > * @path: the path to a policy cache directory
> > *
> > diff --git a/parser/parser_main.c b/parser/parser_main.c
> > index 8aee148..3ba2d1a 100644
> > --- a/parser/parser_main.c
> > +++ b/parser/parser_main.c
> > @@ -898,6 +898,8 @@ int main(int argc, char *argv[])
> >
> > if ((!skip_cache && (write_cache || !skip_read_cache)) ||
> > force_clear_cache) {
> > + uint16_t max_caches = write_cache && cond_clear_cache ? 1 : 0;
> > +
> > if (!cacheloc && asprintf(&cacheloc, "%s/cache", basedir) == -1) {
> > PERROR(_("Memory allocation error."));
> > return 1;
> > @@ -917,7 +919,7 @@ int main(int argc, char *argv[])
> > pwarn(_("The --create-cache-dir option is deprecated. Please use --write-cache.\n"));
> >
> > retval = aa_policy_cache_new(&policy_cache, features, cacheloc,
> > - write_cache);
> > + max_caches);
>
> This is not correct, previously aa_policy_cache_new was called with its create parameter set only
> by write_cache. Now it depends on write_cache && cond_clear_cache
>
> aa_policy_cache_new will need another parameter to get the semanitcs of cond_clear_cache right.
I think --skip-bad-cache/cond_clear_cache semantics are still preserved.
(Note that the policy cache tests still pass)
By default, write_cache is 0 and cond_clear_cache is 1. That means that
max_caches will be 0 and no new cache files will be created.
If the --write-cache option is specified, write_cache is set to 1 and,
by default, cond_clear_cache is 1 so max_caches will be 1. That means
that invalid cache files will be cleared and new cache files will be
written.
If the --skip-bad-cache option is specified, cond_clear_cache is 0 and,
by default, write_cache is 0 so max_caches will be 0. That means that
only existing cache files will be preserved and no new cache files will
be written.
If the --write-cache and --skip-bad-cache options are specified,
write_cache is set to 1 and cond_clear_cache is set to 0 so max_caches
will be 0. At first glance, it feels like this is where the semantics
break down. However, the writing of new cache files isn't yet handled by
libapparmor. It is still controlled by the write_cache global in the
parser and is performed by the process_profile() function in
parser_main.c.
Is there a scenario that I'm missing? I'll be happy to add tests to
parser/tst/caching.py for that scenario and then make the necessary
adjustments.
Tyler
>
>
> > if (retval) {
> > if (errno != ENOENT) {
> > PERROR(_("Failed setting up policy cache (%s): %s\n"),
> > @@ -925,22 +927,16 @@ int main(int argc, char *argv[])
> > return 1;
> > }
> >
> > - write_cache = 0;
> > - 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)) {
> > - if (show_cache)
> > + if (show_cache) {
> > + if (max_caches > 0)
> > PERROR("Cache write disabled: Cannot create cache '%s': %m\n",
> > cacheloc);
> > - write_cache = 0;
> > - skip_read_cache = 1;
> > - } else if (!write_cache || !cond_clear_cache) {
> > - if (show_cache)
> > + else
> > PERROR("Cache read/write disabled: Policy cache is invalid\n");
> > - write_cache = 0;
> > - skip_read_cache = 1;
> > }
> > +
> > + write_cache = 0;
> > + skip_read_cache = 1;
> > }
> > }
> >
> > diff --git a/tests/regression/apparmor/aa_policy_cache.c b/tests/regression/apparmor/aa_policy_cache.c
> > index b08fd1f..48b3a71 100644
> > --- a/tests/regression/apparmor/aa_policy_cache.c
> > +++ b/tests/regression/apparmor/aa_policy_cache.c
> > @@ -16,36 +16,33 @@
> >
> > #include <errno.h>
> > #include <fcntl.h>
> > +#include <limits.h>
> > #include <stdio.h>
> > #include <stdlib.h>
> > #include <string.h>
> >
> > #include <sys/apparmor.h>
> >
> > -#define OPT_CREATE "create"
> > -#define OPT_IS_VALID "is-valid"
> > #define OPT_NEW "new"
> > -#define OPT_NEW_CREATE "new-create"
> > #define OPT_REMOVE "remove"
> > #define OPT_REMOVE_POLICY "remove-policy"
> > #define OPT_REPLACE_ALL "replace-all"
> > +#define OPT_FLAG_MAX_CACHES "--max-caches"
> >
> > static void usage(const char *prog)
> > {
> > fprintf(stderr,
> > - "FAIL - usage: %s %s <PATH>\n"
> > - " %s %s <PATH>\n"
> > - " %s %s <PATH>\n"
> > - " %s %s <PATH>\n"
> > + "FAIL - usage: %s %s [%s N] <PATH>\n"
> > " %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_NEW_CREATE, prog, OPT_REMOVE, prog, OPT_REMOVE_POLICY,
> > - prog, OPT_REPLACE_ALL);
> > + " %s %s [%s N] <PATH>\n",
> > + prog, OPT_NEW, OPT_FLAG_MAX_CACHES,
> > + prog, OPT_REMOVE,
> > + prog, OPT_REMOVE_POLICY,
> > + prog, OPT_REPLACE_ALL, OPT_FLAG_MAX_CACHES);
> > }
> >
> > -static int test_create(const char *path)
> > +static int test_new(const char *path, uint16_t max_caches)
> > {
> > aa_features *features = NULL;
> > aa_policy_cache *policy_cache = NULL;
> > @@ -56,64 +53,7 @@ static int test_create(const char *path)
> > goto out;
> > }
> >
> > - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> > - perror("FAIL - aa_policy_cache_new");
> > - goto out;
> > - }
> > -
> > - if (aa_policy_cache_create(policy_cache)) {
> > - perror("FAIL - aa_policy_cache_create");
> > - goto out;
> > - }
> > -
> > - rc = 0;
> > -out:
> > - aa_features_unref(features);
> > - aa_policy_cache_unref(policy_cache);
> > - return rc;
> > -}
> > -
> > -static int test_is_valid(const char *path)
> > -{
> > - aa_features *features = NULL;
> > - aa_policy_cache *policy_cache = NULL;
> > - int rc = 1;
> > -
> > - if (aa_features_new_from_kernel(&features)) {
> > - perror("FAIL - aa_features_new_from_kernel");
> > - goto out;
> > - }
> > -
> > - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> > - perror("FAIL - aa_policy_cache_new");
> > - goto out;
> > - }
> > -
> > - if (!aa_policy_cache_is_valid(policy_cache)) {
> > - errno = EINVAL;
> > - perror("FAIL - aa_policy_cache_is_valid");
> > - goto out;
> > - }
> > -
> > - rc = 0;
> > -out:
> > - aa_features_unref(features);
> > - aa_policy_cache_unref(policy_cache);
> > - return rc;
> > -}
> > -
> > -static int test_new(const char *path, bool create)
> > -{
> > - aa_features *features = NULL;
> > - aa_policy_cache *policy_cache = NULL;
> > - int rc = 1;
> > -
> > - if (aa_features_new_from_kernel(&features)) {
> > - perror("FAIL - aa_features_new_from_kernel");
> > - goto out;
> > - }
> > -
> > - if (aa_policy_cache_new(&policy_cache, features, path, create)) {
> > + if (aa_policy_cache_new(&policy_cache, features, path, max_caches)) {
> > perror("FAIL - aa_policy_cache_new");
> > goto out;
> > }
> > @@ -167,7 +107,7 @@ out:
> > return rc;
> > }
> >
> > -static int test_replace_all(const char *path)
> > +static int test_replace_all(const char *path, uint16_t max_caches)
> > {
> > aa_features *features = NULL;
> > aa_policy_cache *policy_cache = NULL;
> > @@ -178,7 +118,7 @@ static int test_replace_all(const char *path)
> > goto out;
> > }
> >
> > - if (aa_policy_cache_new(&policy_cache, features, path, false)) {
> > + if (aa_policy_cache_new(&policy_cache, features, path, max_caches)) {
> > perror("FAIL - aa_policy_cache_new");
> > goto out;
> > }
> > @@ -197,27 +137,45 @@ out:
> >
> > int main(int argc, char **argv)
> > {
> > + uint16_t max_caches = 0;
> > + const char *str = NULL;
> > int rc = 1;
> >
> > - if (argc != 3) {
> > + if (!(argc == 3 || argc == 5)) {
> > usage(argv[0]);
> > exit(1);
> > }
> >
> > - if (strcmp(argv[1], OPT_CREATE) == 0) {
> > - rc = test_create(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) {
> > - rc = test_new(argv[2], false);
> > - } else if (strcmp(argv[1], OPT_NEW_CREATE) == 0) {
> > - rc = test_new(argv[2], true);
> > - } else if (strcmp(argv[1], OPT_REMOVE) == 0) {
> > - rc = test_remove(argv[2]);
> > - } else if (strcmp(argv[1], OPT_REMOVE_POLICY) == 0) {
> > - rc = test_remove_policy(argv[2]);
> > + str = argv[argc - 1];
> > +
> > + if (argc == 5) {
> > + unsigned long tmp;
> > +
> > + errno = 0;
> > + tmp = strtoul(argv[3], NULL, 10);
> > + if ((errno == ERANGE && tmp == ULONG_MAX) ||
> > + (errno && tmp == 0)) {
> > + perror("FAIL - strtoul");
> > + exit(1);
> > + }
> > +
> > + if (tmp > UINT16_MAX) {
> > + fprintf(stderr, "FAIL - %lu larger than UINT16_MAX\n",
> > + tmp);
> > + exit(1);
> > + }
> > +
> > + max_caches = tmp;
> > + }
> > +
> > + if (strcmp(argv[1], OPT_NEW) == 0) {
> > + rc = test_new(str, max_caches);
> > + } else if (strcmp(argv[1], OPT_REMOVE) == 0 && argc == 3) {
> > + rc = test_remove(str);
> > + } else if (strcmp(argv[1], OPT_REMOVE_POLICY) == 0 && argc == 3) {
> > + rc = test_remove_policy(str);
> > } else if (strcmp(argv[1], OPT_REPLACE_ALL) == 0) {
> > - rc = test_replace_all(argv[2]);
> > + rc = test_replace_all(str, max_caches);
> > } else {
> > usage(argv[0]);
> > }
> > diff --git a/tests/regression/apparmor/aa_policy_cache.sh b/tests/regression/apparmor/aa_policy_cache.sh
> > index fb9a830..dfb75a8 100755
> > --- a/tests/regression/apparmor/aa_policy_cache.sh
> > +++ b/tests/regression/apparmor/aa_policy_cache.sh
> > @@ -36,7 +36,7 @@ remove_cachedir()
> >
> > create_empty_cache()
> > {
> > - $test new-create "$cachedir" > /dev/null
> > + $test new --max-caches 1 "$cachedir" > /dev/null
> > }
> >
> > create_cache_files()
> > @@ -105,28 +105,18 @@ runchecktest "AA_POLICY_CACHE new (no cachedir)" fail new "$cachedir"
> > create_cachedir
> > runchecktest "AA_POLICY_CACHE new (no .features)" fail new "$cachedir"
> > remove_cachedir
> > -runchecktest "AA_POLICY_CACHE new-create (no cachedir)" pass new-create "$cachedir"
> > -runchecktest "AA_POLICY_CACHE new-create (existing cache)" pass new-create "$cachedir"
> > +runchecktest "AA_POLICY_CACHE new create (no cachedir)" pass new --max-caches 1 "$cachedir"
> > +runchecktest "AA_POLICY_CACHE new create (existing cache)" pass new --max-caches 1 "$cachedir"
> > runchecktest "AA_POLICY_CACHE new (existing cache)" pass new "$cachedir"
> >
> > -runchecktest "AA_POLICY_CACHE is-valid (good .features)" pass is-valid "$cachedir"
> > install_bad_features_file
> > -runchecktest "AA_POLICY_CACHE is-valid (bad .features)" fail is-valid "$cachedir"
> > -remove_cachedir
> > -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"
> > -remove_features_file
> > -runchecktest "AA_POLICY_CACHE create (no .features)" fail create "$cachedir"
> > -remove_cachedir
> > -runchecktest "AA_POLICY_CACHE create (no cachedir)" fail create "$cachedir"
> > +runchecktest "AA_POLICY_CACHE new (bad .features)" fail new "$cachedir"
> > +runchecktest "AA_POLICY_CACHE new create (bad .features)" pass new --max-caches 1 "$cachedir"
> >
> > # Make sure that no test policies are already loaded
> > verify_policies_are_not_loaded
> >
> > +remove_cachedir
> > runchecktest "AA_POLICY_CACHE replace-all (no cachedir)" fail replace-all "$cachedir"
> > create_cachedir
> > runchecktest "AA_POLICY_CACHE replace-all (no .features)" fail replace-all "$cachedir"
> >
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150529/8925e16c/attachment-0001.pgp>
More information about the AppArmor
mailing list