New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

Fix certificate saving after policy table update #2218

Merged

Jack-Byrne merged 5 commits into develop from fix/fix_certificate_saving_after_ptu

Jun 18, 2018

Contributor

AKalinich-Luxoft commented May 25, 2018 •

edited

Loading

Fixes #2191

This PR is ready for review.

Risk

This PR makes no API changes.

Testing Plan

Will be tested on the integration branch
Unit tests

Summary

SDL Core should update the module certificate in the local file system when a policy table update occurs. Currently SDL core is retrieving its certificate directly out of the policy table. This fix provides functionality for saving module certificate to the file system.

NOTE
This fix is closely related to #2217 and should be merged after it.
This fix should be merged after #2105 and #2068.

Changelog

Following changes were done:

Added getters for CertificatePath and KeyPath parameters in SecurityManagerSettings class to provide another components an access to these properties
Added methods for saving certificate and private key data to the files specified by CertificatePath and KeyPath keywords
CryptoManager component implementation was updated. Now this component also saves certificate data to files (if write permission allowed) after PTU

CLA

I have signed the CLA

AKalinich-Luxoft requested review from AByzhynar, mkloouo, Ypostolov, a user, SNiukalov, KVGrygoriev and APipko

May 25, 2018 12:34

AKalinich-Luxoft mentioned this pull request

Fix nonfunctional certificate/key path keywords #2217

Merged

1 task

mrapitis reviewed

View reviewed changes

src/components/security_manager/src/crypto_manager_impl.cc Outdated

                   return false;
                 }
+                // TODO(AKalinich): remove all lines below till the end of function when

Contributor

mrapitis May 25, 2018

Can we please remove the todo, as we can't merge these comments into develop?

Contributor Author

AKalinich-Luxoft May 25, 2018 •

edited

Loading

@mrapitis sure, but we should not forget to remove redundant lines of code after these two pull requests is merged
Fixed in 389a64f

KVGrygoriev reviewed

View reviewed changes

src/components/security_manager/src/crypto_manager_impl.cc Outdated

                 return *settings_;
               }
-              bool CryptoManagerImpl::set_certificate(const std::string& cert_data) {
+              bool CryptoManagerImpl::SaveCertificateData(const std::string& cert_data) {
                 LOG4CXX_AUTO_TRACE(logger_);

Contributor

KVGrygoriev May 25, 2018

Function doesn't change internal state, it should be const
It would be better function will be called with prefix "Is", IsSaveCertificateData

Contributor

AByzhynar May 25, 2018

@AKalinich-Luxoft Agree with const , disagree with prefix Is.

Contributor Author

AKalinich-Luxoft May 25, 2018

@KVGrygoriev also disagree with prefix Is
Const added in 389a64f

ghost reviewed

View reviewed changes

src/components/security_manager/src/crypto_manager_impl.cc Outdated

@@ @@ -411,4 +407,59 @@ void CryptoManagerImpl::InitCertExpTime() { @@
                 strptime("1 Jan 1970 00:00:00", "%d %b %Y %H:%M:%S", &expiration_time_);
               }
+              bool CryptoManagerImpl::SaveModuleCertificateToFile(X509* certificate) const {

ghost May 25, 2018

Same as for PR #2217 - I suppose we should free allocated resources...

Contributor Author

AKalinich-Luxoft May 25, 2018

@olohvinenko thanks, fixed in 389a64f

AByzhynar suggested changes

View reviewed changes

src/components/security_manager/src/crypto_manager_impl.cc Outdated

                 return *settings_;
               }
-              bool CryptoManagerImpl::set_certificate(const std::string& cert_data) {
+              bool CryptoManagerImpl::SaveCertificateData(const std::string& cert_data) {
                 LOG4CXX_AUTO_TRACE(logger_);

Contributor

AByzhynar May 25, 2018

@AKalinich-Luxoft Agree with const , disagree with prefix Is.

AByzhynar suggested changes

View reviewed changes

src/components/security_manager/src/crypto_manager_impl.cc Outdated

+                const std::string key_path = get_settings().module_key_path();
+                BIO* bio_key = BIO_new_file(key_path.c_str(), "w");
+                if (NULL == bio_key) {

Contributor

AByzhynar May 25, 2018

@AKalinich-Luxoft Use please if (!bio_key)

Contributor Author

AKalinich-Luxoft May 25, 2018

@AByzhynar fixed in 389a64f

src/components/security_manager/src/crypto_manager_impl.cc Outdated

+                utils::ScopeGuard bio_guard = utils::MakeGuard(BIO_free, bio_key);
+                UNUSED(bio_guard);
+                if (0 == PEM_write_bio_PrivateKey(bio_key, key, NULL, NULL, 0, NULL, NULL)) {

Contributor

AByzhynar May 25, 2018

@AKalinich-Luxoft Use please (!PEM_write_bio_PrivateKey...)
Like described here: PEM_write_bio_PrivateKey

Contributor Author

AKalinich-Luxoft May 25, 2018

@AByzhynar fixed in 389a64f

src/components/security_manager/src/crypto_manager_impl.cc Outdated

+              bool CryptoManagerImpl::SaveModuleKeyToFile(EVP_PKEY* key) const {
+                LOG4CXX_AUTO_TRACE(logger_);
+                if (NULL == key) {

Contributor

AByzhynar May 25, 2018

@AKalinich-Luxoft Same as above - if(!key)...

Contributor Author

AKalinich-Luxoft May 25, 2018

@AByzhynar fixed in 389a64f

This was referenced May 29, 2018

Feature/get system time #2105

Merged

Feature/dtls encryption #2068

Merged

AByzhynar approved these changes

View reviewed changes

SNiukalov approved these changes

View reviewed changes

AKalinich-Luxoft force-pushed the fix/fix_certificate_saving_after_ptu branch 2 times, most recently from cb0575a to 9ecb880 Compare

May 30, 2018 10:50

AStasiuk requested review from askirk, mrapitis, yang1070 and robinmk

May 30, 2018 12:30

ghost approved these changes

View reviewed changes

mrapitis approved these changes

View reviewed changes

Contributor

mrapitis commented Jun 5, 2018

@theresalech this PR is ready for review.

KVGrygoriev approved these changes

View reviewed changes

AKalinich-Luxoft added 2 commits

June 18, 2018 21:10


          Fix nonfunctional CertificatePath and KeyPath parameters in INI

9967a6a

Currently, SDL Core ignores both the CertificatePath and KeyPath
keywords that would allow the system integrator to specify certificates
for their environment, instead SDL Core only processes the certificate
provided via the policy table.

This fix makes these keywords functional. Following changes were done:
- Added getters for CertificatePath and KeyPath parameters in
SecurityManagerSettings class to provide another components an access
to these properties
- Added methods for loading certificate and private key data from the
files specified by CertificatePath and KeyPath keywords
- CryptoManager component implementation was updated. Now this component
also read certificate data from files (if they are present and accessible)
on its own initialization


          Fix cert processing and module saving after policy table update

8d2379c

SDL Core should update the module certificate in the local file system when
a policy table update occurs. Currently SDL core is retrieving its
certificate directly out of the policy table.

This fix provides functionality for saving module certificate to the file
system. Following changes were done:
- Added getters for CertificatePath and KeyPath parameters in
SecurityManagerSettings class to provide another components an access
to these properties
- Added methods for saving certificate and private key data to the
files specified by CertificatePath and KeyPath keywords
- CryptoManager component implementation was updated. Now this component
also saves certificate data to files (if write permission allowed) after
PTU


          Fixed affected mocks and UT's

6c33fca

Added new expectations for a security tests due to some
changes in the security flow

Collaborator

Jack-Byrne commented Jun 18, 2018

@AKalinich-Luxoft Please resolve merge conflicts.


          Fix leaked objects and add const

fbfc18d

Conflicts:
	src/components/security_manager/src/crypto_manager_impl.cc

Jack-Byrne reviewed

View reviewed changes

src/components/protocol_handler/src/protocol_handler_impl.cc Outdated

-                is_ptu_triggered_ = false;
-              #endif  // ENABLE_SECURITY
-              }
+              void ProtocolHandlerImpl::OnPTUFinished(const bool ptu_result) {}

Collaborator

Jack-Byrne Jun 18, 2018

Can we just remove this function if it doesn't do anything?

Contributor Author

AKalinich-Luxoft Jun 18, 2018

@JackLivio yes, it has default empty implementation in base class.
Fixed in 0a7317d


          Added logic related to certificate saving

0a7317d

Also was removed redundant logic

AKalinich-Luxoft force-pushed the fix/fix_certificate_saving_after_ptu branch from c0a84c8 to 0a7317d Compare

June 18, 2018 19:13

Contributor Author

AKalinich-Luxoft commented Jun 18, 2018

@JackLivio all conflicts was resolved

AKalinich-Luxoft mentioned this pull request

Fix V5 protocol message bson params #2235

Merged

1 task

Jack-Byrne approved these changes

View reviewed changes

Jack-Byrne merged commit a920e71 into develop

Jack-Byrne deleted the fix/fix_certificate_saving_after_ptu branch

June 18, 2018 20:39

# for free to join this conversation on GitHub. Already have an account? # to comment

Reviewers

AByzhynar AByzhynar approved these changes

mrapitis mrapitis approved these changes

Jack-Byrne Jack-Byrne approved these changes

SNiukalov SNiukalov approved these changes

KVGrygoriev KVGrygoriev approved these changes

mkloouo Awaiting requested review from mkloouo

Ypostolov Awaiting requested review from Ypostolov

APipko Awaiting requested review from APipko

askirk Awaiting requested review from askirk

yang1070 Awaiting requested review from yang1070

robinmk Awaiting requested review from robinmk

Labels

None yet