Skip to content
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 OL9 (ol9-shim-x86_64-aarch64-20240212) #378

Closed
8 tasks done
iokomin opened this issue Feb 12, 2024 · 18 comments
Closed
8 tasks done

Shim 15.8 for OL9 (ol9-shim-x86_64-aarch64-20240212) #378

iokomin opened this issue Feb 12, 2024 · 18 comments
Assignees
Labels
accepted Submission is ready for sysdev

Comments

@iokomin
Copy link

iokomin commented Feb 12, 2024

Confirm the following are included in your repo, checking each box:

  • completed README.md file with the necessary information
  • shim.efi to be signed
  • public portion of your certificate(s) embedded in shim (the file passed to VENDOR_CERT_FILE)
  • binaries, for which hashes are added to vendor_db ( if you use vendor_db and have hashes allow-listed )
  • any extra patches to shim via your own git tree or as files
  • any extra patches to grub via your own git tree or as files
  • build logs
  • a Dockerfile to reproduce the build of the provided shim EFI binaries

What is the link to your tag in a repo cloned from rhboot/shim-review?


https://github.com/oracle/shim-review/tree/ol9-shim-x86_64-aarch64-20240212


What is the SHA256 hash of your final SHIM binary?


29aa4c77f237df563941029c2b49d9d75509bc4da3d516c0181b0f1f3400e789 shimaa64.efi
26ddbfb224c1a09368b4e2472de6707a26c3ce26aa00f783620e8fb0b98eec67 shimx64.efi


What is the link to your previous shim review request (if any, otherwise N/A)?


#305

@SherifNagy
Copy link
Collaborator

While I am not an official reviewer, here are my comments "looking at latest tag: https://github.com/oracle/shim-review/tree/ol9-shim-x86_64-aarch64-20240212":

  • SHIM reproduce okay with the provided dockerfile
  • SHIM sources within the SRPM matches the release hash
  • SHIM includes 3 EV certs for x64 and different kernel EV certs for aa64, they are valid between 12 and 18 months
  • SHIM SBAT entries looks good
  • SHIM SBAT entry for the vendor looks good, didn't get bumped up , remains at shim.ol,3
  • Contacts GPG keys looks good
  • Certs protection with HSM story looks good
  • Grub modules looks good, no NTFS module
  • SBAT entry for grub looks good and it's grub,3 since no NTFS patches has been applied to their grub sources
  • NX is disabled and doesn't exist in the boot chain

@SherifNagy
Copy link
Collaborator

Just noticed one thing actually, the grub2 sbat entry has @@Version@@ instead of the actual version, would make sense to get those values from the .efi directly

@iokomin
Copy link
Author

iokomin commented Feb 21, 2024

@SherifNagy thanks for finding, updated grub2 sbat metadata to actual values from built OL9 grub{arch}.efi binary.

@SherifNagy
Copy link
Collaborator

One small note, I think the UKI sbat entry aren't declared in the issue

@iokomin
Copy link
Author

iokomin commented Mar 4, 2024

@SherifNagy Oracle Linux does not build UKI images and hence does not sign it at the moment. Once UKI feature supported, new SBAT entry will be considered in corresponding shim update submission.

@SherifNagy
Copy link
Collaborator

@iokomin I might be confused, I am talking about kernel-uki-virt rpm from here https://yum.oracle.com/repo/OracleLinux/OL9/baseos/latest/x86_64/getPackage/kernel-uki-virt-5.14.0-284.30.1.el9_2.x86_64.rpm

objcopy --only-section .sbat -O binary lib/modules/5.14.0-284.30.1.el9_2.x86_64/vmlinuz-virt.efi /dev/stdout
sbat,1,SBAT Version,sbat,1,https://github.com/rhboot/shim/blob/main/SBAT.md
systemd,1,The systemd Developers,systemd,252,https://systemd.io/
systemd.ol,1,Oracle Linux Server,systemd,252-14.0.1.el9_2.3,https://github.com/oracle/oracle-linux
pesign -l -i lib/modules/5.14.0-284.30.1.el9_2.x86_64/vmlinuz-virt.efi
---------------------------------------------
certificate address is 0x7f61a3de27d8
Content was not encrypted.
Content is detached; signature cannot be verified.
The signer's common name is Oracle America, Inc.
No signer email address.
No signing time included.
There were certs or crls included.
---------------------------------------------

@iokomin
Copy link
Author

iokomin commented Mar 4, 2024

@SherifNagy, checked kernel-uki-virt topic with OL release management team and indeed we had few versions of this package signed and released before building of this sub-package was disabled that I was aware of. Corresponding SBAT entries added to the OL9 shim-15.8 review tag.
Thanks for your thorough review, let me know if any other questions are still not answered.

