-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shim 15.8 for Azure Linux 3.0 (x86_64 and aarch64) #427
Comments
Hi, just checking if anything else is needed for this review. |
@ddstreetmicrosoft no don't think so. I gave it just an initial read and will do a complete review in the next few days. In the meantime it really helps us to speed up the queue if submitters review also other submissions. |
There seems something wrong with the gcc installation in the Dockerfile:
@ddstreetmicrosoft can you have a look at it? |
@THS-on thanks for taking a look - I'll look into it and see what is going on. |
@THS-on @ddstreetmicrosoft I've identified the issue and would like to update our Azure Linux 3.0 submission to reflect the changes. New tagged submission - https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240701
Let me know if you'd prefer a new GH issue for the submission or to handle in some other way. Details for the submission difference:
|
Review for
|
updated; I'll let Chris respond to the other questions |
Sorry about the confusion. We are releasing with 2.06 with Fedora 34 patches for invoking EFI Handover Protocol. We had initially made the move to 2.12 but encountered an unexpected regression where the TPM Event Log was not being handed over to the kernel, only when Secure Boot was enabled. We'll be sticking here with 2.06 for the foreseeable future of Azure Linux 3.0.
All of our default images and configurations set We have previously mulled over bringing in the out-of-tree kernel patches to link Secure Boot to forcing lockdown mode directly, but we thus far decided to stick closer to upstream stable LTS kernel for the time being. |
@ddstreetmicrosoft @christopherco thank you for the clarifications. Regarding the grub config. Forcing |
Ah yes, in our case, it is possible for a user that has root privileges to remove the lockdown setting since the kernel command line setting is located in our grub.cfg. I can discuss internally on pulling in the lockdown patches to cover the difference. |
Yeah you can either include the lockdown patches or build your signed kernel Note tho that the following lockdown patch is not upstream, so you probably want to include either way: |
Following up here - our 6.6.47 kernel now has the lockdown patches that will tie kernel lockdown to secure boot state
|
@christopherco great! The the question is if it makes already sense to also rotate your signing certificate for the kernel and then put the old one on the revocation list, so that older kernels cannot be booted with the new shim? |
@THS-on This shim actually has a new certificate that has not been used to sign any boot artifacts for Azure Linux yet. Once this shim is approved, we will be updating our signing certificate in our official Azure Linux 3.0 builds to sign with the new signing cert. So, we had already planned in a certificate rotation as part of this new shim. |
@christopherco thanks for the clarification. I saw that the current certificate was already included in the other Shims DB, but now also saw checked that the Grub in 2.0 is still signed with the other cert ( LGTM, just needs another reviewer. |
I'm taking the extra review here, but have run into some trouble. The July version of the review (https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240701) is unable to be built, as the container registry's DNS no longer resolves. Additionally, I cannot find the specified tag on the I tried to use the 3.0 tag from mcr.microsoft.com for azurelinux, but the repositories no longer have the versions of packages that the Dockerfile is expecting. Pausing review until this issue is resolved, though the rest of procedural things look OK. |
@NeilHanlon Thanks for doing the extra review and apologies about the trouble. Note: the shim binary hashes did not change between the July tag and this new tag.
Thanks @gjswalling for making the Dockerfile changes on our side! |
Very tempted to NACK this review. Neither docker nor podman will work for me here. |
If run with docker buildx build . it actually work and reproduces for x86 at least. |
We say "docker build ." in the docs, no mention of buildx. |
As noted here, we created the single Dockerfile using a here-doc, which requires use of buildx instead of build. I did this to simplify the work of reviewers, so that a single Dockerfile could be used for both architectures, as I saw no way to craft a single Dockerfile that would allow building the shim on both architectures. As shown using the Fedora docker command, the old
However, it looks like Fedora doesn't provide buildx until release 41: I had been testing this using Ubuntu which has provided docker buildx as an option in current stable releases, so I didn't realize this would add difficulty to reproducing the docker build on Fedora. In any case, we can certainly split the Dockerfile into two per-arch Dockerfiles to make it easier for you to review, if that's the easiest path forward. |
I split the Dockerfile into per-arch files, located at Using Fedora 40, I can run Using Fedora 41, because it switches the docker backend builder over to the newer BuildKit, using either file will work, i.e. Obviously, using the arch-specific Dockerfile requires actually running on the correct arch (i.e. use This is included in this updated tag: |
I'm using Debian 12 (Bookworm, current stable release) here - Fedora doesn't help... It would be nice if Docker provided stable interfaces and useful isolation. But hey, moon on a stick... |
One more minor update, sorry: This only fixes the aarch64-specific Dockerfile, which had some of the lines using
Unfortunately Debian Bookworm doesn't seem to provide any package for docker buildx, but the arch-specific Dockerfile does build correctly:
|
|
@lorddoskias thanks for the review! We'll include the fedora SBAT entries in addition to our own and update our submission here.
https://src.fedoraproject.org/rpms/shim-unsigned-x64/blob/f40/f/sbat.redhat.csv |
@lorddoskias We have updated our submission: This update introduces the Fedora SBAT entries in addition to our Azure Linux SBAT entries.
Updated hashes:
|
Minor update to our submission's readme.md to reflect the latest submission state and update the various answers from this review for reviewers: |
Gentle bump @NeilHanlon, @steve-mcintyre, @lorddoskias . Checking if there are any concerns with the most recent submission information |
Shim 15.8 for Azure Linux 3.0 (x86_64 and aarch64)GoodGeneral
Shim
GRUB
fwupd
sd-boot
Linux
Issues / queries
|
Thanks @steve-mcintyre for the review!
Ah apologies for that. This is our updated grub SBAT data from the current grub build.
If curious, this was pulled just now from our current preview RPMs located here -> https://packages.microsoft.com/azurelinux/3.0/preview/base/x86_64/Packages/g/grub2-2.06-21.azl3.x86_64.rpm
|
@christopherco thanks, just what I was asking for. Good to go! |
Thanks for the help, folks, greatly appreciated! |
For good measure, and because I was half done with it last night... My extra review :)
Review of
|
Confirm the following are included in your repo, checking each box:
What is the link to your tag in a repo cloned from rhboot/shim-review?
Initial submission:
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240531
Update (2024-07-01):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240701
Update (2024-09-23):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240923
Update (2024-09-30):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240930
Another Update, sorry (2024-09-30):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20240930.1
Update (2024-10-28):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20241028
Another Update, sorry (2024-10-28):
https://github.com/microsoft/shim-review/tree/azurelinux-shim-aa64-x64-20241028.1
What is the SHA256 hash of your final SHIM binary?
Initial submission:
0eff03502514be459b182c3cda1cef6a3762cbf10462591685a17c356e42774d shimaa64.efi b6a99795d9f3e882afa318d11bdb648dff7e547470d14bfeba03af385eb452fc shimx64.efi
Update (2024-07-01):
7905c30de3109eb4ff8a5d198f5077bceb35b0fc3559b03924cf78a96e511bd0 shimaa64.efi
83c927eada08e0811adbaab47323841808d79e51d11aa8072c552bfbf80af801 shimx64.efi
Update (2024-10-28):
ddf770c9cac6a5cd693928bb047ea7c0d0dce51a3b7f4cde4dc08a919ab4538a shimaa64.efi
ff49c422cab4d6252631e0e8593ab28d1e8e2f30b6c7b86593381f368bf1e314 shimx64.efi
What is the link to your previous shim review request (if any, otherwise N/A)?
#387
If no security contacts have changed since verification, what is the link to your request, where they've been verified (if any, otherwise N/A)?
Dan Streetman: #387 (comment)
Chris Co: #387 (comment)
The text was updated successfully, but these errors were encountered: