[sr-dev] [kamailio/kamailio] tls: add support for OpenSSL engine and private keys in HSM (#1484)

[Henning Westerholt]

henningw commented on this pull request.



> @@ -1113,7 +1119,73 @@ static int passwd_cb(char *buf, int size, int rwflag, void *filename)
 #endif
 }
 
+#ifndef OPENSSL_NO_ENGINE
+/**

Great that you added doxygen comments

> @@ -496,3 +527,59 @@ int mod_register(char *path, int *dlflags, void *p1, void *p2)
 
 	return 0;
 }
+
+
+
+#ifndef OPENSSL_NO_ENGINE
+/*
+ * initialize OpenSSL engine in child process
+ * PKCS#11 libraries are not guaranteed to be fork() safe
+ *
+ */
+static int engine_init()
+{
+	/*

As discussed, this should be either in the module README or as dedicated cfg example in the module dir.

> +	 *
+	 * # OpenSSL config file: /usr/local/etc/kamailio/tls_gemalto.conf
+	 * # https://www.openssl.org/docs/manmaster/man5/config.html
+	 * # /usr/lib64/openssl/engines/libgem.so is the Gemalto OpenSSL engine for CloudHSM/SafeNet Luna
+	 *
+	 * kamailio = kamailio_init
+	 * [kamailio_init]
+	 * engines = engine_section
+	 * [engine_section]
+	 * gem = gemalto_section
+	 * [ gemalto_section ]
+	 * # engines support ctrl strings; this is Gemalto specific
+	 * # ENGINE_INIT = <slot>:<appid>:<appid>:password=<slot-password>
+	 * ENGINE_INIT = 0:23:24:password=1233-ABCD-EFGH-XY32
+	 */
+	LM_INFO("With OpenSSL engine support\n");

Is this an information for the user? Otherwise change it to debug level.

> @@ -30,6 +30,15 @@
 #include "../../core/locking.h"
 #include "tls_domain.h"
 
+#ifndef OPENSSL_NO_ENGINE
+typedef struct tls_engine {
+        str engine;
+        str engine_config;
+        str engine_algorithms;
+} tls_engine_t;
+#include <openssl/conf.h>

I think you don't need this includes in the header file, consider move this to the tls_mod.c file.

> @@ -30,6 +30,15 @@
 #include "../../core/locking.h"
 #include "tls_domain.h"
 
+#ifndef OPENSSL_NO_ENGINE
+typedef struct tls_engine {
+        str engine;
+        str engine_config;
+        str engine_algorithms;
+} tls_engine_t;
+#include <openssl/conf.h>
+#include <openssl/engine.h>

same

> +			err = CONF_modules_load_file(engine_settings.engine_config.s, "kamailio", 0);
+			if (!err) goto error;
+		}
+		engine = ENGINE_by_id(engine_settings.engine.s);
+		if (!engine) goto error;
+		err = ENGINE_init(engine);
+		if (!err) goto error;
+		if (strncmp(engine_settings.engine_algorithms.s, "NONE", 4)) {
+			err = ENGINE_set_default_string(engine, engine_settings.engine_algorithms.s);
+			if (!err) goto error;
+		}
+		LM_INFO("OpenSSL engine %*s initialized\n", engine_settings.engine.len, engine_settings.engine.s);
+	}
+	return 0;
+error:
+	return -1;

It would be great to add some error logging here, to give some hints to the user that the engine, the engine config or the algorithm setting failed.

> + * @return 0 on success, -1 on error
+ *
+ * Do this in mod_child() as PKCS#11 libraries are not guaranteed
+ * to be fork() safe
+ *
+ * private_key setting which starts with /engine: is assumed to be
+ * an HSM key and not a file-based key
+ */
+static int load_engine_private_key(tls_domain_t* d)
+{
+	int idx, ret_pwd, i;
+	EVP_PKEY *pkey;
+	int procs_no;
+
+	if (!d->pkey_file.s || !d->pkey_file.len) {
+		DBG("%s: No private key specified\n", tls_domain_str(d));

Is there a valid condition that we should continue here without an private key? If not, we should log an error and return -1, or I am wrong?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/1484#pullrequestreview-105629697
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.kamailio.org/pipermail/sr-dev/attachments/20180321/58f52369/attachment-0001.html>