vTPM : add state management

[shjala]

This PR add state management to vTPM, allowing the domain manager/kvm to :

• Send purge request when the VM is being deleted. Purge request will delete the data of the particular SWTPM instance associated with the VM/App.
• Send terminate request if kvm failed to run the VM/App. Delete kills the SWTPM instance if stalled and updates the live instances counter.

(Research has shown Friday is best day to write buggy code, so I keep it in draft state to test some more with Eden and make sure everything works as expected)

[eriknordmark]

When you delete an app instance, do you first purge and then delete the swtpm instance?

Down the road might need garbage collection as well to handle the case when

1. app instance is deployed - running
2. device is powered off
3. app instance is deleted in controller
4. device is powered on

In that sequence the EVE agents to not see any delete handler being called; merely the absense of a create handler. We have specific logic in e.g., volumemgr to look for which volumes and content-trees (latter managed by containerd) no longer have any references from the config and gabage collect those to handle such a sequence.
Other things which store state in /persist need something similar.

Kicking off tests.

[OhmSpectator]

When you delete an app instance, do you first purge and then delete the swtpm instance?

Down the road might need garbage collection as well to handle the case when

1. app instance is deployed - running
2. device is powered off
3. app instance is deleted in controller
4. device is powered on

In that sequence the EVE agents to not see any delete handler being called; merely the absense of a create handler. We have specific logic in e.g., volumemgr to look for which volumes and content-trees (latter managed by containerd) no longer have any references from the config and gabage collect those to handle such a sequence. Other things which store state in /persist need something similar.

That's a good point. As far as I understand, the current implementation will also not handle the case when we start swTPM for the same domain twice (for example, after rebooting the node), as it will try to create some files again, but they already exist. Or maybe I'm wrong?
If that's the case, will we solve it in the scope of this PR? It looks relevant...

[shjala]

(for example, after rebooting the node)

No, if a (id,pid) pair already exist in the list of instances, vTPM just returns with no action, but I'll add a comment to make it more clear, in any other case, it just checks that the required directory exists and creates them if not (no overwriting).

eve/pkg/vtpm/swtpm-vtpm/src/main.go

Lines 283 to 285 in 7e06339

	if _, ok := pids[id]; ok {
	return fmt.Errorf("SWTPM instance with id %s already running", id)
	}

[OhmSpectator]

(for example, after rebooting the node)

No, if a (id,pid) pair already exist in the list of instances, vTPM just returns with no action, but I'll add a comment to make it more clear, in any other case, it just checks that the required directory exists and creates them if not (no overwriting).

eve/pkg/vtpm/swtpm-vtpm/src/main.go

Lines 283 to 285 in 7e06339

	if _, ok := pids[id]; ok {
	return fmt.Errorf("SWTPM instance with id %s already running", id)
	}

But the pids slice is empty after the board reboot... So it will pass this check.

[shjala]

But the pids slice is empty after the board reboot... So it will pass this check.

Sorry I thought you mean rebooting the VM, in that case it is still safe, vTPM calls MkdirAll which If path is already a directory, it does nothing and returns.

In no case vTPM overwrites the data, it just creates if no dir exist with ID name, and deletes the ID dir when VM is being deleted (not turned off, rebooted or crashed, only deleted).

[shjala]

But the pids slice is empty after the board reboot... So it will pass this check.

Sorry I thought you mean rebooting the VM, in that case it is still safe, vTPM calls MkdirAll which If path is already a directory, it does nothing and returns.

In no case vTPM overwrites the data, it just creates if no dir exist with ID name, and deletes the ID dir when VM is being deleted (not turned off, rebooted or crashed, only deleted).

commented in the code for brevity.

[eriknordmark]

That's a good point. As far as I understand, the current implementation will also not handle the case when we start swTPM for the same domain twice (for example, after rebooting the node), as it will try to create some files again, but they already exist. Or maybe I'm wrong? If that's the case, will we solve it in the scope of this PR? It looks relevant...

Do we have any test cases for this?
If not it would make sense to add a test case (in addition to making sure it works manually.)
It doesn't need to be part of this PR, but I think it needs to be fixed before the next LTS when users are more likely to start using the new vTPM implementation.

[OhmSpectator]

But the pids slice is empty after the board reboot... So it will pass this check.

Sorry I thought you mean rebooting the VM, in that case it is still safe, vTPM calls MkdirAll which If path is already a directory, it does nothing and returns.

In no case vTPM overwrites the data, it just creates if no dir exist with ID name, and deletes the ID dir when VM is being deleted (not turned off, rebooted or crashed, only deleted).

