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

PKCS#12 Keystore Enhancement #1494

Merged
merged 4 commits into from
Dec 8, 2017
Merged

PKCS#12 Keystore Enhancement #1494

merged 4 commits into from
Dec 8, 2017

Conversation

AMHIT
Copy link
Contributor

@AMHIT AMHIT commented Oct 30, 2017

Added the feature to use PKCS#12 keystores as the repository for a client authentication connection

@AMHIT AMHIT mentioned this pull request Oct 30, 2017
Closed
edenhill
edenhill suggested changes Nov 7, 2017
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

This is very valuable. The Java client will support PKCS#12 keystores when using Java 9.

src/rdkafka_conf.c Outdated
@@ -471,6 +471,16 @@ static const struct rd_kafka_property rd_kafka_properties[] = {
_RK(ssl.crl_location),
"Path to CRL for verifying broker's certificate validity."
},
/* Andrea Minuto (PKCS12) */
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove your name from the comment, the git history already contains that information and it is not valuable in the source itself.

src/rdkafka_conf.c Outdated
@@ -471,6 +471,16 @@ static const struct rd_kafka_property rd_kafka_properties[] = {
_RK(ssl.crl_location),
"Path to CRL for verifying broker's certificate validity."
},
/* Andrea Minuto (PKCS12) */
{ _RK_GLOBAL, "ssl.pkcs12.keystore.location", _RK_C_STR,
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need to synchronize these property names with the Java client.
Ping @ijuma

src/rdkafka_transport.c Outdated
@@ -920,6 +920,61 @@ int rd_kafka_transport_ssl_ctx_init (rd_kafka_t *rk,
}
}

/* Andrea Minuto (PKCS12) */
if (rk->rk_conf.ssl.pkcs12.keystore_location) {
FILE *fp;
Copy link
Contributor

Choose a reason for hiding this comment

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

use 8 whitespaces for tab.

src/rdkafka_transport.c Outdated
PKCS12 *p12;

rd_kafka_dbg(rk, SECURITY, "SSL",
"Loading client's keystore file from %s",
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation should match the vertical location of the open-paren

src/rdkafka_transport.c Outdated
goto fail;
}

ERR_load_crypto_strings();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be done just once in rd_kafka_transport_ssl_init()

src/rdkafka_transport.c Outdated

r = SSL_CTX_use_certificate(ctx, cert);
if (r != 1) {
rd_snprintf(errstr, errstr_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to free the cert here?

src/rdkafka_transport.c Outdated
r = SSL_CTX_use_certificate(ctx, cert);
if (r != 1) {
rd_snprintf(errstr, errstr_size,
"ssl.pkcs12.keystore.location failed: ");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Failed to use ssl.pkcs12.keystore.location certificate: "

src/rdkafka_transport.c Outdated

if (!PKCS12_parse(p12, rk->rk_conf.ssl.pkcs12.keystore_password, &pkey, &cert, &ca)) {
rd_snprintf(errstr, errstr_size,
"Error parsing PKCS#12 file: ");
Copy link
Contributor

Choose a reason for hiding this comment

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

"Failed to parse PKCS#12 file: %s: ", ..location

src/rdkafka_transport.c Outdated

r = SSL_CTX_use_PrivateKey(ctx, pkey);
if (r != 1) {
rd_snprintf(errstr, errstr_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

"Failed to use ssl.pkcs12.keystore.location private key: "

src/rdkafka_transport_int.h Outdated
@@ -35,6 +35,9 @@
#if WITH_SSL
#include <openssl/ssl.h>
#include <openssl/err.h>
/* Andrea Minuto (PKCS12) */
Copy link
Contributor

Choose a reason for hiding this comment

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

dito

@edenhill
Copy link
Contributor

Make sure to run the tests before pushing PR, this breaks the 0004 test suite (see CI links on PR page)

@AMHIT
Copy link
Contributor Author

AMHIT commented Nov 14, 2017

I checked why the first test failed.
The only error was:
3: ### Test "0066_plugins" failed at /project/repo/checkout/tests/testcpp.h:67:test_FAIL(): ### 3: set(plugin.library.paths) failed: No such configuration property: "plugin.library.paths" 3: ### Test random seed was 91200605 ###
In my environment this test doesn’t exist and I’m sure that I never changed anything about the ‘plugin.library.paths’ config property.
Could you help me to understand how to fix the error?
Thank you very much
Andrea

@edenhill
Copy link
Contributor

I think that test failure is unrelated to your change, try rebasing your commit on latest master and see if it is still occurring.

I will push off merging this (after all comments fixed) until after the upcoming maintenance release.

@AMHIT
PKCS#12 Keystore Enhancement
c9d6b1a 
Added the feature to use PKCS#12 keystores as the repository for a client authentication connection
@AMHIT
Corrections
8db9e6f 
Corrections based on Edenhill's remarks
@AMHIT
Fixed test case 0004-conf.c
dd4d2bb 
I fixed the test case 0004-conf.c in order to manage the config changes.
edenhill
edenhill suggested changes Dec 6, 2017
View reviewed changes
Copy link
Contributor

@edenhill edenhill left a comment

Choose a reason for hiding this comment

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

two minor fixes needed then we're good to go!

src/rdkafka_conf.c Outdated
#endif /* WITH_SSL */

/* Point user in the right direction if they try to apply
* Java client SSL / JAAS properties. */
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this code instead of commenting it out

src/rdkafka_transport.c Outdated
r = SSL_CTX_use_certificate(ctx, cert);
X509_free(cert);
if (r != 1) {
rd_snprintf(errstr, errstr_size,
Copy link
Contributor

Choose a reason for hiding this comment

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

the pkey is leaked here

@AMHIT
Possible pkey leak fix
4da6389 
Fixed a possible pkey leak inside the PKCS#12 parsing and management
@edenhill edenhill merged commit 4b361e7 into confluentinc:master Dec 8, 2017
@edenhill
Copy link
Contributor

edenhill commented Dec 8, 2017

Big thanks for this @AMHIT !

@AMHIT AMHIT deleted the pkcs12-kesytore-enhancement branch January 21, 2018 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edenhill edenhill edenhill approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@AMHIT @edenhill