Skip to content

Conversation

maayank
Copy link

@maayank maayank commented Jul 31, 2024

We (Fireblocks) had several clients asking for ways to integrate the EIFs we supply with their own container/image scanning flows. We also have such use cases internally.

This PR includes a small refactoring of EifReader where the iteration over sections is done using a new Iterator that encapsulates the relevant parsing logic. This new iterator is then used to create a new eif_extract utility which receives an EIF and extracts the ramdisks. These can then be extracted using cpio and repacked using tar for maximum versatility.

In any case, I think it is a nice addition to the example utilities. I left the Cargo.toml in eif_extract as was on my PC, please modify accordingly/all feedback and changes are welcome.

Also added documentation.

Thanks,
Maayan

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Example usage:

~/r/eif_extractor2 (master)> ./target/release/eif_extract example.eif out ramdisk
Starting extraction process...
Reading section data...
Saved ramdisk to out/ramdisk0.dat
Saved ramdisk to out/ramdisk1.dat
Extraction completed successfully.
Successfully extracted ramdisks to 'out'
~/r/eif_extractor2 (master)> ls -alh out
total 450M
drwxrwxr-x 2 maayan maayan 4.0K Aug  1 00:35 .
drwxrwxr-x 7 maayan maayan 4.0K Aug  1 00:35 ..
-rw-rw-r-- 1 maayan maayan 746K Aug  1 00:35 ramdisk0.dat
-rw-rw-r-- 1 maayan maayan 449M Aug  1 00:35 ramdisk1.dat

@sabin-rapan
Copy link
Contributor

Hey @maayank , thank you for the PR. Would you mind adding your Signed-off-by line to each commit via git commit -s. Just so commits consistent with the rest in this repo. Also, commits 1 and 2 should be squashed together since they are part of the same logical change. The same goes for commits 3 and 4.

@maayank maayank force-pushed the main branch 3 times, most recently from efc88b0 to 558bc7f Compare August 2, 2024 12:32
@maayank
Copy link
Author

maayank commented Aug 2, 2024

Hey @sabin-rapan , thanks for the quick and prompt reply. Done! :)

Ok(())
}

fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something worth considering is to merge eif-extract and eif-build into a single applcation "eif" with build and extract subcommands. This way it would be easier for users to consume both, and to maintain as well.

Copy link
Author

@maayank maayank Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, will add this tomorrow and then squash.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see PR #31 - I prefer to merge the two utilities only once that PR is merged, since it makes significant changes to eif_builder. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me

maayank added 2 commits August 5, 2024 22:48
The iterator returns the section header, section header buffer,
and the section buffer for each sector.

Signed-off-by: Maayan Keshet <[email protected]>
This commit introduces the eif_extract utility, which is used to extract
the ramdisk sections from EIF files. Usage examples and a detailed
description have been added to README.

Signed-off-by: Maayan Keshet <[email protected]>
@sabin-rapan
Copy link
Contributor

sabin-rapan commented Aug 6, 2024

There's also #26 to be taken into consideration for this PR.

@maayank
Copy link
Author

maayank commented Aug 6, 2024

@sabin-rapan Re: #26 - started to implement something similar in my branch. CRCing the gaps during iteration is a bit more tricky if I separate the iteration logic out of the EifReader class. Since piece-wise CRCing all of the sectors and their gaps is equivalent to just CRCing everything after the header at once[1], I was thinking of just CRCing the whole eif together in another pass. WDYT? Much simpler code, but another pass of the EIF.

[1] i.e.

crc.update(A)
crc.update(B)

is the same as crc.update(A + B)

let mut ramdisk_count = 0;

println!("Reading section data...");
for section in sections.filter_map(|x| x.ok()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here filter_map(|x| x.ok()) will hide the sections that couldn't be read for whatever reason, which doesn't sound like the right thing to do for an eif extract command. Likely the user would like to know if an error happened and for the program to exit in this case. Otherwise it would be too easy to trick ourselves into thinking we extracted all the ramdisks.

I think you can do:

for section in sections {
   let section = section.map_err(|e| io::Error::new(io::ErrorKind::Other, format!("Unable to read section: {:?}", e)))?;
   // and then do the if == EifSectionType::EifSectionRamdisk
}

This is not compile-tested

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! :)

@sabin-rapan
Copy link
Contributor

@sabin-rapan Re: #26 - started to implement something similar in my branch. CRCing the gaps during iteration is a bit more tricky if I separate the iteration logic out of the EifReader class. Since piece-wise CRCing all of the sectors and their gaps is equivalent to just CRCing everything after the header at once[1], I was thinking of just CRCing the whole eif together in another pass. WDYT? Much simpler code, but another pass of the EIF.

[1] i.e.

crc.update(A)
crc.update(B)

is the same as crc.update(A + B)

Functionally wise sounds okay, but I haven't fully wrapped my head around the use-case of gaps in EIF files. Maybe piggyback data from one party (EIF producer) to another party (EIF user) so that the latter can validate something with the data in those gaps?

CC: @foersleo

- Use section fields in the EIF header for iteration
- Introduce EifSector to represent coupled header and buffer
- Fix missing CRC update by section data

Signed-off-by: Maayan Keshet <[email protected]>
@maayank
Copy link
Author

maayank commented Aug 26, 2024

Hey @sabin-rapan , @foersleo ,
I wanted to follow up on this PR to see if there's anything further needed from my end. The current PR doesn't change the current behavior on the main branch. If you'd like, maybe we can merge this and then I'll issue a separate PR for the CRC and eif utility unification? WDYT?

Comment on lines +55 to +60
// Extract the ramdisks
if let Err(e) = extract_ramdisks(eif_path, output_dir, prefix) {
eprintln!("Failed to extract ramdisks: {}", e);
} else {
println!("Successfully extracted ramdisks to '{}'", output_dir);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tool should be able to extract all bits of data from an EIF file. As a full counterpart to eif_build, it should allow you to take an EIF, extract everything and then allow you to call eif_build with the relevant inputs to reassemble a (functionally) identical EIF.

Comment on lines +51 to +53
let eif_path = &args[1];
let output_dir = &args[2];
let prefix = &args[3];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eif_build uses clap for command line parsing. Especially considering that we want to merge eif_build and eif_extract into a single eif tool with subarguments build and extract, we need to make sure we converge on the command line experience early on. So please use clap here as well :).

Comment on lines +101 to +103
### eif_extract

You can use the `eif_extract` tool to extract artifacts (e.g. for scanning) from an AWS Nitro Enclave image file:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've learned recently that cargo.io wants subprojects to have their own creates.io submission which then includes a separate README file in their own directory. See PR #35 for reference. Would you mind to follow the same pattern for eif_extract?

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

Successfully merging this pull request may close these issues.

3 participants