Sync varstore certificates in XAPI with those on disks #4659

benjamreis · 2022-03-24T09:41:49Z

Pool.set_uefi_certificates is implemented and calls Host.set_uefi_certificates on all hosts
Host.set_uefi_certificates writes certificates on disks
On XAPI startup certificates stored in XAPI's Host.uefi_certificates are written on disks
When a host joins the pool Host.set_uefi_certificates is called with the certificates stored in XAPI's Pool.uefi_certificates

This means:

At every XAPI startup the certificates in host disks are synced with XAPI's Host.uefi_certificates
When Pool.set_uefi_certificates is called all hosts are synced in XAPi and disks with XAPI's Pool.uefi_certificates
When Host.set_uefi_certificates is called the host certificates on disk are synced with XAPI's Host.uefi_certificates

Also: Host.set_uefi_certificates no longer calls Pool.set_uefi_certificates, this requires changes
in external libs (varstored, uefistored, etc) to set the pool's certificates: call Pool.set_uefi_certificates.

See: #4647

Signed-off-by: BenjiReis benjamin.reis@vates.fr

lindig · 2022-03-24T11:23:59Z

ocaml/idl/datamodel_pool.ml

+ ~params:
+ [
+ (Ref _pool, "self", "The pool")
+ ; (String, "value", "The certificates to apply to the pool anbd its hosts")


Typo in and. Is the string the certificate (encoded?) or a reference to it, like a path? It would be good to make this clear.

Like for Host.set_uefi_certificates it's the tarball.

lindig · 2022-03-24T11:26:46Z

ocaml/xapi/xapi_host.ml

+let extract_certificate_file name =
+ if String.contains name '/' then
+ (* Internal error: tarfile not created correctly *)
+ failwith ("Invalid path in certificate tarball: " ^ name) ;


The error could be read as referring to a path in a tar file but it refers to the path of the tar file itself.

No it references the path inside the tarball to an .auth file.

You could make this more specific: Path in certificate tarball %s contains /

lindig · 2022-03-24T11:28:45Z

ocaml/xapi/xapi_host.ml

+ List.iter
+ (fun name ->
+ let path = Filename.concat !Xapi_globs.varstore_dir name in
+ debug "*** BRS: Remove UEFI cert %s" path ;


Remove BRS:

This are temporary logs for my tests :)
Should I let them once I'm done? (without the *** BRS: of course)

lindig · 2022-03-24T11:30:17Z

ocaml/xapi/xapi_host.ml

+ debug "*** BRS: Remove UEFI cert %s" path ;
+ try Sys.remove path
+ with
+ | Sys_error e


Matching the string seems to me like a poor way to detect this problem. Use a different function to remove a file that uses a better interface for errors.

I'm not aware of another lib to remove a file.

Unix.unlink

lindig · 2022-03-24T11:31:30Z

ocaml/xapi/xapi_host.ml

+ ()
+ )
+ ["PK.auth"; "KEK.auth"; "db.auth"; "dbx.auth"] ;
+ if contents <> "" then (


Why is the empty string special? Might want to log this.

If the string is empty it means there's no certificates in XAPI to extract on the host disk.

lindig · 2022-03-24T13:18:31Z

ocaml/xapi/xapi_host.mli

@@ -479,6 +479,9 @@ val nvidia_vf_setup :
 val allocate_resources_for_vm :
 __context:Context.t -> self:API.ref_host -> vm:API.ref_VM -> live:bool -> unit

+val save_uefi_certificates_to_dir :


Is the name confusing? It has dir in its name but not in the parameter list.

benjamreis · 2022-03-25T18:33:20Z

I was wondering: how would you feel to remove the host.{set/get}_uefi_certificates? For now it can be deprecated and call the Pool ones and be removed in the future.

It seems this API and field are useless (unless there is a usecase where a host needs to have different certificates than the others in the pool but I can't think of one).

What do you think?

lindig · 2022-03-28T08:20:52Z

ocaml/xapi/xapi_host.ml

+ path
+
+let with_temp_file_contents ~contents f =
+ let filename, out = Filename.open_temp_file "xapi_uefi_certificates" "tar" in


It's more common to use a dash (-) in file names than the underscore.

It comes from already existing code, but I can change that 👍

benjamreis · 2022-03-29T11:24:36Z

Latest commit is an implementation of what is proposed in my previous comment, and @stormi 's one in the issue.

Deprecate Host.uefi_certificates in favor of Pool API:

It means an host can't have different certs than the pool ones
No need to update certificate on a joining hosts since it will be synched after its toolstack is restarted
Host.set_uefi_certificates now calls Pool.set_uefi_certificates to update all pool's hosts' certificates
Host.uefi_certificates can be empty if the Pool API is used

I think it's the right way to go since having different certs on pool's hosts seems really dangerous to me.

lindig · 2022-03-30T07:19:54Z

ocaml/idl/datamodel_host.ml

+let write_uefi_certificates_to_disk =
+ call ~name:"write_uefi_certificates_to_disk"
+ ~lifecycle:[(Published, rel_next, "")]
+ ~doc:"Writes the UEFI certificates on a host disk"


"writes to"?

lindig · 2022-03-30T07:21:16Z

ocaml/idl/datamodel_pool.ml

+let set_uefi_certificates =
+ call ~name:"set_uefi_certificates"
+ ~lifecycle:[(Published, rel_next, "")]
+ ~doc:"Sets the UEFI certificates on a pool and all its hosts"


"Sets .. for a pool"?

lindig · 2022-03-30T07:23:23Z

ocaml/xapi/xapi_host.ml

+let extract_certificate_file name =
+ if String.contains name '/' then
+ (* Internal error: tarfile not created correctly *)
+ failwith ("Invalid path in certificate tarball: " ^ name) ;


You could make this more specific: Path in certificate tarball %s contains /

benjamreis · 2022-04-01T07:23:09Z

I'll rebase and rework the commit message once the code stop changing before the merge. :)

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

For XCP-ng > 8.2.1 there is a new behavior regarding SB certs management See: xapi-project/xen-api#4659 Keep previous behavior for XCP-ng <= 8.2.1 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

- `Pool.set_uefi_certificates` is implemented and writes the certificates on all its hosts' disks - `Host.set_uefi_certificates` is now deprecated and transmit the call to the pool method - `Host.uefi_certificates` is deprecated as well as it's getter, the value is not updated. - On XAPI startup certificates stored in XAPI's `Pool.uefi_certificates` are written on disks - When a host joins the pool's certificates are written on its disk. This means: - At every XAPI startup the certificates in host disks are synced with XAPI's `Pool.uefi_certificates` - When `Pool.set_uefi_certificates` is called all hosts are synced on their disks with XAPI's `Pool.uefi_certificates`. Also: `Host.set_uefi_certificates` calls should be replaced by `Pool.set_uefi_certificates`, this requires changes in external libs (varstored, uefistored, etc) to set the pool's certificates: call `Pool.set_uefi_certificates`. See: xapi-project#4647 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis · 2022-04-07T08:29:33Z

Hi!

I rebased the code into one commit with a (hopefully) clear commit message.
I wrote tests for this in our test repo and they're successful so everything good on our side. :)
Plus my manual testing also showed the expected behavior.

For XCP-ng > 8.2.1 there is a new behavior regarding SB certs management See: xapi-project/xen-api#4659 Keep previous behavior for XCP-ng <= 8.2.1 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>

For XCP-ng > 8.2.1 there is a new behavior regarding SB certs management See: xapi-project/xen-api#4659 Keep previous behavior for XCP-ng <= 8.2.1 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

Starting from next release, the host disk certificate will be updated by the pool ones at XAPI startup instead of when a VM starts. See: xapi-project/xen-api#4659 Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

psafont marked this pull request as draft March 24, 2022 10:54

lindig reviewed Mar 24, 2022

View reviewed changes

lindig approved these changes Mar 28, 2022

View reviewed changes

benjamreis force-pushed the synch-varstored-between-hosts branch 8 times, most recently from 28722de to 8dc49ff Compare March 29, 2022 18:29

lindig reviewed Mar 30, 2022

View reviewed changes

benjamreis force-pushed the synch-varstored-between-hosts branch from 8dc49ff to 4140850 Compare March 30, 2022 09:33

benjamreis marked this pull request as ready for review March 31, 2022 09:18

benjamreis changed the title ~~[WIP] Sync varstore certificates in XAPI with those on disks~~ Sync varstore certificates in XAPI with those on disks Mar 31, 2022

benjamreis added a commit to xcp-ng/uefistored that referenced this pull request Apr 1, 2022

Adapt code to new uefi certificates API

456d50a

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis mentioned this pull request Apr 1, 2022

Update secureboot-certs for new UEFI certificates API xcp-ng/uefistored#46

Merged

benjamreis added a commit to xcp-ng/uefistored that referenced this pull request Apr 1, 2022

Update secureboot-certs for new UEFI certificates API

050f670

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis mentioned this pull request Apr 5, 2022

Update cert inheritance tests to match new xapi behavior xcp-ng/xcp-ng-tests#93

Merged

benjamreis force-pushed the synch-varstored-between-hosts branch from e73b8df to 53d656b Compare April 7, 2022 08:28

edwintorok approved these changes Apr 13, 2022

View reviewed changes

edwintorok merged commit 85fdc21 into xapi-project:master Apr 13, 2022

psafont deleted the synch-varstored-between-hosts branch April 13, 2022 13:06

psafont restored the synch-varstored-between-hosts branch April 13, 2022 13:06

benjamreis deleted the synch-varstored-between-hosts branch April 13, 2022 16:29

psafont added a commit to psafont/xen-api that referenced this pull request Apr 13, 2022

CP-39551: avoid warnings from PR xapi-project#4659

02b3944

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>

psafont added a commit to psafont/xen-api that referenced this pull request Apr 13, 2022

CP-39551: avoid warnings from PR xapi-project#4659

016ba07

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>

psafont added a commit to psafont/xen-api that referenced this pull request Apr 14, 2022

CP-39551: avoid warnings from PR xapi-project#4659

9a52bfc

Signed-off-by: Pau Ruiz Safont <pau.safont@citrix.com>

benjamreis mentioned this pull request Apr 14, 2022

RFC: improved UEFI Secure Boot certificates management in XAPI #4647

Closed

benjamreis added a commit to xcp-ng/uefistored that referenced this pull request Apr 14, 2022

Update secureboot-certs for new UEFI certificates API

d5b2ab6

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis added a commit to xcp-ng/uefistored that referenced this pull request Apr 14, 2022

Update secureboot-certs for new UEFI certificates API

5a2a7c2

See: xapi-project/xen-api#4659 `Host` API is deprecated in favor of `Pool` API Signed-off-by: BenjiReis <benjamin.reis@vates.fr>

benjamreis mentioned this pull request Apr 19, 2022

Update Guest SB doc to reflect next release behavior xcp-ng/xcp-ng-org#170

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Sync varstore certificates in XAPI with those on disks #4659

Sync varstore certificates in XAPI with those on disks #4659

benjamreis commented Mar 24, 2022 •

edited

Loading

lindig Mar 24, 2022 •

edited

Loading

benjamreis Mar 24, 2022

lindig Mar 24, 2022

benjamreis Mar 24, 2022

lindig Mar 30, 2022

lindig Mar 24, 2022

benjamreis Mar 24, 2022

lindig Mar 24, 2022

benjamreis Mar 24, 2022

lindig Mar 24, 2022

lindig Mar 24, 2022

benjamreis Mar 24, 2022

lindig Mar 24, 2022

benjamreis commented Mar 25, 2022 •

edited

Loading

lindig Mar 28, 2022

benjamreis Mar 28, 2022

benjamreis commented Mar 29, 2022

lindig Mar 30, 2022

lindig Mar 30, 2022

lindig Mar 30, 2022

benjamreis commented Apr 1, 2022

benjamreis commented Apr 7, 2022

Sync varstore certificates in XAPI with those on disks #4659

Sync varstore certificates in XAPI with those on disks #4659

Conversation

benjamreis commented Mar 24, 2022 • edited Loading

lindig Mar 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamreis commented Mar 25, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamreis commented Mar 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjamreis commented Apr 1, 2022

benjamreis commented Apr 7, 2022

benjamreis commented Mar 24, 2022 •

edited

Loading

lindig Mar 24, 2022 •

edited

Loading

benjamreis commented Mar 25, 2022 •

edited

Loading