Add TPM key support #38
Conversation
This does nothing useful yet, except detecting that we were given a TSS2 private key blob and reporting that it's not supported. It does help to ensure that when we come to implement this, we'll get the user experience right. Which is "just give us the file". We can do the actual implementation either via OpenSSL with one of the TPM engines/providers, or by parsing the TSS2 ASN.1 ourselves and driving the TPM directly. Not sure which is easier from Go.
Just parse the ASN.1 (working around historical bugs and the lack of BER support in asn1.Unmarshal()). Returns a Signer that won't work yet.
This allows us to use the swtpm, via the unix socket
| func fixupEmptyAuth(tpmData *[]byte) { | ||
| var pos int = 0 | ||
|
|
||
| // Skip the SEQUENCE tag and length |
There was a problem hiding this comment.
Is this a typo? Should it say, "Return if the ASN.1 can't be parsed as a SEQUENCE"?
There was a problem hiding this comment.
It does both. If it isn't a sequence it returns and does nothing. If it is, it skips the tag and the length.
| lenByte := (*tpmData)[pos] | ||
| if lenByte < 0x80 { | ||
| pos = pos + 1 | ||
| } else if lenByte < 0x85 { |
There was a problem hiding this comment.
Are we assuming that the length of the SEQUENCE can be stored in four or less bytes? Is that just because it's a reasonable assumption to make, or is there a length constraint defined somewhere?
There was a problem hiding this comment.
Yes, I believe all SEQUENCE lengths in DER have to be 4 or fewer bytes.
| } | ||
|
|
||
| if !tpmData.Oid.Equal(oidLoadableKey) { | ||
| return nil, "", errors.New("Invalid OID for TPMv2 key:" + tpmData.Oid.String()) |
There was a problem hiding this comment.
Minor, but would it be good to include information about importable and sealed keys not being supported yet if one of those OIDs are found?
There was a problem hiding this comment.
Yes, that should be simple enough to add.
| } | ||
|
|
||
| func handleIsPersistent(h int) bool { | ||
| return (h >> 24) == int(tpm2.HandleTypePersistent) |
There was a problem hiding this comment.
Unclear whether it matters at all, but should we use this constant instead? The HandleTypePersistent constant is defined in a file under the legacy folder.
There was a problem hiding this comment.
I think that should work, yes.
| tpmData.Parent != int(tpm2.HandlePlatform) { | ||
| return nil, "", errors.New("Invalid parent for TPMv2 key") | ||
| } | ||
| if len(tpmData.Pubkey) < 2 || |
There was a problem hiding this comment.
We have to do all this validation ourselves? This and the functionality encapsulated within the fixupEmptyAuth function feel like things that should be done by go-tpm. I haven't done a deep dive on what functionality is currently supported by the repository though (and I do see that it says that the entire TPM 2.0 specification isn't yet supported). This just feels like something we shouldn't have to do ourselves.
There was a problem hiding this comment.
Agreed. google/go-tpm#312
| if err != nil { | ||
| return nil, err | ||
| } | ||
| signature, err = asn1.Marshal(struct { |
There was a problem hiding this comment.
Unclear whether it's worth doing, but we could leverage the encodeEcdsaSigValue function defined within signer.go here. But we would have to concatenate the two byte slices before feeding it into the function, making it potentially not worth it.
There was a problem hiding this comment.
Yeah, I think I tried that once and preferred it this way.
| @@ -164,6 +164,23 @@ in the PKCS#11 token, but if that is not sufficient to uniquely identify | |||
| the certificate, further refinement of matching certificates can be | |||
| achieved through the `--cert-selector` flag. | |||
|
|
|||
| #### TPMv2 Integration | |||
There was a problem hiding this comment.
Minor, but it might be good to include some documentation around TPM_DEVICE here. Or if there's a resource we can link to, maybe that would be better.
| var paths []string | ||
| tpmdev := os.Getenv("TPM_DEVICE") | ||
| if tpmdev != "" { | ||
| paths = append(paths, tpmdev) |
There was a problem hiding this comment.
Minor, but should we include explicit sensible defaults (maybe only for Windows)? The documentation of OpenTPM states the following:
// This function may also be invoked with no paths, as tpm2.OpenTPM(). In this
// case, the default paths on Linux (/dev/tpmrm0 then /dev/tpm0), will be used.
There was a problem hiding this comment.
I'm happier letting go-tpm get this right; didn't I only add the TPM_DEVICE support to make the testing using the sim work correctly?
|
Merged into the |
This adds the TPM key file support requested in issue #36, along with self tests using the swtpm (and optionally a local hardware TPM).
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.