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

Fix support for Ed448 private keys in PKCS#8 format #2003

Merged
merged 1 commit into from
May 25, 2024

Conversation

LeonMelis
Copy link
Contributor

This is the second attempt at a PR for Ed448 private key support in PKCS#8 format against 3.0 branch (original PR: 2000 )

  • Fixes parsing of Ed448 private keys in PKCS#8 format (PKCS8.loadEdDSA())
  • Fixes exporting of Ed448 private keys in PKCS#8 format (PKCS8.savePrivateKey())
  • Adds unittests testEd448PublicKey() and testEd448PrivateKey()

I tried to retain the original coding style, so the if/else structure will assume Ed448 if the curve is not Ed25519 (which mimics the ternary operator that was there before this patch), it would be cleaner to refactor this to a switch statement in the future.

Also, I'd recommend adding a proper wrapper for an ASN.1 OCTET STRING, instead relying on the hardcoded \x04\x20 and \x04\x39.

In case it wasn't yet clear: In the ASN.1 of an Ed448 private key the key data is a double wrapped octet string. So, the error text "The first two bytes of the private key field should be 0x0420" is incorrect, it's not part of the key, is an octet string header (0x04) and length field (0x20/0x39).

See for example:
https://lapo.it/asn1js/#MEcCAQAwBQYDK2VxBDsEOettXaJYob4hJNKJNOD-FfMvdesLKNp0KwochI6AKmAbtWhtkn99WOjd1PsGMh9zz2Vhdg3MwasOMQ

As you can see, lots of improvements to be done here, but I didn't want to increase the scope of this PR. The important thing is that Ed448 private keys in PKCS#8 are now at least supported, and tested.

@terrafrost terrafrost merged commit 541887c into phpseclib:3.0 May 25, 2024
36 of 37 checks passed
@terrafrost
Copy link
Member

I tried to retain the original coding style, so the if/else structure will assume Ed448 if the curve is not Ed25519 (which mimics the ternary operator that was there before this patch), it would be cleaner to refactor this to a switch statement in the future.

Currently the only EdDSA curves are Ed25519 / Ed448 so it's fine!

Also, I'd recommend adding a proper wrapper for an ASN.1 OCTET STRING, instead relying on the hardcoded \x04\x20 and \x04\x39.

I went ahead and implemented this suggestion:

b718a63

So, the error text "The first two bytes of the private key field should be 0x0420" is incorrect, it's not part of the key, is an octet string header (0x04) and length field (0x20/0x39).

I don't agree with that. The private key field is from the ASN.1 description in RFC5958:

     OneAsymmetricKey ::= SEQUENCE {
       version                   Version,
       privateKeyAlgorithm       PrivateKeyAlgorithmIdentifier,
       privateKey                PrivateKey,
       attributes            [0] Attributes OPTIONAL,
       ...,
       [[2: publicKey        [1] PublicKey OPTIONAL ]],
       ...
     }

     PrivateKey ::= OCTET STRING
                        -- Content varies based on type of key.  The
                        -- algorithm identifier dictates the format of
                        -- the key.

I mean, sure, the PrivateKey OCTET STRING can be further broken down, but I think the error text is plenty accurate as is.

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