Improve Windows support #317
Conversation
There's currently no support for creating application keys on Windows systems. This patch transitions the Windows key type to specifically refer to attestation keys, and reuses the existing wrapped key support for application keys. This allows the creation of keys in the platform store, while still allowing said keys to be manipulated with existing TPM functionality rather than duplicating it.
When generating a new key using a Windows TPM, a `wrappedKey20` was returned, which couldn't be used for signing on Windows, as it's backed by a `windowsTPM`. The `wrappedKey20` seems to be a type specifically aimed at usage with a `wrappedTPM20`, which in turn seems to be used on Linux and for testing, but not when instantiating a TPM on Windows. This commit adds the `newWindowsKey20` function, which returns a key backed by a `windowsTPM`. The key is a `windowsAK20`, now also conforming to the `key` interface, so that it can be used for signing purposes.
On Windows, when the key is managed by the OS, keys are stored on the filesystem. When trying to create a key with the same name, this will fail with the following error: `NCryptCreatePersistedKey returned 8009000F: The operation completed successfully.` This commit adds support for deleting these keys, so that a new key can be created with the same name. Have only tested this on Windows so far. My assumption is that for keys created with `go-attestation` on Linux, the keys aren't persisted "inside the TPM", so there's nothing to do when deleting them, except for any keys managed externally.
|
Hey! Thanks so much for sending this and apologies for the delay. This is a pretty large amount of code. Would it be possible to set up a video call with @brandonweeks and I to chat about this patch? That might be faster than going over github |
|
Meeting notes:
|
|
@brandonweeks and I are going to setup some time to chat with go-tpm. @hslatman if there are specific things we can add to this package, feel free to break up this change and send individual API improvements. Happy to review that. Or we can wait and see how our conversations with the go-tpm folk go |
|
@ericchiang just saw @mjg59 pushing the changes from this PR to his branch (#274), so I think that's what's going to be tested in full now. Short summary of what's being changed / added:
Then the additions, all dependent on the Windows changes:
Let me know if you would like references to the actual lines. Changes are currently spread over multiple commits; I can clean those up and get some nicer commits in if that's preferred. |
|
I'd be happy to take the windows changes. Maybe send PRs one at a time for those? @brandonweeks do you know what our testing story for Windows changes is? I see a Windows environment in our test matrix, but do those come with TPMs? https://github.com/google/go-attestation/blob/master/.github/workflows/test.yml |
|
I have a couple of cleanups and one patch that fixes ECDSA signing for me built on top of Herman's work - I'll push those separately for review, but 4 patches gets us to the point where application keys can be generated and used for signing. I think it makes sense to land that functionality and then move forward on the additional work? (I just successfully got an application cert issued and stored in the Windows cert store backed by a TPM key) |
|
Updated #274 with the minimal set of patches that lands useful new functionality. Selfishly I think this entirely deals with my use case, but I think it's a reviewable chunk of code that we can build on for the rest of this :) |
|
I'll start with the current state of #274 and move my additions/changes on top of that. @ericchiang multiple PRs makes sense; I'll try to make that work. Some changes were fairly small though, so I might combine some again. |
This PR started with the changes from @mjg59 in #274 and the changes from @brandonweeks in master...brandonweeks:go-attestation:acme-device-attest and master...brandonweeks:go-attestation:x509ext. I added some functions on top to improve support for Windows.
It would be great if we could get these changes upstreamed. We're currently relying on a renamed fork instead.