Improve cryptography tests. #3979

teo-tsirpanis · 2023-03-20T14:13:00Z

Improved test coverage for the hash algorithms by using official test vectors.

Prerequisite for #3973.

TYPE: NO_HISTORY
DESC: Improve cryptography tests.

eric-hughes-tiledb · 2023-04-05T14:14:49Z

tiledb/sm/crypto/crypto_win32.cc

@@ -32,11 +32,15 @@

 #ifdef _WIN32

-#include "tiledb/sm/crypto/crypto_win32.h"
+#include <windows.h>


The NOMINMAX macro definition needs to accompany the inclusion of windows.h.

eric-hughes-tiledb · 2023-04-05T14:24:07Z

tiledb/sm/crypto/crypto_win32.cc

-#include "tiledb/sm/crypto/crypto_win32.h"
+#include <windows.h>
+
+#include <bcrypt.h>


Do we need <windows.h>, or is this header alone sufficient?

Also, if there's an inclusion order dependency between external headers, that should be documented.

tiledb/sm/crypto/crypto_win32.h

tiledb/sm/crypto/test/unit_crypto.cc

eric-hughes-tiledb · 2023-04-05T14:54:58Z

tiledb/sm/crypto/test/unit_crypto.cc

+// hex. The function is generic over the hash and the input type.
+template <class Hash>
+static void test_hash(
+    const uint8_t* input, uint64_t length, const std::string& expected_hash) {


Prefer span of separate character pointer and length.

You mean std::span? It in C++ 20. Until then I used std::string, to unify tests for SHA256 where the input is in hex and MD5 where it isn't.

We have a replacement for span in our code base. It's available through common.h.

eric-hughes-tiledb · 2023-04-05T14:56:59Z

tiledb/sm/crypto/test/unit_crypto.cc

+      expected_hash);
+}
+
+struct MD5Hash {


This is a traits class and should be named accordingly.

I changed the implementation a bit. Instead of trait types the test function directly takes the hash function and its digest size as template arguments.

tiledb/sm/crypto/test/unit_crypto.cc

eric-hughes-tiledb · 2023-04-05T15:34:31Z

tiledb/sm/crypto/test/unit_crypto.cc

+};
+
+TEST_CASE("Crypto: Test MD5", "[crypto][md5]") {
+  auto test_md5 = [](const std::string input, const std::string expected_hash) {


This function definition is more or less repeated below. It would be appropriate to define a function template and to instantiate it in a using statement. using test_md5 = validate_argument_vs_return<MD5Hash>

My refactorings have reduced the code duplication.

eric-hughes-tiledb

Also, it would be better to put the citations for where the test vectors come from up in the file-level documentation at the top. @section Test Plan can contain them. It should explain that the test plan is to verify the test vectors in the reference materials.

The test plan should also have argument validation, for example against nullptr. I'm not seeing those tests.

tiledb/sm/crypto/test/unit_crypto.cc

We fill two 64-byte buffers with random data and check that their content is not equal. Also `crypto_win32.h` was refactored to not require `windows.h`.

They are redundant now that we have the new ones and were using the deprecated sprintf.

teo-tsirpanis · 2023-04-06T08:36:55Z

(force-pushed to update the branch)

…ointers. It became easier to use.

…cc`.

…rom the tests.

…of trait types. This also validates that the expected hash has the correct length.

teo-tsirpanis · 2023-04-06T17:19:01Z

The test plan should also have argument validation, for example against nullptr. I'm not seeing those tests.

The hash and RNG implementations don't check for null. I don't think anything more than an assert(ptr != nullptr) in the code is warranted, these APIs are internal.

tiledb/sm/crypto/test/unit_crypto.cc

eric-hughes-tiledb

New code needs span. We have incorporated it into the global namespace in common.h. The implementation is a replacement for C++20 std::span. It's in our code base as part of an effort to write out occurrences of Buffer. Using string instead of span is hack that we don't need.

Switching to span and adding citations for the AES-GCM look like the only further changes we'll need.

FYI: We also want to replace Buffer with span in all the crypto functions. I think there's a story for that already.

eric-hughes-tiledb · 2023-04-28T13:08:15Z

tiledb/sm/crypto/crypto.cc

+}
+
+Status Crypto::get_random_bytes(unsigned char* output, unsigned num_bytes) {
+  return PlatformCrypto::get_random_bytes(output, num_bytes);


This new function should use span.

tiledb/sm/crypto/test/unit_crypto.cc

eric-hughes-tiledb · 2023-04-28T13:13:03Z

tiledb/sm/crypto/test/unit_crypto.cc

+ *
+ * SHA-256 test vectors were taken from SHA256ShortMsg.rsp, from "SHA Test
+ * Vectors for Hashing Bit-Oriented Messages"
+ * (https://csrc.nist.gov/Projects/cryptographic-algorithm-validation-program/Secure-Hashing)


Exactly what's needed. Citations allow auditing the code against published standards.

eric-hughes-tiledb · 2023-04-28T13:31:55Z

tiledb/sm/crypto/test/unit_crypto.cc

-        0);
+  auto test_md5 =
+      test_expected_hash_value<Crypto::md5, Crypto::MD5_DIGEST_BYTES>;
+  static const std::vector<std::pair<std::string, std::string>> test_cases{


string_view can be initialized with string literals and converts readily to span.

teo-tsirpanis · 2024-07-15T16:50:19Z

CI is green, this is ready for review.

eric-hughes-tiledb

Major: Why do we need get_random_bytes exposed. Doesn't seem like it has any upside.

eric-hughes-tiledb · 2024-07-17T01:32:28Z

tiledb/sm/crypto/CMakeLists.txt

+	    #this_target_link_libraries(OpenSSL::Crypto)
+	    target_link_libraries(tiledb_crypto PRIVATE OpenSSL::Crypto)


If you're not going to use this_target_link_libraries, you need documentation explaining why.

Nit: indentation

Fixed indentation. I don't know why we didn't use this_target_link_libraries, let me investigate…

eric-hughes-tiledb · 2024-07-17T01:36:45Z

tiledb/sm/crypto/test/main.cc

-  (void)sizeof(tiledb::sm::Crypto);
-  return 0;
-}
+#define CATCH_CONFIG_MAIN


Since there's only one test file, and there are unlikely to ever be more than that one, this definition could just go in the test source file, which would allow this file to be eliminated.

Actually this file is not needed at all and the entry point is provided by the Catch2::Catch2WithMain target. I removed it.

eric-hughes-tiledb · 2024-07-17T01:53:03Z

tiledb/sm/crypto/crypto.h

@@ -158,6 +158,14 @@ class Crypto {
   */
  static Status sha256(
      const void* input, uint64_t input_read_size, Buffer* output);
+
+  /**
+   * Generates a number of cryptographically random bytes.


A single line of documentation for something acting as a PRNG is inadequate.

How's the IV initialized?

Where's the state held between calls?

Is the state global?

Is it thread safe?

As long as these functions were not exposed to the outside, these issues didn't matter as much. If you're going to run tests against them, it matter. And the one test that does anything would pass with poor RNGs such as CRC shift or linear congruential.

Better yet. Simplify this PR and eliminate this function and its do-almost-nothing tests.

Also. We have a PRNG in the code that has all this documented. We don't need another, and we certainly do not want another that's poorly documented.

eric-hughes-tiledb · 2024-07-17T01:59:37Z

tiledb/sm/crypto/crypto_win32.h

@@ -129,16 +143,6 @@ class Win32CNG {
      uint64_t input_read_size,
      Buffer* output,
      LPCWSTR hash_algorithm);
-
- private:


Leave it private. Or better yet, since it's static, take it out of the class and define it only in the .cc file.

eric-hughes-tiledb · 2024-07-17T02:00:27Z

tiledb/sm/crypto/test/unit_tiledb_crypto.cc

+  CHECK(Crypto::get_random_bytes(buf1).ok());
+  CHECK(Crypto::get_random_bytes(buf2).ok());
+  CHECK(buf1 != buf2);


This test doesn't do anything useful. What's it here for?

eric-hughes-tiledb · 2024-07-17T02:07:27Z

tiledb/sm/crypto/test/unit_tiledb_crypto.cc

+ * Test that the given input (optionally in hex) has the expected hash value in
+ * hex. The function is generic over the hash and the input size.
+ */
+template <Status Hash(const void*, uint64_t, Buffer*), int Digest_Bytes>


Sizes should be size_t unless they need to be compatible with existing code that (incorrectly) uses any other type for in-memory sizes. Here's that's Digest_Bytes.

Nit: template arguments should be lower case.

The functions in the Crypto class use uint64_t, but changing the test code here to size_t works. Updated.

Reverted the change to size_t due to macOS CI failures. It will go away soon either way when we use spans instead of pointer-size pairs.

This file is no longer necessary with Catch2 v3, which provides an entry point through the `Catch2::Catch2WithMain` target.

teo-tsirpanis · 2024-07-17T10:28:39Z

Feedback addressed. I had added the get_random_bytes test as minimal coverage, but now removed it.

Switching it to `size_t` caused failures in macOS CI.

eric-hughes-tiledb

One remaining question about OpenSSL::Crypto in the object library specification.

eric-hughes-tiledb · 2024-07-22T18:20:01Z

tiledb/sm/crypto/CMakeLists.txt

-	#this_target_link_libraries(OpenSSL::Crypto)
-	target_link_libraries(tiledb_crypto PRIVATE OpenSSL::Crypto)
+        #this_target_link_libraries(OpenSSL::Crypto)
+        target_link_libraries(tiledb_crypto PRIVATE OpenSSL::Crypto)


What's the resolution on this? It hasn't been addressed from last time.

The difference is that this_target_link_libraries links with PUBLIC visibility, which is undesirable in this case, because we use OpenSSL as an internal implementation detail. Changing it right now does not break anything, but it will have undesirable effects once the tiledb_crypto object library is incorporated to the main build (and the OpenSSL::Crypto target would be a requirement even for shared libraries).

I am going to remove the commented line, and add a comment on why we can't use this_target_link_libraries.

tiledb/sm/crypto/crypto_openssl.h

teo-tsirpanis · 2024-08-02T19:40:21Z

@eric-hughes-tiledb do you have any additional comments?

teo-tsirpanis · 2024-08-19T08:46:48Z

@eric-hughes-tiledb, @KiterLuc is there anything left to do here? The last review comment was on a line that already existed before this PR.

eric-hughes-tiledb

LGTM

Fixes #5249. Because the `<span>` header in MSVC does not include `<array>`, a simultaneous merging of #5233 which uses `<span>` with #3979 which used `std::array` in the Windows crypto code caused compile errors. --- TYPE: NO_HISTORY

teo-tsirpanis requested a review from eric-hughes-tiledb March 20, 2023 14:13

teo-tsirpanis force-pushed the crypto-tests branch from 29d039a to bbc768c Compare March 20, 2023 16:38

eric-hughes-tiledb suggested changes Apr 5, 2023

View reviewed changes

eric-hughes-tiledb reviewed Apr 5, 2023

View reviewed changes

tiledb/sm/crypto/test/unit_crypto.cc Outdated Show resolved Hide resolved

teo-tsirpanis added 7 commits April 6, 2023 11:35

Move the crypto unit tests to a dedicated project.

5abe57d

Test the cryptographic random number generator.

85966b3

We fill two 64-byte buffers with random data and check that their content is not equal. Also `crypto_win32.h` was refactored to not require `windows.h`.

Add SHA256 tests from the NIST test vectors.

9dfef2b

Add MD5 tests from RFC1321 test vectors.

ae89383

Fix compile errors around mismatched loop counter types.

0ade268

Remove the previous hash tests.

7cbd02f

They are redundant now that we have the new ones and were using the deprecated sprintf.

Make sure formatting does not put bcrypt.h before windows.h.

7083a61

teo-tsirpanis force-pushed the crypto-tests branch from bbc768c to 7083a61 Compare April 6, 2023 08:36

teo-tsirpanis added 7 commits April 6, 2023 14:11

Put Win32CNG::md5 and sha256 in the header.

453f5c6

Change get_random_bytes to not require a buffer and work with raw p…

f39f026

…ointers. It became easier to use.

Use an alias to choose the platform-specific crypto class in `crypto.…

e4289cd

…cc`.

Add Crypto::get_random_bytes, removing the platform-specific code f…

8305fb6

…rom the tests.

Refactor test_hash to use template parameters for the hash instead …

488c4a0

…of trait types. This also validates that the expected hash has the correct length.

Use a vector of test cases.

fb741c1

Improve documentation and naming.

a2837c8

Fix formatting.

e8dd34d

teo-tsirpanis commented Apr 11, 2023

View reviewed changes

tiledb/sm/crypto/test/unit_crypto.cc Outdated Show resolved Hide resolved

ihnorton requested a review from eric-hughes-tiledb April 26, 2023 13:16

eric-hughes-tiledb suggested changes Apr 28, 2023

View reviewed changes

teo-tsirpanis added 2 commits April 28, 2023 17:56

Use spans in Crypto::get_random.

3c06639

Add documentation for the AES-GCM tests.

da7e47b

teo-tsirpanis mentioned this pull request May 5, 2023

Refactor the UUID generator. #4082

Closed

2 tasks

teo-tsirpanis added 2 commits May 5, 2023 23:55

Fix compile errors.

9e0fa00

Fix CI.

535d352

teo-tsirpanis added 2 commits July 12, 2024 16:45

Refactor the AES-GCM TestCase class to use spans and constexpr.

2e68386

Update to_hex to accept a span.

10ab36d

teo-tsirpanis force-pushed the crypto-tests branch 2 times, most recently from 243fbb3 to 10ab36d Compare July 12, 2024 15:42

Fix infinite loop.

bf805d3

teo-tsirpanis marked this pull request as ready for review July 15, 2024 16:50

teo-tsirpanis requested a review from eric-hughes-tiledb July 15, 2024 16:50

eric-hughes-tiledb suggested changes Jul 17, 2024

View reviewed changes

teo-tsirpanis added 4 commits July 17, 2024 13:14

Fix indentation.

9073c89

Make get_random_bytes private and remove its test.

4dd0359

Remove tiledb/sm/crypto/test/main.cc.

100ce2b

This file is no longer necessary with Catch2 v3, which provides an entry point through the `Catch2::Catch2WithMain` target.

Update template definition in the hash test.

fdc9aad

teo-tsirpanis requested a review from eric-hughes-tiledb July 17, 2024 10:28

Revert changing the size parameter in the hash function template.

8b08750

Switching it to `size_t` caused failures in macOS CI.

teo-tsirpanis force-pushed the crypto-tests branch from e717a68 to 8b08750 Compare July 17, 2024 12:52

Merge branch 'dev' into crypto-tests

ff4d989

eric-hughes-tiledb reviewed Jul 22, 2024

View reviewed changes

Add comment about linking to OpenSSL target.

c67b6f7

teo-tsirpanis requested a review from eric-hughes-tiledb July 30, 2024 14:27

Merge branch 'dev' into crypto-tests

fb5c7e6

eric-hughes-tiledb approved these changes Aug 20, 2024

View reviewed changes

KiterLuc merged commit e51e6cd into TileDB-Inc:dev Aug 20, 2024
60 checks passed

teo-tsirpanis deleted the crypto-tests branch August 20, 2024 16:25

teo-tsirpanis mentioned this pull request Aug 21, 2024

Fix missing header. #5250

Merged

		#this_target_link_libraries(OpenSSL::Crypto)
		target_link_libraries(tiledb_crypto PRIVATE OpenSSL::Crypto)

Improve cryptography tests. #3979

Improve cryptography tests. #3979

Conversation

teo-tsirpanis commented Mar 20, 2023 • edited Loading

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

eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Apr 6, 2023

teo-tsirpanis commented Apr 6, 2023

eric-hughes-tiledb 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

teo-tsirpanis commented Jul 15, 2024

eric-hughes-tiledb 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

teo-tsirpanis Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teo-tsirpanis commented Jul 17, 2024

eric-hughes-tiledb 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

teo-tsirpanis commented Aug 2, 2024

teo-tsirpanis commented Aug 19, 2024

eric-hughes-tiledb left a comment

Choose a reason for hiding this comment

teo-tsirpanis commented Mar 20, 2023 •

edited

Loading

teo-tsirpanis Jul 17, 2024 •

edited

Loading