search

alonbl / gnupg-pkcs11-scd

PKCS#11 GnuPG SCD
http://gnupg-pkcs11.sourceforge.net/
Other
69 stars 18 forks source link

Support EcDSA Keys with Named Curves #47

Open rkeene opened 2 years ago

rkeene commented 2 years ago

This changeset adds support for EcDSA Keys with Named Curves. Additionally, it may make it easier to add other kinds of keys in the future.

Not done as part of this change: Support GnuTLS

alonbl commented 2 years ago

Hi,

Great work!

First, please do not mark as fix #17 as I already explained to you that the problem there was the existence of EC certificate which caused the RSA certificates not to be accepted.

Please refactor/rename the entire keyutil module to keyinfo as it does nothing but that. As far as I understand the interface would be something like:

struct keyinfo_s;
typedef struct keyinfo_s *keyinfo;

struct key_data_list_s {
    struct key_data_list_s *next;
    char *type;
    char *value;
}

keyinfo keyinfo_new();
void keyinfo_free(keyinfo keyinfo);
int keyinfo_get_type(keyinfo keyinfo);
int keyinfo_from_der(keyinfo keyinfo, unsigned char *der, size_t len);
gcry_sexp_t *keyinfo_to_sexp(keyinfo keyinfo);
key_data_list *keyinfo_get_key_data(keyinfo keyinfo);
void key_data_free(key_data_list list);

What do you think? This way we focus in the key logic within the keyinfo module. Notice that the type and I believe the curve should be acquired from the certificate.

Thank you for your help! Alon

rkeene commented 2 years ago

I'm not sure I understand the key_data_list_s linked-list. A given der (certificate)'s resultant keyinfo would only ever hold 1 key, is the list used for some other purpose ? What would value and type be ?

alonbl commented 2 years ago

I'm not sure I understand the key_data_list_s linked-list. A given der (certificate)'s resultant keyinfo would only ever hold 1 key, is the list used for some other purpose ? What would value and type be ?

all that needed in order to provide the key to assuan socket without the code that within command.c knows the internals of the key.

For RSA (and current implementation) it will be:

- type: KEY-DATA
  value: hex(n)
- type: KEY-DATA
  value: hex(e)

Not sure how we transmit the key type, but what I would like to achieve is a command.c which gets this list, iterate it and send it over the assuan socket without actually knows what the key type is.

rkeene commented 2 years ago

I refactored the code.

rkeene commented 2 years ago

@alonbl I'm trying to add GENKEY support now. Do you know where it's documented for RSA and ECC ? e.g., sending KEY-DATA with n and e for RSA -- where is that specified ?

I'm guessing that something similar needs to be done for EcDSA for q and curve, but do we also need to specify that ECC is being used and not RSA ?

rkeene commented 2 years ago

Based on some postings I found online, it looks like the "KEY-DATA" for "q" (as hex-encoded integer) and "curve" as DER-encoded OID (minus tag) is what's needed.

rkeene commented 2 years ago

This functionality is almost entirely working:

gpg: key 10ECAD2B69B1E2E3 marked as ultimately trusted
gpg: directory '/home/rkeene/.gnupg/openpgp-revocs.d' created
gpg: error reading rest of packet: Invalid argument
gpg: can't encode a 512 bit MD into a 72 bits frame, algo=10
gpg: revocation certificate stored as '/home/rkeene/.gnupg/openpgp-revocs.d/3AAF050CCCE2EE667BDAB3D010ECAD2B69B1E2E3.rev'
public and secret key created and signed.

gpg: can't encode a 512 bit MD into a 72 bits frame, algo=10
pub   nistp256 2022-10-19 [INVALID_ALGO]
      3AAF050CCCE2EE667BDAB3D010ECAD2B69B1E2E3
uid                      xxxxx <x@x.com>
rkeene commented 2 years ago

I think the remaining issues are with cmd_getattr(), but it's not clear why some of the things being done are being done there, especially KEY-ATTR and EXTCAP which mention 2048 (which I assume is a reference to the RSA key size).

In KEY-ATTR there's a loop of size 3 but why isn't clear either.

rkeene commented 2 years ago

I'm back to this -- do you have have any ideas @alonbl ? Otherwise I'm going to have to dig through the GPG code.

alonbl commented 2 years ago

I'm back to this -- do you have have any ideas @alonbl ? Otherwise I'm going to have to dig through the GPG code.

You will need to reverse engineer gnupg own scd and see what you get for each case. As there is no formal spec for the IPC this is the only way to make it work.

rkeene commented 2 years ago

You will need to reverse engineer gnupg own scd and see what you get for each case. As there is no formal spec for the IPC this is the only way to make it work.

I'm almost done with this.

rkeene commented 2 years ago

I'm making progress here, creating subkeys is working but signing is not yet working.

$ gpg --list-secret-keys
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
/home/rkeene/.gnupg/pubring.kbx
-------------------------------
sec#  nistp256 2022-10-31 [SCA]
      F053661CDB471F87D9D7C31B158A385D4B4ECDA7
uid           [ultimate] xxxxx <x@x.com>

$ echo test | gpg --clear-sign F053661CDB471F87D9D7C31B158A385D4B4ECDA7
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
gpg: no default secret key: Unusable secret key
gpg: F053661CDB471F87D9D7C31B158A385D4B4ECDA7: clear-sign failed: Unusable secret key

There are bugs in GPG when dealing with EcDSA keys that reside on cards.

alonbl commented 2 years ago

There are bugs in GPG when dealing with EcDSA keys that reside on cards.

there usually are... very frustrating to chase non-standard non-cooperative undocumented unstable proprietary non-modular project to provide standard interface.

thank your help, in the meanwhile I think you have done many good cleanups, if we review them one by one we may reach to a point in which the EC part will be left more or less ready until gnupg support will be sorted out.

