gh-libtom/libtomcrypt

Watch

Fork 0

Minor improvements #353

Merged

sjaeckel merged 10 commits from minor/improvements into develop

2018-03-23 09:15:31 -05:00

sjaeckel commented

2018-03-02 02:32:31 -06:00

(Migrated from github.com)

Preparation for further work

karel-m (Migrated from github.com) reviewed

2018-03-02 12:51:13 -06:00

src/headers/tomcrypt_pk.h

Please revert the changes above. There are even more key types in my new ecc branch:

enum {
   PK_PUBLIC=0,
   PK_PRIVATE=1
};

/* Indicates standard output formats that can be read e.g. by OpenSSL or GnuTLS */
#define PK_STD          0x1000
/* Indicates compressed public ECC key */
#define PK_COMPRESSED   0x2000
/* Indicates ECC key with the curve specified by OID */
#define PK_CURVEOID     0x4000

Let's discuss it later.

Please revert the changes above. There are even more key types in my new ecc branch: ```C enum { PK_PUBLIC=0, PK_PRIVATE=1 }; /* Indicates standard output formats that can be read e.g. by OpenSSL or GnuTLS */ #define PK_STD 0x1000 /* Indicates compressed public ECC key */ #define PK_COMPRESSED 0x2000 /* Indicates ECC key with the curve specified by OID */ #define PK_CURVEOID 0x4000 ``` Let's discuss it later.

sjaeckel (Migrated from github.com) reviewed

2018-03-02 12:52:09 -06:00

src/headers/tomcrypt_pk.h

why can't you just put them in there?

sjaeckel (Migrated from github.com) reviewed

2018-03-02 12:59:41 -06:00

src/headers/tomcrypt_pk.h

btw. it doesn't even really clash...
I just successfully rebased the ecc-branch on top of this one and there was only a trivial merge-conflict that could be auto-resolved by my mergetool.
Compilation works without a warning and the tests succeed as well.

btw. it doesn't even really clash... I just successfully rebased the ecc-branch on top of this one and there was only a trivial merge-conflict that could be auto-resolved by my mergetool. Compilation works without a warning and the tests succeed as well.

sjaeckel (Migrated from github.com) reviewed

2018-03-02 13:02:57 -06:00

src/headers/tomcrypt_pk.h

and here the patch that integrates your new defines in the enum:

diff --git a/src/headers/tomcrypt_pk.h b/src/headers/tomcrypt_pk.h
index fcac2fa..f8332be 100644
--- a/src/headers/tomcrypt_pk.h
+++ b/src/headers/tomcrypt_pk.h
@@ -12,13 +12,15 @@
 enum public_key_type {
-   PK_PUBLIC   = 0x0000,
-   PK_PRIVATE  = 0x0001,
-   PK_STD      = 0x1000,
-   PK_STD_PUB  = PK_STD | PK_PUBLIC,
-   PK_STD_PRIV = PK_STD | PK_PRIVATE,
+   PK_PUBLIC      = 0x0000,
+   PK_PRIVATE     = 0x0001,
+   /* Indicates standard output formats that can be read e.g. by OpenSSL or GnuTLS */
+   PK_STD         = 0x1000,
+   /* Indicates compressed public ECC key */
+   PK_COMPRESSED  = 0x2000,
+   /* Indicates ECC key with the curve specified by OID */
+   PK_CURVEOID    = 0x4000,
+
+   PK_STD_PUB     = PK_STD | PK_PUBLIC,
+   PK_STD_PRIV    = PK_STD | PK_PRIVATE,
 };
 
-/* Indicates compressed public ECC key */
-#define PK_COMPRESSED   0x2000
-/* Indicates ECC key with the curve specified by OID */
-#define PK_CURVEOID     0x4000
 int rand_prime(void *N, long len, prng_state *prng, int wprng);

and here the patch that integrates your new defines in the enum: ``` diff --git a/src/headers/tomcrypt_pk.h b/src/headers/tomcrypt_pk.h index fcac2fa..f8332be 100644 --- a/src/headers/tomcrypt_pk.h +++ b/src/headers/tomcrypt_pk.h @@ -12,13 +12,15 @@ enum public_key_type { - PK_PUBLIC = 0x0000, - PK_PRIVATE = 0x0001, - PK_STD = 0x1000, - PK_STD_PUB = PK_STD | PK_PUBLIC, - PK_STD_PRIV = PK_STD | PK_PRIVATE, + PK_PUBLIC = 0x0000, + PK_PRIVATE = 0x0001, + /* Indicates standard output formats that can be read e.g. by OpenSSL or GnuTLS */ + PK_STD = 0x1000, + /* Indicates compressed public ECC key */ + PK_COMPRESSED = 0x2000, + /* Indicates ECC key with the curve specified by OID */ + PK_CURVEOID = 0x4000, + + PK_STD_PUB = PK_STD | PK_PUBLIC, + PK_STD_PRIV = PK_STD | PK_PRIVATE, }; -/* Indicates compressed public ECC key */ -#define PK_COMPRESSED 0x2000 -/* Indicates ECC key with the curve specified by OID */ -#define PK_CURVEOID 0x4000 int rand_prime(void *N, long len, prng_state *prng, int wprng); ```

