Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Crypto lib #16825

Merged
merged 8 commits into from
Mar 5, 2024
Merged

Crypto lib #16825

merged 8 commits into from
Mar 5, 2024

Conversation

michael-redpanda
Copy link
Contributor

Implementation of a cryptographic middleware that abstracts away the underlying cryptographic library.

See the README for information on how to use the library.

Fixes: https://github.com/redpanda-data/core-internal/issues/1096

This replaces #16752

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v23.3.x
  • v23.2.x
  • v23.1.x

Release Notes

  • none

Signed-off-by: Michael Boquard <michael@redpanda.com>
Added myself to CODEOWNERS for crypto library

Signed-off-by: Michael Boquard <michael@redpanda.com>
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
Copy link
Contributor Author

Force push 37a2cd1:

  • Added reset support

graphcareful
graphcareful previously approved these changes Mar 4, 2024
Copy link
Contributor

@graphcareful graphcareful left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really nice

src/v/crypto/CMakeLists.txt Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/digest.cc Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/key.cc Show resolved Hide resolved
Comment on lines +21 to +26
if (1 != RAND_priv_bytes_ex(nullptr, buf.data(), buf.size(), 0)) {
throw internal::ossl_error(
"Failed to get random output from private RNG");
}
} else {
if (1 != RAND_bytes_ex(nullptr, buf.data(), buf.size(), 0)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does it mean for "The bytes generated will have a security strength of at least strength bits", when strength is 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe '0' means "don't care" (it's not documented at all... RAND_bytes calls RAND_bytes_ex with a strength of 0). The strength indicates an expected level of security. So if the DRBG supports 128 bits of security but the caller requests 256, it will fail. I don't believe it's necessarily a parameter to condition the DRBG output but just to ensure it meets a minimum level.

But I will look into it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah confirmed.... strength is just a check to ensure the RNG being used can supply the appropriate security level, per their docs:

The bytes generated will have a security strength of at least strength bits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still not clear to me what it is. This is about as far as I got trying to figure it out: openssl/openssl#15560

src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/include/crypto/crypto.h Outdated Show resolved Hide resolved
src/v/crypto/key.cc Show resolved Hide resolved
* MD5
* SHA256
* SHA512

Signed-off-by: Michael Boquard <michael@redpanda.com>
Signed-off-by: Michael Boquard <michael@redpanda.com>
Support loading PEM encoded RSA public and private keys and
RSA public keys via modulus and public exponent.

Signed-off-by: Michael Boquard <michael@redpanda.com>
Signed-off-by: Michael Boquard <michael@redpanda.com>
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
Copy link
Contributor Author

Force push ec0b9ac:

  • Addressed PR comments

Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@michael-redpanda michael-redpanda merged commit 3fa20fe into redpanda-data:dev Mar 5, 2024
16 checks passed
Copy link
Contributor

@rockwotj rockwotj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM nice work! Very clean.

@@ -124,6 +124,7 @@ add_subdirectory(wasm)
add_subdirectory(transform)
add_subdirectory(container)
add_subdirectory(strings)
add_subdirectory(crypto)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a note to src/v/README.md about the crypto library.

ctx.update(bytes_view_to_string_view(sha256_test_val));
bytes dgst(bytes::initialized_later{}, ctx.size());
ctx.reset(bytes_span_to_char_span(dgst));
ASSERT_EQ(dgst, sha256_expected_val);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW the "recommended" way to use GTest is to always use EXPECT_ except when not doing so would lead to a crash or UB.

Example of when to use assert: https://github.com/abseil/abseil-cpp/blob/8dc90ff07402cd027daec520bb77f46e51855889/absl/algorithm/container_test.cc#L314-L319

TEST_F(NonMutatingTest, LowerBound) {
  std::list<int>::iterator i = absl::c_lower_bound(sequence_, 3);
  ASSERT_TRUE(i != sequence_.end());
  EXPECT_EQ(2, std::distance(sequence_.begin(), i));
  EXPECT_EQ(3, *i);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants