RSA import pkcs8 #157

2017-03-01T15:10:29-06:00

karel-m commented

2017-03-01 15:10:29 -06:00

(Migrated from github.com)

as requested in #79

karel-m commented

2017-03-02 12:47:25 -06:00

(Migrated from github.com)

Indentation is not 3-spaces

karel-m commented

2017-03-03 05:48:25 -06:00

(Migrated from github.com)

from me this PR is ready for review / merge

karel-m commented

2017-03-14 11:07:06 -05:00

(Migrated from github.com)

Maybe:

- int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, rsa_key *key);
+ int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, unsigned char *passwd, unsigned long passwdlen, rsa_key *key);

Although the current implementation does not support encrypted (password protected) pkcs8 keys, it might be a good idea to support the password in the function call.

Maybe: ``` - int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, rsa_key *key); + int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, unsigned char *passwd, unsigned long passwdlen, rsa_key *key); ``` Although the current implementation does not support encrypted (password protected) pkcs8 keys, it might be a good idea to support the password in the function call.

coveralls commented

2017-03-30 18:10:53 -05:00

(Migrated from github.com)

Coverage increased (+0.02%) to 87.579% when pulling fda842158b1fa9083e529365841c930105bc85f9 on feature/rsa_import_pkcs8 into 77019928dd on develop.

[![Coverage Status](https://coveralls.io/builds/10859425/badge)](https://coveralls.io/builds/10859425) Coverage increased (+0.02%) to 87.579% when pulling **fda842158b1fa9083e529365841c930105bc85f9 on feature/rsa_import_pkcs8** into **77019928ddd2dd810601b33db045722f312b8618 on develop**.

sjaeckel commented

2017-04-03 15:53:30 -05:00

(Migrated from github.com)

- int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, rsa_key *key);
+ int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, unsigned char *passwd, unsigned long passwdlen, rsa_key *key);

Sounds like a good idea! I don't know the details about the passwords that can be used, but if it can also be a binary stream I'd prefer to use ..., const void* passwd, unsigned long passwdlen, .... In case that it's limited to a "string" I'd prefer ..., const char* passwd, rsa_key* ....

> ``` > - int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, rsa_key *key); > + int rsa_import_pkcs8(unsigned char *in, unsigned long inlen, unsigned char *passwd, unsigned long passwdlen, rsa_key *key); > ``` Sounds like a good idea! I don't know the details about the passwords that can be used, but if it can also be a binary stream I'd prefer to use `..., const void* passwd, unsigned long passwdlen, ...`. In case that it's limited to a "string" I'd prefer `..., const char* passwd, rsa_key* ...`.

karel-m commented

2017-04-03 16:15:55 -05:00

(Migrated from github.com)

passwords are not limited to strings

coveralls commented

2017-04-04 02:42:36 -05:00

(Migrated from github.com)

Coverage increased (+0.02%) to 87.821% when pulling 09efc63b787a761b1c09168fcb6d6a46863227f7 on feature/rsa_import_pkcs8 into fbe7d222c3 on develop.

