hardware key runners #1977
hardware key runners #1977
Conversation
James-Pickett
commented
Dec 4, 2024
•
James-Pickett
commented
- adds secure enclave create key command to desktop user server, client, and root desktop runner
- adds a "runner" style struct to handle initializing keys for both tpm and secure enclave that is added to the rungroup in main launcher command, this allows for initializing (and retrying failures) of tpm and secure enclave keys
- signing with secure enclave will come in a follow on PR
| "err", err, | ||
| ) | ||
|
|
||
| if currentRetryInterval < maxRetryInterval { |
There was a problem hiding this comment.
lazy backoff ... is there a better way or is this good enough?
There was a problem hiding this comment.
I like this and think it's good enough! Since you use it here and in the tpmrunner (and since I can imagine it being useful in other areas too) I wonder if we might want to extract it into pkg/backoff?
There was a problem hiding this comment.
Oh, I just saw we do actually have a Backoff in that package -- I forgot we had anything in there besides WaitFor 😅 . Maybe an opportunity to use that/combine?
There was a problem hiding this comment.
I didn't really like what was in there for this application, because there was no way to stop, so I created a custom ticker with "Multiplicative Backoff" (duration * ticks) ... maybe went a little overboard =)
| defer ser.mux.Unlock() | ||
|
|
||
| cu, err := firstConsoleUser(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
Do we want a special error for no console users found (from line 265 below) so we can distinguish between that case and consoleuser.CurrentUsers(ctx) returning an error (257) here?
There was a problem hiding this comment.
I started to do this, but I don't see where we would need to switch on the error and behave differently. Do you have a specific case in mind?
There was a problem hiding this comment.
I suggested this while thinking about the other comment on handling the case where there's no console user -- #1977 (comment). I thought we might want special handling for the case where there's no console user (e.g. maybe back off to a longer interval faster? or something else?), so we'd need a specific error to be able to detect that state.
| @@ -0,0 +1,241 @@ | |||
| package tpmrunner | |||
There was a problem hiding this comment.
Would you add a quick diagram/writeup in the docs/architecture folder for this? Since this work traverses both root and user launcher, and the implementation differs between darwin/other OSes (plus the keys are pretty important) I think it's worth documenting how the different pieces fit together. Doesn't have to be in this PR!
There was a problem hiding this comment.
I'm a little curious about this as well. I understand why we need to move the secure enclave signing. But I'm much less sure what's happening with the TPM
There was a problem hiding this comment.
@RebeccaMahany , I will write up some docs for this PR
@directionless there are 2 reasons I went ahead and made the tpm a "runner" style like secure enclave.
-
We do see errors pop up with TPM operations that I can't reproduce. I'm hoping that these have something to do with the TPM not being ready or unavailable for some reason, taking this route we'll keep trying in the background hoping it becomes available.
-
It felt cleaner to have both hardware signature mechanisms operate in the same way. Understanding that no kind of hardware signing can be expected to be available immediately after start up.
| "err", err, | ||
| ) | ||
|
|
||
| if currentRetryInterval < maxRetryInterval { |
There was a problem hiding this comment.
I like this and think it's good enough! Since you use it here and in the tpmrunner (and since I can imagine it being useful in other areas too) I wonder if we might want to extract it into pkg/backoff?
| "github.com/kolide/launcher/pkg/traces" | ||
| ) | ||
|
|
||
| func setupHardwareKeys(ctx context.Context, slogger *slog.Logger, store types.GetterSetterDeleter) (keyInt, error) { | ||
| // SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. |
There was a problem hiding this comment.
| // SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. | |
| // SetHardwareKeysRunner creates a secure enclave runner and sets it as the agent hardware key as it also implements the keyInt/crypto.Signer interface. |
| } | ||
|
|
||
| k, err := tpm.New(priData, pubData) | ||
| // SetHardwareKeysRunner creates a tpm runner and sets it as the agent hardware key as it also implements the keyInt/cyrpto.Signer interface. |
There was a problem hiding this comment.
Naming question... If this returns a runner, why is it called Set?
There was a problem hiding this comment.
The runner also implements keyInt & crypto.Signer and gets set as the hardware key, maybe CreateSetHardwareKeysRunner? Or possible different functions for create and set?
| @@ -0,0 +1,241 @@ | |||
| package tpmrunner | |||
There was a problem hiding this comment.
I'm a little curious about this as well. I understand why we need to move the secure enclave signing. But I'm much less sure what's happening with the TPM
