What about an ECC API facelift?

[karel-m]

The new RSA API has moved from long ad hoc argument lists to parameter(s) structs. What about using a similar approach for ECC as well?

Things are a bit more complicated with ECC so one ..._opts struct may not be enough. Here is the basic idea based on structs: ltc_ecc_sign_opts_v3, ltc_ecc_verify_opts_v3, ltc_ecc_recovery_opts_v3, and ltc_ecc_crypt_opts_v3

New public ECC functions:

int ecc_sign_hash_v3(const unsigned char *in,  unsigned long inlen,
                           unsigned char *out, unsigned long *outlen,
                     const ltc_ecc_sign_opts_v3 *opts,
                           int *recid_out,
                     const ecc_key *key);

int ecc_verify_hash_v3(const unsigned char *sig,  unsigned long siglen,
                       const unsigned char *hash, unsigned long hashlen,
                       const ltc_ecc_verify_opts_v3 *opts,
                             int *stat,
                       const ecc_key *key);

int ecc_recover_key_v3(const unsigned char *sig,  unsigned long siglen,
                       const unsigned char *hash, unsigned long hashlen,
                       const ltc_ecc_recovery_opts_v3 *opts,
                             ecc_key *key);

int ecc_encrypt_key_v3(const unsigned char *in,  unsigned long inlen,
                             unsigned char *out, unsigned long *outlen,
                       const ltc_ecc_crypt_opts_v3 *opts,
                       const ecc_key *key);

int ecc_decrypt_key_v3(const unsigned char *in,  unsigned long inlen,
                             unsigned char *out, unsigned long *outlen,
                       const ltc_ecc_crypt_opts_v3 *opts,
                       const ecc_key *key);

Deprecated (but still available) public ECC functions:

ecc_sign_hash
ecc_sign_hash_v2
ecc_verify_hash
ecc_verify_hash_v2
ecc_sign_hash_rfc7518
ecc_verify_hash_rfc7518
ecc_encrypt_key
ecc_decrypt_key
ecc_recover_key

@sjaeckel what do you think?

[sjaeckel]

The current API is as follows:

libtomcrypt/src/headers/tomcrypt_pk.h

Lines 450 to 474 in 68aae28

	int ecc_sign_hash_v2(const unsigned char *in,
	unsigned long inlen,
	unsigned char *out,
	unsigned long *outlen,
	ltc_ecc_sig_opts *opts,
	const ecc_key *key);
	int ecc_verify_hash_v2(const unsigned char *sig,
	unsigned long siglen,
	const unsigned char *hash,
	unsigned long hashlen,
	ltc_ecc_sig_opts *opts,
	int *stat,
	const ecc_key *key);
	#if defined(LTC_DER)
	int ecc_encrypt_key(const unsigned char *in, unsigned long inlen,
	unsigned char *out, unsigned long *outlen,
	prng_state *prng, int wprng, int hash,
	const ecc_key *key);
	int ecc_decrypt_key(const unsigned char *in, unsigned long inlen,
	unsigned char *out, unsigned long *outlen,
	const ecc_key *key);
	#endif /* LTC_DER */

libtomcrypt/src/headers/tomcrypt_pk.h

Lines 545 to 550 in 68aae28

	int ecc_recover_key(const unsigned char *sig,
	unsigned long siglen,
	const unsigned char *hash,
	unsigned long hashlen,
	ltc_ecc_sig_opts *opts,
	ecc_key *key);

and already basically looks like what you proposed!?

Can you please specify what the different types of opts struct should contain? As candidates I see only prng_state *prng, int wprng, int hash into a ltc_ecc_crypt_opts for ecc_encrypt_key().

But then we should maybe think one step further and make the common parts the same for all PK algs? And more, there's FMU only prng_state *prng, int wprng which could be shared, which is minimized to prng_state *prng once #515 would be merged.

[karel-m]

Something like this:

typedef struct ltc_ecc_sign_opts_v3 {
   ecc_signature_type type;         /* Signature type/format */
   ltc_ecc_nonce_mode nonce_mode;   /* Nonce generation policy */
   int rfc6979_hash_idx;            /* Hash algorithm index for RFC6979, -1 if unset */
   prng_state *prng;                /* PRNG used by signing blinding (required even when nonce_mode == LTC_ECC_NONCE_RFC6979) */
   int wprng;                       /* PRNG descriptor index used by signing blinding */
} ltc_ecc_sign_opts_v3;