Thanks. @shjala ! And what about the case that @eriknordmark has described? Removal of the app that was deleted while the device was not booted. Hence, we do not have a call to handleDelete that triggers removing the files.

[shjala]

That's a good point. As far as I understand, the current implementation will also not handle the case when we start swTPM for the same domain twice (for example, after rebooting the node), as it will try to create some files again, but they already exist. Or maybe I'm wrong? If that's the case, will we solve it in the scope of this PR? It looks relevant...

Do we have any test cases for this? If not it would make sense to add a test case (in addition to making sure it works manually.) It doesn't need to be part of this PR, but I think it needs to be fixed before the next LTS when users are more likely to start using the new vTPM implementation.

We have unit test and eden test, the eden test tests both Aziot and vTPM, but still not merged because #4204 is not part of any release yet :
https://github.com/lf-edge/eden/blob/4f46f8a900b1ab7a256a686490da9923f51b362f/tests/aziot/aziot_test.go#L1-L12

I have ran both tests manually and it pass, and I'm cleaning the TPM required unit tests a bit, in another PR I'll add the vTPM unit test to that workflow.

[shjala]

Removal of the app that was deleted while the device was not booted. Hence, we do not have a call to handleDelete that triggers removing the files.

@OhmSpectator I need to dig into it and figure out how to do it, I'll do it in another PR if thats OK.

[OhmSpectator]

Removal of the app that was deleted while the device was not booted. Hence, we do not have a call to handleDelete that triggers removing the files.

@OhmSpectator I need to dig into it and figure out how to do it, I'll do it in another PR if thats OK.

Ok, that makes sense.

[shjala]

@eriknordmark I'll expand the eden test to test for the case you described too.

[shjala]

I would put the TPM field in DomainStatus.VMConfig struct.

it was already part DomainStatus.VMConfig, I changed it based on your comment. I'm confused.

[shjala]

I would put the TPM field in DomainStatus.VMConfig struct.

it was already part DomainStatus.VMConfig, I changed it based on your comment. I'm confused.

my bad, names are too similar.

[OhmSpectator]

In general, I like the new approach! Clean and clear!

[OhmSpectator]

I would put the TPM field in DomainStatus.VMConfig struct.

it was already part DomainStatus.VMConfig, I changed it based on your comment. I'm confused.

my bad, names are too similar.

yeah, sorry for the confusion...

[OhmSpectator]

In general, looks good to me. Nevertheless, let's @eriknordmark also take a look.

Also, let's run the tests.

[OhmSpectator]

Waiting for https://api.github.com/repos/lf-edge/eve/actions/workflows/build.yml/runs?head_sha=1bc865f22e6bf6b4f46ca253863ce691787d17a4&status=success to return a non-empty list and then rerun the tests...

[OhmSpectator]

Rerun the tests

[OhmSpectator]

@shjala, by the way, for the case when we use the swtpm tool for the same domain after reboot, we think it will work as we store the state on /persist. But shouldn't we start the tool with the --flags not-need-init option?

I would like to clarify it before merging.

[shjala]

@shjala, by the way, for the case when we use the swtpm tool for the same domain after reboot, we think it will work as we store the state on /persist. But shouldn't we start the tool with the --flags not-need-init option?

I would like to clarify it before merging.

AFAIK in HW TPM_Init flag sets some internal TPM flags to make it operational, here it is sent by Qemu through the SWTPM control socket. I like to keep it this way to be as close to the HW implementation as possible.

[shjala]

@OhmSpectator if you like to know more :

Seabios sends the TPM_Startup for us :
https://github.com/coreboot/seabios/blob/2424e4c04aa30d90e85073ea41d18a7845460783/src/tcgbios.c#L1107

[OhmSpectator]

@shjala, by the way, for the case when we use the swtpm tool for the same domain after reboot, we think it will work as we store the state on /persist. But shouldn't we start the tool with the --flags not-need-init option?

I would like to clarify it before merging.

AFAIK in HW TPM_Init flag sets some internal TPM flags to make it operational, here it is sent by Qemu through the SWTPM control socket. I like to keep it this way to be as close to the HW implementation as possible.

Got it, thanks!

[eriknordmark]

We have unit test and eden test, the eden test tests both Aziot and vTPM, but still not merged because #4204 is not part of any release yet :

FWIW 13.2.0 is now out and includes #4204

[shjala]

We have unit test and eden test, the eden test tests both Aziot and vTPM, but still not merged because #4204 is not part of any release yet :

FWIW 13.2.0 is now out and includes #4204

Great, I will clean the vTPM eden tests for merging.

[OhmSpectator]

All green! @shjala, don't you mind if we merge it? =)

[shjala]

All green! @shjala, don't you mind if we merge it? =)

go for it thanks!