Re: [PATCH v8 02/12] ndctl: add passphrase update to ndctl

[Dan Williams <dan.j.williams@intel.com>]

Some comments below...

On Mon, Jan 14, 2019 at 12:06 PM Dave Jiang <dave.jiang@intel.com> wrote:
>
> Add API call for triggering sysfs knob to update the security for a DIMM
> in libndctl. Also add the ndctl "update-passphrase" to trigger the
> operation.
>
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> ---
>  Documentation/ndctl/Makefile.am                 |    4
>  Documentation/ndctl/ndctl-enable-passphrase.txt |   42 ++
>  Documentation/ndctl/ndctl-update-passphrase.txt |   38 ++
>  configure.ac                                    |   19 +
>  ndctl.spec.in                                   |    2
>  ndctl/Makefile.am                               |    3
>  ndctl/builtin.h                                 |    2
>  ndctl/dimm.c                                    |   94 +++++-
>  ndctl/lib/Makefile.am                           |    8
>  ndctl/lib/dimm.c                                |   39 ++
>  ndctl/lib/keys.c                                |  390 +++++++++++++++++++++++
>  ndctl/lib/libndctl.sym                          |    4
>  ndctl/libndctl.h                                |   35 ++
>  ndctl/ndctl.c                                   |    2
>  14 files changed, 669 insertions(+), 13 deletions(-)
>  create mode 100644 Documentation/ndctl/ndctl-enable-passphrase.txt
>  create mode 100644 Documentation/ndctl/ndctl-update-passphrase.txt
>  create mode 100644 ndctl/lib/keys.c
>
> diff --git a/Documentation/ndctl/Makefile.am b/Documentation/ndctl/Makefile.am
> index a30b139b..7ad6666b 100644
> --- a/Documentation/ndctl/Makefile.am
> +++ b/Documentation/ndctl/Makefile.am
> @@ -47,7 +47,9 @@ man1_MANS = \
>         ndctl-inject-smart.1 \
>         ndctl-update-firmware.1 \
>         ndctl-list.1 \
> -       ndctl-monitor.1
> +       ndctl-monitor.1 \
> +       ndctl-enable-passphrase.1 \
> +       ndctl-update-passphrase.1
>
>  CLEANFILES = $(man1_MANS)
>
> diff --git a/Documentation/ndctl/ndctl-enable-passphrase.txt b/Documentation/ndctl/ndctl-enable-passphrase.txt
> new file mode 100644
> index 00000000..c14a206c
> --- /dev/null
> +++ b/Documentation/ndctl/ndctl-enable-passphrase.txt
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +ndctl-enable-passphrase(1)
> +==========================
> +
> +NAME
> +----
> +ndctl-enable-passphrase - enable the security passphrase for a NVDIMM

*an NVDIMM

> +
> +SYNOPSIS
> +--------
> +[verse]
> +'ndctl enable-passphrase' <dimm> [<options>]

Is this true, there are no other required options besides the nmem
device? No support for more than one nmem at a time?

> +
> +DESCRIPTION
> +-----------
> +Provide a command to enable the security passphrase for the NVDIMM.

No need to say "Provide a command" that's assumed for a man page named
after a binary.

So I'd say "Enable the security passphrase for one or more NVDIMMs.

> +It is expected that the master key has already been loaded into the user

Without listing the key-id as a required parameter it's not clear what
the usage should be.

> +key ring. The encrypted key blobs will be created in /etc/nvdimm directory

Is this stale, should be /etc/ndctl/keys, right? Given the directory
is changeable by the build system this source file should be built
with the key directory as a variable.

> +with the file name of "nvdimm-<hostname>-<dimm unique id>.blob".

That's quite a bit to type out? Is there a command to discover this
file name, or can we provide a short-hand?

> +
> +The command will fail if the nvdimm key is already in the user key ring and/or
> +the key blob already resides in /etc/nvdimm.

I feel like this is missing a create-passphrase step, and/or that
there needs to be an example in the man page. The man page should show
sohow to create the pre-requisite material, or reference another
command that will handle the details. I don't think the user interface
should ever expose "nvdimm-<hostname>-<dimm unique id>.blob" as
something the user needs to type... if at all possible. Maybe a global
"set-kek" command to write out the key-encryption-key to a
configuration file that enable-passphrase can consult by default
rather than require it to be passed to every enable-passphrase
invocation?

