raito/lanzaboote
0
0
Fork 0
Code Issues 45 Pull requests 6 Projects Releases 6 Packages Wiki Activity Actions

stub: add fat variant #182

Merged
nikstur merged 1 commit from fat-uki into master 2023-06-01 20:43:54 +00:00
Conversation 20 Commits 1 Files changed 9 +488 -230
nikstur commented 2023-05-22 22:08:02 +00:00 (Migrated from github.com)
Copy link

A compile time feature is introduced that allows to build "fat" stubs that can be used to build "fat" UKIs. "fat" here means that the actual kernel and initrd are embedded in the PE binary, not only the file path and hash. This brings us one step closer to feature partiy with systemd-stub and thus one step closer to replacing it fully. Such a "fat" or "real" UKI is also interesting for image-based deployments of NixOS.

The new tests use runTest the future of testing in NixOS. See https://github.com/NixOS/nixpkgs/pull/230523

Thank you @RaitoBezarius and especially @blitz (and all the others who have contributed). The stub code is so well structured and well commented so that it was easy to extend even without any prior experience in UEFI or no-std environments.

A compile time feature is introduced that allows to build "fat" stubs that can be used to build "fat" UKIs. "fat" here means that the actual kernel and initrd are embedded in the PE binary, not only the file path and hash. This brings us one step closer to feature partiy with systemd-stub and thus one step closer to replacing it fully. Such a "fat" or "real" UKI is also interesting for image-based deployments of NixOS. The new tests use `runTest` the future of testing in NixOS. See https://github.com/NixOS/nixpkgs/pull/230523 Thank you @RaitoBezarius and especially @blitz (and all the others who have contributed). The stub code is so well structured and well commented so that it was easy to extend even without any prior experience in UEFI or no-std environments.
❤️ 2
blitz (Migrated from github.com) reviewed 2023-05-22 22:08:02 +00:00
RaitoBezarius (Migrated from github.com) reviewed 2023-05-22 22:20:36 +00:00
rust/stub/src/fat.rs
@ -0,0 +33,4 @@
impl EmbeddedConfiguration {
fn new(file_data: &[u8]) -> Result<Self> {
Ok(Self {
kernel: extract_bytes(file_data, ".linux")?,
RaitoBezarius (Migrated from github.com) commented 2023-05-22 22:20:36 +00:00
Copy link

You should actually probably relies on UnifiedSections here, it would make the code much nicer in line with systemd expectations?

You should actually probably relies on UnifiedSections here, it would make the code much nicer in line with systemd expectations?
nikstur (Migrated from github.com) reviewed 2023-05-22 22:30:33 +00:00
rust/stub/src/fat.rs
@ -0,0 +33,4 @@
impl EmbeddedConfiguration {
fn new(file_data: &[u8]) -> Result<Self> {
Ok(Self {
kernel: extract_bytes(file_data, ".linux")?,
nikstur (Migrated from github.com) commented 2023-05-22 22:30:33 +00:00
Copy link

Good idea. Let's do this in a separate refactoring.

Good idea. Let's do this in a separate refactoring.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:47:25 +00:00
flake.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:47:24 +00:00
Copy link

(I wish crane provided a proper ‘features array)

(I wish `crane` provided a proper `‘features` array)
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:48:09 +00:00
flake.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:48:09 +00:00
Copy link

Would be nice if upstream found a way to expose the binaries properly?

Would be nice if upstream found a way to expose the binaries properly?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:48:36 +00:00
nix/modules/uki.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:48:35 +00:00
Copy link

that's not prod ready :D

that's not prod ready :D
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:49:00 +00:00
nix/modules/uki.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:49:00 +00:00
Copy link

Did you apply shellcheck on the script?

Did you apply shellcheck on the script?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:49:57 +00:00
nix/modules/uki.nix
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:49:57 +00:00
Copy link

I really wish we use our tooling here someday and not objcopy, etc.

I really wish we use our tooling here someday and not objcopy, etc.
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:51:29 +00:00
rust/stub/src/fat.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:51:29 +00:00
Copy link

I'm not sure those log are useful, aren't they breaking the quiet thing too?

I'm not sure those log are useful, aren't they breaking the quiet thing too?
RaitoBezarius (Migrated from github.com) reviewed 2023-05-23 23:51:50 +00:00
rust/stub/src/fat.rs
RaitoBezarius (Migrated from github.com) commented 2023-05-23 23:51:50 +00:00
Copy link

(maybe keep them actually, because we have max level warn on prod anyway)

(maybe keep them actually, because we have max level warn on prod anyway)
RaitoBezarius (Migrated from github.com) approved these changes 2023-05-23 23:54:15 +00:00
RaitoBezarius (Migrated from github.com) left a comment
Copy link

It does introduce some code duplication in certain areas, but I feel like this is an interesting step. :)

It does introduce some code duplication in certain areas, but I feel like this is an interesting step. :)
nikstur (Migrated from github.com) reviewed 2023-05-24 19:27:35 +00:00
flake.nix
nikstur (Migrated from github.com) commented 2023-05-24 19:27:35 +00:00
Copy link

I don't know if it makes sense to have pkgs.systemdUkify. But I understand that they don't want Python in their normal systemd closure.

I don't know if it makes sense to have `pkgs.systemdUkify`. But I understand that they don't want Python in their normal systemd closure.
nikstur (Migrated from github.com) reviewed 2023-05-24 19:27:45 +00:00
flake.nix
nikstur (Migrated from github.com) commented 2023-05-24 19:27:44 +00:00
Copy link

That'd be nice

That'd be nice
nikstur (Migrated from github.com) reviewed 2023-05-24 19:28:29 +00:00
nix/modules/uki.nix
nikstur (Migrated from github.com) commented 2023-05-24 19:28:29 +00:00
Copy link

Well, since we can't really rebuild properly in a VM test anyway its as prod ready as we need for the tests :D

Well, since we can't really rebuild properly in a VM test anyway its as prod ready as we need for the tests :D
nikstur (Migrated from github.com) reviewed 2023-05-24 19:28:56 +00:00
nix/modules/uki.nix
nikstur (Migrated from github.com) commented 2023-05-24 19:28:56 +00:00
Copy link

True, but it's also nice that we can test the upstream systemd stuff.

True, but it's also nice that we can test the upstream systemd stuff.
nikstur (Migrated from github.com) reviewed 2023-05-24 19:30:26 +00:00
rust/stub/src/fat.rs
nikstur (Migrated from github.com) commented 2023-05-24 19:30:26 +00:00
Copy link

It propbably doesn't make too much sense to keep them. It just a left over when I wanted to make sure that I'm actually booting a fat UKI.

It propbably doesn't make too much sense to keep them. It just a left over when I wanted to make sure that I'm actually booting a fat UKI.
nikstur (Migrated from github.com) reviewed 2023-05-24 20:10:14 +00:00
nix/modules/uki.nix
nikstur (Migrated from github.com) commented 2023-05-24 20:10:14 +00:00
Copy link

I replace it with writeShellApplication which will always run shellcheck.

I replace it with `writeShellApplication` which will always run shellcheck.
nikstur (Migrated from github.com) reviewed 2023-05-24 20:10:35 +00:00
rust/stub/src/fat.rs
nikstur (Migrated from github.com) commented 2023-05-24 20:10:34 +00:00
Copy link

Removed them. They were also kind of in the wrong spot.

Removed them. They were also kind of in the wrong spot.
nikstur commented 2023-05-24 20:12:43 +00:00 (Migrated from github.com)
Copy link

Thanks for the review!

It does introduce some code duplication

I think most of the duplication is incidental duplication though. And I think it is better to have separate modules than a main littered with cfg() statements everywhere even if that increases duplication a little.

I'll give @blitz some time to also look over this PR. :)

Thanks for the review! > It does introduce some code duplication I think most of the duplication is [incidental duplication](https://anthonysciamanna.com/2018/07/28/the-dry-principle-and-incidental-duplication.html) though. And I think it is better to have separate modules than a main littered with `cfg()` statements everywhere even if that increases duplication a little. I'll give @blitz some time to also look over this PR. :)
blitz commented 2023-06-02 08:54:29 +00:00 (Migrated from github.com)
Copy link

I'll give @blitz some time to also look over this PR. :)

We discussed offline that I just look over this after it's merged. 👍

> I'll give @blitz some time to also look over this PR. :) We discussed offline that I just look over this after it's merged. :+1:
Sign in to join this conversation.
Reviewers
No reviewers
blitz
RaitoBezarius
Labels
Clear labels   
bug

Something isn't working

  
dependency

Issues related to our dependencies

  
documentation

Improvements or additions to documentation

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
review-next

   
security

   
stub

   
tool

   
wontfix

This will not be worked on

No labels
bug
dependency
documentation
duplicate
enhancement
good first issue
help wanted
invalid
question
review-next
security
stub
tool
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: raito/lanzaboote#182
No description provided.
Delete branch "fat-uki"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?