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

Add support for RSA3072 and RSA4096. #114

Merged
merged 3 commits into from
Mar 7, 2024
Merged

Conversation

vathpela
Copy link
Contributor

No description provided.

@Blarse
Copy link
Contributor

Blarse commented Feb 1, 2024

Hi,

I've noticed that your PR does not take into account the change of the ASN.1 type in bundle_signature(). The type offset in the DER encoded signature depends on the size of the signature.

I propose the following patch:

--- a/src/efikeygen.c
+++ b/src/efikeygen.c
@@ -141,7 +141,8 @@ bundle_signature(cms_context *cms, SECItem *sigder, SECItem *data,
                errx(1, "could not encode certificate: %s",
                        PORT_ErrorToString(PORT_GetError()));
 
-       sigder->data[sigder->len - 261] = DER_BIT_STRING;
+       //Note: offset is signature size + 5 bytes for DER encoding
+       sigder->data[sigder->len - (signature->len + 5)] = DER_BIT_STRING;
 
        return 0;
 }

Other than that this PR look good to me, is there any reason not to merge it yet?

@vathpela
Copy link
Contributor Author

vathpela commented Feb 6, 2024

Well, it wasn't actually working when I tried it, possibly due to this exact issue. Hopefully I'll get back to it in a week or two and figure out why.

vathpela and others added 3 commits March 7, 2024 13:05
We need to be able to generate keys other than just RSA2048 now.  As a
result, we need to have the key generation data determined outside of
generate_keys() itself.

This makes those parameters part of the generate_keys() call, and moves
the default values into efikeygen itself.

Signed-off-by: Peter Jones <[email protected]>
This adds a "--algorithm" flag to which you can pass rsa2048, rsa3072,
and rsa4096.

Signed-off-by: Peter Jones <[email protected]>
In ea7a2c4, when bundling the signature, the bitstring type field
is being set manually with a hacky offset.  That offset is only valid
with specific signature types, and so with any signature of a different
size, this is just corrupting data either in the signature or after it.

This change from Egor fixes the egregious hack to manually set the type
so that it computes the location based on the signature length, rather
than hard-coding a value.

Signed-off-by: Peter Jones <[email protected]>
@vathpela
Copy link
Contributor Author

vathpela commented Mar 7, 2024

Yep, that change gets me from:
efikeygen: efikeygen.c:main:1287: could not import signature: security library: invalid arguments.
to working key generation and signatures that validate. Thanks!

@vathpela vathpela marked this pull request as ready for review March 7, 2024 20:40
@vathpela vathpela merged commit d734b6a into rhboot:main Mar 7, 2024
2 checks passed
@vathpela vathpela deleted the rsa4096 branch March 14, 2024 18:46
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.

2 participants