Add TPMKMS support #71

hslatman · 2023-06-07T14:52:40Z

This PR adds support for using the TPMKMS with step-kms-plugin

Attestations for the tpm format are created in a slightly different manner than the existing formats. Instead of signing data with the private key, the certification of a key is done at key creation time. The certification facts are recorded in the TPMKMS (in a go.step.sm/crypto/tpm/storage.Store implementation, actually), so that they can be used by other applications. In this case, step kms attest --format tpm 'tpmkms:name=keyName' will return an attestation statement that includes these key certification parameters.

Will do some more testing and add examples in follow up commits.

In the `crypto/kms` package, the `CreateAttestation` implementation for YubiKeys was changed to include the leaf as the first certificate in the chain. Looping through the chain is thus sufficient for getting all certs including the leaf in PEM format.

A KMS URI can now be provided using just the scheme, without a colon. E.g. `--kms tpmkms` is now a valid value. Before this change, providing `--kms tpmkms` would result in an error: `Error: error parsing tpmkms: scheme is missing`. It isn't immediately clear that it's the colon that's missing, so decided to make the KMS URI processing a bit smarter.

Added a flag `--bundle` to `step kms certificate`. For the case in which a certificate is only printed, a `CertFS` implementation is used, which doesn't currently support reading chains. Added a workaround to read from a `CertificateChainManager` instead.

maraino

Looks good, but I would bundle by default, it is what we were doing before.

cmd/attest.go

cmd/certificate.go

maraino · 2023-06-08T23:06:56Z

cmd/certificate.go

