You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by ma...@apache.org on 2016/03/30 21:27:30 UTC
svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c
xdocs/miscellaneous/changelog.xml
Author: markt
Date: Wed Mar 30 19:27:29 2016
New Revision: 1737154
URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
Log:
Add support for obtaining the certificate chain from a Java keystore
Modified:
tomcat/native/trunk/native/src/sslcontext.c
tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
Modified: tomcat/native/trunk/native/src/sslcontext.c
URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
==============================================================================
--- tomcat/native/trunk/native/src/sslcontext.c (original)
+++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
@@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
certs = d2i_X509(NULL, &tmp, lengthOfCert);
if (certs == NULL) {
ERR_error_string(ERR_get_error(), err);
- tcn_Throw(e, "Error reading certificat (%s)", err);
+ tcn_Throw(e, "Error reading certificate (%s)", err);
rv = JNI_FALSE;
goto cleanup;
}
@@ -1119,6 +1119,50 @@ cleanup:
free(cert);
return rv;
}
+
+TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
+ jbyteArray javaCert)
+{
+ jsize lengthOfCert;
+ unsigned char* cert;
+ X509 * certs;
+ EVP_PKEY * evp;
+ const unsigned char *tmp;
+ BIO * bio;
+
+ tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
+ jboolean rv = JNI_TRUE;
+ char err[256];
+
+ /* we get the cert contents into a byte array */
+ jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
+ lengthOfCert = (*e)->GetArrayLength(e, javaCert);
+ cert = malloc(lengthOfCert);
+ memcpy(cert, bufferPtr, lengthOfCert);
+ (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
+
+ UNREFERENCED(o);
+ TCN_ASSERT(ctx != 0);
+
+ tmp = (const unsigned char *)cert;
+ certs = d2i_X509(NULL, &tmp, lengthOfCert);
+ if (certs == NULL) {
+ ERR_error_string(ERR_get_error(), err);
+ tcn_Throw(e, "Error reading certificate (%s)", err);
+ rv = JNI_FALSE;
+ goto cleanup;
+ }
+
+ if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
+ ERR_error_string(ERR_get_error(), err);
+ tcn_Throw(e, "Error setting certificate (%s)", err);
+ rv = JNI_FALSE;
+ }
+
+cleanup:
+ free(cert);
+ return rv;
+}
static int ssl_array_index(apr_array_header_t *array,
const char *s)
Modified: tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
URL: http://svn.apache.org/viewvc/tomcat/native/trunk/xdocs/miscellaneous/changelog.xml?rev=1737154&r1=1737153&r2=1737154&view=diff
==============================================================================
--- tomcat/native/trunk/xdocs/miscellaneous/changelog.xml (original)
+++ tomcat/native/trunk/xdocs/miscellaneous/changelog.xml Wed Mar 30 19:27:29 2016
@@ -54,6 +54,9 @@
<fix>
Fix some compiler warnings in native ssl code. (rjung)
</fix>
+ <add>
+ Add support for using Java keystores for certificate chains. (markt)
+ </add>
</changelog>
</section>
<section name="Changes in 1.2.5">
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Mark,
On 3/30/16 3:33 PM, Mark Thomas wrote:
> On 30/03/2016 20:27, markt@apache.org wrote:
>> Author: markt
>> Date: Wed Mar 30 19:27:29 2016
>> New Revision: 1737154
>>
>> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
>> Log:
>> Add support for obtaining the certificate chain from a Java keystore
>
> This needs a review by someone who knows C better than I do.
See below.
> The implementation is essentially a copy/paste of setCertificateRaw with
> what looked to be the right changes to remove the unnecessary private
> key code and to call the right OpenSSL method to set the chain.
>
> It does work - in that SSL Labs sees the full chain - but the code may
> well be terrible. I wouldn't be surprised if it leaked memory.
>
> Once this has been reviewed and fixed, I plan to do a tc-native release
> so we can up the minimum required version in 9.0.x and 8.5.x and ship
> the next releases with the necessary tc-native code to use this feature.
>
> Mark
>
>
>>
>> Modified:
>> tomcat/native/trunk/native/src/sslcontext.c
>> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>>
>> Modified: tomcat/native/trunk/native/src/sslcontext.c
>> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
>> ==============================================================================
>> --- tomcat/native/trunk/native/src/sslcontext.c (original)
>> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
>> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
>> certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> if (certs == NULL) {
>> ERR_error_string(ERR_get_error(), err);
>> - tcn_Throw(e, "Error reading certificat (%s)", err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> rv = JNI_FALSE;
>> goto cleanup;
>> }
>> @@ -1119,6 +1119,50 @@ cleanup:
>> free(cert);
>> return rv;
>> }
>> +
>> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
>> + jbyteArray javaCert)
>> +{
>> + jsize lengthOfCert;
>> + unsigned char* cert;
>> + X509 * certs;
>> + EVP_PKEY * evp;
>> + const unsigned char *tmp;
>> + BIO * bio;
>> +
>> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
>> + jboolean rv = JNI_TRUE;
>> + char err[256];
>> +
>> + /* we get the cert contents into a byte array */
>> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
>> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
>> + cert = malloc(lengthOfCert);
>> + memcpy(cert, bufferPtr, lengthOfCert);
>> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
Since bufferPtr is the byte array you want to use, you can probably just
use that directly for the call to d2i_x509(). I think the
malloc/memcpy/free is not necessary.
Obviously, don't call ReleaseByteArray until after calling d2i_509.
>> +
>> + UNREFERENCED(o);
>> + TCN_ASSERT(ctx != 0);
>> +
>> + tmp = (const unsigned char *)cert;
>> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> + if (certs == NULL) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> + rv = JNI_FALSE;
>> + goto cleanup;
>> + }
>> +
>> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error setting certificate (%s)", err);
>> + rv = JNI_FALSE;
>> + }
>> +
>> +cleanup:
>> + free(cert);
>> + return rv;
>> +}
You could probably avoid the label/goto with a slight more complicated
conditional structure, but I don't see a particular reason to do so.
-chris
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2016-03-30 22:33 GMT+03:00 Mark Thomas <ma...@apache.org>:
> On 30/03/2016 20:27, markt@apache.org wrote:
>> Author: markt
>> Date: Wed Mar 30 19:27:29 2016
>> New Revision: 1737154
>>
>> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
>> Log:
>> Add support for obtaining the certificate chain from a Java keystore
>
> This needs a review by someone who knows C better than I do.
>
> The implementation is essentially a copy/paste of setCertificateRaw with
> what looked to be the right changes to remove the unnecessary private
> key code and to call the right OpenSSL method to set the chain.
>
> It does work - in that SSL Labs sees the full chain - but the code may
> well be terrible. I wouldn't be surprised if it leaked memory.
>
> Once this has been reviewed and fixed, I plan to do a tc-native release
> so we can up the minimum required version in 9.0.x and 8.5.x and ship
> the next releases with the necessary tc-native code to use this feature.
>
There is second half of this file (sslcontext.c) that defines stubs
for all these methods for the case when the library is compiled
without OpenSSL.
Neither setCertificateRaw method, nor the new one are declared there.
#else
/* OpenSSL is not supported.
* Create empty stubs.
*/
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 31/03/2016 00:37, Christopher Schultz wrote:
> Chuck,
>
> On 3/30/16 5:10 PM, Caldarale, Charles R wrote:
>>> From: Christopher Schultz
>>> Subject: RE: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
>>
>>> Since bufferPtr is the byte array you want to use, you can probably just
>>> use that directly for the call to d2i_x509(). I think the
>>> malloc/memcpy/free is not necessary.
>>
>> Does calling d2i_X509() have the potential to block for any
>> significant length of time? If so, the byte array would be pinned in
>> the heap for the duration, which may impact GC.
>
> Good question. I assumed it was a conversion routine (foo2bar) and was
> just going to be converting from byte array to an internal
> representation of the X509 certificate.
>
> My justification for avoiding the malloc/memcpy/free was to reduce
> memory churn and improve performance, but you're right: if d2i_X509 is
> likely to take any significant amount of time, that outstanding pined
> array can cause a slowdown in other areas.
>
> In either case, I believe correctness is maintained so it will all come
> down to performance. I'll have to read about d2i_X509 and maybe read the
> implementation (which is likely to cause nightmares) to see.
I tried - and failed - to find the implementation. Since I have no
better information that the method I copied, I'm going to leave this as
is for now.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Christopher Schultz <ch...@christopherschultz.net>.
Chuck,
On 3/30/16 5:10 PM, Caldarale, Charles R wrote:
>> From: Christopher Schultz
>> Subject: RE: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
>
>> Since bufferPtr is the byte array you want to use, you can probably just
>> use that directly for the call to d2i_x509(). I think the
>> malloc/memcpy/free is not necessary.
>
> Does calling d2i_X509() have the potential to block for any
> significant length of time? If so, the byte array would be pinned in
> the heap for the duration, which may impact GC.
Good question. I assumed it was a conversion routine (foo2bar) and was
just going to be converting from byte array to an internal
representation of the X509 certificate.
My justification for avoiding the malloc/memcpy/free was to reduce
memory churn and improve performance, but you're right: if d2i_X509 is
likely to take any significant amount of time, that outstanding pined
array can cause a slowdown in other areas.
In either case, I believe correctness is maintained so it will all come
down to performance. I'll have to read about d2i_X509 and maybe read the
implementation (which is likely to cause nightmares) to see.
-chris
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Christopher Schultz
> Subject: RE: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
> Since bufferPtr is the byte array you want to use, you can probably just
> use that directly for the call to d2i_x509(). I think the
> malloc/memcpy/free is not necessary.
Does calling d2i_X509() have the potential to block for any significant length of time? If so, the byte array would be pinned in the heap for the duration, which may impact GC.
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 30/03/2016 21:43, Mark Thomas wrote:
> On 30/03/2016 21:31, Caldarale, Charles R wrote:
>>> From: Mark Thomas [mailto:markt@apache.org]
>>> Subject: Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
>>
>>> The implementation is essentially a copy/paste of setCertificateRaw with
>>> what looked to be the right changes to remove the unnecessary private
>>> key code and to call the right OpenSSL method to set the chain.
>>
>>> It does work - in that SSL Labs sees the full chain - but the code may
>>> well be terrible. I wouldn't be surprised if it leaked memory.
>>
>> I don't see any obvious leaks (although I'm unfamiliar with OpenSSL semantics),
>
> ACK. Thanks.
>
>> but using a goto is generally frowned upon. Better code might be something like this:
>
> My defence is that I was copying the style of the previous method. If we
> fix one, we should fix both. I'll see what I can do.
It is easy to drop the goto in this method. The previous method not so
much so I did just fix the one.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 30/03/2016 21:31, Caldarale, Charles R wrote:
>> From: Mark Thomas [mailto:markt@apache.org]
>> Subject: Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
>
>> The implementation is essentially a copy/paste of setCertificateRaw with
>> what looked to be the right changes to remove the unnecessary private
>> key code and to call the right OpenSSL method to set the chain.
>
>> It does work - in that SSL Labs sees the full chain - but the code may
>> well be terrible. I wouldn't be surprised if it leaked memory.
>
> I don't see any obvious leaks (although I'm unfamiliar with OpenSSL semantics),
ACK. Thanks.
> but using a goto is generally frowned upon. Better code might be something like this:
My defence is that I was copying the style of the previous method. If we
fix one, we should fix both. I'll see what I can do.
Cheers,
Mark
> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
> + if (certs == NULL) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> + rv = JNI_FALSE;
> + } else if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error setting certificate (%s)", err);
> + rv = JNI_FALSE;
> + }
> +
> + free(cert);
> + return rv;
>
> - Chuck
>
>
> THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
>
>
> -----Original Message-----
> From: Mark Thomas [mailto:markt@apache.org]
> Sent: 2016 March 30, Wednesday 14:33
> To: dev@tomcat.apache.org
> Subject: Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
>
> On 30/03/2016 20:27, markt@apache.org wrote:
>> Author: markt
>> Date: Wed Mar 30 19:27:29 2016
>> New Revision: 1737154
>>
>> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
>> Log:
>> Add support for obtaining the certificate chain from a Java keystore
>
> This needs a review by someone who knows C better than I do.
>
> The implementation is essentially a copy/paste of setCertificateRaw with
> what looked to be the right changes to remove the unnecessary private
> key code and to call the right OpenSSL method to set the chain.
>
> It does work - in that SSL Labs sees the full chain - but the code may
> well be terrible. I wouldn't be surprised if it leaked memory.
>
> Once this has been reviewed and fixed, I plan to do a tc-native release
> so we can up the minimum required version in 9.0.x and 8.5.x and ship
> the next releases with the necessary tc-native code to use this feature.
>
> Mark
>
>
>>
>> Modified:
>> tomcat/native/trunk/native/src/sslcontext.c
>> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>>
>> Modified: tomcat/native/trunk/native/src/sslcontext.c
>> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
>> ==============================================================================
>> --- tomcat/native/trunk/native/src/sslcontext.c (original)
>> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
>> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
>> certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> if (certs == NULL) {
>> ERR_error_string(ERR_get_error(), err);
>> - tcn_Throw(e, "Error reading certificat (%s)", err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> rv = JNI_FALSE;
>> goto cleanup;
>> }
>> @@ -1119,6 +1119,50 @@ cleanup:
>> free(cert);
>> return rv;
>> }
>> +
>> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
>> + jbyteArray javaCert)
>> +{
>> + jsize lengthOfCert;
>> + unsigned char* cert;
>> + X509 * certs;
>> + EVP_PKEY * evp;
>> + const unsigned char *tmp;
>> + BIO * bio;
>> +
>> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
>> + jboolean rv = JNI_TRUE;
>> + char err[256];
>> +
>> + /* we get the cert contents into a byte array */
>> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
>> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
>> + cert = malloc(lengthOfCert);
>> + memcpy(cert, bufferPtr, lengthOfCert);
>> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
>> +
>> + UNREFERENCED(o);
>> + TCN_ASSERT(ctx != 0);
>> +
>> + tmp = (const unsigned char *)cert;
>> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> + if (certs == NULL) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> + rv = JNI_FALSE;
>> + goto cleanup;
>> + }
>> +
>> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error setting certificate (%s)", err);
>> + rv = JNI_FALSE;
>> + }
>> +
>> +cleanup:
>> + free(cert);
>> + return rv;
>> +}
>>
>> static int ssl_array_index(apr_array_header_t *array,
>> const char *s)
>>
>> Modified: tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/xdocs/miscellaneous/changelog.xml?rev=1737154&r1=1737153&r2=1737154&view=diff
>> ==============================================================================
>> --- tomcat/native/trunk/xdocs/miscellaneous/changelog.xml (original)
>> +++ tomcat/native/trunk/xdocs/miscellaneous/changelog.xml Wed Mar 30 19:27:29 2016
>> @@ -54,6 +54,9 @@
>> <fix>
>> Fix some compiler warnings in native ssl code. (rjung)
>> </fix>
>> + <add>
>> + Add support for using Java keystores for certificate chains. (markt)
>> + </add>
>> </changelog>
>> </section>
>> <section name="Changes in 1.2.5">
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
>> For additional commands, e-mail: dev-help@tomcat.apache.org
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
RE: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by "Caldarale, Charles R" <Ch...@unisys.com>.
> From: Mark Thomas [mailto:markt@apache.org]
> Subject: Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
> The implementation is essentially a copy/paste of setCertificateRaw with
> what looked to be the right changes to remove the unnecessary private
> key code and to call the right OpenSSL method to set the chain.
> It does work - in that SSL Labs sees the full chain - but the code may
> well be terrible. I wouldn't be surprised if it leaked memory.
I don't see any obvious leaks (although I'm unfamiliar with OpenSSL semantics), but using a goto is generally frowned upon. Better code might be something like this:
+ certs = d2i_X509(NULL, &tmp, lengthOfCert);
+ if (certs == NULL) {
+ ERR_error_string(ERR_get_error(), err);
+ tcn_Throw(e, "Error reading certificate (%s)", err);
+ rv = JNI_FALSE;
+ } else if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
+ ERR_error_string(ERR_get_error(), err);
+ tcn_Throw(e, "Error setting certificate (%s)", err);
+ rv = JNI_FALSE;
+ }
+
+ free(cert);
+ return rv;
- Chuck
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers.
-----Original Message-----
From: Mark Thomas [mailto:markt@apache.org]
Sent: 2016 March 30, Wednesday 14:33
To: dev@tomcat.apache.org
Subject: Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
On 30/03/2016 20:27, markt@apache.org wrote:
> Author: markt
> Date: Wed Mar 30 19:27:29 2016
> New Revision: 1737154
>
> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
> Log:
> Add support for obtaining the certificate chain from a Java keystore
This needs a review by someone who knows C better than I do.
The implementation is essentially a copy/paste of setCertificateRaw with
what looked to be the right changes to remove the unnecessary private
key code and to call the right OpenSSL method to set the chain.
It does work - in that SSL Labs sees the full chain - but the code may
well be terrible. I wouldn't be surprised if it leaked memory.
Once this has been reviewed and fixed, I plan to do a tc-native release
so we can up the minimum required version in 9.0.x and 8.5.x and ship
the next releases with the necessary tc-native code to use this feature.
Mark
>
> Modified:
> tomcat/native/trunk/native/src/sslcontext.c
> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>
> Modified: tomcat/native/trunk/native/src/sslcontext.c
> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
> ==============================================================================
> --- tomcat/native/trunk/native/src/sslcontext.c (original)
> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
> certs = d2i_X509(NULL, &tmp, lengthOfCert);
> if (certs == NULL) {
> ERR_error_string(ERR_get_error(), err);
> - tcn_Throw(e, "Error reading certificat (%s)", err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> rv = JNI_FALSE;
> goto cleanup;
> }
> @@ -1119,6 +1119,50 @@ cleanup:
> free(cert);
> return rv;
> }
> +
> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
> + jbyteArray javaCert)
> +{
> + jsize lengthOfCert;
> + unsigned char* cert;
> + X509 * certs;
> + EVP_PKEY * evp;
> + const unsigned char *tmp;
> + BIO * bio;
> +
> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
> + jboolean rv = JNI_TRUE;
> + char err[256];
> +
> + /* we get the cert contents into a byte array */
> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
> + cert = malloc(lengthOfCert);
> + memcpy(cert, bufferPtr, lengthOfCert);
> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
> +
> + UNREFERENCED(o);
> + TCN_ASSERT(ctx != 0);
> +
> + tmp = (const unsigned char *)cert;
> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
> + if (certs == NULL) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> + rv = JNI_FALSE;
> + goto cleanup;
> + }
> +
> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error setting certificate (%s)", err);
> + rv = JNI_FALSE;
> + }
> +
> +cleanup:
> + free(cert);
> + return rv;
> +}
>
> static int ssl_array_index(apr_array_header_t *array,
> const char *s)
>
> Modified: tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/xdocs/miscellaneous/changelog.xml?rev=1737154&r1=1737153&r2=1737154&view=diff
> ==============================================================================
> --- tomcat/native/trunk/xdocs/miscellaneous/changelog.xml (original)
> +++ tomcat/native/trunk/xdocs/miscellaneous/changelog.xml Wed Mar 30 19:27:29 2016
> @@ -54,6 +54,9 @@
> <fix>
> Fix some compiler warnings in native ssl code. (rjung)
> </fix>
> + <add>
> + Add support for using Java keystores for certificate chains. (markt)
> + </add>
> </changelog>
> </section>
> <section name="Changes in 1.2.5">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 30/03/2016 20:27, markt@apache.org wrote:
> Author: markt
> Date: Wed Mar 30 19:27:29 2016
> New Revision: 1737154
>
> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
> Log:
> Add support for obtaining the certificate chain from a Java keystore
This needs a review by someone who knows C better than I do.
The implementation is essentially a copy/paste of setCertificateRaw with
what looked to be the right changes to remove the unnecessary private
key code and to call the right OpenSSL method to set the chain.
It does work - in that SSL Labs sees the full chain - but the code may
well be terrible. I wouldn't be surprised if it leaked memory.
Once this has been reviewed and fixed, I plan to do a tc-native release
so we can up the minimum required version in 9.0.x and 8.5.x and ship
the next releases with the necessary tc-native code to use this feature.
Mark
>
> Modified:
> tomcat/native/trunk/native/src/sslcontext.c
> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>
> Modified: tomcat/native/trunk/native/src/sslcontext.c
> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
> ==============================================================================
> --- tomcat/native/trunk/native/src/sslcontext.c (original)
> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
> certs = d2i_X509(NULL, &tmp, lengthOfCert);
> if (certs == NULL) {
> ERR_error_string(ERR_get_error(), err);
> - tcn_Throw(e, "Error reading certificat (%s)", err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> rv = JNI_FALSE;
> goto cleanup;
> }
> @@ -1119,6 +1119,50 @@ cleanup:
> free(cert);
> return rv;
> }
> +
> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
> + jbyteArray javaCert)
> +{
> + jsize lengthOfCert;
> + unsigned char* cert;
> + X509 * certs;
> + EVP_PKEY * evp;
> + const unsigned char *tmp;
> + BIO * bio;
> +
> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
> + jboolean rv = JNI_TRUE;
> + char err[256];
> +
> + /* we get the cert contents into a byte array */
> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
> + cert = malloc(lengthOfCert);
> + memcpy(cert, bufferPtr, lengthOfCert);
> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
> +
> + UNREFERENCED(o);
> + TCN_ASSERT(ctx != 0);
> +
> + tmp = (const unsigned char *)cert;
> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
> + if (certs == NULL) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> + rv = JNI_FALSE;
> + goto cleanup;
> + }
> +
> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error setting certificate (%s)", err);
> + rv = JNI_FALSE;
> + }
> +
> +cleanup:
> + free(cert);
> + return rv;
> +}
>
> static int ssl_array_index(apr_array_header_t *array,
> const char *s)
>
> Modified: tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/xdocs/miscellaneous/changelog.xml?rev=1737154&r1=1737153&r2=1737154&view=diff
> ==============================================================================
> --- tomcat/native/trunk/xdocs/miscellaneous/changelog.xml (original)
> +++ tomcat/native/trunk/xdocs/miscellaneous/changelog.xml Wed Mar 30 19:27:29 2016
> @@ -54,6 +54,9 @@
> <fix>
> Fix some compiler warnings in native ssl code. (rjung)
> </fix>
> + <add>
> + Add support for using Java keystores for certificate chains. (markt)
> + </add>
> </changelog>
> </section>
> <section name="Changes in 1.2.5">
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Rémy Maucherat <re...@apache.org>.
2016-03-30 15:43 GMT-05:00 Mark Thomas <ma...@apache.org>:
> > This implementation matches the code of setCertificateRaw(), I see no
> > obvious errors (just high-level review comparing the two methods).
>
> Thanks. I'll look at implementing these tomorrow.
>
> > I wonder about "idx" argument in setCertificateRaw() - the case of
> > using several certificate types in parallel (RSA, DSA, ECC -- see
> > SSL_AIDX_DSA etc. in include/ssl_private.h and Javadoc for this
> > method).
> >
> > I think that each certificate has its own chain going up to different
> > root CA certificate.
>
> No. They have to have the same chain. That is a 'feature' of OpenSSL.
>
> I can confirm that since I had a look because the init code looked a bit
odd. The different types for a single certificate have to share the same
chain. OTOH, the feature wouldn't be *so* useful either.
Rémy
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Mark Thomas <ma...@apache.org>.
On 30/03/2016 21:41, Konstantin Kolinko wrote:
> 2016-03-30 22:27 GMT+03:00 <ma...@apache.org>:
>> Author: markt
>> Date: Wed Mar 30 19:27:29 2016
>> New Revision: 1737154
>>
>> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
>> Log:
>> Add support for obtaining the certificate chain from a Java keystore
>>
>> Modified:
>> tomcat/native/trunk/native/src/sslcontext.c
>> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>>
>> Modified: tomcat/native/trunk/native/src/sslcontext.c
>> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
>> ==============================================================================
>> --- tomcat/native/trunk/native/src/sslcontext.c (original)
>> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
>> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
>> certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> if (certs == NULL) {
>> ERR_error_string(ERR_get_error(), err);
>> - tcn_Throw(e, "Error reading certificat (%s)", err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> rv = JNI_FALSE;
>> goto cleanup;
>> }
>> @@ -1119,6 +1119,50 @@ cleanup:
>> free(cert);
>> return rv;
>> }
>> +
>> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
>> + jbyteArray javaCert)
>> +{
>> + jsize lengthOfCert;
>> + unsigned char* cert;
>> + X509 * certs;
>> + EVP_PKEY * evp;
>> + const unsigned char *tmp;
>> + BIO * bio;
>
> The "BIO" and "evp" variables are declared, but never used. Can be removed.
>
>> +
>> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
>> + jboolean rv = JNI_TRUE;
>> + char err[256];
>> +
>> + /* we get the cert contents into a byte array */
>> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
>> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
>> + cert = malloc(lengthOfCert);
>> + memcpy(cert, bufferPtr, lengthOfCert);
>> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
>> +
>> + UNREFERENCED(o);
>> + TCN_ASSERT(ctx != 0);
>> +
>> + tmp = (const unsigned char *)cert;
>> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
>> + if (certs == NULL) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error reading certificate (%s)", err);
>> + rv = JNI_FALSE;
>> + goto cleanup;
>> + }
>> +
>> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
>> + ERR_error_string(ERR_get_error(), err);
>> + tcn_Throw(e, "Error setting certificate (%s)", err);
>
> "Error adding certificate"
>
>> + rv = JNI_FALSE;
>> + }
>> +
>> +cleanup:
>> + free(cert);
>> + return rv;
>> +}
>>
>
>
> This implementation matches the code of setCertificateRaw(), I see no
> obvious errors (just high-level review comparing the two methods).
Thanks. I'll look at implementing these tomorrow.
> I wonder about "idx" argument in setCertificateRaw() - the case of
> using several certificate types in parallel (RSA, DSA, ECC -- see
> SSL_AIDX_DSA etc. in include/ssl_private.h and Javadoc for this
> method).
>
> I think that each certificate has its own chain going up to different
> root CA certificate.
No. They have to have the same chain. That is a 'feature' of OpenSSL.
Cheers,
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: svn commit: r1737154 - in /tomcat/native/trunk:
native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Posted by Konstantin Kolinko <kn...@gmail.com>.
2016-03-30 22:27 GMT+03:00 <ma...@apache.org>:
> Author: markt
> Date: Wed Mar 30 19:27:29 2016
> New Revision: 1737154
>
> URL: http://svn.apache.org/viewvc?rev=1737154&view=rev
> Log:
> Add support for obtaining the certificate chain from a Java keystore
>
> Modified:
> tomcat/native/trunk/native/src/sslcontext.c
> tomcat/native/trunk/xdocs/miscellaneous/changelog.xml
>
> Modified: tomcat/native/trunk/native/src/sslcontext.c
> URL: http://svn.apache.org/viewvc/tomcat/native/trunk/native/src/sslcontext.c?rev=1737154&r1=1737153&r2=1737154&view=diff
> ==============================================================================
> --- tomcat/native/trunk/native/src/sslcontext.c (original)
> +++ tomcat/native/trunk/native/src/sslcontext.c Wed Mar 30 19:27:29 2016
> @@ -1051,7 +1051,7 @@ TCN_IMPLEMENT_CALL(jboolean, SSLContext,
> certs = d2i_X509(NULL, &tmp, lengthOfCert);
> if (certs == NULL) {
> ERR_error_string(ERR_get_error(), err);
> - tcn_Throw(e, "Error reading certificat (%s)", err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> rv = JNI_FALSE;
> goto cleanup;
> }
> @@ -1119,6 +1119,50 @@ cleanup:
> free(cert);
> return rv;
> }
> +
> +TCN_IMPLEMENT_CALL(jboolean, SSLContext, addChainCertificateRaw)(TCN_STDARGS, jlong ctx,
> + jbyteArray javaCert)
> +{
> + jsize lengthOfCert;
> + unsigned char* cert;
> + X509 * certs;
> + EVP_PKEY * evp;
> + const unsigned char *tmp;
> + BIO * bio;
The "BIO" and "evp" variables are declared, but never used. Can be removed.
> +
> + tcn_ssl_ctxt_t *c = J2P(ctx, tcn_ssl_ctxt_t *);
> + jboolean rv = JNI_TRUE;
> + char err[256];
> +
> + /* we get the cert contents into a byte array */
> + jbyte* bufferPtr = (*e)->GetByteArrayElements(e, javaCert, NULL);
> + lengthOfCert = (*e)->GetArrayLength(e, javaCert);
> + cert = malloc(lengthOfCert);
> + memcpy(cert, bufferPtr, lengthOfCert);
> + (*e)->ReleaseByteArrayElements(e, javaCert, bufferPtr, 0);
> +
> + UNREFERENCED(o);
> + TCN_ASSERT(ctx != 0);
> +
> + tmp = (const unsigned char *)cert;
> + certs = d2i_X509(NULL, &tmp, lengthOfCert);
> + if (certs == NULL) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error reading certificate (%s)", err);
> + rv = JNI_FALSE;
> + goto cleanup;
> + }
> +
> + if (SSL_CTX_add0_chain_cert(c->ctx, certs) <= 0) {
> + ERR_error_string(ERR_get_error(), err);
> + tcn_Throw(e, "Error setting certificate (%s)", err);
"Error adding certificate"
> + rv = JNI_FALSE;
> + }
> +
> +cleanup:
> + free(cert);
> + return rv;
> +}
>
This implementation matches the code of setCertificateRaw(), I see no
obvious errors (just high-level review comparing the two methods).
I wonder about "idx" argument in setCertificateRaw() - the case of
using several certificate types in parallel (RSA, DSA, ECC -- see
SSL_AIDX_DSA etc. in include/ssl_private.h and Javadoc for this
method).
I think that each certificate has its own chain going up to different
root CA certificate.
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org