rfc6979_hash_alg question

[karel-m]

This is a design question: should rfc6979_hash_alg be part of the ecc_key structure?

Is rfc6979_hash_alg a property of the key? Or is it an extra parameter/flag for the ecc_sign function?

It seems more logical to pass rfc6979_hash_alg as an additional parameter to ecc_sign_hash_internal and ecc_rfc6979_key.

It looks like we’re unnecessarily polluting the ecc_key structure with data that isn’t related to the key itself.

[sjaeckel]

Good question. I went the easy way and put it into the struct because it gives support w/o touching the API. If we were putting it into the API, we'll have to adjust all 4 public APIs + internals.

You're right, in theory it's no property of the key, but the property of the signature operation. Similar to what we had before with the ecc_signature_type, but an option on top of this.

IMO we can put it into the API, but then I'd suggest the following:

$ git --no-pager diff -U1              
diff --git a/src/headers/tomcrypt_pk.h b/src/headers/tomcrypt_pk.h
index aeb2188c..7e7cdf62 100644
--- a/src/headers/tomcrypt_pk.h
+++ b/src/headers/tomcrypt_pk.h
@@ -299,13 +299,23 @@ typedef struct {
     void *k;
-
-    /** The hash algorithm to use when creating a signature.
-     *  Setting this will enable RFC6979 compatible signature generation.
-     *  The macro ECC_SET_RFC6979_HASH_ALG() is provided as a helper
-     *  to set this.*/
-    const char *rfc6979_hash_alg;
 } ecc_key;
 
-#define ECC_SET_RFC6979_HASH_ALG(key, alg) do { \
-   (key)->rfc6979_hash_alg = (alg);             \
-} while(0)
+typedef struct ltc_ecc_sig_opts {
+   /** The PRNG to use.
+    *  This must be set in case deterministic signature generation
+    *  according to RFC6979 is not enabled.
+    */
+   prng_state *prng;
+          int  wprng;
+
+   /** Enable generation of a recovery ID.
+    *  This must be set in case one requires the recovery ID of a
+    *  signature operation.
+    */
+          int *recid;
+
+   /** The hash algorithm to use when creating a signature.
+    *  Setting this will enable RFC6979 compatible signature generation.
+    */
+   const char *rfc6979_hash_alg;
+} ltc_ecc_sig_opts;
 
@@ -372,4 +382,3 @@ int ecc_sign_hash(const unsigned char *in,
                         unsigned long *outlen,
-                           prng_state *prng,
-                                  int  wprng,
+                        ltc_ecc_sig_opts *opts,
                   const       ecc_key *key);
@@ -396,5 +405,3 @@ int ecc_sign_hash_rfc7518_ex(const unsigned char *in,
                                    unsigned long *outlen,
-                                      prng_state *prng,
-                                             int  wprng,
-                                             int *recid,
+                                   ltc_ecc_sig_opts *opts,
                              const       ecc_key *key);
@@ -413,4 +420,3 @@ int ecc_sign_hash_rfc5656(const unsigned char *in,
                                 unsigned long *outlen,
-                                   prng_state *prng,
-                                          int  wprng,
+                                ltc_ecc_sig_opts *opts,
                           const       ecc_key *key);
@@ -429,4 +435,3 @@ int ecc_sign_hash_eth27(const unsigned char *in,
                               unsigned long *outlen,
-                                 prng_state *prng,
-                                        int  wprng,
+                              ltc_ecc_sig_opts *opts,
                         const       ecc_key *key);

We could then even think of reverting the duplication per ecc_signature_type of the APIs, which was done in #450, and add an ecc_signature_type field to the opts ...

[karel-m]

I support your suggestion incl. moving ecc_signature_type / sigformat to ltc_ecc_sig_opts

Maybe introducing completely new ecc_sign_hash_with_opts(...) or something like that (+maybe some backwards compatible wrappers/macros with currently available functions).

[sjaeckel]

Maybe introducing completely new ecc_sign_hash_with_opts(...) or something like that (+maybe some backwards compatible wrappers/macros with currently available functions).

If that's fine for you I'll simply replace ecc_sign_hash(). We're already breaking some of the RSA APIs besides others, so why not break some more ...

meh .. OK, I'll make it ecc_sign_hash_v2() ... even though I already hate that name 15seconds after writing it down ...

[sjaeckel]

Better? Feel free to review #699 :)