> Do not touch the /etc/nvdimm
> +directory and let ndctl manage the keys, unless you know what you are doing.

I think the "unless you know what you are doing." sentiment goes
without saying for a man page. If anything I'd ship a README file in
/etc/ndctl/keys if you're worried about manual edits to that
directory.

> +
> +OPTIONS
> +-------
> +<dimm>::
> +include::xable-dimm-options.txt[]
> +
> +-m::
> +--master=::
> +       Key name for the master key used to seal the NVDIMM security keys.
> +       The format would be <key_type>:<master_key_name>
> +       i.e.: trusted:master-nvdimm

"master" is ambiguous when we have master passphrases and master keys.
Let's call it the KEK (key-encryption-key) and reserve "master" for
"master passphrase".

> +-p::
> +--key-path=::
> +       Path to where key related files resides. This parameter is optional
> +       and the default is set to /etc/ndctl/keys.

Is this flexibility needed? Seems like something that can be omitted
unless/until a need arises.

> +
> +include::../copyright.txt[]
> diff --git a/Documentation/ndctl/ndctl-update-passphrase.txt b/Documentation/ndctl/ndctl-update-passphrase.txt
> new file mode 100644
> index 00000000..dd6e4e4e
> --- /dev/null
> +++ b/Documentation/ndctl/ndctl-update-passphrase.txt
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +ndctl-update-passphrase(1)
> +==========================
> +
> +NAME
> +----
> +ndctl-update-passphrase - update the security passphrase for a NVDIMM

*an NVDIMM

> +
> +SYNOPSIS
> +--------
> +[verse]
> +'ndctl update-passphrase' <dimm> [<options>]

Same comment about required options.

> +
> +DESCRIPTION
> +-----------
> +Provide a command to update the security key for NVDIMM.

Same comment about "Provide a command"

> +It is expected that the current and the new (if new master key is desired)
> +master key has already been loaded into the user key ring. The new encrypted
> +key blobs will be created in /etc/nvdimm directory
> +with the file name of "nvdimm-<hostname>-<dimm unique id>.blob".
> +
> +OPTIONS
> +-------
> +<dimm>::
> +include::xable-dimm-options.txt[]
> +
> +-m::
> +--master::
> +       New key name for the master key to seal the new nvdimm key, or the
> +       existing master key name. i.e trusted:master-key.
> +
> +-p::
> +--key-path=::
> +       Path to where key related files resides. This parameter is optional
> +       and the default is set to /etc/ndctl/keys.

Same comment about an example and the need for key-path.

> +
> +include::../copyright.txt[]
> diff --git a/configure.ac b/configure.ac
> index aa07ec7b..22efc871 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -154,6 +154,7 @@ fi
>  AC_SUBST([systemd_unitdir])
>  AM_CONDITIONAL([ENABLE_SYSTEMD_UNITS], [test "x$with_systemd" = "xyes"])
>
> +
>  ndctl_monitorconfdir=${sysconfdir}/ndctl
>  ndctl_monitorconf=monitor.conf
>  AC_SUBST([ndctl_monitorconfdir])
> @@ -161,6 +162,24 @@ AC_SUBST([ndctl_monitorconf])
>  AC_DEFINE_UNQUOTED(NDCTL_CONF_FILE, ["$ndctl_monitorconfdir/$ndctl_monitorconf"],
>         [default ndctl monitor conf path])
>
> +AC_ARG_WITH([keyutils],
> +           AS_HELP_STRING([--with-keyutils],
> +                       [Enable keyutils functionality (security).  @<:@default=yes@:>@]), [], [with_keyutils=yes])
> +
> +if test "x$with_keyutils" = "xyes"; then
> +       AC_CHECK_HEADERS([keyutils.h],,[
> +               AC_MSG_ERROR([keyutils.h not found, consider installing
> +                             keyutils-libs-devel.])
> +               ])
> +fi
> +AS_IF([test "x$with_keyutils" = "xyes"],
> +       [AC_DEFINE([ENABLE_KEYUTILS], [1], [Enable keyutils support])])
> +AM_CONDITIONAL([ENABLE_KEYUTILS], [test "x$with_keyutils" = "xyes"])
> +ndctl_keysdir=${sysconfdir}/ndctl/keys
> +AC_SUBST([ndctl_keysdir])
> +AC_DEFINE_UNQUOTED(NDCTL_KEYS_DIR, ["$ndctl_keysdir"],
> +       [default ndctl keys path])