[![Coverage Status](https://coveralls.io/builds/10916860/badge)](https://coveralls.io/builds/10916860) Coverage increased (+0.02%) to 87.821% when pulling **09efc63b787a761b1c09168fcb6d6a46863227f7 on feature/rsa_import_pkcs8** into **fbe7d222c3fafb3df7f0048cdbeb532a06ea198d on develop**.

sjaeckel (Migrated from github.com) reviewed

2017-04-04 09:50:06 -05:00

src/pk/rsa/rsa_import_pkcs8.c

					
				@ -0,0 +95,4 @@

				   if (err == CRYPT_OK) {

				      LTC_UNUSED_PARAM(passwd);

				      LTC_UNUSED_PARAM(passwdlen);

				      /* XXX: TODO encrypted pkcs8 not implemented yet */

I just remembered that there was something on the ML https://groups.google.com/d/msg/libtom/sFdia0rRcfM/4yIzA_crK0AJ

I don't know if this is what we need here, but it looks kinda similar :)

Will you have a look? Otherwise I'll do it when I have the time.

[edit] we should also keep this list in mind http://oid-info.com/get/1.2.840.113549.1.5 [/edit]

I just remembered that there was something on the ML https://groups.google.com/d/msg/libtom/sFdia0rRcfM/4yIzA_crK0AJ I don't know if this is what we need here, but it looks kinda similar :) Will you have a look? Otherwise I'll do it when I have the time. [edit] we should also keep this list in mind http://oid-info.com/get/1.2.840.113549.1.5 [/edit]

karel-m (Migrated from github.com) reviewed

2017-04-04 13:01:03 -05:00

src/pk/rsa/rsa_import_pkcs8.c

					
				@ -0,0 +95,4 @@

				   if (err == CRYPT_OK) {

				      LTC_UNUSED_PARAM(passwd);

				      LTC_UNUSED_PARAM(passwdlen);

				      /* XXX: TODO encrypted pkcs8 not implemented yet */

Exactly, to handle the encrypted keys we should be able to decrypt via pbeWithMD5AndDES-CBC and/or similar.

The latest spec for PBES1 / PBES2 are IMO in RFC 8018 (aka PKCS5 v2.1)

The code from mailing list looks similar to PbeWithMD5AndTripleDES

Anyway, it might be a good idea to have something like src\misc\pkcs5\pbes1.c + src\misc\pkcs5\pbes2.c so that we can later reuse it in ecc_import_pkcs8 or event implement stuff like rsa_import_pkcs12.

Slightly OT: this PKCS8 related bug report related to EC keys https://github.com/DCIT/perl-CryptX/issues/28 demonstrates that PKCS8 is not easy

Exactly, to handle the encrypted keys we should be able to decrypt via ` pbeWithMD5AndDES-CBC` and/or similar. The latest spec for PBES1 / PBES2 are IMO in RFC 8018 (aka PKCS5 v2.1) * https://tools.ietf.org/html/rfc8018#section-6.1 * https://tools.ietf.org/html/rfc8018#section-6.2 The code from mailing list looks similar to PbeWithMD5AndTripleDES Anyway, it might be a good idea to have something like `src\misc\pkcs5\pbes1.c` + `src\misc\pkcs5\pbes2.c` so that we can later reuse it in `ecc_import_pkcs8` or event implement stuff like `rsa_import_pkcs12`. Slightly OT: this PKCS8 related bug report related to EC keys https://github.com/DCIT/perl-CryptX/issues/28 demonstrates that PKCS8 is not easy

sjaeckel commented

2017-05-30 11:36:25 -05:00

(Migrated from github.com)

We can merge this one (as soon as it built) in my opinion and open a new issue for encrypted pkcs#8 files

karel-m commented

2017-05-30 14:17:57 -05:00

(Migrated from github.com)

I would also merge this one but there are 2 things to mention if anybody will hack on this in the future:

1/ This implementation does not support password protected keys (yes, we know it).

2/ The implementation more or less follows https://tools.ietf.org/html/rfc5208; however, it was obsoleted by https://tools.ietf.org/html/rfc5958 Which basically means that there can exist some correct/valid pkcs#8 formatted keys that my implementation cannot load - see pkcs8 related troubles in https://github.com/DCIT/perl-CryptX/issues/28

I was thinking about replacing the current implementation with our ASN.1 flexi parser.

I would also merge this one but there are 2 things to mention if anybody will hack on this in the future: 1/ This implementation does not support password protected keys (yes, we know it). 2/ The implementation more or less follows https://tools.ietf.org/html/rfc5208; however, it was obsoleted by https://tools.ietf.org/html/rfc5958 Which basically means that there can exist some correct/valid pkcs#8 formatted keys that my implementation cannot load - see pkcs8 related troubles in https://github.com/DCIT/perl-CryptX/issues/28 I was thinking about replacing the current implementation with our ASN.1 flexi parser.

karel-m commented

2017-06-08 13:14:32 -05:00

(Migrated from github.com)