Renew certificate fix #159

mykola-kobets-epam · 2024-09-24T13:24:23Z

No description provided.

codecov · 2024-09-24T13:32:58Z

Codecov Report

Attention: Patch coverage is 92.57143% with 13 lines in your changes missing coverage. Please review.

Project coverage is 88.30%. Comparing base (d615de5) to head (21b58da).
Report is 125 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/iam/certmodules/pkcs11/pkcs11.cpp	73.91%	6 Missing ⚠️
src/iam/certhandler/certhandler.cpp	91.52%	5 Missing ⚠️
src/iam/certmodules/certmodule.cpp	77.77%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #159      +/-   ##
===========================================
+ Coverage    86.56%   88.30%   +1.74%     
===========================================
  Files           85      109      +24     
  Lines         7448     9310    +1862     
  Branches         0     1018    +1018     
===========================================
+ Hits          6447     8221    +1774     
- Misses        1001     1089      +88

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

al1img · 2024-10-01T12:42:41Z

include/aos/iam/certhandler.hpp

+ *
+ * 1 per client & server + cIAMCertModulesMaxCount for message proxy
+ */
+constexpr auto cIAMCertSubsMaxCount = cIAMCertModulesMaxCount * 2 + cIAMCertModulesMaxCount;


It should be private of CertHandler const. Also unclear why we need +cIAMCertModulesMaxCount

1 per client & server + cIAMCertModulesMaxCount for message proxy
Comment updated, constant moved to private section.

al1img · 2024-10-01T12:45:54Z

src/iam/certhandler/certhandler.cpp

    return ErrorEnum::eNone;
 }

