MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

asdaraujo · 2019-10-22T14:56:48Z

There are two issues fixed in this commit:

SSLContextService was ignoring additional certs in the P12
file, which was preventing the server to establish trust of
the client certificate when it was signed by an intermediate
CA.
TLSSocket only supported PEM certificates. It has been changed
to delegate the handling of cert files to the SSLContextService.

Thank you for submitting a contribution to Apache NiFi - MiNiFi C++.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

Is there a JIRA ticket associated with this PR? Is it referenced
in the commit message?
Does your PR title start with MINIFICPP-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.
Has your PR been rebased against the latest commit within the target branch (typically master)?
Is your initial contribution a single, squashed commit?

For code changes:

If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
If applicable, have you updated the LICENSE file?
If applicable, have you updated the NOTICE file?

For documentation related changes:

Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check travis-ci for build issues and submit an update to your PR as soon as possible.

asdaraujo · 2019-10-22T15:00:34Z

I've tested the changes with PEM and PKCS12 certs, signed directly with a root CA as well as with an intermediate and all the combinations work well.

arpadboda · 2019-10-22T22:30:18Z

libminifi/src/controllers/SSLContextService.cpp

@@ -80,7 +82,16 @@ bool SSLContextService::configure_ssl_context(SSL_CTX *ctx) {
        X509_free(cert);


My gut feeling says that whenever we do this we leak the page pointed by "ca".
PKCS12_parse allocates that as well, so I think we need to free it.

I would also prefer using scope guards for the resources here to avoid leaks.

Changed the code to use scope guards.

arpadboda · 2019-10-22T22:31:01Z

libminifi/src/controllers/SSLContextService.cpp

+			  logging::LOG_ERROR(logger_) << "Failed to set additional certificate from  " << certificate << ", " << getLatestOpenSSLErrorString();
+			  EVP_PKEY_free(pkey);
+			  X509_free(cacert);
+			  return false;


So we iterate through "ca" and free one of the certs in case of error and leave the rest. This definitely feels wrong.

Fixed this with scope guard as well

arpadboda

Look good, nice work, thanks!

The changes you added are indented with tabs instead of 2 spaces. Please fix that (preferably also in your IDE), otherwise I’m happy to merge.

arpadboda · 2019-10-23T09:21:20Z

@asdaraujo : according to CI it seems security related tests fail with segfault.

You can execute them with the following command:

ctest --parallel 12

Please make sure they pass!

asdaraujo · 2019-10-24T05:33:08Z

Thanks, @arpadboda ! Fixed the tabs and the segfault.

arpadboda · 2019-10-24T11:55:58Z

Looks good, will merge soon, thanks!

@asdaraujo : next time please take a look at linter errors! (You can run linter by executing "make linter"). Will fix the current ones when merging.

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:101:  An else should appear on the same line as the preceding }  [whitespace/newline] [4]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:101:  If an else has a brace on one side, it should have it on both  [readability/braces] [5]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:31:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:73:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:87:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:88:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:89:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:90:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:91:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:92:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:93:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

/Users/travis/build/apache/nifi-minifi-cpp/libminifi/src//controllers/SSLContextService.cpp:94:  Unexpected \r (^M) found; better to use only \n  [whitespace/newline] [1]

There are two issues fixed in this commit: * SSLContextService was ignoring additional certs in the P12 file, which was preventing the server to establish trust of the client certificate when it was signed by an intermediate CA. * TLSSocket only supported PEM certificates. It has been changed to delegate the handling of cert files to the SSLContextService.

asdaraujo · 2019-10-24T15:14:37Z

Thanks, @arpadboda, and thanks for the linter tip. I'm sending an update with the linter errors fixed.

arpadboda · 2019-10-28T17:15:05Z

Merged, just forgot to close in scope of that commit ( aabcbee )

Manually closing now.

Thanks again @asdaraujo !

arpadboda reviewed Oct 22, 2019

View reviewed changes

asdaraujo force-pushed the tls-p12-fix branch from 6781bff to 05aa249 Compare October 23, 2019 05:50

arpadboda reviewed Oct 23, 2019

View reviewed changes

asdaraujo force-pushed the tls-p12-fix branch from 05aa249 to d299547 Compare October 24, 2019 05:21

asdaraujo force-pushed the tls-p12-fix branch from d299547 to 64c9927 Compare October 24, 2019 15:15

arpadboda closed this Oct 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

asdaraujo commented Oct 22, 2019

asdaraujo commented Oct 22, 2019

arpadboda Oct 22, 2019 •

edited

Loading

asdaraujo Oct 23, 2019

arpadboda Oct 22, 2019

asdaraujo Oct 23, 2019

arpadboda left a comment

arpadboda commented Oct 23, 2019

asdaraujo commented Oct 24, 2019

arpadboda commented Oct 24, 2019 •

edited

Loading

asdaraujo commented Oct 24, 2019

arpadboda commented Oct 28, 2019

		@@ -80,7 +82,16 @@ bool SSLContextService::configure_ssl_context(SSL_CTX *ctx) {
		X509_free(cert);

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

Conversation

asdaraujo commented Oct 22, 2019

For all changes:

For code changes:

For documentation related changes:

Note:

asdaraujo commented Oct 22, 2019

arpadboda Oct 22, 2019 • edited Loading

Choose a reason for hiding this comment

asdaraujo Oct 23, 2019

Choose a reason for hiding this comment

arpadboda Oct 22, 2019

Choose a reason for hiding this comment

asdaraujo Oct 23, 2019

Choose a reason for hiding this comment

arpadboda left a comment

Choose a reason for hiding this comment

arpadboda commented Oct 23, 2019

asdaraujo commented Oct 24, 2019

arpadboda commented Oct 24, 2019 • edited Loading

asdaraujo commented Oct 24, 2019

arpadboda commented Oct 28, 2019

arpadboda Oct 22, 2019 •

edited

Loading

arpadboda commented Oct 24, 2019 •

edited

Loading