Add PEM support - Githubissues

sjaeckel commented 2 years ago

Checklist

[x] documentation is added or updated
[x] tests are added or updated

Summary

This adds support to decode most variations of PEM files.

Changes to existing public APIs

The following public APIs have been changed. None of those APIs have been officially released.

ed25519_import_pkcs8()
ecc_import_pkcs8()
rsa_import_pkcs8()
x25519_import_pkcs8()

New public APIs and structs

structs

typedef struct password_ctx - a struct containing a call-back function that will be called once a password is required and the according opaque userdata pointer usually provided
typedef struct ltc_pka_key - a union containing all supported PK keys

PKCS#8 APIs

dh_import_pkcs8()
dsa_import_pkcs8()

PEM bytewise APIs

pem_decode()
pem_decode_pkcs()
pem_decode_openssh()

PEM `FILE`-based APIs

pem_decode_filehandle()
pem_decode_pkcs_filehandle()
pem_decode_openssh_filehandle()

New demos

openssh-privkey - not really a usable demo, more like a historical artifact of what this started from

Details

It brings support for:

OpenSSH style private key storage, both plain and encrypted, and public keys (in PEM format. authorized_keys format not supported yet)
PEM style private key storage, both plain and encrypted, and public keys
PKCS#8 style private key storage, both plain and encrypted

All supported PK crypto algorithms can be decoded:

Curve25519 (Ed25519 & X25519)
DH
DSA
ECC
RSA

sjaeckel commented 1 year ago

@karel-m do you maybe have time to review this? otherwise I'll merge it in the next days

karel-m commented 12 months ago

In my perl module test suite I have these (the password is: secret)

Do we want to support them?

karel-m commented 12 months ago

I see these warnings

$ make CFLAGS="-O2 -DUSE_LTM -DLTM_DESC -I../libtommath -Wall" -f makefile.unix
cc -Isrc/headers -Itests -DLTC_SOURCE -O2 -DUSE_LTM -DLTM_DESC -I../libtommath -Wall -c src/pk/ecc/ecc_import_openssl.c -o src/pk/ecc/ecc_import_openssl.o
src/pk/ecc/ecc_import_openssl.c: In function ‘s_ecc_import_private_with_oid’:
src/pk/ecc/ecc_import_openssl.c:30:10: warning: implicit declaration of function ‘ecc_import_with_oid’ [-Wimplicit-function-declaration]
   30 |    err = ecc_import_with_oid(bin_k, seq_priv[1].size, curveoid, custom[0].size, PK_PRIVATE, key);
      |          ^~~~~~~~~~~~~~~~~~~
src/pk/ecc/ecc_import_openssl.c:14:25: warning: unused variable ‘curve’ [-Wunused-variable]
   14 |    const ltc_ecc_curve *curve;
      |                         ^~~~~
src/pk/ecc/ecc_import_openssl.c:13:9: warning: unused variable ‘OID’ [-Wunused-variable]
   13 |    char OID[256];
      |         ^~~
src/pk/ecc/ecc_import_openssl.c:12:18: warning: unused variable ‘len’ [-Wunused-variable]
   12 |    unsigned long len, pkver = 0, curveoid[16];
      |                  ^~~
cc -Isrc/headers -Itests -DLTC_SOURCE -O2 -DUSE_LTM -DLTM_DESC -I../libtommath -Wall -c src/pk/ecc/ecc_import_x509.c -o src/pk/ecc/ecc_import_x509.o
src/pk/ecc/ecc_import_x509.c: In function ‘s_ecc_import_x509_with_oid’:
src/pk/ecc/ecc_import_x509.c:21:10: warning: implicit declaration of function ‘ecc_import_with_oid’; did you mean ‘s_ecc_import_x509_with_oid’? [-Wimplicit-function-declaration]
   21 |    err = ecc_import_with_oid(bin_xy, len_xy, curveoid, len_oid, PK_PUBLIC, key);
      |          ^~~~~~~~~~~~~~~~~~~
      |          s_ecc_import_x509_with_oid
src/pk/ecc/ecc_import_x509.c:13:25: warning: unused variable ‘curve’ [-Wunused-variable]
   13 |    const ltc_ecc_curve *curve;
      |                         ^~~~~
src/pk/ecc/ecc_import_x509.c:12:9: warning: unused variable ‘OID’ [-Wunused-variable]
   12 |    char OID[256];
      |         ^~~
src/pk/ecc/ecc_import_x509.c:11:35: warning: unused variable ‘len’ [-Wunused-variable]
   11 |    unsigned long len_xy, len_oid, len;
      |                                   ^~~
src/pk/ecc/ecc_import_x509.c: In function ‘ecc_import_subject_public_key_info’:
src/pk/ecc/ecc_import_x509.c:34:10: warning: implicit declaration of function ‘ecc_import_with_curve’ [-Wimplicit-function-declaration]
   34 |    err = ecc_import_with_curve(in, inlen, PK_PUBLIC, key);
      |          ^~~~~~~~~~~~~~~~~~~~~

sjaeckel commented 12 months ago

Yeah, something is lost somewhere ... currently looking into it.

karel-m commented 12 months ago

If we want to support even more ciphers here is how you can generate more test vectors via openssl

openssl genpkey -algorithm rsa -out rsa_priv.pem

openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-cbc       -out rsa_priv-aes-128-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-cfb       -out rsa_priv-aes-128-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-cfb1      -out rsa_priv-aes-128-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-cfb8      -out rsa_priv-aes-128-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-ctr       -out rsa_priv-aes-128-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-128-ofb       -out rsa_priv-aes-128-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-cbc       -out rsa_priv-aes-192-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-cfb       -out rsa_priv-aes-192-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-cfb1      -out rsa_priv-aes-192-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-cfb8      -out rsa_priv-aes-192-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-ctr       -out rsa_priv-aes-192-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-192-ofb       -out rsa_priv-aes-192-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-cbc       -out rsa_priv-aes-256-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-cfb       -out rsa_priv-aes-256-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-cfb1      -out rsa_priv-aes-256-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-cfb8      -out rsa_priv-aes-256-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-ctr       -out rsa_priv-aes-256-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aes-256-ofb       -out rsa_priv-aes-256-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-cbc      -out rsa_priv-aria-128-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-cfb      -out rsa_priv-aria-128-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-cfb1     -out rsa_priv-aria-128-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-cfb8     -out rsa_priv-aria-128-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-ctr      -out rsa_priv-aria-128-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-128-ofb      -out rsa_priv-aria-128-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-cbc      -out rsa_priv-aria-192-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-cfb      -out rsa_priv-aria-192-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-cfb1     -out rsa_priv-aria-192-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-cfb8     -out rsa_priv-aria-192-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-ctr      -out rsa_priv-aria-192-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-192-ofb      -out rsa_priv-aria-192-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-cbc      -out rsa_priv-aria-256-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-cfb      -out rsa_priv-aria-256-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-cfb1     -out rsa_priv-aria-256-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-cfb8     -out rsa_priv-aria-256-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-ctr      -out rsa_priv-aria-256-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -aria-256-ofb      -out rsa_priv-aria-256-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -bf-cbc            -out rsa_priv-bf-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -bf-cfb            -out rsa_priv-bf-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -bf-ofb            -out rsa_priv-bf-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-cbc  -out rsa_priv-camellia-128-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-cfb  -out rsa_priv-camellia-128-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-cfb1 -out rsa_priv-camellia-128-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-cfb8 -out rsa_priv-camellia-128-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-ctr  -out rsa_priv-camellia-128-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-128-ofb  -out rsa_priv-camellia-128-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-cbc  -out rsa_priv-camellia-192-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-cfb  -out rsa_priv-camellia-192-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-cfb1 -out rsa_priv-camellia-192-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-cfb8 -out rsa_priv-camellia-192-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-ctr  -out rsa_priv-camellia-192-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-192-ofb  -out rsa_priv-camellia-192-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-cbc  -out rsa_priv-camellia-256-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-cfb  -out rsa_priv-camellia-256-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-cfb1 -out rsa_priv-camellia-256-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-cfb8 -out rsa_priv-camellia-256-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-ctr  -out rsa_priv-camellia-256-ctr.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -camellia-256-ofb  -out rsa_priv-camellia-256-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -cast5-cbc         -out rsa_priv-cast5-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -cast5-cfb         -out rsa_priv-cast5-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -cast5-ofb         -out rsa_priv-cast5-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -chacha20          -out rsa_priv-chacha20.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-cbc           -out rsa_priv-des-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-cfb           -out rsa_priv-des-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-cfb1          -out rsa_priv-des-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-cfb8          -out rsa_priv-des-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede-cbc       -out rsa_priv-des-ede-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede-cfb       -out rsa_priv-des-ede-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede-ofb       -out rsa_priv-des-ede-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede3-cbc      -out rsa_priv-des-ede3-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede3-cfb      -out rsa_priv-des-ede3-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede3-cfb1     -out rsa_priv-des-ede3-cfb1.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede3-cfb8     -out rsa_priv-des-ede3-cfb8.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ede3-ofb      -out rsa_priv-des-ede3-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -des-ofb           -out rsa_priv-des-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -desx-cbc          -out rsa_priv-desx-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -idea-cbc          -out rsa_priv-idea-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -idea-cfb          -out rsa_priv-idea-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -idea-ofb          -out rsa_priv-idea-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc2-40-cbc        -out rsa_priv-rc2-40-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc2-64-cbc        -out rsa_priv-rc2-64-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc2-cbc           -out rsa_priv-rc2-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc2-cfb           -out rsa_priv-rc2-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc2-ofb           -out rsa_priv-rc2-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc5-cbc           -out rsa_priv-rc5-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc5-cfb           -out rsa_priv-rc5-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -rc5-ofb           -out rsa_priv-rc5-ofb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -seed-cbc          -out rsa_priv-seed-cbc.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -seed-cfb          -out rsa_priv-seed-cfb.pem
openssl pkey -in rsa_priv.pem -inform PEM -traditional -outform PEM -passout pass:secret -seed-ofb          -out rsa_priv-seed-ofb.pem

sjaeckel commented 12 months ago

In my perl module test suite I have these (the password is: secret)

https://github.com/DCIT/perl-CryptX/blob/master/t/data/dsa-seed.pem

https://github.com/DCIT/perl-CryptX/blob/master/t/data/ec-seed.pem

https://github.com/DCIT/perl-CryptX/blob/master/t/data/rsa-seed.pem

Do we want to support them?

I've also added support for seed. I had to create our own keys, since the ones you have don't match the private keys we already use.

FYI seed is now a legacy algorithm in OpenSSL

sjaeckel commented 12 months ago

If we want to support even more ciphers here is how you can generate more test vectors via openssl

hmm, I'll have a look

karel-m commented 12 months ago

Everything builds fine; however, adopting the new concept of password_ctx into my perl module will take some time before I can approve that it is working for me.

sjaeckel commented 12 months ago

I built openssl with RC5 enabled, but pkey doesn't seem to recognize the different rc5 options and I'm too tired to debug what went wrong ... Do you have a working openssl with RC5 support? If yes, could you please create the rc5 files from tests/pem/rsa.pem and upload them here?

karel-m commented 11 months ago

Do you have a working openssl with RC5 support? If yes, could you please create the rc5 files from tests/pem/rsa.pem and upload them here?