@THS-on
Copy link
Collaborator

THS-on commented Mar 5, 2024

Review of ol9-shim-x86_64-aarch64-20240212

Because the submission is very similar to #377, I'll mainly focus on the differences.

Shim

26ddbfb224c1a09368b4e2472de6707a26c3ce26aa00f783620e8fb0b98eec67  /usr/share/shim/15.8-1.0.3.el9/x64/shimx64.efi
26ddbfb224c1a09368b4e2472de6707a26c3ce26aa00f783620e8fb0b98eec67  /shimx64.efi

29aa4c77f237df563941029c2b49d9d75509bc4da3d516c0181b0f1f3400e789  /usr/share/shim/15.8-1.0.2.el9/aa64/shimaa64.efi
29aa4c77f237df563941029c2b49d9d75509bc4da3d516c0181b0f1f3400e789  /shimaa64.efi

GRUB

Kernel

  • Has the required patches
  • ephemeral key singing is used
  • UKI SBAT entry looks different to the RHEL or new documentation (see f84b68c)

Notes

Besides the GRUB SBAT entries, LGTM. With the UKI thing we need to converge to a unified set of entries at some point and revoke the old ones, but this is probably out of scope for this submission.

@THS-on THS-on added question Reviewer(s) waiting on response and removed extra review wanted labels Mar 5, 2024
@THS-on THS-on self-assigned this Mar 5, 2024
@iokomin
Copy link
Author

iokomin commented Mar 5, 2024

@THS-on thanks again for review!

On grub2 upstream entry, as we discussed in #377 (comment) for Oracle GRUB2 revocation convenience corresponding upstream grub.rh entry will be added in all OL releases with next grub2 errata. In case of OL9 it will match existing RHEL9 entry:

grub.rh,2,Red Hat,grub2,2.06-73.el9,mailto:[email protected]

Please let me know if it addresses your question.

@THS-on
Copy link
Collaborator

THS-on commented Mar 5, 2024

Thanks, yes that answers that 👍

I would like to get some comment on the UKI SBAT situation from other reviewers, if we are fine to move forward for now and standardize after the fact. Maybe @vathpela or @bluca can comment on that.

@iokomin
Copy link
Author

iokomin commented Mar 5, 2024

@THS-on, UKI SBAT standardization sounds reasonable to me as well.

Irrespective to UKI SBAT entry discussion, we do not plan to issue new UKI kernels and already have Oracle specific entries ( systemd.ol,1 ) in existing UKI images, that allow revocation in case of vulnerabilities discovered.
We can confirm that for any possible future UKI images we will follow up-to-date policies for those, and we just do not want to end up to be blocked with shim review up until UKI policies are set in stone.

@jsetje
Copy link
Collaborator

jsetje commented Mar 11, 2024

@THS-on I agree that we need a more standard approach for UKIs, but that didn't block other reviews, and if anything this looks closer to what's being proposed here: https://github.com/rhboot/shim-review/pull/398/files than the other reviews.

(I work for the same Org that's submitting this review, so please consider me a submitter and not a reviewer on the OL ones.)

@THS-on
Copy link
Collaborator

THS-on commented Mar 11, 2024

@jsetje because those signed out now in the wild anyway, there is not much we can change right now. I'm fine with accepting it, but would like to hear from at least one other person that actually ships UKIs or is more involved in the current discussion.

cc: @SherifNagy @bluca @kukrimate @vathpela

@SherifNagy
Copy link
Collaborator

@THS-on I think the plan now is to keep track of those submission and the SBAT records for UKIs and work on the standard and a mechanism to revoke those if needed, we had a chat about this in today's meeting and we think we can go ahead approving the submissions for now but keep tracking records "this is my personal suggestion but would like to hear other points of view as well", we also think that the PR for the examples still needs a bit of work to be done on it. I am also ccing @steve-mcintyre for more input

@jsetje
Copy link
Collaborator

jsetje commented Mar 11, 2024

FWIW, we have to deal with all of those UKIs no matter what since those are trusted by previous shims as well. We should probably track a list somewhere, possibly in another meta issue.

@jsetje
Copy link
Collaborator

jsetje commented Mar 13, 2024

Is there anything blocking this review from being approved at this point? I think the question has been resolved, right?

@SherifNagy
Copy link
Collaborator

I chatted with @THS-on yesterday and we agreed to mark it accepted based on the following:

Marking accepted

@SherifNagy SherifNagy added accepted Submission is ready for sysdev and removed question Reviewer(s) waiting on response labels Mar 19, 2024
@iokomin
Copy link
Author

iokomin commented Mar 22, 2024

submission ID: 13762470690072885
binaries signed by Microsoft, closing review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Submission is ready for sysdev
Projects
None yet
Development

No branches or pull requests

5 participants