Basic support for numeric keys #456

cmickeyb · 2023-12-23T00:33:46Z

New stuff:

support for the numeric format for ecdsa keys (constructors)
first start at a test harness for crypto tests, greatly expanded suite of tests for ecdsa keys

Clean up:

"modernizing" the error handling in the ECDSA key classes
canonicalizing all of the types for handling smart pointers with openssl
consolidation of (a lot of) redundant code in the ecdsa key classes

bvavala

Some minor comments -- looks good overall.

common/crypto/sig_private_key.cpp

common/crypto/sig_public_key.cpp

common/interpreter/wawaka_wasm/WasmCryptoExtensions.cpp

common/log.cpp

Added a verbose flag to the cmake project variables in order to make finding errors in the build output a little easier. When true (which is the default), the setting will ensure that all warning messages are generated. An environment variable allows for override. Signed-off-by: Mic Bowman <[email protected]>

Update the default (very simple) builtin untrusted logger to send output to stderr and then flush. This is mostly the result of lost logging that happens when abnormal termination occurs (precisely the time when you want accurate and complete logs). Signed-off-by: Mic Bowman <[email protected]>

Add a new contructor for the sig_private class that takes a curve identifier and a byte array with the bignum encoding of the private key. Push the new constructor through to the contract interpreter. This should enable construction of BIP32 extended keys. Signed-off-by: Mic Bowman <[email protected]>

We had definitions for memory safe pointers in several files. This moves all of those definitions into a single file. Add a ResetKey() method for the sig_public and sig_private classes to manage memory allocation and resetting the pointer appropriately. Note that there is a behavior change. The key is reset prior to attempts to update it. This means that the key will be unset if there is, for example, an error deserializing a key. The tests must be updated for this (they assumed that the key was still valid after an invalid attempt to update). Signed-off-by: Mic Bowman <[email protected]>

Removed/consolidated code for deserializing ecdsa public and private keys from a string. Simplify and consolidate the constructors. Use the common error handling functions. A lot of the code in the crypto classes predates the logging and error processing functions that were added later. This PR updates at least the ones in the crypto signature classes. And... this fixes a few memory leaks and potential issues with memory corruption during creation & assignment of keys. Removed the XY serialization functions from the sig public class. Added a boolean operator that can be used to test whether a key is initialized. Cleaned up a lot of the exception generation. Attempted to use MemoryError when it appears that memory allocation failed and CryptoError any time an OpenSSL call fails unexpectedly. Signed-off-by: Mic Bowman <[email protected]>

Add constructors for public and private ECDSA keys for numeric keys represented by octets stored in a ByteArray. Add methods to retrieve the numeric keys from the classes. Signed-off-by: Mic Bowman <[email protected]>

Moved the signature tests into a separate file. Added a bunch of new signature tests to ensure that uninitialized keys are handled correctly. Removed signature tests for functions that no longer exist. Introduced some macros that make it a little easier to make writing and intepreting test failures a little easier. Signed-off-by: Mic Bowman <[email protected]>

Co-authored-by: Bruno Vavala <[email protected]> Signed-off-by: Mic Bowman <[email protected]>

Fix the error strings in sig_public_key.cpp to reflect that the errors are in the public key, not the private key. Replace the typedefs for bignumbers in the crypto extenions with the existing definitions from the crypto library. Not sure the shared header file should be universally visible but it is for now. Signed-off-by: Mic Bowman <[email protected]>

Signed-off-by: Mic Bowman <[email protected]>

g2flyer

Runs well and in general looks good. A few nit-picking (and somewhat debatable) comments, though.
PS: sorry for the late review. Having ancient outlook refile rules which i forgot about didn't help ;-)

common/crypto/sig.h

common/log.cpp

common/crypto/sig_private_key.cpp

g2flyer · 2024-01-10T21:03:37Z

common/crypto/sig_private_key.cpp

+// Custom curve constructor with initial key specified as a bignum
+pcrypto::sig::PrivateKey::PrivateKey(
+    const pcrypto::sig::SigCurve& sigCurve,
+    const ByteArray& numeric_key) :


wouldn't binary_key here and in (de)serialization (as well for public key) be clearer than numeric_key as difference is really ascii (PEM) vs binary encoding of what is in both case a number (which for public keys is anyway a bit debatable at is a point which is not really an (algebraic) number?

i could go either way... its numeric to me because the code i'm writing around it is treating it as a number (hierarchical key derivation). public keys make more sense because they are compressed points.

well, even the string/PEM version is still an (encoded) number? I definitely wouldn't want to veto this name but it just wasn't very intuitive for me ...

g2flyer · 2024-01-10T21:24:16Z

common/crypto/sig_private_key.cpp

    }
+    sigDetails_ = privateKey.sigDetails_;
 }  // pcrypto::sig::PrivateKey::PrivateKey (copy constructor)

 // XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 // Move constructor
 // throws RuntimeError
 pcrypto::sig::PrivateKey::PrivateKey(pcrypto::sig::PrivateKey&& privateKey)


