[libvirt] [PATCH v4 15/23] security_manager: Load lock plugin on init

[John Ferlan]

On 09/10/2018 05:36 AM, Michal Privoznik wrote:
> Now that we know what metadata lock manager user wishes to use we
> can load it when initializing security driver. This is achieved
> by adding new argument to virSecurityManagerNewDriver() and
> subsequently to all functions that end up calling it.
> 
> The cfg.mk change is needed in order to allow lock_manager.h
> inclusion in security driver without 'syntax-check' complaining.
> This is safe thing to do as locking APIs will always exist (it's
> only backend implementation that changes). However, instead of
> allowing the include for all other drivers (like cpu, network,
> and so on) allow it only for security driver. This will still
> trigger the error if including from other drivers.
> 
> Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
> ---
>  cfg.mk                           |  4 +++-
>  src/lxc/lxc_controller.c         |  3 ++-
>  src/lxc/lxc_driver.c             |  2 +-
>  src/qemu/qemu_driver.c           |  3 +++
>  src/security/security_manager.c  | 22 ++++++++++++++++++++++
>  src/security/security_manager.h  |  2 ++
>  tests/seclabeltest.c             |  2 +-
>  tests/securityselinuxlabeltest.c |  2 +-
>  tests/securityselinuxtest.c      |  2 +-
>  tests/testutilsqemu.c            |  2 +-
>  10 files changed, 37 insertions(+), 7 deletions(-)
> 
> diff --git a/cfg.mk b/cfg.mk
> index 609ae869c2..e0a7b5105a 100644
> --- a/cfg.mk
> +++ b/cfg.mk
> @@ -787,8 +787,10 @@ sc_prohibit_cross_inclusion:
>  	  case $$dir in \
>  	    util/) safe="util";; \
>  	    access/ | conf/) safe="($$dir|conf|util)";; \
> -	    cpu/| network/| node_device/| rpc/| security/| storage/) \
> +	    cpu/| network/| node_device/| rpc/| storage/) \
>  	      safe="($$dir|util|conf|storage)";; \
> +	    security/) \
> +	      safe="($$dir|util|conf|storage|locking)";; \
>  	    xenapi/ | xenconfig/ ) safe="($$dir|util|conf|xen|cpu)";; \
>  	    *) safe="($$dir|$(mid_dirs)|util)";; \
>  	  esac; \
> diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c
> index 4e84391bf5..5f957eb1f8 100644
> --- a/src/lxc/lxc_controller.c
> +++ b/src/lxc/lxc_controller.c
> @@ -2625,7 +2625,8 @@ int main(int argc, char *argv[])
>      ctrl->handshakeFd = handshakeFd;
>  
>      if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver,
> -                                                        LXC_DRIVER_NAME, 0)))
> +                                                        LXC_DRIVER_NAME,
> +                                                        "nop", 0)))
>          goto cleanup;
>  
>      if (ctrl->def->seclabels) {
> diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c
> index 8867645cdc..93aa25e9e6 100644
> --- a/src/lxc/lxc_driver.c
> +++ b/src/lxc/lxc_driver.c
> @@ -1532,7 +1532,7 @@ lxcSecurityInit(virLXCDriverConfigPtr cfg)
>          flags |= VIR_SECURITY_MANAGER_REQUIRE_CONFINED;
>  
>      virSecurityManagerPtr mgr = virSecurityManagerNew(cfg->securityDriverName,
> -                                                      LXC_DRIVER_NAME, flags);
> +                                                      LXC_DRIVER_NAME, "nop", flags);

I think we should use NULL instead of "nop"?  Keeping the default of
"nop" under the covers (so to speak). Similar for other callers...

>      if (!mgr)
>          goto error;
>  

[...]

> diff --git a/src/security/security_manager.c b/src/security/security_manager.c
> index 9f770d8c53..5c8370c159 100644
> --- a/src/security/security_manager.c
> +++ b/src/security/security_manager.c

[...]

>  static virClassPtr virSecurityManagerClass;
> @@ -52,6 +55,9 @@ void virSecurityManagerDispose(void *obj)
>  
>      if (mgr->drv->close)
>          mgr->drv->close(mgr);

Order/setup issue here mgr->drv-> cannot be assumed right now!

> +
> +    virObjectUnref(mgr->lockPlugin);
> +
>      VIR_FREE(mgr->privateData);
>  }
>  
> @@ -71,6 +77,7 @@ VIR_ONCE_GLOBAL_INIT(virSecurityManager);
>  static virSecurityManagerPtr
>  virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>                              const char *virtDriver,
> +                            const char *lockManagerPluginName,
>                              unsigned int flags)
>  {
>      virSecurityManagerPtr mgr = NULL;
> @@ -90,6 +97,14 @@ virSecurityManagerNewDriver(virSecurityDriverPtr drv,
>      if (!(mgr = virObjectLockableNew(virSecurityManagerClass)))
>          goto error;
>  
> +    if (!lockManagerPluginName)
> +        lockManagerPluginName = "nop";
> +
> +    if (!(mgr->lockPlugin = virLockManagerPluginNew(lockManagerPluginName,
> +                                                    NULL, NULL, 0))) {
> +        goto error;
> +    }
> +

Want to watching things die spectacularly?  Don't pass NULL, "nop", or
"lockd" here... For example, change securityselinuxlabeltest.c to pass
"foobar" and enjoy.

Problem is that mgr->drv-> and eventually mgr->fd == -1 is assumed in
virSecurityManagerDispose.

>      mgr->drv = drv;
>      mgr->flags = flags;
>      mgr->virtDriver = virtDriver;

[...]

> diff --git a/tests/securityselinuxlabeltest.c b/tests/securityselinuxlabeltest.c
> index 48fee7cd28..85797411eb 100644
> --- a/tests/securityselinuxlabeltest.c
> +++ b/tests/securityselinuxlabeltest.c
> @@ -349,7 +349,7 @@ mymain(void)
>      if (!rc)
>          return EXIT_AM_SKIP;
>  
> -    if (!(mgr = virSecurityManagerNew("selinux", "QEMU",
> +    if (!(mgr = virSecurityManagerNew("selinux", "QEMU", "nop",

Just for "completeness", why not pass "lockd" here?

Wonder if it's worth creating a false test that passes a non existent
image (foobar) just to ensure it fails properly so that someone doesn't
inadvertently undo what you'll need to change...

>                                        VIR_SECURITY_MANAGER_DEFAULT_CONFINED |
>                                        VIR_SECURITY_MANAGER_PRIVILEGED))) {
>          VIR_TEST_VERBOSE("Unable to initialize security driver: %s\n",

With the callers passing NULL instead of "nop" and cleaning up the
New/Dispose code properly -

Reviewed-by: John Ferlan <jferlan at redhat.com>

John

[...]