VTPM: rework datamodel, fix nits needed by clients / detected by QA #4752
Conversation
Doing it once is enough for our purposes Add a check before saving so we can ensure the contents are base64-encoded. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
We don't want users of the constructor to have full control of the record, in particular control over the contents. This is because this leaks the concrete storage backend, which makes this very brittle. Instead limit the constructor to take only a VM reference. In the future the constructor we probably want to change the constructor to accept the profile the VTPM needs to have. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
VM admin is allowed to manage VM devices, so change vtpm create and destroy VTPM allowed roles Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont
force-pushed
the
private/paus/b128
branch
2 times, most recently
from
b732257
to
68004bb
Compare
ocaml/xapi/xapi_vtpm.ml
Outdated
| let backend = `xapi in | ||
| let contents = Xapi_secret.create ~__context ~value:"" ~other_config:[] in | ||
| let ref = introduce ~__context ~uuid ~vM ~backend ~profile ~contents in | ||
| Db.VTPM.create ~__context ~ref ~uuid ~vM ~backend ~unique:false |
There was a problem hiding this comment.
We should add unique and protected as parameters to create, defaulting to false if absent.
There was a problem hiding this comment.
While I agree on the unique field, I do not think it makes sense to give control to users of protected. It's a property of how the system handles the vtpm contents and should be read-only.
For the unique field I expected to defer this until it's properly implemented. I can change the constructor and ignore the value of the field for the time being.
There was a problem hiding this comment.
They are readonly after the object is created, but how else would the user chose whether they want a protected vTPM or not if not at create time? (you might want some vTPMs to be protected and others not). Anyway neither field is currently implemented, so we can defer adding them if it helps.
There was a problem hiding this comment.
I would say that the whole point of TPMs is to have some secrets protected, so I don't see the point in making users choose. I see it as a way to report the system property.
(I'm working on exposing the unique field in the create)
There was a problem hiding this comment.
I've added the unique parameter, there's no easy way to have it as an option: having option types in create functions is disallowed by the api generator, and because a unit parameter needs to be added to remove ambiguity cause by partial application.
I believe this is not such a bad compromise:
- for backwards compatibility we have the platform metadata in templates, which will always create non-unique vtpms, this is fine because this is wanted only to be able to install / boot windows 11 guests.
- New clients will need an updated SDK to create VTPMs anyway, this will make the option obvious to them and can make choices for their users.
There was a problem hiding this comment.
Depending on how we implement protection (e.g. per-host or per-pool encryption key) then a VM exported from a pool will only be reimportable on the same pool (unless you provide the encryption keys in some way, but those should be non-exportable...).
This is fine if you've actually stored secrets in the vTPM, however if you only want to share a Windows11 VM image between multiple pools, which has a vTPM (not for security purposes, but purely to satisfy Win11 requirements), then having it marked as protected will make that quite difficult (in particular none of the existing tooling/applications will work with it because they wouldn't know a key has to be supplied or how to supply it).
In that case you might want to create the VM with unprotected vTPM as a functionality/security tradeoff.
or we should provide a way to export a "wiped"/empty vTPM/vTPM with just public certs, in case a protected vTPM is being exported, acknowledging that this may lead to data loss if the VM has actually stored some secrets or data inside the vTPM.
But we can add the protected flag when we get to implementing it, there might be more flags in the future so we'll need to support a way to add more flags anyway.
psafont
force-pushed
the
private/paus/b128
branch
from
68004bb
to
5b20f7c
Compare
psafont
force-pushed
the
private/paus/b128
branch
from
5b20f7c
to
d92a9c5
Compare
psafont
force-pushed
the
private/paus/b128
branch
2 times, most recently
from
daaad3a
to
77c4697
Compare
psafont
force-pushed
the
private/paus/b128
branch
from
cdf0928
to
09aa6d2
Compare
psafont
force-pushed
the
private/paus/b128
branch
2 times, most recently
from
78e0a1f
to
63fc8f0
Compare
Reintroduces the previous "backend" field which is unused, kept to avoid issues with updating the schema database, now it's DynamicRO as it's not used in the constructor. Introduced the persistence backend field to declare how is the VTPM stored. For now there's only a single possible value: xapi's DB. This is intended to permit several persistence backends at the same time only for the benefit of developers and it's not meant to allow changing the backend for any single VTPM. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
The behaviour was disabled because the class was unused. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
is_unique is now required for creating vtpm objects. When creating VTPMs from templates where it has the vtpm in platform data, is_unique:false is used. This is because the mechanism is only needed for having a backwards-compatible way to create VMs that can run windows 11. Wanting to create unique VTPsM is not part of this so a new SDK will be needed to use this feature. This means removing the default vtpm profile from vms as well. I've also removed some duplication that was present in xapi_xenops for creating an empty vtpm. Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
Using xapi is not helpful, despite what the comment states Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
The previous message could be confusing and give no leads on how it could be fixed Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>
psafont
force-pushed
the
private/paus/b128
branch
from
63fc8f0
to
c2cdf85
Compare
Replace the profile with single fields, which as of now cannot be changed. Unique can be made configured later on.
This means deleting the default_vtpm_profile from the VM objects. The way the design is going it looks like "empty" or otherwise vtpms associated will replace this parameter, which contain this information and more.
vtpm.create's role has been changed and it's now similat to other classes like vgpu so it uses individual fields as parameters instead of whole records. This is because VTPM records cannot be easily created by clients. (for comparison VM does have a create method, but it has big warnings against using it and the default way to create VMs is by cloning templates, both in Xencenter and Xen Orchestra)
VTPM.backend has been reintroduced for internal purposes only
The contents of the VTPM are now only encoded once for serialization purposes. There's still a second encoding, but it's used for verification purposes and the result is discarded.
New builds are in progress