As we override the move constructor, shouldn't we also define an explicit move assignment operator? (admit i'm on thin ground with my C++ knowledge and there are some implicitly defined version, so maybe C++ does do the right thing?)

move for sigdetails? suggest you check with @bvavala since that's his code

no, i meant a move assignment operation for PrivateKey (& PublicKey) ....

g2flyer · 2024-01-10T22:02:57Z

common/interpreter/wawaka_wasm/WasmCryptoExtensions.cpp

+
+        ByteArray key(key_buffer, key_buffer + key_buffer_length);
+
+        pcrypto::sig::PrivateKey privkey(pcrypto::sig::SigCurve::SECP384R1, key);


A bit surprising to see a particular curve hardcoded here? Why are we not using PDO_DEFAULT_SIGCURVE here?

g2flyer · 2024-01-10T22:04:59Z

common/interpreter/wawaka_wasm/WasmCryptoExtensions.cpp

+{
+    wasm_module_inst_t module_inst = wasm_runtime_get_module_inst(exec_env);
+    try {
+        pe::ThrowIf<pe::RuntimeError>(key_buffer_length != 48, "unsupported key size");


Hmm, doesn't the length depend on the curve? So hardcoding 48 here seems a bit brittle? (also, in principle the length could also be greater?)

g2flyer · 2024-01-10T22:10:59Z

contracts/wawaka/common/Cryptography.cpp

+/* ----------------------------------------------------------------- *
+ * NAME: ww::crypto::
+ * ----------------------------------------------------------------- */
+bool ww::crypto::ecdsa::generate_keys(


generate to me would imply we are generating new key but we really (re)create (i.e., deserialize) existing keys?

strictly... yes. in practice, we're creating keys in a form that can be used in other operations.

g2flyer · 2024-01-10T22:12:28Z

contracts/wawaka/interpreter-test/interpreter-test.cpp

+    const ww::types::ByteArray message(message_string.begin(), message_string.end());
+
+    // ---------- get the keys we need ----------
+    ww::types::ByteArray bytes(48);


Again the magic 48 :-)

contracts/wawaka/interpreter-test/interpreter-test.cpp

cmickeyb requested review from g2flyer and bvavala December 23, 2023 00:33

cmickeyb assigned bvavala Dec 23, 2023

bvavala approved these changes Dec 28, 2023

View reviewed changes

cmickeyb force-pushed the mic.dec22.extended_keys branch from 6a583e2 to ae61eeb Compare January 6, 2024 00:17

cmickeyb and others added 11 commits January 8, 2024 16:58

Add the rest of the numeric key functions for signature keys

976cb1c

Add constructors for public and private ECDSA keys for numeric keys represented by octets stored in a ByteArray. Add methods to retrieve the numeric keys from the classes. Signed-off-by: Mic Bowman <[email protected]>

Update common/crypto/sig_private_key.cpp

8466d3a

Co-authored-by: Bruno Vavala <[email protected]> Signed-off-by: Mic Bowman <[email protected]>

Update common/crypto/sig_private_key.cpp

b45c7b8

Co-authored-by: Bruno Vavala <[email protected]> Signed-off-by: Mic Bowman <[email protected]>

Remove an extra log message

7e5d646

Signed-off-by: Mic Bowman <[email protected]>

cmickeyb force-pushed the mic.dec22.extended_keys branch from ae61eeb to 7e5d646 Compare January 8, 2024 23:58

g2flyer reviewed Jan 10, 2024

View reviewed changes

g2flyer self-requested a review January 11, 2024 18:44

g2flyer approved these changes Jan 11, 2024

View reviewed changes

bvavala merged commit 0fc4201 into hyperledger-labs:main Jan 11, 2024
4 checks passed

cmickeyb deleted the mic.dec22.extended_keys branch January 16, 2024 00:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic support for numeric keys #456

Basic support for numeric keys #456

cmickeyb commented Dec 23, 2023

bvavala left a comment

g2flyer left a comment

g2flyer Jan 10, 2024

cmickeyb Jan 11, 2024

g2flyer Jan 11, 2024

g2flyer Jan 10, 2024

cmickeyb Jan 11, 2024

g2flyer Jan 11, 2024

g2flyer Jan 10, 2024

g2flyer Jan 10, 2024

g2flyer Jan 10, 2024

cmickeyb Jan 11, 2024

g2flyer Jan 10, 2024


		ByteArray key(key_buffer, key_buffer + key_buffer_length);

		pcrypto::sig::PrivateKey privkey(pcrypto::sig::SigCurve::SECP384R1, key);

Basic support for numeric keys #456

Basic support for numeric keys #456

Conversation

cmickeyb commented Dec 23, 2023

bvavala left a comment

Choose a reason for hiding this comment

g2flyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment