Re: [PATCH v8 02/12] ndctl: add passphrase update to ndctl
On 1/16/19 10:43 AM, Verma, Vishal L wrote: > > On Tue, 2019-01-15 at 17:56 -0800, Dan Williams wrote: >> Some comments below... >> >> On Mon, Jan 14, 2019 at 12:06 PM Dave Jiang 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 >>> --- >>> 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..7adb 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 ..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' [] >> >> 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--.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? > > I also noticed the actual file created was of the format: > > "nvdimm--.blob" > > One of them should be made consistent with the other.. I'll fix the documentation. I changed the format for ease of parsing sometimes during the development and missed the update to the documentation here. > >> >>> + >>> +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--.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 fi
Re: [PATCH v8 02/12] ndctl: add passphrase update to ndctl
On Tue, 2019-01-15 at 17:56 -0800, Dan Williams wrote: > Some comments below... > > On Mon, Jan 14, 2019 at 12:06 PM Dave Jiang 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 > > --- > > 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..7adb 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 ..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' [] > > 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--.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? I also noticed the actual file created was of the format: "nvdimm--.blob" One of them should be made consistent with the other.. > > > + > > +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--.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 > > +--- > > +:: > > +include::xable-dimm-options.txt[] > > + > > +-m:: > > +--master=:: > > + Key name for the master key us
Re: [PATCH v8 02/12] ndctl: add passphrase update to ndctl
Some comments below... On Mon, Jan 14, 2019 at 12:06 PM Dave Jiang 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 > --- > 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..7adb 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 ..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' [] 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--.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--.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 > +--- > +:: > +include::xable-dimm-options.txt[] > + > +-m:: > +--master=:: > + Key name for the master key used to seal the NVDIMM security keys. > + The format would be : > + 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 some
[PATCH v8 02/12] ndctl: add passphrase update to ndctl
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
---
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..7adb 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 ..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
+
+SYNOPSIS
+
+[verse]
+'ndctl enable-passphrase' []
+
+DESCRIPTION
+---
+Provide a command to enable the security passphrase for the NVDIMM.
+It is expected that the master key has already been loaded into the user
+key ring. The encrypted key blobs will be created in /etc/nvdimm directory
+with the file name of "nvdimm--.blob".
+
+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. Do not touch the /etc/nvdimm
+directory and let ndctl manage the keys, unless you know what you are doing.
+
+OPTIONS
+---
+::
+include::xable-dimm-options.txt[]
+
+-m::
+--master=::
+ Key name for the master key used to seal the NVDIMM security keys.
+ The format would be :
+ i.e.: trusted:master-nvdimm
+
+-p::
+--key-path=::
+ Path to where key related files resides. This parameter is optional
+ and the default is set to /etc/ndctl/keys.
+
+include::../copyright.txt[]
diff --git a/Documentation/ndctl/ndctl-update-passphrase.txt
b/Documentation/ndctl/ndctl-update-passphrase.txt
new file mode 100644
index ..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
+
+SYNOPSIS
+
+[verse]
+'ndctl update-passphrase' []
+
+DESCRIPTION
+---
+Provide a command to update the security key for NVDIMM.
+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--.blob".
+
+OPTIONS
+---
+::
+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.
+
+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).
@<:@de
