Re-factor X_import_radix() etc. API's

[sjaeckel]

In order to resolve #227 and /8 of #228 I propose this as a possible solution.

This evolved from #227 (comment) ff.

Short summary: we want to be able to pass a huge (enough) number and combination of parameters to import functions and we're trying to find a nice way to do that.

First there was rsa_import() et. al which used standardized and/or proprietary formats to {ex,im}port asymmetric keys
Second there were issues (e.g. #136 ) where this wasn't enough and rsa_import_radix() was introduced as an evolution of a newly introduced rsa_import_hex() (c.f. #97 )
Third here we are as we realized that one radix is sometimes not enough, where you e.g. have one part of a key as hex string and another part as binary data.

Thanks for your comments.

[karel-m]

This might be handy as an internal function e.g. mp_read_pk_part(..) or something like that (maybe "pk_part" is not self-explanatory here, but I do not have a better idea).

[karel-m]

Instead of PK_PART_DEC(d) I would define PK_PART_RADIX(d,r) so that we have: PK_PART_HEX, PK_PART_RADIX and PK_PART_BIN

[buggywhip]

If it were me I would... Keep LTC lean. Don't load routines up with a bunch of internal radix conversions and extra arguments. Instead, support the most likely or most convenient radix for the function. Then, if a user has data in other radices let them convert using a set of [to be?] provided utility functions. .

…

On Jun 22, 2017, at 3:16 AM, Steffen Jaeckel ***@***.***> wrote: In order to resolve #227 <#227> and /8 of #228 <#228> I propose this as a possible solution. This evolved from #227 (comment) <#227 (comment)> ff. Short summary: we want to be able to pass a huge (enough) number and combination of parameters to import functions and we're trying to find a nice way to do that. First there was rsa_import() et. al which used standardized and/or proprietary formats to {ex,im}port asymmetric keys Second there were issues (e.g. #136 <#136> ) where this wasn't enough and rsa_import_radix() was introduced as an evolution of a newly introduced rsa_import_hex() (c.f. #97 <#97> ) Third here we are as we realized that one radix is sometimes not enough, where you e.g. have one part of a key as hex string and another part as binary data. Thanks for your comments. You can view, comment on, or merge this pull request online at: #238 <#238> Commit Summary use ltc_pk_part in rsa_import_radix() use compare_testvector() in rsa_test() also test binary import File Changes M src/headers/tomcrypt_pk.h <https://github.com/libtom/libtomcrypt/pull/238/files#diff-0> (12) M src/pk/rsa/rsa_import_radix.c <https://github.com/libtom/libtomcrypt/pull/238/files#diff-1> (34) M tests/rsa_test.c <https://github.com/libtom/libtomcrypt/pull/238/files#diff-2> (54) Patch Links: https://github.com/libtom/libtomcrypt/pull/238.patch <https://github.com/libtom/libtomcrypt/pull/238.patch> https://github.com/libtom/libtomcrypt/pull/238.diff <https://github.com/libtom/libtomcrypt/pull/238.diff> — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#238>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAzofteXxMpI5oc2jIE_SHgQ0eNtjR2vks5sGj71gaJpZM4OCFn3>.

[karel-m]

The most universal is IMO binary/raw format; however, it will end up like this:

int rsa_import_bin(unsigned char *N,  unsigned long Nlen,
                   unsigned char *e,  unsigned long elen,
                   unsigned char *d,  unsigned long dlen,
                   unsigned char *p,  unsigned long plen,
                   unsigned char *q,  unsigned long qlen,
                   unsigned char *dP, unsigned long dPlen,
                   unsigned char *dQ, unsigned long dQlen,
                   unsigned char *qP, unsigned long qPlen,
                   rsa_key *key);

[buggywhip]

On Jun 22, 2017, at 7:53 AM, karel-m ***@***.***> wrote: The most universal is IMO binary/raw format; however, it will end up like this: int rsa_import_bin(unsigned char *N, unsigned long Nlen, unsigned char *e, unsigned long elen, unsigned char *d, unsigned long dlen, unsigned char *p, unsigned long plen, unsigned char *q, unsigned long qlen, unsigned char *dP, unsigned long dPlen, unsigned char *dQ, unsigned long dQlen, unsigned char *qP, unsigned long qPlen, rsa_key *key);

Tough call. The above appears messy but it is arguably the cleanest. ...and in the spirit of lean, keeping it, IMO, makes sense. If we want to provide additional helper/wrapper functions for PEM/DER and/or mixed radices I'm okay with that. ...or perhaps just a set of base conversion utilities? My concern was driven in part because I have seen some params expressed in decimal, some hex, and yes, base64 all in the same set. Ugh. But who's responsibility is it to clean that mess up? A set of simple base conversion utilities can address this and a host of other issues. That said, I defer to you as you are doing the heavy lifting. I'm just the sidewalk super. ;-)

[karel-m]

As for the simplicity the "bin" approach might make sense. We can leave converting dec, hex, base64 or whatever to LTC users.

[sjaeckel]

I just had a chat with @sebastianas and he had a good idea!

how about orienting on the OpenSSL API?

int RSA_set0_key(RSA *r, BIGNUM *n, BIGNUM *e, BIGNUM *d);
int RSA_set0_factors(RSA *r, BIGNUM *p, BIGNUM *q);
int RSA_set0_crt_params(RSA *r,BIGNUM *dmp1, BIGNUM *dmq1, BIGNUM *iqmp);

• all these functions expect binary input data, conversion is up to the user
• data pointers are const, the user has to eventually free himself

int rsa_set_key(const unsigned char *N,  unsigned long Nlen,
                const unsigned char *e,  unsigned long elen,
                const unsigned char *d,  unsigned long dlen, /* is NULL for public keys */
                rsa_key *key);
int rsa_set_factors(const unsigned char *p,  unsigned long plen,
                    const unsigned char *q,  unsigned long qlen,
                    rsa_key *key);
int rsa_set_crt_params(const unsigned char *dP, unsigned long dPlen,
                       const unsigned char *dQ, unsigned long dQlen,
                       const unsigned char *qP, unsigned long qPlen,
                       rsa_key *key);

int dh_set_pg(const unsigned char *p, unsigned long plen,
              const unsigned char *g, unsigned long glen,
              dh_key *key);
/* here we can support either one or both */
int dh_set_key(const unsigned char *pub, unsigned long publen,
               const unsigned char *priv, unsigned long privlen,
               dh_key *key);

int dsa_set_pqg(const unsigned char *p,  unsigned long plen,
                const unsigned char *q,  unsigned long qlen,
                const unsigned char *g,  unsigned long glen,
                dsa_key *key);
/* here we can support either one or both */
int dsa_set_key(const unsigned char *pub, unsigned long publen,
                const unsigned char *priv, unsigned long privlen,
                dsa_key *key);

[karel-m]

how about orienting on the OpenSSL API?

It looks fine.

[sjaeckel]

It looks fine.

alright, I'll add the rest

[karel-m]

BTW the following API is also a bit insane (I know, it is my commit 2014-01-27)

int dsa_make_key_ex(prng_state *prng, int wprng, int group_size, int modulus_size, 
                    dsa_key *key, char* p_hex, char* q_hex, char* g_hex)

[karel-m]

We should perhaps keep DH and DSA API in a similar style

• dh_make_key vs. dsa_make_key
• dh_make_key_dhparam vs. dsa_make_key_dsaparam
• dh_set_pg vs. dsa_set_pqg
• dh_set_key vs. dsa_set_key

[sjaeckel]

We should perhaps keep DH and DSA API in a similar style

that was my intention

[karel-m]

1/ The new seems to cover all my use cases relater to DSA/DH/RSA, which is a great.

2/ I am not sure about the names dsa_make_key_ex and dh_make_key. Up to now the function named *_make_key were sort of "all-in-one-go" functions that you simply call and as a result you get the key. The two cases in question are however just the final part of key generation that do not work in one go.

It would be IMO better to name them:

- dsa_make_key_ex
+ dsa_generate_key    (or shorter dsa_gen_key)

- dh_make_key
+ dh_generate_key    (or shorter dh_gen_key)

We might want (maybe later) introduce dh_make_key as a simple wrapper around dh_set_pg_groupsize + dh_generate_key (in "all-in-one-go" style).

3/ Ad radix_to_bin - why do we have const void *in and not const char *in?

4/ I am not sure if I like the idea of *_KEY_INITIALIZER

• it is a bit inconsistent as it is used just by DH and DSA (not RSA)
• I think it is destined to be easily forgotten by users
• a simple dsa_key key; might (sometimes) set the whole structure to "all-zeroes" which migh be hard distinguish from correct dsa_key key = LTC_DSA_KEY_INITIALIZER;

Considering the fact that the "custom" keys have to be created by calling 2 functions in given order - like:

dh_set_pg(p, plen, g, glen, key);
dh_set_key(pub, publen, priv, privlen, key);
//OR
dsa_set_pqg(p, plen, q, qlen, g, glen, key);
dsa_set_key(pub, publen, priv, privlen, key);

we can IMO simply initialize the keys in dh_set_pg* or dsa_set_pqg* or not?

If you think that *_KEY_INITIALIZER is a good idea we should perhaps add something like PK_INTIALIZED (non zero) and use define like: #define LTC_DSA_KEY_INITIALIZER { PK_INITIALIZED, 0, NULL, NULL, NULL, NULL, NULL } so that we can easily detect uninitialized key structure.

5/ dh_set.c

I would prefer to have at least dh_set_pg_dhparam in a separate file as when using static linking all the ASN.1 will got linked even if I use just a simple non-ASN.1 dh_set_pg

6/ question related to mp_cleanup_multi

Is it intended as a new best practice to call mp_cleanup_multi instead of mp_clear_multi?

[karel-m]

The 5/ also applies to dsa_set.c

[sjaeckel]

1/ The new seems to cover all my use cases relater to DSA/DH/RSA, which is a great.

that's perfect!

2/ I am not sure about the names dsa_make_key_ex and dh_make_key. Up to now the function named *_make_key were sort of "all-in-one-go" functions that you simply call and as a result you get the key. The two cases in question are however just the final part of key generation that do not work in one go.

It would be IMO better to name them:

- dsa_make_key_ex
+ dsa_generate_key    (or shorter dsa_gen_key)

- dh_make_key
+ dh_generate_key    (or shorter dh_gen_key)

you're right! done

We might want (maybe later) introduce dh_make_key as a simple wrapper around dh_set_pg_groupsize + dh_generate_key (in "all-in-one-go" style).

ah, the user can do that, 1.17 had no dh_make_key() ;-) and I would've removed dsa_make_key() if it wouldn't break the API

3/ Ad radix_to_bin - why do we have const void *in and not const char *in?

because I prefer void* over char* or unsigned char* where it's not 100% clear what the input is a type of. I could perfectly think of someone having their own MPI provider where mp_read_radix() has support for a radix that's not stored as a string anymore.

4/ I am not sure if I like the idea of *_KEY_INITIALIZER

I also wasn't sure if I should like it or not, therefore I removed it. All the PK algos lived without it until now so let's keep it like that.

5/ dh_set.c

I would prefer to have at least dh_set_pg_dhparam in a separate file as when using static linking all the ASN.1 will got linked even if I use just a simple non-ASN.1 dh_set_pg

done

6/ question related to mp_cleanup_multi

Is it intended as a new best practice to call mp_cleanup_multi instead of mp_clear_multi?

I think it's an easy and safe method to prevent use-after-free -> yes, we should treat is as the new best practice

[sjaeckel]

Regarding rand_bn_range() I've still a patch locally which I'm not sure if it'd make sense... @karel-m do we need this?

diff --git a/src/math/rand_bn.c b/src/math/rand_bn.c
index 3d4f10c..2916da1 100755
--- a/src/math/rand_bn.c
+++ b/src/math/rand_bn.c
@@ -53,5 +53,5 @@ cleanup:
 /**
-  Generate a random number N in a range: 1 <= N < upper
+  Generate a random number N in a range: lower < N < upper
 */
