CP-30434: Save XAPI uefi_certificates to varstored directory if the certificate files don't already exist there and parse secureboot=auto #3813
Conversation
ocaml/xapi/xapi_vm.ml
Outdated
|
|
||
| (* Check to see if we're using correct device-model when vm has VUSBs*) | ||
| if not (List.exists (fun e -> e = "PK.auth") (Array.to_list (Sys.readdir "/usr/share/varstored"))) then |
There was a problem hiding this comment.
You can use Array.exists directly.
rwdrich
force-pushed
the
private/richarddav/CP-30434
branch
from
d1aafc0
to
492b316
Compare
|
Don't merge. Need to move away from deprecated package |
ocaml/xapi/xapi_vm.ml
Outdated
| let contents = (Xapi_stdext_base64.Base64.decode (Db.Pool.get_uefi_certificates ~__context ~self)) in | ||
| Unixext.write_string_to_file filename contents; | ||
| Tar_unix.Archive.extract (fun _ -> !Xapi_globs.varstore_dir) file; | ||
| Unix.close file |
There was a problem hiding this comment.
You want to use finally to make sure this close is always executed.
ocaml/xapi/xapi_vm.ml
Outdated
| let save_uefi_certificates_to_dir ~__context ~self = | ||
| let dir = try Sys.readdir !Xapi_globs.varstore_dir | ||
| with Sys_error _ -> | ||
| Xapi_stdext_unix.Unixext.mkdir_rec "/usr/share/varstored" 0o755; |
There was a problem hiding this comment.
This could still fail if /usr/share/varstored is not a directory but an existing file. It might be worth checking the exception more carefully and to fail in such a case.
ocaml/xapi/xapi_vm.ml
Outdated
| with _ -> | ||
| debug "Error writing uefi certificates to file"; | ||
| begin | ||
| let _dir = Sys.readdir !Xapi_globs.varstore_dir in |
There was a problem hiding this comment.
You could use Sys.file_exists ... && Sys.is_directory ... instead of waiting for exception
ocaml/xapi/xapi_vm.ml
Outdated
| Tar_unix.Archive.extract (fun _ -> !Xapi_globs.varstore_dir) file; | ||
| finally (fun () -> Unix.close file) (fun () -> ()) | ||
| end | ||
| with _ -> |
There was a problem hiding this comment.
Would be good to know what the error was, there is a D.log_and_ignore_exn you could use for that.
Alternatively you can call D.log_backtrace () and log (Printexc.to_string e)
There was a problem hiding this comment.
This with doesn't exist anymore by changing to if's above
ocaml/xapi/xapi_vm.ml
Outdated
| let contents = (Xapi_stdext_base64.Base64.decode (Db.Pool.get_uefi_certificates ~__context ~self)) in | ||
| Unixext.write_string_to_file filename contents; | ||
| Tar_unix.Archive.extract (fun _ -> !Xapi_globs.varstore_dir) file; | ||
| finally (fun () -> Unix.close file) (fun () -> ()) |
There was a problem hiding this comment.
The finally should wrap everything right after you opened the file: any of them can raise an exception, and you'd leak a file descriptor. As it is written the finally here is a noop.
Would be better if you used Unixext.with_file instead which already does the right thing internally.
| with Sys_error _ -> | ||
| debug "varstored directory does not exist"; | ||
| end | ||
|
|
There was a problem hiding this comment.
Would be good to keep track of whether this function succeeded or not, either by making it return true/false, or (re)raising an exception when something fails.
For this code it is probably better to reraise the exception, e.g. failwith "Failed to save UEFI certificates: varstored directory does not exist". I think this will get converted to an internal error at the API level, probably worth double checking by triggerring the failure on purpose.
There was a problem hiding this comment.
Do I want to reraise the exception. What's the expected path for a non-UEFI host? Should there be a condition before the function call, or should the function be a no-op on a BIOS machine (because varstored won't be running, so won't have any dir)?
There was a problem hiding this comment.
The directory is created by the host installer when the varstored package is installed, should always be there.
If it is missing nothing will notice that these files are missing, and starting a UEFI VM would fail regardless of secureboot already, so if we want to check anything related to that then secureboot is not the place for it.
Lets drop this whole else try branch.
rwdrich
force-pushed
the
private/richarddav/CP-30434
branch
from
84b0a0c
to
b78e7a1
Compare
|
Don't merge |
rwdrich
force-pushed
the
private/richarddav/CP-30434
branch
2 times, most recently
from
9e5244d
to
def7d25
Compare
| with Sys_error _ -> | ||
| debug "varstored directory does not exist"; | ||
| end | ||
|
|
There was a problem hiding this comment.
The directory is created by the host installer when the varstored package is installed, should always be there.
If it is missing nothing will notice that these files are missing, and starting a UEFI VM would fail regardless of secureboot already, so if we want to check anything related to that then secureboot is not the place for it.
Lets drop this whole else try branch.
The first time a VM is booted transform `platform:secureboot=auto` to `true` if `Pool.uefi_certificates` field is present and into `false` otherwise. Signed-off-by: Richard Davies <richard.davies@citrix.com>
rwdrich
force-pushed
the
private/richarddav/CP-30434
branch
from
bbdfe11
to
355804a
Compare
No description provided.