Skip to content

Commit

Permalink
Review comments addressed - 2.
Browse files Browse the repository at this point in the history
  • Loading branch information
Pankaj committed Aug 23, 2022
1 parent 888911b commit 46e96a6
Show file tree
Hide file tree
Showing 19 changed files with 68 additions and 100 deletions.
12 changes: 11 additions & 1 deletion bftengine/include/bftengine/ReplicaConfig.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,8 @@ class ReplicaConfig : public concord::serialize::SerializableFactory<ReplicaConf
serialize(outStream, diagnosticsServerPort);
serialize(outStream, useUnifiedCertificates);
serialize(outStream, kvBlockchainVersion);
serialize(outStream, operatorMsgSigningAlgo);
serialize(outStream, replicaMsgSigningAlgo);
}
void deserializeDataMembers(std::istream& inStream) {
deserialize(inStream, isReadOnly);
Expand Down Expand Up @@ -515,6 +517,8 @@ class ReplicaConfig : public concord::serialize::SerializableFactory<ReplicaConf
deserialize(inStream, diagnosticsServerPort);
deserialize(inStream, useUnifiedCertificates);
deserialize(inStream, kvBlockchainVersion);
deserialize(inStream, operatorMsgSigningAlgo);
deserialize(inStream, replicaMsgSigningAlgo);
}

private:
Expand Down Expand Up @@ -600,6 +604,10 @@ inline std::ostream& operator<<(std::ostream& os, const ReplicaConfig& rc) {
rc.adaptivePruningIntervalPeriod,
rc.dbSnapshotIntervalSeconds.count());
os << ",";
const auto replicaMsgSignAlgo =
(concord::crypto::SIGN_VERIFY_ALGO::RSA == rc.replicaMsgSigningAlgo) ? "rsa" : "eddsa";
const auto operatorMsgSignAlgo =
(concord::crypto::SIGN_VERIFY_ALGO::ECDSA == rc.operatorMsgSigningAlgo) ? "ecdsa" : "eddsa";
os << KVLOG(rc.dbCheckpointMonitorIntervalSeconds.count(),
rc.dbCheckpointDiskSpaceThreshold,
rc.enableMultiplexChannel,
Expand All @@ -608,7 +616,9 @@ inline std::ostream& operator<<(std::ostream& os, const ReplicaConfig& rc) {
rc.enablePreProcessorMemoryPool,
rc.diagnosticsServerPort,
rc.useUnifiedCertificates,
rc.kvBlockchainVersion);
rc.kvBlockchainVersion,
replicaMsgSignAlgo,
operatorMsgSignAlgo);
os << ", ";
for (auto& [param, value] : rc.config_params_) os << param << ": " << value << "\n";
return os;
Expand Down
3 changes: 2 additions & 1 deletion bftengine/src/bftengine/KeyExchangeManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,8 @@ void KeyExchangeManager::exchangeTlsKeys(const std::string& type, const SeqNum&
prev_key_pem = EdDSAHexToPem(std::make_pair(SigManager::instance()->getSelfPrivKey(), "")).first;
}