-int rand_bn_range(void *N, void *limit, prng_state *prng, int wprng)
+int rand_bn_range(void *N, ltc_mp_digit lower, void *upper, prng_state *prng, int wprng)
 {

[karel-m]

Ad radix_to_bin - if we are going to use void * instead of char * then we should perhaps pass also inlen like:

- int radix_to_bin(const void *in, int radix, void *out, size_t* len);
+ int radix_to_bin(const void *in, size_t inlen int radix, void *out, size_t* outlen);

BTW what about to add bin_to_radix as well?

Ad rand_bn_range - after thinking a bit about your idea of having:

int rand_bn_range(void *N, ltc_mp_digit lower, void *upper, prng_state *prng, int wprng)

I think that we should rename the current function to:

int rand_bn_limit(void *N, void *limit, prng_state *prng, int wprng)

I am not sure whether we should add rand_bn_range(N, lower, upper, prng, wprng).

The rest is OK from my point of view. Although I have not checked whether the test suite of my perl bindings passes after this API facelift, IMO it would better to check it with the whole 1.18-RC.

[sjaeckel]

I'd leave radix_to_bin() as it is.

I don't see a case for bin_to_radix() (yet?).

How about rand_bn_upto() ?

[karel-m]

I'd leave radix_to_bin() as it is.

Then in must be a NUL-terminated string (or NUL-terminater buffer), right?

How about rand_bn_upto() ?

Yes.

[sjaeckel]

Then in must be a NUL-terminated string (or NUL-terminater buffer), right?

Yep, the read_radix() implementation has to detect the end of the stream.

[karel-m]

OK, I would at least put that information in comment like:

-    @param in    The input
+    @param in    The input (NUL-terminated buffer)

the read_radix() implementation has to detect the end of the stream.

Yes, but libtomcrypt's interface should be independent on underlying implementation.

As for bin_to_radix I also do not have a use case right now. I was just thinking about making them both accessible from my perl bindings. We can skip it.

[karel-m]

👍 please merge