| Message ID | 1440739738-19587-1-git-send-email-marcel@holtmann.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
Am Freitag, 28. August 2015, 07:28:58 schrieb Marcel Holtmann: Hi Marcel, >The RSA algorithm provides two ASN.1 key types. One for RSA Private Key >and another for RSA Public Key. Use these two already defined ASN.1 >definitions instead of inventing a new one. > >Signed-off-by: Marcel Holtmann <marcel@holtmann.org> >--- > crypto/Makefile | 9 ++++++--- > crypto/rsa_helper.c | 13 +++++++++---- > crypto/rsakey.asn1 | 5 ----- > crypto/rsaprivatekey.asn1 | 13 +++++++++++++ > crypto/rsapublickey.asn1 | 4 ++++ > 5 files changed, 32 insertions(+), 12 deletions(-) > delete mode 100644 crypto/rsakey.asn1 > create mode 100644 crypto/rsaprivatekey.asn1 > create mode 100644 crypto/rsapublickey.asn1 > >diff --git a/crypto/Makefile b/crypto/Makefile >index 3cc91c3301c7..0b056c411aa7 100644 >--- a/crypto/Makefile >+++ b/crypto/Makefile >@@ -31,10 +31,13 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o > obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o > obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o > >-$(obj)/rsakey-asn1.o: $(obj)/rsakey-asn1.c $(obj)/rsakey-asn1.h >-clean-files += rsakey-asn1.c rsakey-asn1.h >+$(obj)/rsapublickey-asn1.o: $(obj)/rsapublickey-asn1.c >$(obj)/rsapublickey-asn1.h +clean-files += rsapublickey-asn1.c >rsapublickey-asn1.h > >-rsa_generic-y := rsakey-asn1.o >+$(obj)/rsaprivatekey-asn1.o: $(obj)/rsaprivatekey-asn1.c >$(obj)/rsaprivatekey-asn1.h +clean-files += rsaprivatekey-asn1.c >rsaprivatekey-asn1.h >+ >+rsa_generic-y := rsapublickey-asn1.o rsaprivatekey-asn1.o > rsa_generic-y += rsa.o > rsa_generic-y += rsa_helper.o > obj-$(CONFIG_CRYPTO_RSA) += rsa_generic.o >diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c >index 8d96ce969b44..26617e3132fb 100644 >--- a/crypto/rsa_helper.c >+++ b/crypto/rsa_helper.c >@@ -15,7 +15,8 @@ > #include <linux/err.h> > #include <linux/fips.h> > #include <crypto/internal/rsa.h> >-#include "rsakey-asn1.h" >+#include "rsapublickey-asn1.h" >+#include "rsaprivatekey-asn1.h" > > int rsa_get_n(void *context, size_t hdrlen, unsigned char tag, > const void *value, size_t vlen) >@@ -109,9 +110,13 @@ int rsa_parse_key(struct rsa_key *rsa_key, const void >*key, int ret; > > free_mpis(rsa_key); >- ret = asn1_ber_decoder(&rsakey_decoder, rsa_key, key, key_len); >- if (ret < 0) >- goto error; >+ ret = asn1_ber_decoder(&rsapublickey_decoder, rsa_key, key, key_len); >+ if (ret < 0) { >+ ret = asn1_ber_decoder(&rsaprivatekey_decoder, rsa_key, >+ key, key_len); Wouldn't it be better to have 2 parse_key functions here? We (will) have 2 setkey functions which are the callers of parse_key. The reason is that the added if requires CPU cycles that can be easily avoided. Hence I propose: rsa_parse_pubkey() rsapublickey_decoder rsa_parse_privkey() rsaprivatekey_decoder to avoid parsing a key as pub key even though we already know it is a priv key. >+ if (ret < 0) >+ goto error; >+ } > > return 0; > error: >diff --git a/crypto/rsakey.asn1 b/crypto/rsakey.asn1 >deleted file mode 100644 >index 3c7b5df7b428..000000000000 >--- a/crypto/rsakey.asn1 >+++ /dev/null >@@ -1,5 +0,0 @@ >-RsaKey ::= SEQUENCE { >- n INTEGER ({ rsa_get_n }), >- e INTEGER ({ rsa_get_e }), >- d INTEGER ({ rsa_get_d }) >-} >diff --git a/crypto/rsaprivatekey.asn1 b/crypto/rsaprivatekey.asn1 >new file mode 100644 >index 000000000000..58dddc7c1536 >--- /dev/null >+++ b/crypto/rsaprivatekey.asn1 >@@ -0,0 +1,13 @@ >+RSAPrivateKey ::= SEQUENCE { >+ version Version, >+ modulus INTEGER ({ rsa_get_n }), -- n >+ publicExponent INTEGER ({ rsa_get_e }), -- e >+ privateExponent INTEGER ({ rsa_get_d }), -- d >+ prime1 INTEGER, -- p >+ prime2 INTEGER, -- q >+ exponent1 INTEGER, -- d mod (p-1) >+ exponent2 INTEGER, -- d mod (q-1) >+ coefficient INTEGER -- (inverse of q) mod p >+} >+ >+Version ::= INTEGER >diff --git a/crypto/rsapublickey.asn1 b/crypto/rsapublickey.asn1 >new file mode 100644 >index 000000000000..8f7f8760f2a9 >--- /dev/null >+++ b/crypto/rsapublickey.asn1 >@@ -0,0 +1,4 @@ >+RSAPublicKey ::= SEQUENCE { >+ modulus INTEGER ({ rsa_get_n }), -- n >+ publicExponent INTEGER ({ rsa_get_e }) -- e >+} Ciao Stephan -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephan, >> The RSA algorithm provides two ASN.1 key types. One for RSA Private Key >> and another for RSA Public Key. Use these two already defined ASN.1 >> definitions instead of inventing a new one. >> >> Signed-off-by: Marcel Holtmann <marcel@holtmann.org> >> --- >> crypto/Makefile | 9 ++++++--- >> crypto/rsa_helper.c | 13 +++++++++---- >> crypto/rsakey.asn1 | 5 ----- >> crypto/rsaprivatekey.asn1 | 13 +++++++++++++ >> crypto/rsapublickey.asn1 | 4 ++++ >> 5 files changed, 32 insertions(+), 12 deletions(-) >> delete mode 100644 crypto/rsakey.asn1 >> create mode 100644 crypto/rsaprivatekey.asn1 >> create mode 100644 crypto/rsapublickey.asn1 >> >> diff --git a/crypto/Makefile b/crypto/Makefile >> index 3cc91c3301c7..0b056c411aa7 100644 >> --- a/crypto/Makefile >> +++ b/crypto/Makefile >> @@ -31,10 +31,13 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o >> obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o >> obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o >> >> -$(obj)/rsakey-asn1.o: $(obj)/rsakey-asn1.c $(obj)/rsakey-asn1.h >> -clean-files += rsakey-asn1.c rsakey-asn1.h >> +$(obj)/rsapublickey-asn1.o: $(obj)/rsapublickey-asn1.c >> $(obj)/rsapublickey-asn1.h +clean-files += rsapublickey-asn1.c >> rsapublickey-asn1.h >> >> -rsa_generic-y := rsakey-asn1.o >> +$(obj)/rsaprivatekey-asn1.o: $(obj)/rsaprivatekey-asn1.c >> $(obj)/rsaprivatekey-asn1.h +clean-files += rsaprivatekey-asn1.c >> rsaprivatekey-asn1.h >> + >> +rsa_generic-y := rsapublickey-asn1.o rsaprivatekey-asn1.o >> rsa_generic-y += rsa.o >> rsa_generic-y += rsa_helper.o >> obj-$(CONFIG_CRYPTO_RSA) += rsa_generic.o >> diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c >> index 8d96ce969b44..26617e3132fb 100644 >> --- a/crypto/rsa_helper.c >> +++ b/crypto/rsa_helper.c >> @@ -15,7 +15,8 @@ >> #include <linux/err.h> >> #include <linux/fips.h> >> #include <crypto/internal/rsa.h> >> -#include "rsakey-asn1.h" >> +#include "rsapublickey-asn1.h" >> +#include "rsaprivatekey-asn1.h" >> >> int rsa_get_n(void *context, size_t hdrlen, unsigned char tag, >> const void *value, size_t vlen) >> @@ -109,9 +110,13 @@ int rsa_parse_key(struct rsa_key *rsa_key, const void >> *key, int ret; >> >> free_mpis(rsa_key); >> - ret = asn1_ber_decoder(&rsakey_decoder, rsa_key, key, key_len); >> - if (ret < 0) >> - goto error; >> + ret = asn1_ber_decoder(&rsapublickey_decoder, rsa_key, key, key_len); >> + if (ret < 0) { >> + ret = asn1_ber_decoder(&rsaprivatekey_decoder, rsa_key, >> + key, key_len); > > > Wouldn't it be better to have 2 parse_key functions here? We (will) have 2 > setkey functions which are the callers of parse_key. I am no longer convinced that two setkey function will actually help. The RSA Private Key data structure contains the public and private key. And the RSA Public Key data structure contains just the private key. > The reason is that the added if requires CPU cycles that can be easily > avoided. > > Hence I propose: > > rsa_parse_pubkey() > rsapublickey_decoder > > rsa_parse_privkey() > rsaprivatekey_decoder > > to avoid parsing a key as pub key even though we already know it is a priv > key. If we really care about CPU cycles when setting the key, then we should not even be discussing ASN.1 parsing here. I think we really need to add support for setkeyid callback and use struct key / key serial to reference a given key. This whole parsing of DER over and over again is a bad API design anyway. The kernel has a keys subsystem that knows how to handle asymmetric keys and parse key credentials out of it. Especially akcipher should learn how to use these keys. It is also the only real way to unify the existing RSA implementations and allowing future asymmetric ciphers like ECC to utilize this correctly. I mean, we really need to stop handing DER encoded keys around. The amount of encoding and decoding that needs to be done in the kernel is the real waste of CPU cycles. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Marcel, On 08/27/2015 10:28 PM, Marcel Holtmann wrote: > +++ b/crypto/rsaprivatekey.asn1 > @@ -0,0 +1,13 @@ > +RSAPrivateKey ::= SEQUENCE { > + version Version, > + modulus INTEGER ({ rsa_get_n }), -- n > + publicExponent INTEGER ({ rsa_get_e }), -- e > + privateExponent INTEGER ({ rsa_get_d }), -- d > + prime1 INTEGER, -- p > + prime2 INTEGER, -- q > + exponent1 INTEGER, -- d mod (p-1) > + exponent2 INTEGER, -- d mod (q-1) > + coefficient INTEGER -- (inverse of q) mod p > +} > + > +Version ::= INTEGER If you want to do this you should also update the existing RSA test vectors, because they are failing after this patch is applied. The reason is that there is no version in the private keys in crypto/testmgr.h And the QAT RSA implementation should also be updated so they are consistent. I have already started to do the changes proposed for the akcipher api to add SGLs support and to split the set_key for set_publickey and set_privatekey so I will take care of this. Thanks, T -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Tadeusz, >> +++ b/crypto/rsaprivatekey.asn1 >> @@ -0,0 +1,13 @@ >> +RSAPrivateKey ::= SEQUENCE { >> + version Version, >> + modulus INTEGER ({ rsa_get_n }), -- n >> + publicExponent INTEGER ({ rsa_get_e }), -- e >> + privateExponent INTEGER ({ rsa_get_d }), -- d >> + prime1 INTEGER, -- p >> + prime2 INTEGER, -- q >> + exponent1 INTEGER, -- d mod (p-1) >> + exponent2 INTEGER, -- d mod (q-1) >> + coefficient INTEGER -- (inverse of q) mod p >> +} >> + >> +Version ::= INTEGER > > If you want to do this you should also update the existing RSA test vectors, because > they are failing after this patch is applied. the test vectors have been failing no matter what. The crypto/rsakey.asn1 is actually broken as I explained in previous emails. That the ASN.1 parser accepted the test vectors was a bug which has been fixed now. > The reason is that there is no version in the private keys in crypto/testmgr.h > And the QAT RSA implementation should also be updated so they are consistent. The QAT code should be updated indeed, but honestly I am still maintaining the case that akcipher should just only operate on setkeyid with struct key / key serial and some ASN.1 blob that we have to decode and recode all the time. > I have already started to do the changes proposed for the akcipher api to add SGLs > support and to split the set_key for set_publickey and set_privatekey so I will > take care of this. I am not in favor of just hacking in this split until the semantics are actually understood. As said, the right solution from my point of view is to remove setkey from akcipher and replace it with setkeyid instead. Remember that a private key contains the public key as well. Meaning operations are mutually exclusive. And there is really no point in forcing the caller to split things into two. With asymmetric cryptography you either have private and public key or you just have the public key. The case where you only have the private key is pointless. We could keep the setkey as it is to load the private + public key information and add an extra setpubkey for loading only the public key. Then again a setkeyid would solve both of these problems since the key material would be nicely represented in a struct key. However we actually want to load the keys into the asymmetric key type and use it from there. The asymmetric key type should be the only entity that has to deal with ASN.1 encoding. Having an akcipher deal with ASN.1 is just wrong. What we really want is to be able to use keys from certificates to verify and encrypt information. And we want this all in one single place and not duplicate this whole ASN.1 stuff in the keys subsystem and the crypto subsystem. This means I am strongly against trying to add setkey, setpubkey, setcert, setkeychain or whatever else you might need when it comes to akcipher. There is one thing that is needed and that is setkeyid and have to reference the key serial from the keys subsystem. Regards Marcel -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcel Holtmann <marcel@holtmann.org> wrote: > > I am not in favor of just hacking in this split until the semantics are actually understood. As said, the right solution from my point of view is to remove setkey from akcipher and replace it with setkeyid instead. It's the keys system that should not be decoding the keys since it doesn't know what to do with them. The keys system should not have any algorithm-specific knowledge in it. You need to have algorithm-specific knowledge to parse the keys, so that's why the parsing should happen in the crypto API and not in the key storage system. > Remember that a private key contains the public key as well. Meaning operations are mutually exclusive. And there is really no point in forcing the caller to split things into two. With asymmetric cryptography you either have private and public key or you just have the public key. The case where you only have the private key is pointless. > > We could keep the setkey as it is to load the private + public key information and add an extra setpubkey for loading only the public key. Then again a setkeyid would solve both of these problems since the key material would be nicely represented in a struct key. > > However we actually want to load the keys into the asymmetric key type and use it from there. The asymmetric key type should be the only entity that has to deal with ASN.1 encoding. Having an akcipher deal with ASN.1 is just wrong. > > What we really want is to be able to use keys from certificates to verify and encrypt information. And we want this all in one single place and not duplicate this whole ASN.1 stuff in the keys subsystem and the crypto subsystem. This means I am strongly against trying to add setkey, setpubkey, setcert, setkeychain or whatever else you might need when it comes to akcipher. There is one thing that is needed and that is setkeyid and have to reference the key serial from the keys subsystem. I disagree. I think having separate functions for setting public and private keys makes sense. Cheers,
On 08/28/2015 05:54 PM, Marcel Holtmann wrote: >> If you want to do this you should also update the existing RSA test vectors, because >> > they are failing after this patch is applied. > the test vectors have been failing no matter what. The crypto/rsakey.asn1 is actually broken as I explained in previous emails. That the ASN.1 parser accepted the test vectors was a bug which has been fixed now. > So that's a very nice bug, which makes things work nicely. I wish all the bugs were like this one ;) -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/28/2015 11:55 PM, Herbert Xu wrote: > I think having separate functions for setting public > and private keys makes sense. So I'm going to proceed with this approach. Thanks everyone for your comments. -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Herbert Xu <herbert@gondor.apana.org.au> wrote: > > I am not in favor of just hacking in this split until the semantics are > > actually understood. As said, the right solution from my point of view is > > to remove setkey from akcipher and replace it with setkeyid instead. > > It's the keys system that should not be decoding the keys since > it doesn't know what to do with them. The keys system should not > have any algorithm-specific knowledge in it. You need to have > algorithm-specific knowledge to parse the keys, so that's why > the parsing should happen in the crypto API and not in the key > storage system. You're right and wrong, I think. The asymmetric key type should be calling akcipher to do the algorithmic stuff, yes, *but* the key must also be parsed on loading to check that the bits we require for a particular algorithm are there. Now this could be done by starting up an akcipher blob and loading the key data into it to see if it croaks on initialisation (which will presumably check the contents of the data blob), but we wouldn't necessarily be able to actually *use* the key at this point. David -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/crypto/Makefile b/crypto/Makefile index 3cc91c3301c7..0b056c411aa7 100644 --- a/crypto/Makefile +++ b/crypto/Makefile @@ -31,10 +31,13 @@ obj-$(CONFIG_CRYPTO_HASH2) += crypto_hash.o obj-$(CONFIG_CRYPTO_PCOMP2) += pcompress.o obj-$(CONFIG_CRYPTO_AKCIPHER2) += akcipher.o -$(obj)/rsakey-asn1.o: $(obj)/rsakey-asn1.c $(obj)/rsakey-asn1.h -clean-files += rsakey-asn1.c rsakey-asn1.h +$(obj)/rsapublickey-asn1.o: $(obj)/rsapublickey-asn1.c $(obj)/rsapublickey-asn1.h +clean-files += rsapublickey-asn1.c rsapublickey-asn1.h -rsa_generic-y := rsakey-asn1.o +$(obj)/rsaprivatekey-asn1.o: $(obj)/rsaprivatekey-asn1.c $(obj)/rsaprivatekey-asn1.h +clean-files += rsaprivatekey-asn1.c rsaprivatekey-asn1.h + +rsa_generic-y := rsapublickey-asn1.o rsaprivatekey-asn1.o rsa_generic-y += rsa.o rsa_generic-y += rsa_helper.o obj-$(CONFIG_CRYPTO_RSA) += rsa_generic.o diff --git a/crypto/rsa_helper.c b/crypto/rsa_helper.c index 8d96ce969b44..26617e3132fb 100644 --- a/crypto/rsa_helper.c +++ b/crypto/rsa_helper.c @@ -15,7 +15,8 @@ #include <linux/err.h> #include <linux/fips.h> #include <crypto/internal/rsa.h> -#include "rsakey-asn1.h" +#include "rsapublickey-asn1.h" +#include "rsaprivatekey-asn1.h" int rsa_get_n(void *context, size_t hdrlen, unsigned char tag, const void *value, size_t vlen) @@ -109,9 +110,13 @@ int rsa_parse_key(struct rsa_key *rsa_key, const void *key, int ret; free_mpis(rsa_key); - ret = asn1_ber_decoder(&rsakey_decoder, rsa_key, key, key_len); - if (ret < 0) - goto error; + ret = asn1_ber_decoder(&rsapublickey_decoder, rsa_key, key, key_len); + if (ret < 0) { + ret = asn1_ber_decoder(&rsaprivatekey_decoder, rsa_key, + key, key_len); + if (ret < 0) + goto error; + } return 0; error: diff --git a/crypto/rsakey.asn1 b/crypto/rsakey.asn1 deleted file mode 100644 index 3c7b5df7b428..000000000000 --- a/crypto/rsakey.asn1 +++ /dev/null @@ -1,5 +0,0 @@ -RsaKey ::= SEQUENCE { - n INTEGER ({ rsa_get_n }), - e INTEGER ({ rsa_get_e }), - d INTEGER ({ rsa_get_d }) -} diff --git a/crypto/rsaprivatekey.asn1 b/crypto/rsaprivatekey.asn1 new file mode 100644 index 000000000000..58dddc7c1536 --- /dev/null +++ b/crypto/rsaprivatekey.asn1 @@ -0,0 +1,13 @@ +RSAPrivateKey ::= SEQUENCE { + version Version, + modulus INTEGER ({ rsa_get_n }), -- n + publicExponent INTEGER ({ rsa_get_e }), -- e + privateExponent INTEGER ({ rsa_get_d }), -- d + prime1 INTEGER, -- p + prime2 INTEGER, -- q + exponent1 INTEGER, -- d mod (p-1) + exponent2 INTEGER, -- d mod (q-1) + coefficient INTEGER -- (inverse of q) mod p +} + +Version ::= INTEGER diff --git a/crypto/rsapublickey.asn1 b/crypto/rsapublickey.asn1 new file mode 100644 index 000000000000..8f7f8760f2a9 --- /dev/null +++ b/crypto/rsapublickey.asn1 @@ -0,0 +1,4 @@ +RSAPublicKey ::= SEQUENCE { + modulus INTEGER ({ rsa_get_n }), -- n + publicExponent INTEGER ({ rsa_get_e }) -- e +}
The RSA algorithm provides two ASN.1 key types. One for RSA Private Key and another for RSA Public Key. Use these two already defined ASN.1 definitions instead of inventing a new one. Signed-off-by: Marcel Holtmann <marcel@holtmann.org> --- crypto/Makefile | 9 ++++++--- crypto/rsa_helper.c | 13 +++++++++---- crypto/rsakey.asn1 | 5 ----- crypto/rsaprivatekey.asn1 | 13 +++++++++++++ crypto/rsapublickey.asn1 | 4 ++++ 5 files changed, 32 insertions(+), 12 deletions(-) delete mode 100644 crypto/rsakey.asn1 create mode 100644 crypto/rsaprivatekey.asn1 create mode 100644 crypto/rsapublickey.asn1