@@ -116,4 +175,5 @@ func init() {
 	flags.SortFlags = false

 	flags.String("import", "", "The certificate `file` to import")
+	flags.Bool("bundle", false, "Print all certificates in the chain/bundle")


I would print the bundled certificate by default. A different option can be added to show only the leaf. Probably something cleaner that --bundled=false

I've left this as is, to be consistent with our other tooling.

step-kms-plugin certificate is mainly used to load a certificate from a KMS when step ca certificate --x5c-cert is used. The --x5c-cert flag expects a bundled certificate, that is what you will use with files. The flag --x5c-chain was added to support a bundled certificate in those KMSs that only support a single certificate. There is some inconsistency because not all KMSs support the same things, but I don't think we want to add --x5c-chain unless it is necessary.

We can keep the flag here, for "consistency" with step certificate inspect, but we will need to change the cli to add the --bundle option. This will cause some incompatibility depending if you are using the last version of the plugin or not.

See #71 (comment).

go.mod

hslatman · 2023-06-09T04:30:38Z

Looks good, but I would bundle by default, it is what we were doing before.

Is it, though? There was only (Load|Store)Certificate, which would work with / return a single certificate? I may have missed something, but that was the reason to add the CertificateChainManager. So now, when the KMS supports chains, the default is to show just one cert in the output.

maraino · 2023-06-09T05:19:06Z

The other KMSs only support one, but for example in attest we show the bundle.

hslatman · 2023-06-09T08:27:23Z

Now I get what you mean! Yes, the behavior changed slightly for the YubiKey, due to adding the leaf as the first cert of the chain, and not looping through the entire chain now.

I think for step kms certificate returning a single certificate by default makes most sense. It would be similar to our step certificate inspect, which will print the info for a single cert only, unless --bundle is supplied. It doesn't say step kms chain, after all, and it's more consistent with that existing command.

For attestation use cases, I think most users will need the chain by default, so I agree that we may want the behavior of --bundle there by default. I could add --leaf to show just the leaf? If someone were to run step kms attest .... | step certificate inspect, they would need to add --bundle to show the full information, though. For step kms attest --format <format> nothing needs to change, as the format prescribes what goes in and is printed.

dwmw2 · 2023-06-09T09:20:17Z

Not entirely sure if this is the correct place for this "informational note":

For TPM wrapped keys, there is a file format defined at https://www.hansenpartnership.com/draft-bottomley-tpm2-keys.html (PEM files containing -----BEGIN TSS2 PRIVATE KEY-----) which is supported by other tools including the two OpenSSL engines, GnuTLS and OpenConnect.

The vision is that any software which can use keys from a file, should automatically recognise when the file they're given contains such a key (in either PEM or DER form) and Do The Right Thing™.

Tools which create keys to be used with a TPM should also use this storage format or at least be able to export to it.

hslatman · 2023-06-09T10:03:02Z

@dwmw2 thank you for your insight! 🙂 I was just looking through your comment on smallstep/crypto#260, and saw the the link to your draft RFC, which in turn linked to the TPMKEY URI draft. That draft did inspire parts of our TPMKMS implementation, as well as the underlying packages. We chose to start with tpmkms:, because that fit our current implementation (and underlying packages) and we wouldn't be locked to the semantics of the draft RFC. We also needed a way to make a distinction between attestation and application keys.

We're close to making step-tpm-plugin public, which has more TPM specific functionality than this plugin. There's some functionality to make AKs and keys created by this KMS plugin and the TPM plugin be interoperable with the tpm2_tools, for example. It sounds like the file format you describe could also be supported through that plugin. The simplest would probably be to support exports in that format, similar to what we do for the tpm2_tools interoperability.

We currently default to storing the serialized TPM objects (and corresponding certificates) as files in a directory. The files themselves contain a JSON representation of the TPM objects, which includes the serialized TPM data, as well as some metadata. It's a simple and effective solution and we don't need to maintain an additional (meta)data store that way. For the PEM format BEGIN TSS2 PRIVATE KEY I think we would need to keep some (meta)data outside of the PEM file? Or we could try encoding that in a PEM header, assuming tooling that doesn't understand our header, ignores it? Our current storage backend does allow for extension, so I could give it a try.

dwmw2 · 2023-06-09T10:29:56Z

Note that the URI and the file format are fairly separate.

The URI would be used for keys which are stored within the TPM, in its NV storage. There was that old tpmuri draft from @nmav for TPMv1.2, and I've recently been prodding the folks working on OpenSSL engines to get their act together and have a single form of identifier for TPMv2.0 too.

The file format is for keys which are stored on the file system (or anywhere else really) in encrypted form and transiently loaded into the TPM as required. The standardisation of that one is much further along; @jejb's document is new but the format is already supported in released versions of both OpenSSL engines as well as OpenConnect and GnuTLS.

Note that the file format does have implications about precisely how you generate the temporary parent key that wraps the stored key.

Being able to export to this PEM form seems like a reasonable choice. I believe tpm2_tools can use it too, so you could perhaps do it instead of the older separate pubkey/privkey blobs that tpm2_tools supported? Again, we do have to ensure the generated parent is compatible.

I think PEM headers should probably work, and you can also put stuff outside the -----BEGIN… … -----END… lines in the PEM file.

hslatman · 2023-06-09T11:03:09Z

Note that the URI and the file format are fairly separate.

Noted and understood!

The URI would be used for keys which are stored within the TPM, in its NV storage. There was that old tpmuri draft from @nmav for TPMv1.2, and I've recently been prodding the folks working on OpenSSL engines to get their act together and have a single form of identifier for TPMv2.0 too.

That makes sense. Is it only for keys within the TPM, though? Because the old URI does support tpmkey-file? There's no explanation other than its name in the draft RFC, and it's just a draft, so maybe it wasn't all thought through nor commented on?

The file format is for keys which are stored on the file system (or anywhere else really) in encrypted form and transiently loaded into the TPM as required. The standardisation of that one is much further along; @jejb's document is new but the format is already supported in released versions of both OpenSSL engines as well as OpenConnect and GnuTLS.

That looks great! I don't remember having seen that document before, but it does describe things I've been thinking about to store in our own serialization format, such as auth policies. The underlying Go libraries we depend on don't support passing these through currently, afaik, but work's being done that might make that possible. So this definitely looks interesting to support in the (near) future.

Note that the file format does have implications about precisely how you generate the temporary parent key that wraps the stored key.

👍

Being able to export to this PEM form seems like a reasonable choice. I believe tpm2_tools can use it too, so you could perhaps do it instead of the older separate pubkey/privkey blobs that tpm2_tools supported? Again, we do have to ensure the generated parent is compatible.

The step-tpm-plugin indeed does rely on the blobs, and I've been able to load the exported blobs with tpm2_tools again, so that's a start. Definitely sounds like something to explore 🙂

I think PEM headers should probably work, and you can also put stuff outside the -----BEGIN… … -----END… lines in the PEM file.

Headers feel a bit neater to me, but that additional data part at the end might be a nice alternative. We do have some utilities for PEM deser thay may, or may not support the latter, but we can probably add support for that. The Go stdlib seems to do the right thing already. One thing that our current format does, is storing any certificate (chains) belonging to an AK or key in the same file as as the serialized key data; with this PEM format, we may want to have a way to separate those into two files depending on how systems using the file want them to look like.

dwmw2 · 2023-06-09T11:45:56Z

That makes sense. Is it only for keys within the TPM, though? Because the old URI does support tpmkey-file? There's no explanation other than its name in the draft RFC, and it's just a draft, so maybe it wasn't all thought through nor commented on?

<rant>
Yeah, I don't think tpmkey-file should have been in that draft; it was a bad idea. A file is a file. Users may have all kinds of stuff in files. PKCS#1, PKCS#8, PKCS#12 are all ways of storing actual private keys, it could be a TPMv1.2 TCPA blob wrapped in -----BEGIN TSS KEY BLOB-----, it could be a TPMv2.0 key in the form from the Bottomley draft, and probably others.

I object vehemently to asking the user to tell the application what kind of thing is stored in the file. The application should just look in the file and Do The Right Thing.

I hate the fact that OpenSSL has different APIs for opening different types of PKCS#(1|8|12) files, even for PEM vs. DER versions of the same format, which leads to tools like curl explicitly pushing that as an "option" to the user instead of doing it right. It's awful.

So asking the user to specify tpmkey:file=foo.pem instead of foo.pem is just outright wrong. We should only do that if we hate our users. Which we don't.
</rant>

Note that the documentation of the TPMv2 format explicitly notes that it starts with an unambiguous OID specifically so that it can be detected even in DER form.

OpenConnect will Just Work™ with whatever file it's given, whether it be a software key in one of the myriad of formats, a TPMv1 TCPA key blob, a TPMv2 file. Or if it isn't a file, and is instead a PKCS#11 URI of the form pkcs11:manufacturer=piv_II;id=%01. It all Just Works.

To make it possible to use the `attest` command similar to how it's used with a YubiKey, for TPMs we need to create a new attested key at the time of creating an attestation. This is because the data that is being attested when attesting the key is part of the input for the TPM when creating the key. It's still possible to do a normal `attest` flow. That requires creating an AK and key (with optional `qualifying-data` set) to be created first. Using the `--new` flag, a new key with `name` is created. This option is not yet documented in an example.

hslatman · 2023-06-09T15:31:33Z

@maraino I added support for creating a new key when running attest because of how key attestation works with the optional QualifyingData input, which depends on the ACME key authorization. It can't be read from stdin yet, but that's probably the next step. It does require creating a new key at the time of running attest, so --new is required. The parameters from name will be taken, so tpmkms:name=new-key;ak=my-ak;qualifying-data=<hex> will work.

The previous method, with the AK and key being created in separate steps, also still works.

I'm trying to fix RSA >2048 bits keys now, which is why I put the default to that. Apparently I've been testing with 2048 all the time; this was used all over the place in go-tpm and go-attestation 😅 The error seems to originate from the TPM (RCValue) and I see it happening with the simulator as well as my hardware TPM. Curiously, tpm2_tools reports the same error when trying -G rsa3072:rsaes, for example. Maybe my TPM and the simulator both don't support it.

Might need to upgrade my tpm2_tools by compiling from source. tpm2_testparms returns it doesn't support rsa3072:rsaes.

maraino

I'm still unconvinced about the --bundle. I've added my reasoning in a comment.

maraino · 2023-06-12T21:27:25Z

cmd/certificate.go

@@ -116,4 +175,5 @@ func init() {
 	flags.SortFlags = false

 	flags.String("import", "", "The certificate `file` to import")
+	flags.Bool("bundle", false, "Print all certificates in the chain/bundle")


step-kms-plugin certificate is mainly used to load a certificate from a KMS when step ca certificate --x5c-cert is used. The --x5c-cert flag expects a bundled certificate, that is what you will use with files. The flag --x5c-chain was added to support a bundled certificate in those KMSs that only support a single certificate. There is some inconsistency because not all KMSs support the same things, but I don't think we want to add --x5c-chain unless it is necessary.

We can keep the flag here, for "consistency" with step certificate inspect, but we will need to change the cli to add the --bundle option. This will cause some incompatibility depending if you are using the last version of the plugin or not.

maraino · 2023-06-12T21:48:11Z

cmd/attest.go

+	flags.Bool("new", false, "(EXPERIMENTAL) Creates and attests a new key instead of attesting an existing one")
+	flags.Var(kty, "kty", "The key `type` to build the certificate upon.\nOptions are EC and RSA")
+	flags.Var(crv, "crv", "The elliptic `curve` to use for EC and OKP key types.\nOptions are P256, P384 and P521")
+	flags.Int("size", 2048, "The key size for an RSA key") // TODO(hs): attesting 3072 bit RSA keys on TPM that doesn't support it returns an ugly error; we want to catch that earlier.
+	flags.Var(alg, "alg", "The hashing `algorithm` to use on RSA PKCS #1 and RSA-PSS signatures.\nOptions are SHA256, SHA384 or SHA512")


Probably mentioning that this is for tpmkms only or just for new keys. You're also using always PKCS#1 so the RSA-PSS in the help seems unnecessary. We can adding it back if we add support for --new on other KMSs.

Fixed in bc260c8.

hslatman · 2023-06-13T07:38:42Z

I'm still unconvinced about the --bundle. I've added my reasoning in a comment.

I don't see how I changed the existing logic for the certificate command. There used to be a ReadCertificate, LoadCertificate and StoreCertificate. These all operate on a single *x509.Certificate. The CertFS also operates on a single certificate, through the CertificateManager interface. As far as I know, it always printed only a single certificate when a single name/kuri was provided. Then cryptoutil.LoadCertificateBundle in the CLI would always get a single certificate.

I've changed ReadCertficate to ReadCertificateBundle. The first cert in the bundle has become the cert from the original call to ReadCertificate. Only when the KMS supports the CertificateChainManager will the chain methods be used (which can be used with a single cert); it'll use the CertificateManager interface otherwise. Only when --bundle is provided is the logic changed for outputting the certificate (chain), but since a single cert was supported only, that's functionally not different. The CertFS case is a special one that I couldn't readily adopt with the CertificateChainManager without refactoring a bit more than I was willing to do right now. It should still be the same logic as before adding --bundle.

Is it possible you were testing this with SoftKMS, with an empty kms in the CLI? Because that's a special case that would return a bundle.

So, to conclude, I think --bundle makes sense, both functionality as well as for consistency with our other CLI utilities and commands. And we can probably add the --bundle to the invocation in cryptoutil.LoadCertificateBundle; that's probably a "must". And maybe set certs := []*x509.Certificate{cert} for the CertificateManager case? For the CertFS case we may want to support a chain there too, but seems less critical.

dwmw2 · 2023-06-13T07:38:44Z

cmd/key.go

+
+  # Get a key from the default TPM KMS:
+  step-kms-plugin key tpmkms:name=my-key
+


Can we have a way to export the key to a TSS2 PRIVATE KEY PEM file, please? And to import them too.

I've added an issue for this: #73. I'm in favor of adding support, but I have a couple other high(er) priority things on my plate to finish first.

maraino

lgtm

dwmw2 · 2023-06-16T07:39:38Z

I think PEM headers should probably work, and you can also put stuff outside the -----BEGIN… … -----END… lines in the PEM file.

Headers feel a bit neater to me, but that additional data part at the end might be a nice alternative. We do have some utilities for PEM deser thay may, or may not support the latter, but we can probably add support for that. The Go stdlib seems to do the right thing already. One thing that our current format does, is storing any certificate (chains) belonging to an AK or key in the same file as as the serialized key data; with this PEM format, we may want to have a way to separate those into two files depending on how systems using the file want them to look like.

I stumbled across RFC7468 today and saw this (penultimate para of p4):

   Unlike legacy PEM encoding [RFC1421], OpenPGP ASCII armor, and the
   OpenSSH key file format, textual encoding does *not* define or permit
   headers to be encoded alongside the data.  Empty space can appear
   between the pre-encapsulation boundary and the base64, but generators
   SHOULD NOT emit such any such spacing.  (The provision for this empty
   area is a throwback to PEM, which defined an "encapsulated header
   portion".)

hslatman added 5 commits June 7, 2023 13:45

Upgrade to latest go.step.sm/crypto with TPMKMS support

0440a8a

Enable TPMKMS

e70fbf9

Add tpm format for KMS attestation

11986c7

Upgrade to latest go.step.sm/crypto with TPMKMS support

9079368

github-actions bot added the needs triage label Jun 7, 2023

hslatman added 5 commits June 8, 2023 13:25

Add examples for create and key for TPMKMS

78a205a

Add examples for attest and certificate

40b9792

Extract logic for printing certificate to stdout

96074a1

Upgrade to latest development version of go.step.sm/crypto

8af6088

hslatman requested a review from maraino June 8, 2023 21:25

maraino requested changes Jun 8, 2023

View reviewed changes

hslatman added 3 commits June 9, 2023 12:32

Fix single certificate to import into YubiKey

f760ae9

Add examples for importing certificates into the TPMKMS

6cda4b6

Invert logic for showing attestation certificate chain vs. leaf

72eabd6

hslatman added 2 commits June 12, 2023 13:48

Upgrade to latest development version of go.step.sm/crypto

f3ee3b7

Upgrade to v0.32.0 of go.step.sm/crypto

f74bb56

hslatman enabled auto-merge June 12, 2023 20:37

hslatman requested a review from maraino June 12, 2023 20:53

maraino reviewed Jun 12, 2023

View reviewed changes

Improve help text for new key in attest subcommand

bc260c8

dwmw2 reviewed Jun 13, 2023

View reviewed changes

hslatman mentioned this pull request Jun 13, 2023

Add support for TSS2 PRIVATE KEY PEM format #73

Closed

hslatman requested a review from maraino June 13, 2023 08:06

maraino approved these changes Jun 13, 2023

View reviewed changes

hslatman merged commit 3fe2b7f into main Jun 13, 2023

hslatman deleted the herman/tpmkms branch June 13, 2023 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add TPMKMS support #71

Add TPMKMS support #71

hslatman commented Jun 7, 2023 •

edited

Loading

maraino left a comment

maraino Jun 8, 2023

hslatman Jun 9, 2023

maraino Jun 12, 2023

hslatman Jun 13, 2023

hslatman commented Jun 9, 2023

maraino commented Jun 9, 2023

hslatman commented Jun 9, 2023 •

edited

Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 •

edited

Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 •

edited

Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 •

edited

Loading

maraino left a comment

maraino Jun 12, 2023

maraino Jun 12, 2023

hslatman Jun 13, 2023

hslatman commented Jun 13, 2023 •

edited

Loading

dwmw2 Jun 13, 2023

hslatman Jun 13, 2023

maraino left a comment

dwmw2 commented Jun 16, 2023


		# Get a key from the default TPM KMS:
		step-kms-plugin key tpmkms:name=my-key

Add TPMKMS support #71

Add TPMKMS support #71

Conversation

hslatman commented Jun 7, 2023 • edited Loading

maraino 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

hslatman commented Jun 9, 2023

maraino commented Jun 9, 2023

hslatman commented Jun 9, 2023 • edited Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 • edited Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 • edited Loading

dwmw2 commented Jun 9, 2023

hslatman commented Jun 9, 2023 • edited Loading

maraino 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

hslatman commented Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maraino left a comment

Choose a reason for hiding this comment

dwmw2 commented Jun 16, 2023

hslatman commented Jun 7, 2023 •

edited

Loading

hslatman commented Jun 9, 2023 •

edited

Loading

hslatman commented Jun 9, 2023 •

edited

Loading

hslatman commented Jun 9, 2023 •

edited

Loading

hslatman commented Jun 9, 2023 •

edited

Loading

hslatman commented Jun 13, 2023 •

edited

Loading