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

shared: generalize signature schemes #303

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

RaitoBezarius
Copy link
Member

Split off #278. Contains only signature generalization contents and misc refactoring to make things easier for the remote signing PR.

…t packages

Now, it is possible to build any package of the workspace in a fine grained fashion.
We want here to capture the required data to assemble a stub, here is a partial structure
modulo ESP generation paths.

Other pieces of code can consume this structure, validate it before passing it to the PE assembler
and the signer.

We convert everything into owned structures because we cannot really do
deserialization in any context with lifetimes going around, but,
allocations are generally very cheap in this context.
@RaitoBezarius RaitoBezarius force-pushed the refactor-signature branch 3 times, most recently from a8fabe0 to 0ca99b0 Compare February 11, 2024 19:45
In order to offer more flexible signature mechanisms in lanzaboote,
we need to take a step back and offer a general PE signature trait.

After this, we will be able to plug various different implementations.
Our lanzaboote integration tests are getting more and more sophisticated and ambitious.

Let's extract them into a "lanzalib", so they can be used with multiple backends.
We didn't test if there *was* a signature, idempotency of removal of signatures (i.e. removing an non-existent signature
is the identity operation) could fool us into believing we had a signed thing then not signed.
We fabricated a lot of initrds which were exactly the same as the one in
our store when we had no initrd secrets. This ends this practice.
@RaitoBezarius
Copy link
Member Author

@blitz I added more documentation (overview too), let me know if this is what you had in mind or where would you like me to expand.

@@ -0,0 +1,34 @@
# Signatures capabilities of Lanzaboote
Copy link
Member

Choose a reason for hiding this comment

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

This documentation is nice, but it looks like it should be the module documentation for the signature module and not a standalone README. Part of this can also go away, because it duplicates documentation of the LanzabooteSigner trait.


use crate::pe::StubParameters;

pub trait LanzabooteSigner {
Copy link
Member

Choose a reason for hiding this comment

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

nit: This could just be Signer. Otherwise we can start to prefix everything with the project name. :)

if std::io::stderr().write_all(&output.stderr).is_err() {
return Ok(false);
};
// XXX(Raito): do we want to bubble up this type of errors? :/
Copy link
Member

Choose a reason for hiding this comment

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

If !success means "either not signed or something else went wrong", we might not want to treat this as an error. But I also understand the other side of the argument.

Copy link
Member

@blitz blitz left a comment

Choose a reason for hiding this comment

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

Looks good! I liked the clear commit history, which made this easier to review.

Will approve when the merge conflicts are resolved. Feel free to address any of the other nitpicky comments as well or ignore them.

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

Successfully merging this pull request may close these issues.

None yet

2 participants