Done

karel-m commented 11 months ago

Are we able to load public keys in PEM format like this?

karel-m commented 11 months ago

Would be nice if we can load this:

sjaeckel commented 11 months ago

Would be nice if we can load this:

Where does this evil file originate? :D 77 chars line width ... that's very naughty and a lot more than the expected 72 chars ...

I'm gonna increase the max line length to 80.

karel-m commented 11 months ago

I am unable to load this X25519 public key

-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA6ngG9yGoVwUSyPbvtOjWIMSaUp5N9eqnfexkb7HofkE=
-----END PUBLIC KEY-----

sjaeckel commented 11 months ago

I am unable to load this X25519 public key

-----BEGIN PUBLIC KEY-----
MCowBQYDK2VuAyEA6ngG9yGoVwUSyPbvtOjWIMSaUp5N9eqnfexkb7HofkE=
-----END PUBLIC KEY-----

I indeed forgot x25519 ... added now!

sjaeckel commented 11 months ago

... and I've added a testvector for a DSA public key.

karel-m commented 11 months ago

Another crazy/evil format I have in my perl module test suite (no idea where it comes from, but providing the fact that it is tested in sshkey.t it might be generated by something like ssh-keygen)

karel-m commented 11 months ago

I am not sure whether this is not too much for pem_decode_pkcs but what about loading public keys from:

karel-m commented 11 months ago

I am trying to use pem_decode_openssh to load this key:

But it seems that both pub/priv buffers: loaded_key.u.ed25519.pub / loaded_key.u.ed25519.priv contain the same data

BD17B2215C443A7A1E9B286A4F0E76288130984CD942ACCCD4F1A064BB749FBE

which is the expected public key value, the expected private key value is:

D5BCFA901E39D1CE557F7B692AF91E5E818D76C42230F66248B955819B4E5904

I think there is a bug in ssh_decode_ed25519