auto cert = generateSelfSignedCert(cert_path, keys.second, prev_key_pem);
auto cert =
generateSelfSignedCert(cert_path, keys.second, prev_key_pem, ReplicaConfig::instance().replicaMsgSigningAlgo);
// Now that we have generated new key pair and certificate, lets do the actual exchange on this replica
std::string pk_path = root_path + "/pk.pem";
std::fstream nec_f(pk_path);
Expand Down
3 changes: 2 additions & 1 deletion client/reconfiguration/src/default_handlers.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,8 @@ void ClientTlsKeyExchangeHandler::exchangeTlsKeys(const std::string& pkey_path,
std::string master_key = sm_->decryptFile(master_key_path_).value_or(std::string());
if (master_key.empty()) master_key = psm_.decryptFile(master_key_path_).value_or(std::string());
if (master_key.empty()) LOG_FATAL(getLogger(), "unable to read the node master key");
auto cert = generateSelfSignedCert(cert_path, new_cert_keys.second, master_key);
auto cert = generateSelfSignedCert(
cert_path, new_cert_keys.second, master_key, ReplicaConfig::instance().replicaMsgSigningAlgo);

sm_->encryptFile(pkey_path, new_cert_keys.first);
psm_.encryptFile(cert_path, cert);
Expand Down
2 changes: 1 addition & 1 deletion threshsign/src/ThresholdSignaturesTypes.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
#include "yaml_utils.hpp"
#include "Logger.hpp"
#include "string.hpp"
#include "crypto_utils.hpp"
#include "crypto.hpp"

using concord::crypto::isValidKey;

Expand Down
2 changes: 1 addition & 1 deletion tools/KeyfileIOUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
#include "KeyfileIOUtils.hpp"
#include "yaml_utils.hpp"
#include "crypto/openssl/EdDSA.hpp"
#include "crypto_utils.hpp"
#include "crypto.hpp"

using concord::crypto::isValidKey;
using bftEngine::ReplicaConfig;
Expand Down
5 changes: 2 additions & 3 deletions util/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ set(util_source_files
src/throughput.cpp
src/crypto.cpp
src/openssl/utils.cpp
src/crypto_utils.cpp
src/crypto/cryptopp/signers.cpp
src/crypto/cryptopp/verifiers.cpp
src/RawMemoryPool.cpp
Expand Down Expand Up @@ -52,13 +51,13 @@ target_link_libraries(util PUBLIC
Threads::Threads ${CRYPTOPP_LIBRARIES}
stdc++fs)
# corebft
target_include_directories(util PUBLIC include ${CRYPTOPP_INCLUDE_DIRS} ../bftengine/include/bftengine include/openssl)
target_include_directories(util PUBLIC include ${CRYPTOPP_INCLUDE_DIRS} include/openssl)

target_link_libraries(util_shared PUBLIC
Threads::Threads ${CRYPTOPP_LIBRARIES}
stdc++fs)
# corebft
target_include_directories(util_shared PUBLIC include ${CRYPTOPP_INCLUDE_DIRS} ../bftengine/include/bftengine include/openssl)
target_include_directories(util_shared PUBLIC include ${CRYPTOPP_INCLUDE_DIRS} include/openssl)

if(USE_OPENTRACING)
if(NOT DEFINED OPENTRACING_INCLUDE_DIR)
Expand Down
31 changes: 15 additions & 16 deletions util/include/crypto.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,14 @@

#pragma once

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpedantic"
#include <cryptopp/dll.h>
#include <cryptopp/pem.h>
#include <cryptopp/rsa.h>
#include <cryptopp/cryptlib.h>
#include <cryptopp/oids.h>
#pragma GCC diagnostic pop

#include <openssl/bio.h>
#include <openssl/ec.h>
#include <openssl/pem.h>
#include <openssl/x509.h>
#include <openssl/evp.h>

#include "crypto_utils.hpp"
#include <stdint.h>
#include <string>

namespace concord::crypto {

enum class KeyFormat : uint16_t { HexaDecimalStrippedFormat, PemFormat };
enum class CurveType : uint16_t { secp256k1, secp384r1 };

/**
* @brief Generates an EdDSA asymmetric key pair (private-public key pair).
*
Expand Down Expand Up @@ -91,4 +80,14 @@ std::pair<std::string, std::string> ECDSAHexToPem(const std::pair<std::string, s
* identifying the input format.
*/
KeyFormat getFormat(const std::string& key_str);

/**
* @brief Validates the key.
*
* @param keyType Key type to be validated.
* @param key Key to be validate.
* @param expectedSize Size of the key to be validated.
* @return Validation result.
*/
bool isValidKey(const std::string& keyType, const std::string& key, size_t expectedSize);
} // namespace concord::crypto
3 changes: 1 addition & 2 deletions util/include/crypto/cryptopp/signers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@

#include <memory>

#include "crypto_utils.hpp"
#include "crypto.hpp"
#include "crypto/signer.hpp"
#include "crypto/verifier.hpp"

namespace concord::crypto::cryptopp {

Expand Down
3 changes: 1 addition & 2 deletions util/include/crypto/cryptopp/verifiers.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@

#include <memory>

#include "crypto_utils.hpp"
#include "crypto/signer.hpp"
#include "crypto.hpp"
#include "crypto/verifier.hpp"

namespace concord::crypto::cryptopp {
Expand Down
2 changes: 1 addition & 1 deletion util/include/crypto/openssl/EdDSA.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
//
#pragma once

#include "crypto_utils.hpp"
#include "crypto.hpp"
#include "SerializableByteArray.hpp"
#include "openssl_crypto.hpp"

Expand Down
2 changes: 1 addition & 1 deletion util/include/crypto/openssl/EdDSASigner.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
#include "EdDSA.hpp"
#include "openssl_crypto.hpp"
#include "crypto/signer.hpp"
#include "crypto_utils.hpp"
#include "crypto.hpp"

namespace concord::crypto::openssl {

Expand Down
34 changes: 0 additions & 34 deletions util/include/crypto_utils.hpp

This file was deleted.

2 changes: 1 addition & 1 deletion util/include/cryptopp/digest_creator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

#pragma once

#include "../digest_creator.hpp"
#include "digestCreator.hpp"
#include "digest_type.hpp"

#if defined MD5_DIGEST
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion util/include/openssl/digest_creator.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
#pragma once

#include "sha_hash.hpp"
#include "../digest_creator.hpp"
#include "digestCreator.hpp"

namespace concord::util::crypto::openssl {

Expand Down
4 changes: 3 additions & 1 deletion util/include/openssl/utils.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,14 @@
#include <openssl/ec.h>
#include <openssl/x509.h>
#include <openssl/evp.h>
#include "crypto/factory.hpp"

namespace concord::crypto {

std::string generateSelfSignedCert(const std::string& origin_cert_path,
const std::string& pub_key,
const std::string& signing_key);
const std::string& signing_key,
const SIGN_VERIFY_ALGO signingAlgo);
/**
* @brief Verifies the signature of certificate 'cert' using public key 'pub_key'.
*
Expand Down
20 changes: 19 additions & 1 deletion util/src/crypto.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,22 @@
#include "Logger.hpp"
#include "assertUtils.hpp"
#include "hex_tools.h"
#include "openssl_crypto.hpp"
#include "string.hpp"
#include "crypto/openssl/EdDSA.hpp"
#include "util/filesystem.hpp"

#include <regex>
#include <utility>

#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wpedantic"
#include <cryptopp/dll.h>
#include <cryptopp/pem.h>
#include <cryptopp/rsa.h>
#include <cryptopp/cryptlib.h>
#include <cryptopp/oids.h>
#pragma GCC diagnostic pop

namespace concord::crypto {
using std::array;
using std::pair;
Expand Down Expand Up @@ -187,4 +196,13 @@ pair<string, string> generateECDSAKeyPair(const KeyFormat fmt, CurveType curve_t
KeyFormat getFormat(const std::string& key) {
return (key.find("BEGIN") != std::string::npos) ? KeyFormat::PemFormat : KeyFormat::HexaDecimalStrippedFormat;
}

bool isValidKey(const std::string& keyName, const std::string& key, size_t expectedSize) {
auto isValidHex = util::isValidHexString(key);
if ((expectedSize == 0 or (key.length() == expectedSize)) and isValidHex) {
return true;
}
throw std::runtime_error("Invalid " + keyName + " key (" + key + ") of size " + std::to_string(expectedSize) +
" bytes.");
}
} // namespace concord::crypto
27 changes: 0 additions & 27 deletions util/src/crypto_utils.cpp

This file was deleted.

11 changes: 6 additions & 5 deletions util/src/openssl/utils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,12 @@
#include "utils.hpp"
#include "Logger.hpp"
#include "util/filesystem.hpp"
#include "ReplicaConfig.hpp"

#include <regex>

namespace concord::crypto {
using std::unique_ptr;
using std::string;
using bftEngine::ReplicaConfig;
using concord::crypto::SIGN_VERIFY_ALGO;
using concord::util::openssl_utils::UniquePKEY;
using concord::util::openssl_utils::UniqueOpenSSLX509;
Expand All @@ -30,7 +28,10 @@ using concord::util::openssl_utils::OPENSSL_SUCCESS;
using concord::util::openssl_utils::OPENSSL_FAILURE;
using concord::util::openssl_utils::OPENSSL_ERROR;

string generateSelfSignedCert(const string& origin_cert_path, const string& public_key, const string& signing_key) {
string generateSelfSignedCert(const string& origin_cert_path,
const string& public_key,
const string& signing_key,
const SIGN_VERIFY_ALGO signingAlgo) {
unique_ptr<FILE, decltype(&fclose)> fp(fopen(origin_cert_path.c_str(), "r"), fclose);
if (nullptr == fp) {
LOG_ERROR(OPENSSL_LOG, "Certificate file not found, path: " << origin_cert_path);
Expand Down Expand Up @@ -72,12 +73,12 @@ string generateSelfSignedCert(const string& origin_cert_path, const string& publ
return {};
}

if (ReplicaConfig::instance().replicaMsgSigningAlgo == SIGN_VERIFY_ALGO::RSA) {
if (SIGN_VERIFY_ALGO::RSA == signingAlgo) {
if (OPENSSL_FAILURE == X509_sign(cert.get(), priv_key.get(), EVP_sha256())) {
LOG_ERROR(OPENSSL_LOG, "Failed to sign certificate using RSA private key.");
return {};
}
} else if (ReplicaConfig::instance().replicaMsgSigningAlgo == SIGN_VERIFY_ALGO::EDDSA) {
} else if (SIGN_VERIFY_ALGO::EDDSA == signingAlgo) {
if (OPENSSL_FAILURE == X509_sign(cert.get(), priv_key.get(), nullptr)) {
LOG_ERROR(OPENSSL_LOG, "Failed to sign certificate using EdDSA private key.");
return {};
Expand Down

0 comments on commit 46e96a6

Please sign in to comment.