karel-m (Migrated from github.com) reviewed

2018-03-02 14:25:23 -06:00

src/headers/tomcrypt_pk.h

Better except:

   PK_STD_PUB     = PK_STD | PK_PUBLIC,
   PK_STD_PRIV    = PK_STD | PK_PRIVATE,

is a bit odd as there can also be something like:

PK_NNN_PUB = PK_BULIC|PK_COMPRESSED|PK_CURVEOID
PK_NNN_PUB = PK_BULIC|PK_COMPRESSED
PK_NNN_PUB = PK_BULIC|PK_CURVEOID
etc.

and enum public_key_type should be perhaps enum key_type

Better except: ``` PK_STD_PUB = PK_STD | PK_PUBLIC, PK_STD_PRIV = PK_STD | PK_PRIVATE, ``` is a bit odd as there can also be something like: ``` PK_NNN_PUB = PK_BULIC|PK_COMPRESSED|PK_CURVEOID PK_NNN_PUB = PK_BULIC|PK_COMPRESSED PK_NNN_PUB = PK_BULIC|PK_CURVEOID etc. ``` and `enum public_key_type` should be perhaps `enum key_type`

sjaeckel (Migrated from github.com) reviewed

2018-03-02 14:32:14 -06:00

src/headers/tomcrypt_pk.h

is a bit odd as there can also be something like: ...

which I think is fine, we should create combinations for all implemented combinations, so someone doesn't start to combine wildly and wonders why it doesn't work.

and enum public_key_type should be perhaps enum key_type

even the wikipedia article is called https://en.wikipedia.org/wiki/Public-key_cryptography so I think that name should be fine :)

> is a bit odd as there can also be something like: ... which I think is fine, we should create combinations for all implemented combinations, so someone doesn't start to combine wildly and wonders why it doesn't work. > and enum public_key_type should be perhaps enum key_type even the wikipedia article is called https://en.wikipedia.org/wiki/Public-key_cryptography so I think that name should be fine :)

karel-m (Migrated from github.com) reviewed

2018-03-04 12:56:40 -06:00

src/headers/tomcrypt_pk.h

which I think is fine, we should create combinations for all implemented combinations, so someone doesn't start to combine wildly and wonders why it doesn't work.

I see your point but I do not like it. I prefer to have exactly one constant for one bit.

even the wikipedia article is called https://en.wikipedia.org/wiki/Public-key_cryptography so I think that name should be fine :)

Yes but having something like:

int  ecc_export(unsigned char *out, unsigned long *outlen, public_key_type type, ecc_key *key);

might be a bit confusing as the user may think that the function exports just public key.

> which I think is fine, we should create combinations for all implemented combinations, so someone doesn't start to combine wildly and wonders why it doesn't work. I see your point but I do not like it. I prefer to have exactly one constant for one bit. > even the wikipedia article is called https://en.wikipedia.org/wiki/Public-key_cryptography so I think that name should be fine :) Yes but having something like: ```C int ecc_export(unsigned char *out, unsigned long *outlen, public_key_type type, ecc_key *key); ``` might be a bit confusing as the user may think that the function exports just public key.

karel-m commented

2018-03-07 15:55:23 -06:00

(Migrated from github.com)

Idea: what about to turn scan_hex into src/misc/base16/base16_decode.c + adding also base16_encode.c?

Idea: what about to turn `scan_hex` into `src/misc/base16/base16_decode.c` + adding also `base16_encode.c`?

sjaeckel (Migrated from github.com) reviewed

2018-03-07 18:02:33 -06:00

src/headers/tomcrypt_pk.h

I see your point but I do not like it. I prefer to have exactly one constant for one bit.

Mkay

may think that the function exports just public key

...and when that person looks at the definition it becomes obvious that this enum also includes private keys :)

I think key_type is not specific enough and I can't find another good name...

> I see your point but I do not like it. I prefer to have exactly one constant for one bit. Mkay > may think that the function exports just public key ...and when that person looks at the definition it becomes obvious that this enum also includes private keys :) I think `key_type` is not specific enough and I can't find another good name...