swtpm: Abstract NVRAM interface for pluggable state store #490

Etsukata · 2021-07-20T06:51:45Z

Related GH issue: #461

This is RFC PR to add an abstraction layer to NVRAM state store
implementation for pluggable storage backends. No functional changes are
intended in this change. The default state store backend ("file backend")
keeps current behavior.

To make swtpm ready for pluggable store, this patch moves file related
operations to the separate file (swtpm_nvstore_file.c) and defined the
interface for plugins (nvram_backend_ops in swtpm_nvstore.h). The
interface can be used by each plugin which will be added later.

With the interface, each plugin can access its "backend_uri" which
points to the location of the backend storage, for example S3 bucket
URL or iSCSI URL, and decide how it stores TPM state data.

The second commit adds two options to specify pluggable backend type and
backend URI.

Ex:
--tpmstate backend=...,backend_uri=...

Backend URI is specific to each backend plugin which points to the
location of the NVRAM files.
Currently, "file" is the only one available backend. In this case
backend_uri should be a path to the directory where files are stored.

This option is designed to compatible with existing "dir" option.
If "dir" is specified, swtpm prioritize "dir" ignoring "backend".

coveralls · 2021-07-20T07:29:56Z

Pull Request Test Coverage Report for Build 3357

171 of 242 (70.66%) changed or added relevant lines in 8 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.07%) to 75.476%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
src/swtpm/swtpm_nvstore.c	13	15	86.67%
src/swtpm/tpmstate.c	16	20	80.0%
src/swtpm/common.c	22	33	66.67%
src/swtpm/swtpm_nvstore_dir.c	111	165	67.27%

Files with Coverage Reduction	New Missed Lines	%
src/swtpm/common.c	1	60.08%
src/swtpm/cuse_tpm.c	1	73.52%

Totals
Change from base Build 3353:	0.07%
Covered Lines:	5629
Relevant Lines:	7458

💛 - Coveralls

stefanberger · 2021-07-21T19:54:34Z

It definitely looks manageable.
What we would also need is entries in man pages and an extension to --print-capabilites that reflects the newly add options.

 swtpm socket --print-capabilities
{ "type": "swtpm", "features": [ "tpm-send-command-header", "flags-opt-startup", "cmdarg-key-fd", "cmdarg-pwd-fd" ] }

(sorry, didn't mean to close it. got on wrong button)

Etsukata · 2021-07-21T21:30:32Z

Thanks for the comment. I'll add entries in man pages and an extension to --print-capability.

stefanberger · 2021-07-30T20:55:53Z

tests/test_commandline

+PID=$!
+
+if wait_port_open $PORT $PID 4; then
+	echo "Test 1 failed: TPM did not open port $PORT"


'Test 4' ... here and below

Oops, thanks.

stefanberger · 2021-07-30T20:59:59Z

src/swtpm/common.c

+        }
+    } else {
+        logprintf(STDERR_FILENO,
+                  "The file parameter is required for the tpmstate option.\n");


it's not the file parameter but the directory or backend+backenduri parameters

Thanks, I'll fix the error message.

src/swtpm/common.c

stefanberger · 2021-07-30T21:00:58Z

Can you please also rebase this on the latest master?

Etsukata · 2021-08-02T03:27:27Z

Can you please also rebase this on the latest master?

Sure, rebased.

tests/test_commandline

stefanberger · 2021-08-02T15:02:37Z

tests/test_commandline

+
+${SWTPM_BIOS} &>/dev/null
+if [ $? -ne 0 ]; then
+	echo "Test 4 failed: tpm_bios did not work"


tpm_bios -> swtpm_bios

or rather ${SWTPM_BIOS}

Thanks, I'll fix that and also "Test 1" as well.

stefanberger · 2021-08-03T20:55:44Z

src/swtpm/swtpm_nvstore_file.c

+
+   state_directory/tpm_number.name
+
+   A temporary filename used to write to may be created. It shold be rename()'d to


shold -> should

stefanberger · 2021-08-03T21:03:39Z

man/man8/swtpm.pod

@@ -317,6 +323,10 @@ The I<--key> option supports the I<fd=> parameter.

 The I<--key> option supports the I<pwdfd=> parameter.

+=item B<nvram-backend-file>
+
+The I<--tpmstate> option supports the I<backend=file> option.


... supports the I<backend=file> and I<backend_uri=uri> parameters.

I hope that everything that's needed by the backends can be encoded into the backend_uri parameter...

stefanberger · 2021-08-03T21:05:23Z

Looks good to me now.

We'll need similar extensions for the swtpm_setup tool as well to choose the backend ...

Etsukata · 2021-08-05T07:03:15Z

We'll need similar extensions for the swtpm_setup tool as well to choose the backend ...

Yes, we need similar extensions in swtpm_setup as well. Implementing the extension as it is (i.e. adding abstraction layer to swtpm_setup) may not be a good idea because it will bring a lot of copy-and-paste codes or we have to libraries common codes.

Instead, let me propose another approach.
Currently tpm_state_path in swtpm_setup is used in the following 2 ways.

(1). For certsdir which is used by swtpm_localca temporarily
(2). For actual tpm state store path, ex:
- check_state_overwride()
- delete_swtpm_statefiles()
- delete_state()

For 1. we can use temporal directory (using mkdtemp(3)) or a directory specified by user using new --tempdir (or --certsdir) option.
It will be prefereble for users who care sensitive data (EK) not written to disk. If swtpm_setup has such an option we can cover that requirement.

For 2. we can introduce new CMD to communicate with swtpm for example CMD_HAS_STATE for check_state_overwride(). Another idea is to introduce --not-overwrite in swtpm itself and pass the option from swtpm_setup then we can do equivalent tasks in swtpm like swtpm_setup does in check_state_overwride() and delete_swtpm_statefiles().
For delete_state(), I'm considering --clear-state for swtpm to remove states when init_tpm{,2} face some errors.

If we go this way, we can also reduce the swtpm_setup dependencies on swtpm implementation details (TPM state file names).

Any comments are appreciated.

stefanberger · 2021-08-05T19:56:32Z

Now that you introduce a URI, I a wondering whether we should work with URIs like the iscsi:// you showed and now we would use a URI file:// or actually as mentioned in PR #513 maybe this should be the directory backend, rather than file, with a dir:// URI since this will want to own the whole directory due to the lock. Just to be consistent.

Etsukata · 2021-08-06T04:16:38Z

with a dir:// URI

Thanks, I like that idea. Will update this PR.

stefanberger · 2021-08-06T17:52:02Z

with a dir:// URI

Thanks, I like that idea. Will update this PR.

That should then obsolete the backend= option parameter but this here should be enough:

--tpmstate backend_uri=dir:/tmp/myvtpm

This assumes of course that every type of backend can be expressed via a URI, but as far as the current backend and the iscsi backends are concerned that should be sufficient.

stefanberger · 2021-08-10T13:40:31Z

@Etsukata You will have to rebase this branch again. Also, if we don't end up merging it by Friday it will likely have to wait another 2 weeks or so since I will be out for a while.

Etsukata · 2021-08-11T06:03:40Z

Hi @stefanberger, thanks for the notice.
I've rebased to master and removed --tpmstate backend.

stefanberger · 2021-08-11T12:48:37Z

man/man8/swtpm.pod

@@ -150,14 +150,18 @@ The following options are support by all interfaces:

 =over 4

-=item B<--tpmstate dir=E<lt>dirE<gt>[,mode=E<lt>0...E<gt>]>
+=item B<--tpmstate dir=E<lt>dirE<gt>[,mode=E<lt>0...E<gt>]|backend_uri=E<lt>uriE<gt>>


Hopefully last change. Please change this to backend-uri to be consistent with other option parameters.

The help screens in swtpm.c, swtpm_chardev.c, and cuse_tpm also need to be extended.

Sure, I'll update them.

stefanberger · 2021-08-11T14:08:35Z

man/man8/swtpm.pod

 The default value is 0640.

+If I<backend_uri> is specified, the TPM state data will be stored to the URI.
+Currently I<backend_uri=dir://<path_to_dir>> is the only one available. In this case,
+URI should specify the path to the directory where files are stored.


If I<path_to_dir> starts with a '/' then the path is interpreted as an absolute path, otherwise it is a path relative to the current directory.

Thanks, I will append the sentence to the man pod.

stefanberger · 2021-08-12T11:44:49Z

@Etsukata I think it's ready to be merged...

pksnx

Thanks much for this, looks good to me.

Related GH issue: stefanberger#461 This patch adds an abstraction layer to NVRAM state store implementation for pluggable storage backends. No functional changes are intended in this change. The default state store backend ("dir backend") keeps current behavior. To make swtpm ready for pluggable store, this patch moves file related operations to the seperate file (swtpm_nvstore_dir.c) and defined the interface for plugins (nvram_backend_ops in swtpm_nvstore.h). The interface can be used by each plugin which will be added later. With the interface, each plugin can access its "backend_uri" which points to the location of the backend storage, for example S3 bucket URL or iSCSI URL, and decide how it stores TPM state data. Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>

Add an options to specify pluggable backend URI. Ex: --tpmstate backend-uri=dir://<path_to_dir> Backend URI is specific to each backend plugin which points to the location of the NVRAM files. Currently, "dir" is the only one available backend. In this case backend-uri should be a path to the directory where files are stored. This option is designed to compatible with existing "dir" option. If "dir" is specified, swtpm prioritize "dir" ignoring "backend-uri". Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>

Append "nvram-backend-dir" to --print-capabilities output. Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>

Etsukata · 2021-08-13T03:31:20Z

Hi @stefanberger, thanks for the comment.
I've added the following change to the first commit. I think it's good to merge.

diff --git a/src/swtpm/swtpm_nvstore_dir.h b/src/swtpm/swtpm_nvstore_dir.h
index 7cf467b..eec1dfd 100644
--- a/src/swtpm/swtpm_nvstore_dir.h
+++ b/src/swtpm/swtpm_nvstore_dir.h
@@ -36,8 +36,8 @@
 /* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.                */
 /********************************************************************************/
 
-#ifndef _SWTPM_NVSTORE_FILE_H
-#define _SWTPM_NVSTORE_FILE_H
+#ifndef _SWTPM_NVSTORE_DIR_H
+#define _SWTPM_NVSTORE_DIR_H
 
 #include <libtpms/tpm_types.h>
 #include <libtpms/tpm_library.h>
@@ -75,4 +75,4 @@ SWTPM_NVRAM_DeleteName_Dir(uint32_t tpm_number,
 
 extern struct nvram_backend_ops nvram_dir_ops;
 
-#endif /* _SWTPM_NVSTORE_FILE_H */
+#endif /* _SWTPM_NVSTORE_DIR_H */

Etsukata · 2021-08-13T03:32:10Z

For swtpm_setup, I'll create several PRs separately. I appreciate if you could take a look at them once you get back, thanks.

ifnkhan · 2021-08-13T04:21:39Z

Looks good to me @Etsukata.
Thanks!

stefanberger closed this Jul 21, 2021

stefanberger reopened this Jul 21, 2021

stefanberger marked this pull request as draft July 21, 2021 20:08

Etsukata force-pushed the abstract-nvstore branch from e441c70 to cb8a82b Compare July 28, 2021 05:01

stefanberger reviewed Jul 30, 2021

View reviewed changes

src/swtpm/common.c Show resolved Hide resolved

Etsukata force-pushed the abstract-nvstore branch from cb8a82b to 567d01a Compare August 2, 2021 03:26

stefanberger reviewed Aug 2, 2021

View reviewed changes

tests/test_commandline Outdated Show resolved Hide resolved

stefanberger reviewed Aug 2, 2021

View reviewed changes

Etsukata force-pushed the abstract-nvstore branch from 567d01a to b1c7ede Compare August 3, 2021 02:58

stefanberger reviewed Aug 3, 2021

View reviewed changes

Etsukata force-pushed the abstract-nvstore branch from b1c7ede to 6593d61 Compare August 5, 2021 06:55

PiMaker mentioned this pull request Aug 5, 2021

swtpm: add "linear" nvram store backend (for single-file storage and block devices) #513

Merged

Etsukata force-pushed the abstract-nvstore branch from 6593d61 to 34011a8 Compare August 11, 2021 06:01

stefanberger reviewed Aug 11, 2021

View reviewed changes

Etsukata force-pushed the abstract-nvstore branch from 34011a8 to 2de9980 Compare August 12, 2021 02:45

stefanberger marked this pull request as ready for review August 12, 2021 11:44

pksnx approved these changes Aug 13, 2021

View reviewed changes

Etsukata changed the title ~~[RFC] swtpm: Abstract NVRAM interface for pluggable state store~~ swtpm: Abstract NVRAM interface for pluggable state store Aug 13, 2021

Etsukata added 3 commits August 13, 2021 12:28

swtpm: Add "nvram-backend-dir" capability

628b862

Append "nvram-backend-dir" to --print-capabilities output. Signed-off-by: Eiichi Tsukata <eiichi.tsukata@nutanix.com>

Etsukata force-pushed the abstract-nvstore branch from 2de9980 to 628b862 Compare August 13, 2021 03:31

stefanberger merged commit 86931b4 into stefanberger:master Aug 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swtpm: Abstract NVRAM interface for pluggable state store #490

swtpm: Abstract NVRAM interface for pluggable state store #490

Etsukata commented Jul 20, 2021

coveralls commented Jul 20, 2021 •

edited

stefanberger commented Jul 21, 2021 •

edited

Etsukata commented Jul 21, 2021

stefanberger Jul 30, 2021

Etsukata Aug 2, 2021

stefanberger Jul 30, 2021

Etsukata Aug 2, 2021

stefanberger commented Jul 30, 2021

Etsukata commented Aug 2, 2021

stefanberger Aug 2, 2021

stefanberger Aug 2, 2021

Etsukata Aug 3, 2021

stefanberger Aug 3, 2021

Etsukata Aug 5, 2021

stefanberger Aug 3, 2021

stefanberger commented Aug 3, 2021

Etsukata commented Aug 5, 2021

stefanberger commented Aug 5, 2021

Etsukata commented Aug 6, 2021

stefanberger commented Aug 6, 2021

stefanberger commented Aug 10, 2021

Etsukata commented Aug 11, 2021

stefanberger Aug 11, 2021

stefanberger Aug 11, 2021

Etsukata Aug 12, 2021

stefanberger Aug 11, 2021

Etsukata Aug 12, 2021

stefanberger commented Aug 12, 2021

pksnx left a comment

Etsukata commented Aug 13, 2021

Etsukata commented Aug 13, 2021

ifnkhan commented Aug 13, 2021


		state_directory/tpm_number.name

		A temporary filename used to write to may be created. It shold be rename()'d to

swtpm: Abstract NVRAM interface for pluggable state store #490

swtpm: Abstract NVRAM interface for pluggable state store #490

Conversation

Etsukata commented Jul 20, 2021

coveralls commented Jul 20, 2021 • edited

Pull Request Test Coverage Report for Build 3357

💛 - Coveralls

stefanberger commented Jul 21, 2021 • edited

Etsukata commented Jul 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanberger commented Jul 30, 2021

Etsukata commented Aug 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanberger commented Aug 3, 2021

Etsukata commented Aug 5, 2021

stefanberger commented Aug 5, 2021

Etsukata commented Aug 6, 2021

stefanberger commented Aug 6, 2021

stefanberger commented Aug 10, 2021

Etsukata commented Aug 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefanberger commented Aug 12, 2021

pksnx left a comment

Choose a reason for hiding this comment

Etsukata commented Aug 13, 2021

Etsukata commented Aug 13, 2021

ifnkhan commented Aug 13, 2021

coveralls commented Jul 20, 2021 •

edited

stefanberger commented Jul 21, 2021 •

edited