rkeene commented 2 years ago

This appears to be working now.

$ gpg --expert --full-generate-key
gpg (GnuPG) 2.3.2-beta105; Copyright (C) 2021 Free Software Foundation, Inc.
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.

gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
gpg: keybox '/home/rkeene/.gnupg/pubring.kbx' created
Please select what kind of key you want:
   (1) RSA and RSA
   (2) DSA and Elgamal
   (3) DSA (sign only)
   (4) RSA (sign only)
   (7) DSA (set your own capabilities)
   (8) RSA (set your own capabilities)
   (9) ECC (sign and encrypt) *default*
  (10) ECC (sign only)
  (11) ECC (set your own capabilities)
  (13) Existing key
  (14) Existing key from card
Your selection? 13
Enter the keygrip: F8BC7DE50508E0C578EE5F60021EB57B46D7F22B

Possible actions for this ECC key: Sign Certify Authenticate 
Current allowed actions: Sign Certify 

   (S) Toggle the sign capability
   (A) Toggle the authenticate capability
   (Q) Finished

> q
Please specify how long the key should be valid.
         0 = key does not expire
      <n>  = key expires in n days
      <n>w = key expires in n weeks
      <n>m = key expires in n months
      <n>y = key expires in n years

> 0
Key does not expire at all

GnuPG needs to construct a user ID to identify your key.

You selected this USER-ID:
    "xxxxxx <x@x.com>"

gpg: key FDE693371D5E0761 marked as ultimately trusted
gpg: revocation certificate stored as '/home/rkeene/.gnupg/openpgp-revocs.d/A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761.rev'
public and secret key created and signed.

pub   nistp256 2022-11-02 [SC]
      A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
uid                      xxxxxx <x@x.com>

$ gpg --check-signatures
/home/rkeene/.gnupg/pubring.kbx
-------------------------------
pub   nistp256 2022-11-02 [SC]
      A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
uid           [ultimate] xxxxxx <x@x.com>
sig!3        FDE693371D5E0761 2022-11-02  xxxxxx <x@x.com>

gpg: 1 good signature

$ echo 'test' | gpg --clear-sign
gpg: NOTE: THIS IS A DEVELOPMENT VERSION!
gpg: It is only intended for test purposes and should NOT be
gpg: used in a production environment or with production keys!
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

test
-----BEGIN PGP SIGNATURE-----

iHUEARMIAB0WIQSlo65T3p4Nac3JjGD95pM3HV4HYQUCY2HWxgAKCRD95pM3HV4H
YWvfAQDZ0ov5/NZxJf6eDrx76uDQ3qnXAKY6EBpbLq0k1+YKVAD/bV52WO8uFtSW
1oq+Fe4jQWCBNnrMmRFA+NRaMwRYs8Q=
=EidH
-----END PGP SIGNATURE-----

$ gpg --verify
...
gpg: Signature made Tue 01 Nov 2022 09:32:38 PM CDT
gpg:                using ECDSA key A5A3AE53DE9E0D69CDC98C60FDE693371D5E0761
gpg: Good signature from "xxxxxx <x@x.com>" [ultimate]
rkeene commented 2 years ago

GPG Patch added to this bug report: https://dev.gnupg.org/T5555

alonbl commented 2 years ago

Great. Please split review to multiple requests so we can review each, start with trivial renames, then refactor.

On Wed, 2 Nov 2022 at 22:41 Roy Keene @.***> wrote:

GPG Patch added to this bug report: https://dev.gnupg.org/T5555

— Reply to this email directly, view it on GitHub https://github.com/alonbl/gnupg-pkcs11-scd/pull/47#issuecomment-1301205953, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJURLPK4GGVV3GEUZZ7QM3WGLGXJANCNFSM6AAAAAARHMGCWQ . You are receiving this because you were mentioned.Message ID: @.***>

rkeene commented 2 years ago

@alonbl There aren't any trivial renames as part of this PR aside from the refactor of "keyutil" leading to keyutil_get_cert_hexgrip being renamed to keyinfo_get_hexgrip

rkeene commented 2 years ago

I've created PRs #48, #49, and #50 with bug fixes, #51 with a refactor that enables multiple kinds of keys, and #52 which adds support for signing of keys which do not use PKCS1 encoding.

mirko commented 1 week ago

I'm eagerly awaiting support of curves being supported. However last activity in either PR was in 2022 and I'm having the feeling things moved on - without this getting merged anytime soon.

Lot's of effort was put into adding this functionality, the pre-work was split up into its own atomic PRs, reviewed, approved, and closed - yet not merged (because those PRs were just intended for review?).

Is there anything one (me?) can do to push things further?

rkeene commented 1 week ago

If it makes any difference, I've been using the patched SCD for about 2 years without any problems

mirko commented 1 week ago

If it makes any difference, I've been using the patched SCD for about 2 years without any problems

It does to me!

Is this branch of your still the current state?

Also, is https://dev.gnupg.org/T5555 still necessary? I tried to comment on the ticket, but the GnuPG "community" is an absolute nightmare. Account creation is necessary, but disabled. One has to register to their mailing list and send credentials to all members to maybe eventually get an account in order to be able to comment on tickets. Anyway, needless to say it's not the only bug report with attached patch which is getting ignored for years.

Is there any chance to also support ed25519 curves - which the YubiKey's retired slots are now also capable of (I think since YubiKey firmware version 5.7.x or something)?

rkeene commented 1 week ago

The changes are still required to GnuPG as far as I know.

Ed25519 isn't significantly different from EcDSA so support for an SCD isn't hard (it's a bit easier, since no hashing has to be done)... however, I'm not sure about support from the GnuPG side through an SCD.

Oops, I accidentally closed it.