-  if ((err = ed25519_import_raw(&privkey[32], 32, PK_PRIVATE, &key->u.ed25519)) != CRYPT_OK) {
+  if ((err = ed25519_import_raw(privkey, 32, PK_PRIVATE, &key->u.ed25519)) != CRYPT_OK) {

karel-m commented 11 months ago

And what about this format?

---- BEGIN SSH2 PUBLIC KEY ----
Comment: "1536-bit RSA, converted by miko@HIROKO from OpenSSH"
AAAAB3NzaC1yc2EAAAADAQABAAAAwQCkYHef/kFhdtEwG4YZfanYY92cmDX9gqw7OE2dxM
yQyj5Eyn/ztaIIERJGv2BG7TlEi8Gd83wFvSrqk4diHycQm2FbLtp258buy/PYCY3cvxYL
1imUxt6aPeLphzxQpt2dSwCom6M5Dq/sxB4YV+lvVpWZEuIFzsfGtqiLO7U4x80lXC1430
MtN5Z6nYTOVtktYkFJCmMSThYice6L4RHdTNejQZlmKGfLEKeR3wTwBIfLO9XFkSlHz2Uk
4q/Clr0=
---- END SSH2 PUBLIC KEY ----

It is basically the same (nearly) as:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAwQCkYHef/kFhdtEwG4YZfanYY92cmDX9gqw7OE2dxMyQyj5Eyn/ztaIIERJGv2BG7TlEi8Gd83wFvSrqk4diHycQm2FbLtp258buy/PYCY3cvxYL1imUxt6aPeLphzxQpt2dSwCom6M5Dq/sxB4YV+lvVpWZEuIFzsfGtqiLO7U4x80lXC1430MtN5Z6nYTOVtktYkFJCmMSThYice6L4RHdTNejQZlmKGfLEKeR3wTwBIfLO9XFkSlHz2Uk4q/Clr0= comment for rsa/1536 key

karel-m commented 11 months ago

I am trying to use pem_decode_openssh to load:

And pem_decode_pkcs to load the same key:

pem_decode_openssh gives me RSA key with these internals:

{
  'N' => 'B8E7C9348A016072F92B0E350AA3480880F5B4FBB4043DC93BFC35C8A460241F31C34D02EEEFF233C410C22FB890845D9E430691FECF525E3B360D5FCE4F749733742324EE0F4B79C79337CFED98EA9BD2C64A2701811F31E4B3E6C68355037A8E22730FE7186181B29862EB2E04C025A4482AFD3F6E0AF2C8BAF4671A10530F',
  'd' => '0F952104B778A43B2C3A6FA912AB6DFFA1769378FED3B8AD43CBDE707941CCE9801518615DE784BECE10277D440D91CA1DF342137DA8D52531D23D504C9FAF90858771098B47CF5C2D13A50787326FE7B8922E3CDD13ABEA78833619E229E69E5BD4063041C801B7FC51B82E036BD4E29FBDC1C7A636215425CC2243BE34CCD1',
  'dP' => '6AF292E939F528716A83C2F2A35B809FDD7AD0E9D36FBDC9C9F2C86E8F8086EEF4E24461F533C4B2964C04B53E650DA90C01371A3B2B60A9883BCF5B27893829',
  'dQ' => '2F010FA8E263C85825D3449D76D82DD4E27393750535501FCDC0718E248440D40A7670A1FD920B78D951BC7EE8D3DF70FD0658FB91F5291DD7CCA6E59BD0BECF',
  'e' => '010001',
  'p' => 'C056C228341621B96B42809E14753BE5F0126C25F0106B4C20EFFE193F617295000798908F039E00C4046ECEDA7B34AE7C66C8C9450BE802411A844645570469',
  'q' => 'F61B2C906F31146E28B01A319B1F5DF5695BCED85B58A5859C9108607EB05DC242C40940D12F8820A86A4E8239C5E161576B6502FC1BCBC920048DFC01854CB7',
  'qP' => '0C5F7EB4D275C6D5B5CFFC12FEED4819390C2D8029CAA50C7F67B07F9CAF0FCCB11F83C4798102F7AFA49FA6DD104AE4EACB0E3F63B59FD8F022547266070A3A',
}

pem_decode_pkcs gives:

{
  'N' => 'B8E7C9348A016072F92B0E350AA3480880F5B4FBB4043DC93BFC35C8A460241F31C34D02EEEFF233C410C22FB890845D9E430691FECF525E3B360D5FCE4F749733742324EE0F4B79C79337CFED98EA9BD2C64A2701811F31E4B3E6C68355037A8E22730FE7186181B29862EB2E04C025A4482AFD3F6E0AF2C8BAF4671A10530F',
  'd' => '0F952104B778A43B2C3A6FA912AB6DFFA1769378FED3B8AD43CBDE707941CCE9801518615DE784BECE10277D440D91CA1DF342137DA8D52531D23D504C9FAF90858771098B47CF5C2D13A50787326FE7B8922E3CDD13ABEA78833619E229E69E5BD4063041C801B7FC51B82E036BD4E29FBDC1C7A636215425CC2243BE34CCD1',
  'dP' => '2F010FA8E263C85825D3449D76D82DD4E27393750535501FCDC0718E248440D40A7670A1FD920B78D951BC7EE8D3DF70FD0658FB91F5291DD7CCA6E59BD0BECF',
  'dQ' => '6AF292E939F528716A83C2F2A35B809FDD7AD0E9D36FBDC9C9F2C86E8F8086EEF4E24461F533C4B2964C04B53E650DA90C01371A3B2B60A9883BCF5B27893829',
  'e' => '010001',
  'p' => 'F61B2C906F31146E28B01A319B1F5DF5695BCED85B58A5859C9108607EB05DC242C40940D12F8820A86A4E8239C5E161576B6502FC1BCBC920048DFC01854CB7',
  'q' => 'C056C228341621B96B42809E14753BE5F0126C25F0106B4C20EFFE193F617295000798908F039E00C4046ECEDA7B34AE7C66C8C9450BE802411A844645570469',
  'qP' => '0C5F7EB4D275C6D5B5CFFC12FEED4819390C2D8029CAA50C7F67B07F9CAF0FCCB11F83C4798102F7AFA49FA6DD104AE4EACB0E3F63B59FD8F022547266070A3A',
}

Please note the swap of p/q + dP/dQ.

It is so strange that there still might be "something" on my side.

karel-m commented 11 months ago

Isn't ssh_decode_rsa missing:

key->u.rsa.type = PK_PRIVATE;

?

karel-m commented 11 months ago

Do we want to support loading DSA keys in openssh format?

sjaeckel commented 11 months ago

I am trying to use pem_decode_openssh to load:

And pem_decode_pkcs to load the same key:

pem_decode_openssh gives me RSA key with these internals:

{
  'N' => 'B8E7C9348A016072F92B0E350AA3480880F5B4FBB4043DC93BFC35C8A460241F31C34D02EEEFF233C410C22FB890845D9E430691FECF525E3B360D5FCE4F749733742324EE0F4B79C79337CFED98EA9BD2C64A2701811F31E4B3E6C68355037A8E22730FE7186181B29862EB2E04C025A4482AFD3F6E0AF2C8BAF4671A10530F',
  'd' => '0F952104B778A43B2C3A6FA912AB6DFFA1769378FED3B8AD43CBDE707941CCE9801518615DE784BECE10277D440D91CA1DF342137DA8D52531D23D504C9FAF90858771098B47CF5C2D13A50787326FE7B8922E3CDD13ABEA78833619E229E69E5BD4063041C801B7FC51B82E036BD4E29FBDC1C7A636215425CC2243BE34CCD1',
  'dP' => '6AF292E939F528716A83C2F2A35B809FDD7AD0E9D36FBDC9C9F2C86E8F8086EEF4E24461F533C4B2964C04B53E650DA90C01371A3B2B60A9883BCF5B27893829',
  'dQ' => '2F010FA8E263C85825D3449D76D82DD4E27393750535501FCDC0718E248440D40A7670A1FD920B78D951BC7EE8D3DF70FD0658FB91F5291DD7CCA6E59BD0BECF',
  'e' => '010001',
  'p' => 'C056C228341621B96B42809E14753BE5F0126C25F0106B4C20EFFE193F617295000798908F039E00C4046ECEDA7B34AE7C66C8C9450BE802411A844645570469',
  'q' => 'F61B2C906F31146E28B01A319B1F5DF5695BCED85B58A5859C9108607EB05DC242C40940D12F8820A86A4E8239C5E161576B6502FC1BCBC920048DFC01854CB7',
  'qP' => '0C5F7EB4D275C6D5B5CFFC12FEED4819390C2D8029CAA50C7F67B07F9CAF0FCCB11F83C4798102F7AFA49FA6DD104AE4EACB0E3F63B59FD8F022547266070A3A',
}

pem_decode_pkcs gives:

{
  'N' => 'B8E7C9348A016072F92B0E350AA3480880F5B4FBB4043DC93BFC35C8A460241F31C34D02EEEFF233C410C22FB890845D9E430691FECF525E3B360D5FCE4F749733742324EE0F4B79C79337CFED98EA9BD2C64A2701811F31E4B3E6C68355037A8E22730FE7186181B29862EB2E04C025A4482AFD3F6E0AF2C8BAF4671A10530F',
  'd' => '0F952104B778A43B2C3A6FA912AB6DFFA1769378FED3B8AD43CBDE707941CCE9801518615DE784BECE10277D440D91CA1DF342137DA8D52531D23D504C9FAF90858771098B47CF5C2D13A50787326FE7B8922E3CDD13ABEA78833619E229E69E5BD4063041C801B7FC51B82E036BD4E29FBDC1C7A636215425CC2243BE34CCD1',
  'dP' => '2F010FA8E263C85825D3449D76D82DD4E27393750535501FCDC0718E248440D40A7670A1FD920B78D951BC7EE8D3DF70FD0658FB91F5291DD7CCA6E59BD0BECF',
  'dQ' => '6AF292E939F528716A83C2F2A35B809FDD7AD0E9D36FBDC9C9F2C86E8F8086EEF4E24461F533C4B2964C04B53E650DA90C01371A3B2B60A9883BCF5B27893829',
  'e' => '010001',
  'p' => 'F61B2C906F31146E28B01A319B1F5DF5695BCED85B58A5859C9108607EB05DC242C40940D12F8820A86A4E8239C5E161576B6502FC1BCBC920048DFC01854CB7',
  'q' => 'C056C228341621B96B42809E14753BE5F0126C25F0106B4C20EFFE193F617295000798908F039E00C4046ECEDA7B34AE7C66C8C9450BE802411A844645570469',
  'qP' => '0C5F7EB4D275C6D5B5CFFC12FEED4819390C2D8029CAA50C7F67B07F9CAF0FCCB11F83C4798102F7AFA49FA6DD104AE4EACB0E3F63B59FD8F022547266070A3A',
}

Please note the swap of p/q + dP/dQ.

It is so strange that there still might be "something" on my side.

Very possible that I got something wrong on the import side there! I'll have a look tomorrow, please keep posting your findings.

karel-m commented 11 months ago

my suggestion (UPDATED - now loads openssh-dsa keys)

diff --git a/src/ltc/misc/pem/pem.c b/src/ltc/misc/pem/pem.c
index fb1f412a..0b64a604 100644
--- a/src/ltc/misc/pem/pem.c
+++ b/src/ltc/misc/pem/pem.c
@@ -31,6 +31,12 @@ const struct pem_header_id pem_std_headers[] = {
      .has_more_headers = no,
      .flags = pf_public,
    },
+   {
+     SET_CSTR(.start, "-----BEGIN RSA PUBLIC KEY-----"),
+     SET_CSTR(.end, "-----END RSA PUBLIC KEY-----"),
+     .has_more_headers = maybe,
+     .pka = LTC_PKA_RSA,
+   },
    /* Regular plain or encrypted private keys */
    {
      SET_CSTR(.start, "-----BEGIN RSA PRIVATE KEY-----"),
diff --git a/src/ltc/misc/pem/pem_ssh.c b/src/ltc/misc/pem/pem_ssh.c
index 9347cf21..9c5d6deb 100644
--- a/src/ltc/misc/pem/pem_ssh.c
+++ b/src/ltc/misc/pem/pem_ssh.c
@@ -108,7 +108,7 @@ int ssh_decode_ed25519(const unsigned char *in, unsigned long *inlen, ltc_pka_ke
       goto cleanup;
    }

-   if ((err = ed25519_import_raw(&privkey[32], 32, PK_PRIVATE, &key->u.ed25519)) != CRYPT_OK) {
+   if ((err = ed25519_import_raw(privkey, 32, PK_PRIVATE, &key->u.ed25519)) != CRYPT_OK) {
       goto cleanup;
    }
    if (pubkeylen == sizeof(key->u.ed25519.pub)) {
@@ -131,7 +131,7 @@ int ssh_decode_rsa(const unsigned char *in, unsigned long *inlen, ltc_pka_key *k
    int err;
    void *tmp1, *tmp2;
    if ((err = mp_init_multi(&tmp1, &tmp2, NULL)) != CRYPT_OK) {
-      goto cleanup;
+      return err;
    }
    if ((err = rsa_init(&key->u.rsa)) != CRYPT_OK) {
       goto cleanup;
@@ -142,9 +142,10 @@ int ssh_decode_rsa(const unsigned char *in, unsigned long *inlen, ltc_pka_key *k
                                         LTC_SSHDATA_MPINT, key->u.rsa.e,
                                         LTC_SSHDATA_MPINT, key->u.rsa.d,
                                         LTC_SSHDATA_MPINT, key->u.rsa.qP,
-                                        LTC_SSHDATA_MPINT, key->u.rsa.q,
                                         LTC_SSHDATA_MPINT, key->u.rsa.p,
+                                        LTC_SSHDATA_MPINT, key->u.rsa.q,
                                         LTC_SSHDATA_EOL,    NULL)) != CRYPT_OK) {
+      rsa_free(&key->u.rsa);
       goto cleanup;
    }

@@ -154,6 +155,7 @@ int ssh_decode_rsa(const unsigned char *in, unsigned long *inlen, ltc_pka_key *k
    if ((err = mp_mod( key->u.rsa.d,  tmp2,  key->u.rsa.dQ)) != CRYPT_OK)         { goto cleanup; } /* dQ = d mod q-1 */

    key->id = LTC_PKA_RSA;
+   key->u.rsa.type = PK_PRIVATE;

 cleanup:
    mp_clear_multi(tmp2, tmp1, NULL);
@@ -162,6 +164,32 @@ cleanup:
 }
 #endif

+#ifdef LTC_MDSA
+int ssh_decode_dsa(const unsigned char *in, unsigned long *inlen, ltc_pka_key *key)
+{
+   int err;
+
+   if ((err = dsa_int_init(&key->u.dsa)) != CRYPT_OK) {
+      return err;
+   }
+   if ((err = ssh_decode_sequence_multi(in, inlen,
+                                        LTC_SSHDATA_MPINT, key->u.dsa.p,
+                                        LTC_SSHDATA_MPINT, key->u.dsa.q,
+                                        LTC_SSHDATA_MPINT, key->u.dsa.g,
+                                        LTC_SSHDATA_MPINT, key->u.dsa.y,
+                                        LTC_SSHDATA_MPINT, key->u.dsa.x,
+                                        LTC_SSHDATA_EOL,    NULL)) != CRYPT_OK) {
+      dsa_free(&key->u.dsa);
+      return err;
+   }
+
+   key->id = LTC_PKA_DSA;
+   key->u.dsa.qord = mp_unsigned_bin_size(key->u.dsa.q);
+   key->u.dsa.type = PK_PRIVATE;
+   return CRYPT_OK;
+}
+#endif
+
 struct ssh_pka {
    const char *name;
    int (*init)(const char*, ltc_pka_key*);
@@ -175,6 +203,9 @@ struct ssh_pka ssh_pkas[] = {
 #ifdef LTC_MRSA
                              { "ssh-rsa",     NULL,              ssh_decode_rsa },
 #endif
+#ifdef LTC_MDSA
+                             { "ssh-dss",     NULL,              ssh_decode_dsa },
+#endif
 #ifdef LTC_MECC
                              { NULL,          ssh_find_init_ecc, ssh_decode_ecdsa },
 #endif

karel-m commented 11 months ago

The full set of various keys created/converted by ssh-keygen are here https://github.com/DCIT/perl-CryptX/tree/master/t/data/ssh

Even with the patch from my previous post we are not able to load RFC4716 format:

---- BEGIN SSH2 PUBLIC KEY ----
Comment: "1024-bit RSA, converted by miko@YUE from OpenSSH"
AAAAB3NzaC1yc2EAAAADAQABAAAAgQC458k0igFgcvkrDjUKo0gIgPW0+7QEPck7/DXIpG
AkHzHDTQLu7/IzxBDCL7iQhF2eQwaR/s9SXjs2DV/OT3SXM3QjJO4PS3nHkzfP7Zjqm9LG
SicBgR8x5LPmxoNVA3qOInMP5xhhgbKYYusuBMAlpEgq/T9uCvLIuvRnGhBTDw==
---- END SSH2 PUBLIC KEY ----

and traditional public key:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQC458k0igFgcvkrDjUKo0gIgPW0+7QEPck7/DXIpGAkHzHDTQLu7/IzxBDCL7iQhF2eQwaR/s9SXjs2DV/OT3SXM3QjJO4PS3nHkzfP7Zjqm9LGSicBgR8x5LPmxoNVA3qOInMP5xhhgbKYYusuBMAlpEgq/T9uCvLIuvRnGhBTDw== This is a test key!

On top of that it might be a good idea to be able to load public keys from PEM certificates like:

All from me for today.

karel-m commented 11 months ago

A build with -Wall -Werror -Wextra fails like this https://github.com/DCIT/perl-CryptX/actions/runs/6525537050/job/17718404851

sjaeckel commented 11 months ago

A build with -Wall -Werror -Wextra fails like this https://github.com/DCIT/perl-CryptX/actions/runs/6525537050/job/17718404851

Not for me with any of those two versions of GCC:

$ cc --version
cc (Ubuntu 11.4.0-1ubuntu1~22.04) [...]

$ cc --version  
cc (GCC) 13.2.1 [...]

... also CI is running per default with those options enabled.

The full set of various keys created/converted by ssh-keygen are here https://github.com/DCIT/perl-CryptX/tree/master/t/data/ssh

Yeah, after having another look I guess it'd make sense to have:

int pem_decode_filehandle(FILE *f, ltc_pka_key *k, const password_ctx *pw_ctx);
int pem_decode(const void *buf, unsigned long len, ltc_pka_key *k, const password_ctx *pw_ctx);

Even with the patch from my previous post we are not able to load RFC4716 format: [...] On top of that it might be a good idea to be able to load public keys from PEM certificates like: [...] Do we want to support loading DSA keys in openssh format? [...]

Those should all be supported now.

and traditional public key:

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAAAgQC458k0igFgcvkrDjUKo0gIgPW0+7QEPck7/DXIpGAkHzHDTQLu7/IzxBDCL7iQhF2eQwaR/s9SXjs2DV/OT3SXM3QjJO4PS3nHkzfP7Zjqm9LGSicBgR8x5LPmxoNVA3qOInMP5xhhgbKYYusuBMAlpEgq/T9uCvLIuvRnGhBTDw== This is a test key!

This is still not supported and I didn't think it through yet ... This format is AFAIK usually only used in authorized_keys files and then it's most of the time multiple lines with multiple keys.

Maybe it'd make sense to provide a separate API int ssh_decode_public_key(const char *in, unsigned long inlen, ltc_pka_key *k);? ... and leave the reading of each line up to the user?

@mkj maybe you also have an opinion on that? :)

sjaeckel commented 11 months ago

int pem_decode_filehandle(FILE *f, ltc_pka_key *k, const password_ctx *pw_ctx);
int pem_decode(const void *buf, unsigned long len, ltc_pka_key *k, const password_ctx *pw_ctx);

I've added those, and the testfiles from https://github.com/DCIT/perl-CryptX/tree/master/t/data/ssh :)

... not sure whether CRYPT_UNKNOWN_PEM is a good error code ... proposals will be accepted!

karel-m commented 11 months ago

Would you mind adding also this:

diff --git a/src/ltc/misc/pem/pem.c b/src/ltc/misc/pem/pem.c
index e259a704..af5a34cc 100644
--- a/src/ltc/misc/pem/pem.c
+++ b/src/ltc/misc/pem/pem.c
@@ -38,6 +38,12 @@ const struct pem_header_id pem_std_headers[] = {
      .has_more_headers = no,
      .flags = pf_public,
    },
+   {
+     SET_CSTR(.start, "-----BEGIN RSA PUBLIC KEY-----"),
+     SET_CSTR(.end, "-----END RSA PUBLIC KEY-----"),
+     .has_more_headers = maybe,
+     .pka = LTC_PKA_RSA,
+   },
    /* Regular plain or encrypted private keys */
    {
      SET_CSTR(.start, "-----BEGIN RSA PRIVATE KEY-----"),

karel-m commented 11 months ago

An attempt to load:

---- BEGIN SSH2 PUBLIC KEY ----
Comment: "256-bit ECDSA, converted by miko@YUE from OpenSSH"
AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBI4QIXX6Mg0NWD6OvW
qb/L88sTs56hqs5sXkAh6vr9NleFczaijCUyn/9qLz/CfWg1pqj3KgOiFZ4ByT3xYfJIs=
---- END SSH2 PUBLIC KEY ----

ends with "Buffer overflow" failure (same for ecdsa_384 / ecdsa_521)

karel-m commented 11 months ago

And this key is loaded in a wrong way

---- BEGIN SSH2 PUBLIC KEY ----
Comment: "256-bit ED25519, converted by miko@YUE from OpenSSH"
AAAAC3NzaC1lZDI1NTE5AAAAIL0XsiFcRDp6Hpsoak8OdiiBMJhM2UKszNTxoGS7dJ++
---- END SSH2 PUBLIC KEY ----

expected pubkey is bd17b2215c443a7a1e9b286a4f0e76288130984cd942acccd4f1a064bb749fbe
however loaded value is 644a2b2b00000000313babf3fc7f0000436f6d6d656e743a20223235362d6269

sjaeckel commented 11 months ago

An attempt to load:

---- BEGIN SSH2 PUBLIC KEY ----
Comment: "256-bit ECDSA, converted by miko@YUE from OpenSSH"
AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBI4QIXX6Mg0NWD6OvW
qb/L88sTs56hqs5sXkAh6vr9NleFczaijCUyn/9qLz/CfWg1pqj3KgOiFZ4ByT3xYfJIs=
---- END SSH2 PUBLIC KEY ----

ends with "Buffer overflow" failure (same for ecdsa_384 / ecdsa_521)

I wonder where those formats are documented ... currently I'm reading openssh code and the binary representation of those keys to reverse how it could/should be ...

And this key is loaded in a wrong way

Yeah, I broke the ed25519 import before :(

Both should be fixed now.

karel-m commented 11 months ago

Great, now I am able to load pretty much every PEM/SSH key format (except "authorized_keys line" ssh pubkey format).

However, testing of my perl bindings revealed bigger issue caused by the fact, that with the new password_ctx concept the password buffer is allocated by the caller but freed by libtomcrypt library (which is hard to achieve particularly on perl @ MS Windows).

Before I start to write a longer text with reasoning a short question: are you open to changing password_ctx concept so that the password buffer should be allocated as well as freed by the caller? (or allocated+freed by the library)

karel-m commented 11 months ago

And when speaking about perl @ Windows this is also an issue :)

/* MSVC can't build PEM */
#if defined(LTC_PEM) && defined(_MSC_VER)
#undef LTC_PEM
#undef LTC_PEM_DECODE_BUFSZ
#undef LTC_PEM_READ_BUFSIZE
#endif

karel-m commented 11 months ago

Just a cosmetics needed to satisfy old MSVC compiler

--- a/src/ltc/prngs/rng_get_bytes.c
+++ b/src/ltc/prngs/rng_get_bytes.c
@@ -112,9 +112,9 @@ static unsigned long s_rng_win32(unsigned char *buf, unsigned long len,
 static unsigned long s_rng_win32(unsigned char *buf, unsigned long len,
                                void (*callback)(void))
 {
+   static HCRYPTPROV hProv = 0;
    LTC_UNUSED_PARAM(callback);

-   static HCRYPTPROV hProv = 0;
    if (hProv == 0) {
       HCRYPTPROV h = 0;
       if (!CryptAcquireContextW(&h, NULL, MS_DEF_PROV_W, PROV_RSA_FULL,

sjaeckel commented 11 months ago

The PEM tests on MSVC still NOP because there's no dirent.h on MSVC and therefore test_process_dir() can't be compiled ... should we keep it like that? or do you want to fix that?

sjaeckel commented 11 months ago

I'm not sure whether I like the approach of the password buffer inside the library ... I don't like to expose the internals.

I guess I'll change that back to the previous approach and add a void (*free)(void *); function pointer to password_ctx. The library would then either call that free if it is non-NULL or else XFREE()...

Would that also solve your problem?

Which approach would you prefer?

CC @mkj

karel-m commented 11 months ago

As for the password callback.

I am fine with the current situation (buffer allocated+freed by the library):

int (*callback)(void *str, unsigned long *len, void *userdata);

BTW it is similat to openssl's https://www.openssl.org/docs/man3.0/man3/PEM_read_PrivateKey.html

I can also live with buffer allocated+freed by the caller:

int (*callback)(void **str, unsigned long *len, void *userdata);

basically the original version without calling free() inside the library.

Passing a free() function pointer might get a bit complicated - for example in perl bindings there is a magic system of macros used for memory allocation/deallocation and I am not sure how well it will play together with this idea.

There is one more point against alloc by caller + free by library - in you original version the place where you called XFREE was quite far/deep from the functions I am calling (pem_decode_pkcs, ecc_import_pkcs8, pem_decode_openssh) and on the way through the libtomcrypt there was many places where something may fail and in most of these cases the library just returned non-CRYPT_OK retrun value but did not call XFREE. In other words potential memory leak.

karel-m commented 11 months ago

BTW I am still dealing with SET_CSTR related troubles like here https://github.com/DCIT/perl-CryptX/actions/runs/6552365284/job/17795515317 + my older than expected MSVC compiler still does not build misc\pem\pem.c (again SET_CSTR related)

sjaeckel commented 11 months ago

BTW I am still dealing with SET_CSTR related troubles like here https://github.com/DCIT/perl-CryptX/actions/runs/6552365284/job/17795515317 + my older than expected MSVC compiler still does not build misc\pem\pem.c (again SET_CSTR related)

Then please add -Wno-missing-field-initializers to the CFLAGS in those builds.

[...] alloc by caller + free by library [...]

Yeah, that was a bad idea and won't happen again.

[...] in you original version the place where you called XFREE was quite far/deep from the functions I am calling (pem_decode_pkcs, ecc_import_pkcs8, pem_decode_openssh) and on the way through the libtomcrypt there was many places where something may fail and in most of these cases the library just returned non-CRYPT_OK retrun value but did not call XFREE. In other words potential memory leak.

I just looked through the code again and I'm not sure what you're referring to. There are three occasions where the callback may happen and all of them free the allocated memory properly.

Passing a free() function pointer might get a bit complicated - for example in perl bindings there is a magic system of macros used for memory allocation/deallocation and I am not sure how well it will play together with this idea.

I guess the easiest would be to then write your own free() function which simply wraps to Safefree() which untangles the macro magic.

void cryptx_free(void *mem)
{
   Safefree(mem);
}

and use that one as the free() pointer.

I'm still thinking over this and I'm not sure yet what I'll do in the end.

sjaeckel commented 11 months ago

@karel-m what's your plan on this? will you be able to continue working on it anytime soon?

sjaeckel commented 7 months ago

@karel-m any chance you're gonna have time to finish the integration?

I tried to fix CryptX but couldn't find where it goes wrong :(

I updated the libtom parts via make update-libtom and created the attached patch, as the API changed since your last modifications.

0001-Provide-a-dummy-free-function-for-the-password-callb.patch.txt

After that I'm stuck with t/rsa-aes192.pem failing to decode, but it succeeds if I decode it directly in the ltc tests.

karel-m commented 1 month ago

@sjaeckel I have adopted CryptX to the current add-pem-support branch. Please merge it to develop

karel-m commented 1 month ago

Compile error with gcc-4.4.3 https://www.cpantesters.org/cpan/report/cbcac22b-6cb6-1014-a7d8-b69db03d33c9

ltc\misc\pem\pem_pkcs.c: In function 'pem_decode_pkcs_filehandle':
ltc\misc\pem\pem_pkcs.c:275: error: unknown field 'f' specified in initializer
ltc\misc\pem\pem_pkcs.c: In function 'pem_decode_pkcs':
ltc\misc\pem\pem_pkcs.c:287: error: unknown field 'buf' specified in initializer
ltc\misc\pem\pem_pkcs.c:287: warning: initialization from incompatible pointer type
ltc\misc\pem\pem_pkcs.c:287: error: unknown field 'buf' specified in initializer
ltc\misc\pem\pem_pkcs.c:287: error: unknown field 'buf' specified in initializer
ltc\misc\pem\pem_pkcs.c:287: warning: initialization makes integer from pointer without a cast

I would ignore it for now.

sjaeckel commented 1 month ago

@levitte do you plan to have a look at this PR or should I merge as is?

levitte commented 1 month ago

Other than my remark, this looks fine, at least by eyeballing (which, considering all the reviewing you've already had and all the work that's been done, should suffice, yeah?)

levitte commented 1 month ago

As a side note, an interesting development forward could be to have a pka registry, just like there already is a cipher implementation registry and a hash implementation registry...

sjaeckel commented 1 month ago

Thanks for the review.

I'm not saying that breaking changes are wrong, but then the libtomcrypt version must probably be bumped to 2.0.0, at least if semantic versioning is assumed. Is that desirable?

Those breaking changes you're highlighting are fine, since they're only within develop and not officially released yet. That's why I also already marked the comments as resolved.

If you look at the diff of the header files from the latest release it's already kind of clear that the next release from develop will have to be 2.0.0. Modifications of struct sizes, essential macros, etc. require this.

As a side note, an interesting development forward could be to have a pka registry, just like there already is a cipher implementation registry and a hash implementation registry.

That's a good question! With #515 I went the other direction, away from the registry approach, but I'm also not sure if that's the best way to go... hence the PR being marked as RFC...

Weird, @coverallsapp didn't post the coverage of this PR and it's marked as "pending completion" on their page ... but this is the only PR where this happened AFAICT. I've already clicked the "SET DONE" button besides the job, but nothing happened yet...

levitte commented 1 month ago

If you look at the diff of the header files from the latest release it's already kind of clear that the next release from develop will have to be 2.0.0. Modifications of struct sizes, essential macros, etc. require this.

Yeah, I suspected something like this. Thanks for clarifying.

As a side note, an interesting development forward could be to have a pka registry, just like there already is a cipher implementation registry and a hash implementation registry.

That's a good question! With #515 I went the other direction, away from the registry approach, but I'm also not sure if that's the best way to go... hence the PR being marked as RFC...

I'll comment there...

But back to this PR, I think it's fine and should go in.

libtom / libtomcrypt

Add PEM support #587

Checklist

Summary

Changes to existing public APIs

New public APIs and structs

structs

PKCS#8 APIs

PEM bytewise APIs

PEM `FILE`-based APIs

New demos

Details

libtom / libtomcrypt

Add PEM support #587

Checklist

Summary

Changes to existing public APIs

New public APIs and structs

structs

PKCS#8 APIs

PEM bytewise APIs

PEM FILE-based APIs

New demos

Details

PEM `FILE`-based APIs