typedef struct ltc_ecc_verify_opts_v3 {
   ecc_signature_type type;         /* Signature type/format */
} ltc_ecc_verify_opts_v3;

typedef struct ltc_ecc_recovery_opts_v3 {
   ecc_signature_type type;         /* Signature type/format *
   int recid;                       /* Recovery ID, -1 if encoded in the signature or unknown */
} ltc_ecc_recovery_opts_v3;

typedef struct ltc_ecc_crypt_opts_v3 {
   int hash_idx;                    /* Hash algorithm used to derive the ECDH mask */
   prng_state *prng;                /* PRNG to use for ephemeral key generation */
   int wprng;                       /* PRNG descriptor index */
   ltc_ecc_ephemeral_key_format epk_format; /* Encoding of the ephemeral public key stored in the packet (used only by encryption) */
} ltc_ecc_crypt_opts_v3;



typedef enum ecc_signature_type {
   LTC_ECCSIG_ANSIX962   = 0x0,     /* ASN.1 encoded, ANSI X9.62 */
   LTC_ECCSIG_RFC7518    = 0x1,     /* raw R, S values */
   LTC_ECCSIG_ETH27      = 0x2,     /* raw R, S, V (+27) values */
   LTC_ECCSIG_RFC5656    = 0x3,     /* SSH + ECDSA signature format defined by RFC5656 */
} ecc_signature_type;

typedef enum ltc_ecc_nonce_mode {
   LTC_ECC_NONCE_RANDOM  = 0x0,     /* Random ephemeral scalar generated from the selected PRNG */
   LTC_ECC_NONCE_RFC6979 = 0x1,     /* Deterministic nonce according to RFC6979 */
} ltc_ecc_nonce_mode;

typedef enum ltc_ecc_ephemeral_key_format {
   LTC_ECC_EPKF_DEFAULT      = 0x0, /* Preserve current behavior: prefer compressed output when available */
   LTC_ECC_EPKF_COMPRESSED   = 0x1, /* Force compressed ANSI X9.63 point form */
   LTC_ECC_EPKF_UNCOMPRESSED = 0x2, /* Force uncompressed ANSI X9.63 point form */
} ltc_ecc_ephemeral_key_format;

[sjaeckel]

As a first point: this wouldn't require a v3. v2 hasn't been officially released yet, we can still do what we want with the current develop branch.

... and then I'm not sure ... the current opts struct is a dump for all of these possibilities that's true, which is not exactly nice, but OTOH I don't like having 4 different structs just for the sake of "cleanliness" ... I'm still missing the functional advantage resp. improvement ...

also

• we'd need 5 different structs then, or ltc_ecc_crypt_opts_v3 should be renamed to ltc_ecc_encrypt_opts_v3.
• there's no need for a type in the recovery struct.

plus two questions regarding the nonce mode:

1. why moving away from the implicit enable when setting a hash alg? (I don't care whether it's by name or by index).
2. why would we suddenly need a PRNG in RFC6979 mode?

I understand what you want, but this just doesn't feel right. Especially that ltc_ecc_verify_opts_v3 and ltc_ecc_recovery_opts_v3 have only one element are an indicator for me, that this isn't thought out (yet).

[karel-m]

OK, no problem.

But one thing I would like to mention here: in the current ltc_ecc_sig_opts, I really do not like the int *recid member. In my opinion, it is an ugly hack that does not fit well here and is quite hard to understand and use.

[sjaeckel]

I really do not like the int *recid member. In my opinion, it is an ugly hack that does not fit well here and is quite hard to understand and use.

Fair enough, that's really not straight-forward.

diff --git a/src/headers/tomcrypt_pk.h b/src/headers/tomcrypt_pk.h
index 5fee1a2ba59d..e36a1e7e1adf 100644
--- a/src/headers/tomcrypt_pk.h
+++ b/src/headers/tomcrypt_pk.h
@@ -406,3 +406,4 @@ typedef struct ltc_ecc_sig_opts {
     */
-   int *recid;
+   unsigned char enable_recovery_id;
+   int recovery_id;

I guess that'd be better!?

[karel-m]

The idea of enable_recovery_id is definitely better.

The only remaining purist issue is that we are kind of abusing the "options struct" to return part of the result/output, but I can live with that.

[sjaeckel]

The only remaining purist issue is that we are kind of abusing the "options struct" to return part of the result/output, but I can live with that.

Yeah, right again. We can still improve this, therefore I'm leaving this issue open.