tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Mark Thomas <ma...@apache.org>
Subject Re: svn commit: r1737154 - in /tomcat/native/trunk: native/src/sslcontext.c xdocs/miscellaneous/changelog.xml
Date Wed, 30 Mar 2016 20:43:22 GMT
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


Mime
View raw message