+Error CertHandler::UnsubscribeCertReceiver(CertReceiverItf& certReceiver)
+{
+    LockGuard lock(mMutex);


LockGuard lock {mMutex};

updated in a separate pr for all the files.

al1img · 2024-10-01T12:52:02Z

include/aos/iam/certhandler.hpp

+     *
+     * @param info certificate info.
+     */
+    void UpdateCert(const CertInfo& info)


It should not be part of interface. The client should have access only to required methods.

interface updated

mlohvynenko · 2024-10-04T10:33:58Z

src/iam/certhandler/certhandler.cpp

+    CertInfo certInfo;
+
+    for (auto& subscription : mCertReceiverSubscriptions) {
+        auto err = certModule.GetCertificate(Array<uint8_t>(), Array<uint8_t>(), certInfo);


Didn't get why GetCertificate is called on each iteration, not before the loop

right, it's not needed, fixed. also added a filter for certype in the method

mlohvynenko · 2024-10-04T10:37:01Z

src/iam/certhandler/certhandler.cpp

+{
+    LockGuard lock(mMutex);
+
+    LOG_DBG() << "Create self signed cert: type = " << certType;


Remove spaces, so
`Create self signed cert: type='

removed in a separate commit for the file

mlohvynenko · 2024-10-04T10:37:50Z

src/iam/provisionmanager/provisionmanager.cpp

@@ -145,4 +145,18 @@ Error ProvisionManager::GetCert(
    return AOS_ERROR_WRAP(mCertHandler->GetCertificate(certType, issuer, serial, resCert));
 }

+Error ProvisionManager::SubscribeCertReceiver(const String& certType, certhandler::CertReceiverItf& certReceiver)
+{
+    LOG_DBG() << "Subscribe cert receiver: type = " << certType;


Remove spaces surrounding =

al1img · 2024-10-04T12:03:44Z

include/aos/iam/certhandler.hpp

+class CertReceiverItf {
+public:
+    /**
+     * Process certificate updates.


al1img · 2024-10-04T12:05:31Z

include/aos/iam/certhandler.hpp

+     * @param certReceiver certificate receiver.
+     * @returns Error.
+     */
+    virtual Error SubscribeCertReceiver(const String& certType, CertReceiverItf& certReceiver) = 0;


SubscribeCertChanged?

yes, it would be more uniform. changed

al1img · 2024-10-04T12:10:03Z

include/aos/iam/certhandler.hpp

@@ -221,16 +271,34 @@ class CertHandler : public CertHandlerItf, private NonCopyable {
    virtual ~CertHandler();

 private:
+    // (Two subscriptions for grpc client & server in iam/sm/cm/um) * modules count +
+    // (one subscription for grpc client in message proxy) * modules count
+    static constexpr auto cIAMCertSubsMaxCount = cIAMCertModulesMaxCount * 2 + cIAMCertModulesMaxCount;


Looks like in general we need only one subscription per storage. So cIAMCertModulesMaxCount should be enough. Also, we can't have this option as const. Add dedicated define to config.hpp with default value equals to modules max count.

I think we need cIAMCertModulesMaxCount * 2 : one per aos service for client & service, because it looks like message-proxy doesn't use IAMServer to receive certificates.
Also there is a correlation between required number of subscriptions & aos-modules, so we don't need additional config value.. the less configuration values we have - the better.

I still do not understand why we need 3*cIAMCertModulesMaxCount subscriptions. Could you provide the example?

Please add dedicated configuration into config and set it to appropriate default value. Having config value doesn't mean configuring it all the time. But it make either to change this value in the feature.

one for sm/um/cm grpc server in communication manager + one for iam grpc server in iammanager
one for grpc client in sm/um/iam manager
And I was expected that message proxy also requested certificates using iam public interface. But it looks like it's not.
So we only require 2*cert-modules-max-count.

I see. MP uses SM and IAM client certificates and probably will use UM client certificates as well. We can't predict future client configuration. That's why config option is REQUIRED. And should be set to 3*AOS_CONFIG_CERTHANDLER_MODULES_MAX_COUNT.

Please add dedicated configuration into config and set it to appropriate default value.

Added in a separate commit

mlohvynenko · 2024-10-16T10:35:29Z

src/iam/certhandler/certhandler.cpp

+    CertInfo certInfo;
+
+    auto err = module->GetCertificate(Array<uint8_t>(), Array<uint8_t>(), certInfo);
+    if (!err.IsNone()) {


Consider this change

Suggested change

if (!err.IsNone()) {

if (!err.IsNone() && !err.Is(ErrorEnum::eNotFound)) {

agreed not to do that

Signed-off-by: Mykola Kobets <[email protected]>

mykola-kobets-epam force-pushed the renew-cert-fix branch from 48a3f86 to 3498eee Compare September 24, 2024 13:29

mykola-kobets-epam changed the title ~~Renew cert fix~~ Renew certificate fix Sep 24, 2024

mykola-kobets-epam changed the title ~~Renew certificate fix~~ [WIP] Renew certificate fix Sep 24, 2024

mykola-kobets-epam force-pushed the renew-cert-fix branch 2 times, most recently from 0e89bcb to 4b10802 Compare September 27, 2024 08:15

al1img reviewed Oct 1, 2024

View reviewed changes

mykola-kobets-epam force-pushed the renew-cert-fix branch 4 times, most recently from 7b9694e to 218e614 Compare October 3, 2024 19:17

mlohvynenko reviewed Oct 4, 2024

View reviewed changes

mykola-kobets-epam force-pushed the renew-cert-fix branch from 218e614 to 685eb60 Compare October 4, 2024 11:08

al1img reviewed Oct 4, 2024

View reviewed changes

mykola-kobets-epam force-pushed the renew-cert-fix branch 3 times, most recently from 91fca3c to 278ade4 Compare October 7, 2024 12:21

al1img approved these changes Oct 7, 2024

View reviewed changes

mlohvynenko approved these changes Oct 7, 2024

View reviewed changes

mykola-kobets-epam force-pushed the renew-cert-fix branch from 278ade4 to b409986 Compare October 7, 2024 12:41

mlohvynenko reviewed Oct 16, 2024

View reviewed changes

mlohvynenko mentioned this pull request Oct 17, 2024

Reconnect on cert change aosedge/aos_core_zephyr#205

Open

MykolaSolyanko approved these changes Oct 21, 2024

View reviewed changes

mykola-kobets-epam added 5 commits October 21, 2024 14:45

[iam] Implement CertReceiver subscription in CertHandler

32a818f

Signed-off-by: Mykola Kobets <[email protected]>

[iam] Implement CertReceiver subscription in ProvisionManager

0d0204d

Signed-off-by: Mykola Kobets <[email protected]>

[all] Use uniform initialization for LockGuard

ab906ca

Signed-off-by: Mykola Kobets <[email protected]>

[iam] Fix to return new certificate for empty serial

3f04e92

Signed-off-by: Mykola Kobets <[email protected]>

[iam] Removed redundant spaces for logs in iam

0568014

Signed-off-by: Mykola Kobets <[email protected]>

[iam] Add config value for certificate change subscriptions

21b58da

Signed-off-by: Mykola Kobets <[email protected]>

mykola-kobets-epam force-pushed the renew-cert-fix branch from bd4b593 to 21b58da Compare October 21, 2024 11:46

mykola-kobets-epam changed the title ~~[WIP] Renew certificate fix~~ Renew certificate fix Oct 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renew certificate fix #159

Renew certificate fix #159

mykola-kobets-epam commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

al1img Oct 1, 2024

mykola-kobets-epam Oct 3, 2024 •

edited

Loading

al1img Oct 1, 2024

mykola-kobets-epam Oct 3, 2024

al1img Oct 1, 2024

mykola-kobets-epam Oct 3, 2024

mlohvynenko Oct 4, 2024

mykola-kobets-epam Oct 4, 2024

mlohvynenko Oct 4, 2024

mykola-kobets-epam Oct 4, 2024

mlohvynenko Oct 4, 2024

mykola-kobets-epam Oct 7, 2024

al1img Oct 4, 2024

mykola-kobets-epam Oct 4, 2024

al1img Oct 4, 2024

mykola-kobets-epam Oct 4, 2024

al1img Oct 4, 2024

mykola-kobets-epam Oct 4, 2024 •

edited

Loading

al1img Oct 7, 2024

mykola-kobets-epam Oct 7, 2024

al1img Oct 7, 2024 •

edited

Loading

mykola-kobets-epam Oct 7, 2024 •

edited

Loading

mlohvynenko Oct 16, 2024

mykola-kobets-epam Oct 21, 2024

	if (!err.IsNone()) {
	if (!err.IsNone() && !err.Is(ErrorEnum::eNotFound)) {

Renew certificate fix #159

Are you sure you want to change the base?

Renew certificate fix #159

Conversation

mykola-kobets-epam commented Sep 24, 2024

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

mykola-kobets-epam Oct 3, 2024 • 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

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

mykola-kobets-epam Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

al1img Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

mykola-kobets-epam Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Sep 24, 2024 •

edited

Loading

mykola-kobets-epam Oct 3, 2024 •

edited

Loading

mykola-kobets-epam Oct 4, 2024 •

edited

Loading

al1img Oct 7, 2024 •

edited

Loading

mykola-kobets-epam Oct 7, 2024 •

edited

Loading