Summary: | Tomcat Native library with OpenSSL Engine private key loading | ||
---|---|---|---|
Product: | Tomcat Native | Reporter: | Edin Hodzic <dino> |
Component: | Library | Assignee: | Tomcat Developers Mailing List <dev> |
Status: | RESOLVED FIXED | ||
Severity: | enhancement | CC: | michaelo |
Priority: | P1 | ||
Version: | 1.2.26 | ||
Target Milestone: | --- | ||
Hardware: | PC | ||
OS: | Linux | ||
Attachments: |
keep structural reference to the SSL Engine pointed to by tcn_ssl_engine
keep structural reference to the SSL Engine pointed to by tcn_ssl_engine |
Description
Edin Hodzic
2021-03-12 20:14:17 UTC
Changed Importance to P1 / Major as this blocks use of OpenSSL Engine with TPMs or HSMs, and OpenSSL Engine support is included in Tomcat Native library already. Changing back to "enhancement" since that's what this is. It may be of major importance to YOU but this is _not_ a bug. A quick review:
> ...
> && tcn_ssl_engine &&
> (c->keys[idx] = ENGINE_load_private_key(tcn_ssl_engine, key_file,
> NULL, NULL)) == NULL
This will attempt to use the tcn_ssl_engine pointer to load the private key, but only if the pointer is non-zero. If you mean NULL, you should explicitly compare the pointer to NULL.
Otherwise the patch looks reasonable to me.
I assume you've tested it to make sure it still works with a "standard" configuration?
Thanks for the updates, the review and the comments. The NULL comparison I can add. The change was tested with and without the SSL engine in the configuration. Is there anything else I can do to get the patch included in the source? Could I submit a pull request somehow? (In reply to Edin Hodzic from comment #4) > Could I submit a pull request somehow? https://github.com/apache/tomcat-native Thanks for the patch. The fix will be in 1.2.27. The Tomcat 10.0.x unit tests passed for the APR/Native connector with this patch applied. Thanks everyone for the prompt action and the pending change. Looking at commit, that implemented this feature for Tomcat Native 1.2.27, 1. There is a bug. https://github.com/apache/tomcat-native/commit/69e884a96a308a2bfdd91c7de3a9b301838031c8 in native/src/sslcontext.c, lines 1039-1040 > - if ((c->keys[idx] = load_pem_key(c, key_file)) == NULL) { > + if ((c->keys[idx] = load_pem_key(c, key_file)) == NULL > +#ifndef OPENSSL_NO_ENGINE > + && tcn_ssl_engine != NULL && > + (c->keys[idx] = ENGINE_load_private_key(tcn_ssl_engine, key_file, > + NULL, NULL)) == NULL > +#endif > + ) { I think that when one uses the default configuration (when there is no custom engine configured with SSLEngine attribute of AprLifecycleListener) and thus tcn_ssl_engine is NULL, the behaviour should remain unchanged. As such, the code above has to be "&& (tcn_ssl_engine == NULL || ENGINE_load_private_key() fails)". With the current code when load_pem_key call fails and tcn_ssl_engine == NULL the error reporting block is skipped. 2. I am OK with the patch with the above bug fixed, but for future improvements there are some thoughts: 1) I wonder, whether in case of a custom engine (tcn_ssl_engine != NULL) it would be better to try a "ENGINE_load_private_key" call first. 2) As "load_pem_key" is our own method (implemented in the same sslcontext.c file), I wonder whether it would be better to move the change there (and maybe rename that method). 3) I see that "ENGINE_load_private_key" method will be deprecated in OpenSSL 3.0 (master) https://www.openssl.org/docs/man1.1.1/man3/ENGINE_load_private_key.html https://www.openssl.org/docs/manmaster/man3/ENGINE_load_private_key.html 4) How about a "ENGINE_load_public_key" method? There is no need to call it? STEPS TO REPRODUCE (for 1. bug) Using release candidate of Tomcat 9.0.45: 1) Install Tomcat, e.g. unzip apache-tomcat-9.0.45-windows-x64.zip on Windows. 2) Edit the conf/server.xml file. - ~line 68: Commend out the default HTTP connector on port 8080 - ~line 102: Uncomment a connector that uses OpenSSL for TLS 3) Create two files with some dummy content (e.g. "foo bar"): conf/localhost-rsa-cert.pem conf/localhost-rsa-key.pem 4) Start Tomcat. ACTUAL BEHAVIOUR: With Tomcat 9.0.45 and Tomcat Native 1.2.27 a startup failure is reported as follows: [[[ 30-Mar-2021 18:28:20.133 WARNING [main] org.apache.tomcat.util.net.openssl.OpenSSLContext.init Error initializing SSL context java.lang.Exception: Unable to load certificate [censored]\conf/localhost-rsa-cert.pem (error:0D06B08E:asn1 encoding routines:asn1_d2i_read_bio:not enough data) at org.apache.tomcat.jni.SSLContext.setCertificate(Native Method) at org.apache.tomcat.util.net.openssl.OpenSSLContext.addCertificate(OpenSSLContext.java:379) at org.apache.tomcat.util.net.openssl.OpenSSLContext.init(OpenSSLContext.java:250) at org.apache.tomcat.util.net.SSLUtilBase.createSSLContext(SSLUtilBase.java:246) at org.apache.tomcat.util.net.AprEndpoint.createSSLContext(AprEndpoint.java:397) at org.apache.tomcat.util.net.AprEndpoint.bind(AprEndpoint.java:363) at org.apache.tomcat.util.net.AbstractEndpoint.bindWithCleanup(AbstractEndpoint.java:1204) at org.apache.tomcat.util.net.AbstractEndpoint.init(AbstractEndpoint.java:1217) ]]] EXPECTED BEHAVIOUR: If I replace bin/tcnative-1.dll file with Tomcat Native 1.2.26 (e.g. the one from Tomcat 9.0.44), the error is reported as follows: [[[ 30-Mar-2021 18:30:02.551 WARNING [main] org.apache.tomcat.util.net.openssl.OpenSSLContext.init Error initializing SSL context java.lang.Exception: Unable to load certificate key [censored]\conf/localhost-rsa-key.pem (error:0909006C:PEM routines:get_name:no start line) at org.apache.tomcat.jni.SSLContext.setCertificate(Native Method) at org.apache.tomcat.util.net.openssl.OpenSSLContext.addCertificate(OpenSSLContext.java:379) at org.apache.tomcat.util.net.openssl.OpenSSLContext.init(OpenSSLContext.java:250) at org.apache.tomcat.util.net.SSLUtilBase.createSSLContext(SSLUtilBase.java:246) at org.apache.tomcat.util.net.AprEndpoint.createSSLContext(AprEndpoint.java:397) at org.apache.tomcat.util.net.AprEndpoint.bind(AprEndpoint.java:363) at org.apache.tomcat.util.net.AbstractEndpoint.bindWithCleanup(AbstractEndpoint.java:1204) at org.apache.tomcat.util.net.AbstractEndpoint.init(AbstractEndpoint.java:1217) ]]] As loading the key in SSLContext::setCertificate (in native/src/sslcontext.c) happens earlier than loading the certificate, the expected error message is "Unable to load certificate key". Seeing the "Unable to load certificate" message means that error handling for the former was skipped. > As such, the code above has to be
"&& (tcn_ssl_engine == NULL || ENGINE_load_private_key() fails)".
I agree, my bad on the original patch. :/
Fixed for 1.2.28 onwards. Created attachment 37870 [details]
keep structural reference to the SSL Engine pointed to by tcn_ssl_engine
Found another problem with the recent engine related change.
Got to keep a "structural" reference to the engine so the tcn_ssl_engine pointer remains valid at the time it's used for loading private key.
Please see the tcn-1.2.28.diff attachment for a patch.
Comment on attachment 37870 [details]
keep structural reference to the SSL Engine pointed to by tcn_ssl_engine
diff -aur tomcat-native-1.2.28-src/native/src/ssl.c tomcat-native-1.2.28-src.changed/native/src/ssl.c
--- tomcat-native-1.2.28-src/native/src/ssl.c 2021-04-01 06:28:54.000000000 -0700
+++ tomcat-native-1.2.28-src.changed/native/src/ssl.c 2021-05-20 13:21:18.569612200 -0700
@@ -367,6 +367,14 @@
#endif
free_dh_params();
+#ifndef OPENSSL_NO_ENGINE
+ if (tcn_ssl_engine != NULL) {
+ /* Release the SSL Engine structural reference */
+ ENGINE_free(tcn_ssl_engine);
+ tcn_ssl_engine = NULL;
+ }
+#endif
+
#if OPENSSL_VERSION_NUMBER >= 0x10100000L && !defined(LIBRESSL_VERSION_NUMBER)
/* Openssl v1.1+ handles all termination automatically. Do
* nothing in this case.
@@ -815,9 +823,9 @@
if (!ENGINE_set_default(ee, ENGINE_METHOD_ALL))
err = APR_ENOTIMPL;
}
- /* Free our "structural" reference. */
+ /* Keep our "structural" reference. */
if (ee)
- ENGINE_free(ee);
+ tcn_ssl_engine = ee;
}
if (err != APR_SUCCESS) {
TCN_FREE_CSTRING(engine);
@@ -825,7 +833,6 @@
tcn_ThrowAPRException(e, err);
return (jint)err;
}
- tcn_ssl_engine = ee;
}
#endif
Created attachment 37871 [details]
keep structural reference to the SSL Engine pointed to by tcn_ssl_engine
improved diff
One other potential improvement in the OpenSSL interaction might be to enable OpenSSL configuration: In ssl.c, initialization is done with the call: OPENSSL_init_ssl(OPENSSL_INIT_ENGINE_ALL_BUILTIN, NULL); The openssl command in OpenSSL source performs initialization like this: OPENSSL_init_ssl(OPENSSL_INIT_ENGINE_ALL_BUILTIN | OPENSSL_INIT_LOAD_CONFIG, NULL); The OPENSSL_INIT_LOAD_CONFIG makes the OpenSSL library load configuration (for example /etc/ssl/openssl.cnf by default on Ubuntu). The configuration could also be specified through a path in OPENSSL_CONF environment variable (that could be set in setenv.sh in Tomcat for full control over the OpenSSL configuration.) Mark, this has been marked as fixed in the changelog for 1.2.28. This appears weird to the user. We should settle this timely or create a followup ticket. I've applied a variation of the additional fix in comment #12. The suggestion in #14 is a separate issue and should be submitted via a new enhancement request. |