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.

Sign up for GitHub

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

Jump to bottom

[WIP] Renew certificate fix #83

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

[WIP] Renew certificate fix #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b26f73c
[WIP] Switch to renew-cert-fix branch
mykola-kobets-epam Sep 26, 2024
85f7bd2
[iamclient] Restart iamclient after certificate renewed
mykola-kobets-epam Sep 30, 2024
88d9d89
[iamserver] Restart iamserver after certificate renew
mykola-kobets-epam Sep 30, 2024
55f2f74
[workflows] Fix CI build
mykola-kobets-epam Oct 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/build_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ jobs:
- name: Prepare
run: |
sudo apt install lcov libsofthsm2 libsystemd-dev cppcheck -y
pip install conan
python -m pip install conan
- name: Build and test
run: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/check_format.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ jobs:
- name: Run cmake-format style check
run: |
python -m pip install --upgrade pip
pip install cmake_format
python -m pip install cmake_format
find . \( \( -not -path '*/build/*' \) -name '*.cmake' -or -name 'CMakeLists.txt' \) \
-exec cmake-format --check {} +;
4 changes: 2 additions & 2 deletions cmake/AosCoreAPI.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ include(FetchContent)

FetchContent_Declare(
aoscoreapi
GIT_REPOSITORY https://github.com/aosedge/aos_core_api.git
GIT_TAG develop
GIT_REPOSITORY https://github.com/mykola-kobets-epam/aos_core_api.git
GIT_TAG renew-cert-fix
GIT_PROGRESS TRUE
GIT_SHALLOW TRUE
)
Expand Down
4 changes: 2 additions & 2 deletions cmake/AosCoreCommon.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ set(aoscorecommon_build_dir ${CMAKE_CURRENT_BINARY_DIR}/aoscorecommon)
ExternalProject_Add(
aoscorecommon
PREFIX ${aoscorecommon_build_dir}
GIT_REPOSITORY https://github.com/aosedge/aos_core_common_cpp.git
GIT_TAG develop
GIT_REPOSITORY https://github.com/mykola-kobets-epam/aos_core_common_cpp.git
GIT_TAG renew-cert-fix
GIT_PROGRESS TRUE
GIT_SHALLOW TRUE
CMAKE_ARGS -Daoscore_build_dir=${aoscore_build_dir}
Expand Down
4 changes: 2 additions & 2 deletions cmake/AosCoreLib.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ set(aoscore_build_dir ${CMAKE_CURRENT_BINARY_DIR}/aoscore)
ExternalProject_Add(
aoscore
PREFIX ${aoscore_build_dir}
GIT_REPOSITORY https://github.com/aosedge/aos_core_lib_cpp.git
GIT_TAG develop
GIT_REPOSITORY https://github.com/mykola-kobets-epam/aos_core_lib_cpp.git
GIT_TAG renew-cert-fix
GIT_PROGRESS TRUE
GIT_SHALLOW TRUE
CMAKE_ARGS -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE} -DCMAKE_INSTALL_PREFIX=${aoscore_build_dir}
Expand Down
58 changes: 48 additions & 10 deletions src/iamclient/iamclient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,19 @@
aos::crypto::x509::ProviderItf& cryptoProvider, aos::iam::nodeinfoprovider::NodeInfoProviderItf& nodeInfoProvider,
bool provisioningMode)
{
mIdentHandler = identHandler;
mNodeInfoProvider = &nodeInfoProvider;
mCertLoader = &certLoader;
mCryptoProvider = &cryptoProvider;
mProvisionManager = &provisionManager;

mStartProvisioningCmdArgs = config.mStartProvisioningCmdArgs;
mDiskEncryptionCmdArgs = config.mDiskEncryptionCmdArgs;
mFinishProvisioningCmdArgs = config.mFinishProvisioningCmdArgs;
mDeprovisionCmdArgs = config.mDeprovisionCmdArgs;
mReconnectInterval = config.mNodeReconnectInterval;
mCACert = config.mCACert;

if (provisioningMode) {
mCredentialList.push_back(grpc::InsecureChannelCredentials());
if (!config.mCACert.empty()) {
Expand All @@ -43,21 +56,19 @@
return AOS_ERROR_WRAP(aos::ErrorEnum::eInvalidArgument);
}

err = provisionManager.SubscribeCertChanged(aos::String(config.mCertStorage.c_str()), *this);

Check warning on line 59 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L59 

Added line #L59 was not covered by tests
if (!err.IsNone()) {
LOG_ERR() << "Subscribe certificate receiver failed: error=" << err.Message();

Check warning on line 61 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L61 

Added line #L61 was not covered by tests

return AOS_ERROR_WRAP(aos::ErrorEnum::eInvalidArgument);

Check warning on line 63 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L63 

Added line #L63 was not covered by tests
}

mCredentialList.push_back(
aos::common::utils::GetMTLSClientCredentials(certInfo, config.mCACert.c_str(), certLoader, cryptoProvider));

mServerURL = config.mMainIAMProtectedServerURL;
}

mIdentHandler = identHandler;
mNodeInfoProvider = &nodeInfoProvider;
mProvisionManager = &provisionManager;

mStartProvisioningCmdArgs = config.mStartProvisioningCmdArgs;
mDiskEncryptionCmdArgs = config.mDiskEncryptionCmdArgs;
mFinishProvisioningCmdArgs = config.mFinishProvisioningCmdArgs;
mDeprovisionCmdArgs = config.mDeprovisionCmdArgs;
mReconnectInterval = config.mNodeReconnectInterval;

mConnectionThread = std::thread(&IAMClient::ConnectionLoop, this);

return aos::ErrorEnum::eNone;
Expand All @@ -71,6 +82,8 @@
mShutdown = true;
mShutdownCV.notify_all();

mProvisionManager->UnsubscribeCertChanged(*this);

if (mRegisterNodeCtx) {
mRegisterNodeCtx->TryCancel();
}
Expand All @@ -85,6 +98,17 @@
* Private
**********************************************************************************************************************/

void IAMClient::OnCertChanged(const aos::iam::certhandler::CertInfo& info)

Check warning on line 101 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L101 

Added line #L101 was not covered by tests
{
std::unique_lock lock {mShutdownLock};

Check warning on line 103 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L103 

Added line #L103 was not covered by tests

mCredentialList.clear();
mCredentialList.push_back(
aos::common::utils::GetMTLSClientCredentials(info, mCACert.c_str(), *mCertLoader, *mCryptoProvider));

Check warning on line 107 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L105-L107 

Added lines #L105 - L107 were not covered by tests

mCredentialListUpdated = true;

Check warning on line 109 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L109 

Added line #L109 was not covered by tests
}

std::unique_ptr<grpc::ClientContext> IAMClient::CreateClientContext()
{
return std::make_unique<grpc::ClientContext>();
Expand Down Expand Up @@ -135,6 +159,8 @@

LOG_DBG() << "Connection established";

mCredentialListUpdated = false;
al1img marked this conversation as resolved.
Show resolved Hide resolved

return true;
}

Expand Down Expand Up @@ -196,6 +222,18 @@
if (!ok) {
break;
}

{
std::unique_lock lock {mShutdownLock};

if (mCredentialListUpdated) {
LOG_DBG() << "Credential list updated: closing connection";

Check warning on line 230 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L230 

Added line #L230 was not covered by tests
al1img marked this conversation as resolved.
Show resolved Hide resolved

mRegisterNodeCtx->TryCancel();

Check warning on line 232 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L232 

Added line #L232 was not covered by tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

And empty line before break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


break;

Check warning on line 234 in src/iamclient/iamclient.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamclient/iamclient.cpp#L234 

Added line #L234 was not covered by tests
}
}
}

} catch (const std::exception& e) {
Expand Down
21 changes: 14 additions & 7 deletions src/iamclient/iamclient.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ using PublicNodeServiceStubPtr = std::unique_ptr<PublicNodeService::StubInterfac
/**
* GRPC IAM client.
*/
class IAMClient {
class IAMClient : private aos::iam::certhandler::CertReceiverItf {
public:
/**
* Initializes IAM client instance.
Expand All @@ -57,6 +57,8 @@ class IAMClient {
~IAMClient();

private:
void OnCertChanged(const aos::iam::certhandler::CertInfo& info) override;

using StreamPtr = std::unique_ptr<
grpc::ClientReaderWriterInterface<iamanager::v5::IAMOutgoingMessages, iamanager::v5::IAMIncomingMessages>>;

Expand Down Expand Up @@ -89,15 +91,20 @@ class IAMClient {

aos::iam::identhandler::IdentHandlerItf* mIdentHandler = nullptr;
aos::iam::provisionmanager::ProvisionManagerItf* mProvisionManager = nullptr;
aos::cryptoutils::CertLoaderItf* mCertLoader = nullptr;
aos::crypto::x509::ProviderItf* mCryptoProvider = nullptr;
aos::iam::nodeinfoprovider::NodeInfoProviderItf* mNodeInfoProvider = nullptr;

std::vector<std::string> mStartProvisioningCmdArgs;
std::vector<std::string> mDiskEncryptionCmdArgs;
std::vector<std::string> mFinishProvisioningCmdArgs;
std::vector<std::string> mDeprovisionCmdArgs;
aos::common::utils::Duration mReconnectInterval;
std::string mServerURL;
std::vector<std::shared_ptr<grpc::ChannelCredentials>> mCredentialList;
bool mCredentialListUpdated = false;

std::vector<std::string> mStartProvisioningCmdArgs;
std::vector<std::string> mDiskEncryptionCmdArgs;
std::vector<std::string> mFinishProvisioningCmdArgs;
std::vector<std::string> mDeprovisionCmdArgs;
aos::common::utils::Duration mReconnectInterval;
std::string mServerURL;
std::string mCACert;

std::unique_ptr<grpc::ClientContext> mRegisterNodeCtx;
StreamPtr mStream;
Expand Down
99 changes: 74 additions & 25 deletions src/iamserver/iamserver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@
{
LOG_DBG() << "IAM Server init";

mConfig = config;
mConfig = config;
mCertLoader = &certLoader;
mCryptoProvider = &cryptoProvider;

aos::Error err;
aos::NodeInfo nodeInfo;
Expand All @@ -118,8 +120,6 @@
}

try {
std::shared_ptr<grpc::ServerCredentials> publicOpt, protectedOpt;

if (!provisioningMode) {
aos::iam::certhandler::CertInfo certInfo;

Expand All @@ -128,16 +128,21 @@
return AOS_ERROR_WRAP(err);
}

publicOpt = aos::common::utils::GetTLSServerCredentials(certInfo, certLoader, cryptoProvider);
protectedOpt = aos::common::utils::GetMTLSServerCredentials(
err = certHandler.SubscribeCertChanged(aos::String(mConfig.mCertStorage.c_str()), *this);
if (!err.IsNone()) {
return AOS_ERROR_WRAP(err);

Check warning on line 133 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L133 

Added line #L133 was not covered by tests
}

mPublicCred = aos::common::utils::GetTLSServerCredentials(certInfo, certLoader, cryptoProvider);
mProtectedCred = aos::common::utils::GetMTLSServerCredentials(
certInfo, mConfig.mCACert.c_str(), certLoader, cryptoProvider);
} else {
publicOpt = grpc::InsecureServerCredentials();
protectedOpt = grpc::InsecureServerCredentials();
mPublicCred = grpc::InsecureServerCredentials();
mProtectedCred = grpc::InsecureServerCredentials();
}

CreatePublicServer(CorrectAddress(mConfig.mIAMPublicServerURL), publicOpt);
CreateProtectedServer(CorrectAddress(mConfig.mIAMProtectedServerURL), protectedOpt);
Start();

} catch (const aos::common::utils::AosException& e) {
return e.GetError();
} catch (const std::exception& e) {
Expand Down Expand Up @@ -206,22 +211,7 @@

IAMServer::~IAMServer()
{
LOG_DBG() << "IAM Server shutdown";

mNodeController.Close();

if (mPublicServer) {
mPublicServer->Shutdown();
mPublicServer->Wait();
}

if (mProtectedServer) {
mProtectedServer->Shutdown();
mProtectedServer->Wait();
}

mPublicMessageHandler.Close();
mProtectedMessageHandler.Close();
Shutdown();
}

/***********************************************************************************************************************
Expand All @@ -242,6 +232,65 @@
return aos::ErrorEnum::eNone;
}

void IAMServer::OnCertChanged(const aos::iam::certhandler::CertInfo& info)

Check warning on line 235 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L235 

Added line #L235 was not covered by tests
{
mPublicCred = aos::common::utils::GetTLSServerCredentials(info, *mCertLoader, *mCryptoProvider);

Check warning on line 237 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L237 

Added line #L237 was not covered by tests
mProtectedCred
= aos::common::utils::GetMTLSServerCredentials(info, mConfig.mCACert.c_str(), *mCertLoader, *mCryptoProvider);

Check warning on line 239 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L239 

Added line #L239 was not covered by tests

// postpone restart so it didn't block ApplyCert
mCertChangedResult = std::async(std::launch::async, [this]() {
sleep(1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks not so good, especially with sleep 1.

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 know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably async grpc server itf might fix the problem with deferred actions with it's OnDone method.

https://grpc.io/docs/languages/cpp/callback/

Copy link
Collaborator

Choose a reason for hiding this comment

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

Will you try?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no. Takes a lot of time to implement. Left std::async

Shutdown();
Start();
});

Check warning on line 246 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L242-L246 

Added lines #L242 - L246 were not covered by tests
}

void IAMServer::Start()
{
if (mIsStarted) {
return;

Check warning on line 252 in src/iamserver/iamserver.cpp

View check run for this annotation

Codecov / codecov/patch

src/iamserver/iamserver.cpp#L252 

Added line #L252 was not covered by tests
}

LOG_DBG() << "IAM Server start";

mNodeController.Start();

mPublicMessageHandler.Start();
mProtectedMessageHandler.Start();

CreatePublicServer(CorrectAddress(mConfig.mIAMPublicServerURL), mPublicCred);
CreateProtectedServer(CorrectAddress(mConfig.mIAMProtectedServerURL), mProtectedCred);

mIsStarted = true;
}

void IAMServer::Shutdown()
{
if (!mIsStarted) {
return;
}

LOG_DBG() << "IAM Server shutdown";

mNodeController.Close();

mPublicMessageHandler.Close();
mProtectedMessageHandler.Close();

if (mPublicServer) {
mPublicServer->Shutdown();
mPublicServer->Wait();
}

if (mProtectedServer) {
mProtectedServer->Shutdown();
mProtectedServer->Wait();
}

mIsStarted = false;
}

void IAMServer::CreatePublicServer(const std::string& addr, const std::shared_ptr<grpc::ServerCredentials>& credentials)
{
LOG_DBG() << "Process create public server: URL=" << addr.c_str();
Expand Down
27 changes: 21 additions & 6 deletions src/iamserver/iamserver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@
*/
class IAMServer : public aos::iam::nodemanager::NodeInfoListenerItf,
public aos::iam::identhandler::SubjectsObserverItf,
public aos::iam::provisionmanager::ProvisionManagerCallbackItf {
public aos::iam::provisionmanager::ProvisionManagerCallbackItf,
private aos::iam::certhandler::CertReceiverItf {
public:
/**
* Constructor.
Expand Down Expand Up @@ -115,15 +116,29 @@ class IAMServer : public aos::iam::nodemanager::NodeInfoListenerItf,
// identhandler::SubjectsObserverItf interface
aos::Error SubjectsChanged(const aos::Array<aos::StaticString<aos::cSubjectIDLen>>& messages) override;

// certhandler::CertReceiverItf interface
void OnCertChanged(const aos::iam::certhandler::CertInfo& info) override;

// lifecycle routines
void Start();
void Shutdown();

// creating routines
void CreatePublicServer(const std::string& addr, const std::shared_ptr<grpc::ServerCredentials>& credentials);
void CreateProtectedServer(const std::string& addr, const std::shared_ptr<grpc::ServerCredentials>& credentials);

Config mConfig;
NodeController mNodeController;
PublicMessageHandler mPublicMessageHandler;
ProtectedMessageHandler mProtectedMessageHandler;
std::unique_ptr<grpc::Server> mPublicServer, mProtectedServer;
Config mConfig;
aos::cryptoutils::CertLoader* mCertLoader;
aos::crypto::x509::ProviderItf* mCryptoProvider;

NodeController mNodeController;
PublicMessageHandler mPublicMessageHandler;
ProtectedMessageHandler mProtectedMessageHandler;
std::unique_ptr<grpc::Server> mPublicServer, mProtectedServer;
std::shared_ptr<grpc::ServerCredentials> mPublicCred, mProtectedCred;

bool mIsStarted = false;
std::future<void> mCertChangedResult;
};

#endif
Loading