| Message ID | 20190612230845.GA13679@alison-desk.jf.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 77566b4b002653ffe25ec58dcb1c23c06c5df339 |
| Headers | show |
| Series | [ndctl] ndctl, test: handle backup_keys in security.sh | expand |
On Wed, 2019-06-12 at 16:08 -0700, Alison Schofield wrote: > Fix a typo in security.sh that causes a script failure > when an nvdimm-master.blob already exists and needs to > be backed up. > > + setup_keys > + '[' '!' -d /etc/ndctl/keys ']' > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm- > master.blob.bak > + 0=1 > ./security.sh: line 39: 0=1: command not found > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit > test") > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > test/security.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Looks good, I'll pick it up for v66 and push it out to pending shortly. > > diff --git a/test/security.sh b/test/security.sh > index 8a36265..c86d2c6 100755 > --- a/test/security.sh > +++ b/test/security.sh > @@ -36,11 +36,11 @@ setup_keys() > > if [ -f "$masterpath" ]; then > mv "$masterpath" "$masterpath.bak" > - $backup_key=1 > + backup_key=1 > fi > if [ -f "$keypath/tpm.handle" ]; then > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > - $backup_handle=1 > + backup_handle=1 > fi > > dd if=/dev/urandom bs=1 count=32 2>/dev/null | keyctl padd user > "$masterkey" @u
On Wed, Jun 12, 2019 at 4:05 PM Alison Schofield <alison.schofield@intel.com> wrote: > > Fix a typo in security.sh that causes a script failure > when an nvdimm-master.blob already exists and needs to > be backed up. > > + setup_keys > + '[' '!' -d /etc/ndctl/keys ']' > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak > + 0=1 > ./security.sh: line 39: 0=1: command not found > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > --- > test/security.sh | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/test/security.sh b/test/security.sh > index 8a36265..c86d2c6 100755 > --- a/test/security.sh > +++ b/test/security.sh > @@ -36,11 +36,11 @@ setup_keys() > > if [ -f "$masterpath" ]; then > mv "$masterpath" "$masterpath.bak" > - $backup_key=1 > + backup_key=1 > fi > if [ -f "$keypath/tpm.handle" ]; then > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > - $backup_handle=1 > + backup_handle=1 > fi Looks obviously correct to me. Reviewed-by: Dan Williams <dan.j.williams@intel.com> ...but that said, why is this test even bothering with the host's configuration? I think it should be using a test local directory that does not disturb the rest of the system, especially because the test is using nfit_test resources. There's no guarantee that the script successfully reaches the post_cleanup() phase to restore the host configuration and could leave it broken. Unless / until we can fix up this test to not touch /etc I think it should be moved to the ENABLE_DESTRUCTIVE set of tests.
On Wed, 2019-06-12 at 16:20 -0700, Dan Williams wrote: > On Wed, Jun 12, 2019 at 4:05 PM Alison Schofield > <alison.schofield@intel.com> wrote: > > Fix a typo in security.sh that causes a script failure > > when an nvdimm-master.blob already exists and needs to > > be backed up. > > > > + setup_keys > > + '[' '!' -d /etc/ndctl/keys ']' > > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak > > + 0=1 > > ./security.sh: line 39: 0=1: command not found > > > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > --- > > test/security.sh | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/test/security.sh b/test/security.sh > > index 8a36265..c86d2c6 100755 > > --- a/test/security.sh > > +++ b/test/security.sh > > @@ -36,11 +36,11 @@ setup_keys() > > > > if [ -f "$masterpath" ]; then > > mv "$masterpath" "$masterpath.bak" > > - $backup_key=1 > > + backup_key=1 > > fi > > if [ -f "$keypath/tpm.handle" ]; then > > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > > - $backup_handle=1 > > + backup_handle=1 > > fi > > Looks obviously correct to me. > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > ...but that said, why is this test even bothering with the host's > configuration? I think it should be using a test local directory that > does not disturb the rest of the system, especially because the test > is using nfit_test resources. > > There's no guarantee that the script successfully reaches the > post_cleanup() phase to restore the host configuration and could leave > it broken. Unless / until we can fix up this test to not touch /etc I > think it should be moved to the ENABLE_DESTRUCTIVE set of tests. Hm, yes good point. I agree with moving it to destructive for now.
On Wed, Jun 12, 2019 at 04:26:16PM -0700, Verma, Vishal L wrote: > > On Wed, 2019-06-12 at 16:20 -0700, Dan Williams wrote: > > On Wed, Jun 12, 2019 at 4:05 PM Alison Schofield > > <alison.schofield@intel.com> wrote: > > > Fix a typo in security.sh that causes a script failure > > > when an nvdimm-master.blob already exists and needs to > > > be backed up. > > > > > > + setup_keys > > > + '[' '!' -d /etc/ndctl/keys ']' > > > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > > > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak > > > + 0=1 > > > ./security.sh: line 39: 0=1: command not found > > > > > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > > --- > > > test/security.sh | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/test/security.sh b/test/security.sh > > > index 8a36265..c86d2c6 100755 > > > --- a/test/security.sh > > > +++ b/test/security.sh > > > @@ -36,11 +36,11 @@ setup_keys() > > > > > > if [ -f "$masterpath" ]; then > > > mv "$masterpath" "$masterpath.bak" > > > - $backup_key=1 > > > + backup_key=1 > > > fi > > > if [ -f "$keypath/tpm.handle" ]; then > > > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > > > - $backup_handle=1 > > > + backup_handle=1 > > > fi > > > > Looks obviously correct to me. > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > ...but that said, why is this test even bothering with the host's > > configuration? I think it should be using a test local directory that > > does not disturb the rest of the system, especially because the test > > is using nfit_test resources. At first glance, it appears that the keys need to be in the {ndctl_keysdir}, aka, the official system location, for some of the ndctl commands to run. So, it's not as simple as just creating the key blob in a temp directory. And, I don't even think that's the nfit_test resource you are referring to anyway. I'll keep a look out for how it can run cleaner, and make it off the ENABLE_DESTRUCTIVE list in the future. > > > > There's no guarantee that the script successfully reaches the > > post_cleanup() phase to restore the host configuration and could leave > > it broken. Unless / until we can fix up this test to not touch /etc I > > think it should be moved to the ENABLE_DESTRUCTIVE set of tests. > > Hm, yes good point. I agree with moving it to destructive for now. Vishal, Do you need a patch that moves it to the naughty list?
On Wed, 2019-06-12 at 18:47 -0700, Alison Schofield wrote: > On Wed, Jun 12, 2019 at 04:26:16PM -0700, Verma, Vishal L wrote: > > On Wed, 2019-06-12 at 16:20 -0700, Dan Williams wrote: > > > On Wed, Jun 12, 2019 at 4:05 PM Alison Schofield > > > <alison.schofield@intel.com> wrote: > > > > Fix a typo in security.sh that causes a script failure > > > > when an nvdimm-master.blob already exists and needs to > > > > be backed up. > > > > > > > > + setup_keys > > > > + '[' '!' -d /etc/ndctl/keys ']' > > > > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > > > > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak > > > > + 0=1 > > > > ./security.sh: line 39: 0=1: command not found > > > > > > > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > > > --- > > > > test/security.sh | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/test/security.sh b/test/security.sh > > > > index 8a36265..c86d2c6 100755 > > > > --- a/test/security.sh > > > > +++ b/test/security.sh > > > > @@ -36,11 +36,11 @@ setup_keys() > > > > > > > > if [ -f "$masterpath" ]; then > > > > mv "$masterpath" "$masterpath.bak" > > > > - $backup_key=1 > > > > + backup_key=1 > > > > fi > > > > if [ -f "$keypath/tpm.handle" ]; then > > > > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > > > > - $backup_handle=1 > > > > + backup_handle=1 > > > > fi > > > > > > Looks obviously correct to me. > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > > ...but that said, why is this test even bothering with the host's > > > configuration? I think it should be using a test local directory that > > > does not disturb the rest of the system, especially because the test > > > is using nfit_test resources. > > At first glance, it appears that the keys need to be in the > {ndctl_keysdir}, aka, the official system location, for some > of the ndctl commands to run. So, it's not as simple as just > creating the key blob in a temp directory. > > And, I don't even think that's the nfit_test resource you are > referring to anyway. I'll keep a look out for how it can run > cleaner, and make it off the ENABLE_DESTRUCTIVE list in the future. Yes I think eventually we want the keys to be configurable on a more fine-grained bases, and that would allow for other locations. > > > > There's no guarantee that the script successfully reaches the > > > post_cleanup() phase to restore the host configuration and could leave > > > it broken. Unless / until we can fix up this test to not touch /etc I > > > think it should be moved to the ENABLE_DESTRUCTIVE set of tests. > > > > Hm, yes good point. I agree with moving it to destructive for now. > Vishal, Do you need a patch that moves it to the naughty list? I wouldn't say no to one!
On Wed, Jun 12, 2019 at 6:44 PM Alison Schofield <alison.schofield@intel.com> wrote: > > On Wed, Jun 12, 2019 at 04:26:16PM -0700, Verma, Vishal L wrote: > > > > On Wed, 2019-06-12 at 16:20 -0700, Dan Williams wrote: > > > On Wed, Jun 12, 2019 at 4:05 PM Alison Schofield > > > <alison.schofield@intel.com> wrote: > > > > Fix a typo in security.sh that causes a script failure > > > > when an nvdimm-master.blob already exists and needs to > > > > be backed up. > > > > > > > > + setup_keys > > > > + '[' '!' -d /etc/ndctl/keys ']' > > > > + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' > > > > + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak > > > > + 0=1 > > > > ./security.sh: line 39: 0=1: command not found > > > > > > > > Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") > > > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > > > > --- > > > > test/security.sh | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/test/security.sh b/test/security.sh > > > > index 8a36265..c86d2c6 100755 > > > > --- a/test/security.sh > > > > +++ b/test/security.sh > > > > @@ -36,11 +36,11 @@ setup_keys() > > > > > > > > if [ -f "$masterpath" ]; then > > > > mv "$masterpath" "$masterpath.bak" > > > > - $backup_key=1 > > > > + backup_key=1 > > > > fi > > > > if [ -f "$keypath/tpm.handle" ]; then > > > > mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" > > > > - $backup_handle=1 > > > > + backup_handle=1 > > > > fi > > > > > > Looks obviously correct to me. > > > > > > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > > > > > > ...but that said, why is this test even bothering with the host's > > > configuration? I think it should be using a test local directory that > > > does not disturb the rest of the system, especially because the test > > > is using nfit_test resources. > > At first glance, it appears that the keys need to be in the > {ndctl_keysdir}, aka, the official system location, for some > of the ndctl commands to run. So, it's not as simple as just > creating the key blob in a temp directory. Currently, yes, but the typical way to override a build time configuration is with an environment variable, similar to what we do with NDCTL_TIMEOUT. > And, I don't even think that's the nfit_test resource you are > referring to anyway. I'll keep a look out for how it can run > cleaner, and make it off the ENABLE_DESTRUCTIVE list in the future. The test uses fake DIMMs from $NFIT_TEST_BUS0.
diff --git a/test/security.sh b/test/security.sh index 8a36265..c86d2c6 100755 --- a/test/security.sh +++ b/test/security.sh @@ -36,11 +36,11 @@ setup_keys() if [ -f "$masterpath" ]; then mv "$masterpath" "$masterpath.bak" - $backup_key=1 + backup_key=1 fi if [ -f "$keypath/tpm.handle" ]; then mv "$keypath/tpm.handle" "$keypath/tmp.handle.bak" - $backup_handle=1 + backup_handle=1 fi dd if=/dev/urandom bs=1 count=32 2>/dev/null | keyctl padd user "$masterkey" @u
Fix a typo in security.sh that causes a script failure when an nvdimm-master.blob already exists and needs to be backed up. + setup_keys + '[' '!' -d /etc/ndctl/keys ']' + '[' -f /etc/ndctl/keys/nvdimm-master.blob ']' + mv /etc/ndctl/keys/nvdimm-master.blob /etc/ndctl/keys/nvdimm-master.blob.bak + 0=1 ./security.sh: line 39: 0=1: command not found Fixes: ba35642d3815 ("ndctl: add a load-keys test in the security unit test") Signed-off-by: Alison Schofield <alison.schofield@intel.com> --- test/security.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)