My bad, apparently AC_DEFINE_UNQUOTED falls over if $ndctl_keysdir is
made up of components that need further expansion. Instead add
NDCTL_KEYS_DIR to the new autogenerated ndctl/config.h file. See this
pending patch where I killed off AC_DEFINE_UNQUOTED usage:
https://patchwork.kernel.org/patch/10763963/

> +
>  my_CFLAGS="\
>  -Wall \
>  -Wchar-subscripts \
> diff --git a/ndctl.spec.in b/ndctl.spec.in
> index 26396d4a..66466db6 100644
> --- a/ndctl.spec.in
> +++ b/ndctl.spec.in
> @@ -21,6 +21,7 @@ BuildRequires:        pkgconfig(uuid)
>  BuildRequires: pkgconfig(json-c)
>  BuildRequires: pkgconfig(bash-completion)
>  BuildRequires: pkgconfig(systemd)
> +BuildRequires: keyutils-libs-devel
>
>  %description
>  Utility library for managing the "libnvdimm" subsystem.  The "libnvdimm"
> @@ -119,6 +120,7 @@ make check
>  %{bashcompdir}/
>  %{_sysconfdir}/ndctl/monitor.conf
>  %{_unitdir}/ndctl-monitor.service
> +%{_sysconfdir}/ndctl/keys/
>
>  %files -n daxctl
>  %defattr(-,root,root)
> diff --git a/ndctl/Makefile.am b/ndctl/Makefile.am
> index ff01e068..120941a4 100644
> --- a/ndctl/Makefile.am
> +++ b/ndctl/Makefile.am
> @@ -30,7 +30,8 @@ ndctl_LDADD =\
>         ../libutil.a \
>         $(UUID_LIBS) \
>         $(KMOD_LIBS) \
> -       $(JSON_LIBS)
> +       $(JSON_LIBS) \
> +       -lkeyutils

This should be conditional on whether ndctl was built with keyutils support.

...reading ahead in the patch I don't think ndctl actually has a
direct dependency on keyutils, right? It's abstracted behind the
libndctl routines, so do we need this?

[..]
> diff --git a/ndctl/lib/dimm.c b/ndctl/lib/dimm.c
> index e03135d9..b64c9568 100644
> --- a/ndctl/lib/dimm.c
> +++ b/ndctl/lib/dimm.c
> @@ -624,3 +624,42 @@ NDCTL_EXPORT int ndctl_dimm_get_security(struct ndctl_dimm *dimm,
>
>         return 0;
>  }
> +
> +NDCTL_EXPORT bool ndctl_dimm_security_supported(struct ndctl_dimm *dimm)
> +{
> +       enum nd_security_state state;
> +       int rc;
> +
> +       rc = ndctl_dimm_get_security(dimm, &state);
> +       if (rc < 0)
> +               return false;
> +
> +       if (state == ND_SECURITY_UNSUPPORTED)
> +               return false;
> +
> +       return true;
> +}

I think the need for this goes away when ndctl_dimm_get_security()
returns the state directly.

[..]
> diff --git a/ndctl/ndctl.c b/ndctl/ndctl.c
> index b01594e0..9d109b34 100644
> --- a/ndctl/ndctl.c
> +++ b/ndctl/ndctl.c
> @@ -88,6 +88,8 @@ static struct cmd_struct commands[] = {
>         { "inject-smart", { cmd_inject_smart } },
>         { "wait-scrub", { cmd_wait_scrub } },
>         { "start-scrub", { cmd_start_scrub } },
> +       { "enable-passphrase", { cmd_passphrase_setup } },

Maybe call the command setup-passphrase. All the other enable commands
are just toggling dynamic state, this one is creating persistent
key-material.

> +       { "update-passphrase", { cmd_passphrase_update } },
>         { "list", { cmd_list } },
>         { "monitor", { cmd_monitor } },
>         { "help", { cmd_help } },
>
_______________________________________________
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm