Cryptenroll specify handle index #29427

ddstreet · 2023-10-03T14:16:53Z

Allow user to specify what handle index to use for systemd-cryptenroll to use for tpm2 sealing. This defaults to the SRK.

This is on top of #29426 and split out from #28519.

bluca · 2023-10-03T22:29:18Z

man/systemd-cryptenroll.xml

+
+        <listitem><para>Configures which parent key to use for sealing, using the TPM handle of the
+        key. Expects a hexadecimal 32bit integer, optionally prefixed with <literal>0x</literal>. Allowable
+        values are any handle index in the persistent (0x81000000-0x81ffffff), transient


wrap these ranges in <literal> too

ack, updated, thanks

bluca

Can you add a test for this? Doesn't have to be complex, just the minimum to send this through ASAN&friends

poettering · 2023-10-04T11:38:33Z

man/systemd-cryptenroll.xml

@@ -411,6 +411,26 @@
        <xi:include href="version-info.xml" xpointer="v248"/></listitem>
      </varlistentry>

+      <varlistentry>
+        <term><option>--tpm2-handle=</option><replaceable>HANDLE</replaceable></term>


can we call this --tpm2-storage-key= maybe? i.e. "handle" is just so generic, I think it would be better to say that we reference a key here, and what kind of key it is. The handle after all is not the primary concept we want to configure here, but the the object the handle refers to...

other ideas: --tpm2-parent-key= or --tpm2-primary-key= or --tpm2-root-key= or --tpm2-wrap-key= or --tpm2-seal-key= something like that.

so i read from the other PR that this is supposed to deal with a numeric handler as opposed to a key file, hence maybe call this --tpm2-seal-key-handle= and --tpm2-seal-key-file=?

sounds good, updated to --tpm2-seal-key-handle=

poettering · 2023-10-04T11:39:04Z

src/cryptenroll/cryptenroll-tpm2.c

@@ -133,6 +133,7 @@ int enroll_tpm2(struct crypt_device *cd,
                const void *volume_key,
                size_t volume_key_size,
                const char *device,
+                uint32_t handle,


i'd also call this seal_key or storage_key or so (depending on how the cmdline switch is going to be called)

ack, updated to seal_key_handle (since we'll add seal_key_file later also)

poettering · 2023-10-04T11:40:40Z

src/shared/tpm2-util.c

@@ -3931,6 +3932,7 @@ static int tpm2_deserialize(
 }

 int tpm2_seal(Tpm2Context *c,
+              uint32_t handle_index,


so here you call it the "handle_index", and earlier you called it the "handle". Please stick to one. (And as mentioned, I think we should rather call this storage_key or sealing_key or so)

ack, changed it to seal_key_handle here as well.

poettering · 2023-10-04T11:41:09Z

this should have a test, really.

test/units/testsuite-70.sh

ddstreet · 2023-10-09T16:38:50Z

Updated to address review comments, as well as adding tests (to both test-tpm2 and TEST-70-TPM2).

I also looked more closely at the spec and realized NV indexes cannot be used as keys, so I removed NV index from the allowable ranges for --tpm2-seal-key-handle=; now it must be either a persistent key or transient key.

Also the test uncovered that we can't use Esys_TR_Close(), so added a commit to stop calling it. Technically if tpm2-tss library added internal reference counting to its handles (so Esys_TR_Close() didn't remove a handle in use by multiple callers) we could call it again, but for now it's easiest (and I think ok) to just avoid calling it. I also opened a tpm2-tss issue.

poettering · 2023-10-10T08:21:00Z

src/shared/tpm2-util.c

+                 * references will be cleaned up when we free our ESYS_CONTEXT.
+                 *
+                 * An upstream bug is open here: https://github.com/tpm2-software/tpm2-tss/issues/2693 */
+                rc = TSS2_RC_SUCCESS /* sym_Esys_TR_Close(esys_context, &esys_handle) */;


maybe add a // FIXME style comment here

i.e. // FIXME: restore sym_Esys_TR_Close() use once tpm2-tss is fixed and adopted widely enough

ack, updated

poettering · 2023-10-10T08:21:28Z

lgtm

This defaults to the SRK index.

In TEST-70-TPM2, test systemd-cryptenroll --tpm2-seal-key-handle using the default (0) as well as the SRK handle (0x81000001), and test using a non-SRK handle index after creating and persisting a primary key. In test/test-tpm2, test tpm2_seal() and tpm2_unseal() using default (0), the SRK handle, and a transient handle.

Unfortunately, the tpm2-tss library doesn't reference count handles, and a call to Esys_TR_Close() will remove the handle that could be in use by other code. So stop calling Esys_TR_Close(), and leave the handle around until we cleanup the entire ESYS_CONTEXT.

ddstreet · 2023-10-10T10:03:13Z

rebased on main, updated with // FIXME comment, and fixed test/units/testsuite-70.sh cleanup function to use if-fi instead of && (so it doesn't incorrectly cause test to fail)

ddstreet · 2023-10-10T15:09:48Z

Hmm, on Arch it fails to find the transient key, it seems like GetCapability for transient handles might not work there...I'll boot up my Arch VM and take a look

The kernel tpm "resource manager" interface doesn't report that any transient handles exist, even if they do, so don't bother asking if the handle is transient.

ddstreet · 2023-10-10T20:59:57Z

Added one more commit to avoid calling GetCapability() to check if a handle exists in the tpm, if the handle is transient, because the kernel tpm "resource manager" (i.e. /dev/tpmrm0) will never report that any transient handles exist, even if they actually do.

github-actions bot added documentation util-lib tests sd-boot/sd-stub/bootctl repart tpm2 please-review PR is ready for (re-)review by a maintainer labels Oct 3, 2023

ddstreet force-pushed the cryptenroll_specify_handle_index branch from 26f2b68 to 470d087 Compare October 3, 2023 16:58

bluca added needs-rebase and removed please-review PR is ready for (re-)review by a maintainer labels Oct 3, 2023

ddstreet force-pushed the cryptenroll_specify_handle_index branch from 470d087 to d34092e Compare October 3, 2023 21:52

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Oct 3, 2023

bluca reviewed Oct 3, 2023

View reviewed changes

bluca requested changes Oct 3, 2023

View reviewed changes

bluca added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Oct 3, 2023

poettering reviewed Oct 4, 2023

View reviewed changes

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed good-to-merge/with-minor-suggestions labels Oct 4, 2023

ddstreet force-pushed the cryptenroll_specify_handle_index branch from d34092e to 8b6b50b Compare October 9, 2023 16:32

github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Oct 9, 2023

github-advanced-security bot found potential problems Oct 9, 2023

View reviewed changes

test/units/testsuite-70.sh Fixed Show resolved Hide resolved

test/units/testsuite-70.sh Fixed Show resolved Hide resolved

ddstreet force-pushed the cryptenroll_specify_handle_index branch from 8b6b50b to e9281e3 Compare October 9, 2023 16:45

poettering reviewed Oct 10, 2023

View reviewed changes

poettering added good-to-merge/with-minor-suggestions and removed please-review PR is ready for (re-)review by a maintainer labels Oct 10, 2023

ddstreet added 3 commits October 10, 2023 05:40

cryptenroll: allow specifying handle index of key to use for sealing

382bfd9

This defaults to the SRK index.

ddstreet force-pushed the cryptenroll_specify_handle_index branch from e9281e3 to 1524184 Compare October 10, 2023 10:01

poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed good-to-merge/with-minor-suggestions labels Oct 10, 2023

tpm2: don't use GetCapability() to check transient handles

9c18019

The kernel tpm "resource manager" interface doesn't report that any transient handles exist, even if they do, so don't bother asking if the handle is transient.

poettering merged commit 98d8c37 into systemd:main Oct 11, 2023
48 checks passed

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Oct 11, 2023

ddstreet deleted the cryptenroll_specify_handle_index branch October 11, 2023 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cryptenroll specify handle index #29427

Cryptenroll specify handle index #29427

ddstreet commented Oct 3, 2023

bluca Oct 3, 2023

ddstreet Oct 6, 2023

bluca left a comment

poettering Oct 4, 2023

poettering Oct 5, 2023

ddstreet Oct 6, 2023

poettering Oct 4, 2023

ddstreet Oct 6, 2023

poettering Oct 4, 2023

ddstreet Oct 6, 2023

poettering commented Oct 4, 2023

ddstreet commented Oct 9, 2023

poettering Oct 10, 2023 •

edited

ddstreet Oct 10, 2023

poettering commented Oct 10, 2023

ddstreet commented Oct 10, 2023

ddstreet commented Oct 10, 2023

ddstreet commented Oct 10, 2023

Cryptenroll specify handle index #29427

Cryptenroll specify handle index #29427

Conversation

ddstreet commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluca left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Oct 4, 2023

ddstreet commented Oct 9, 2023

poettering Oct 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poettering commented Oct 10, 2023

ddstreet commented Oct 10, 2023

ddstreet commented Oct 10, 2023

ddstreet commented Oct 10, 2023

poettering Oct 10, 2023 •

edited