refactor(client): Rework the client certificate password callback

include/open62541/client.h

@@ -26,6 +26,10 @@
 #include <open62541/plugin/eventloop.h>
 #include <open62541/plugin/securitypolicy.h>
 
+/* Forward declarations */
+struct UA_ClientConfig;
+typedef struct UA_ClientConfig UA_ClientConfig;
+
 _UA_BEGIN_DECLS
 
 /**
@@ -67,7 +71,7 @@ _UA_BEGIN_DECLS
  *
  * The :ref:`tutorials` provide a good starting point for this. */
 
-typedef struct {
+struct UA_ClientConfig {
     void *clientContext;       /* User-defined pointer attached to the client */
     const UA_Logger *logging;  /* Plugin for log output */
 
@@ -211,14 +215,15 @@ typedef struct {
     size_t sessionLocaleIdsSize;
 
 #ifdef UA_ENABLE_ENCRYPTION
-    /* If the private key is in PEM format and password protected,
+    /* If the private key is in PEM format and password protected, this callback
-     * this callback is called during initialization to get the password
+     * is called during initialization to get the password to decrypt the
-     * to decrypt the private key.
+     * private key. The memory containing the password is freed by the client
-     * The callback must be set before invoking UA_ClientConfig_setDefaultEncryption(). */
+     * after use. The callback should be set early, other parts of the client
-    UA_PKI_PrivateKeyPasswordCallback privateKeyPasswordCallback;
+     * config setup may depend on it. */
-    void *privateKeyPasswordCallbackContext;
+    UA_StatusCode (*privateKeyPasswordCallback)(UA_ClientConfig *cc,
+                                                UA_ByteString *password);
 #endif
-} UA_ClientConfig;
+};
 
 /**
  * @brief It makes a partial deep copy of the clientconfig. It makes a shallow

include/open62541/plugin/pki.h

@@ -30,20 +30,6 @@ _UA_BEGIN_DECLS
  * The lifecycle of the plugin is attached to a server or client config. The
  * ``clear`` method is called automatically when the config is destroyed. */
 
-/* Enum and functions for the private key password */
-typedef enum  {
-    UA_PRIVATEKEYPASSWORDSTATE_INITIAL = 0,
-    UA_PRIVATEKEYPASSWORDSTATE_WRONG = 1,
-} UA_PrivateKeyPasswordState;
-
-/* This callback is called when UA_PKI_decryptPemWithPassword is called and detects
- * a password protected private key in the PEM format.
- * If a wrong password is encountered, the callback will be called repeatedly until
- * doRetry is set to false. */
-typedef UA_String (*UA_PKI_PrivateKeyPasswordCallback)(UA_PrivateKeyPasswordState state,
-                                                       UA_Boolean *doRetry,
-                                                       void *context);
-
 struct UA_CertificateVerification;
 typedef struct UA_CertificateVerification UA_CertificateVerification;
 
@@ -75,6 +61,17 @@ struct UA_CertificateVerification {
     const UA_Logger *logging;
 };
 
+/* Decrypt a private key in PEM format using a password. The output is the key
+ * in the binary DER format. Also succeeds if the PEM private key does not
+ * require a password or is already in the DER format. The outDerKey memory is
+ * allocated internally.
+ *
+ * Returns UA_STATUSCODE_BADSECURITYCHECKSFAILED if the password is wrong. */
+UA_EXPORT UA_StatusCode
+UA_PKI_decryptPrivateKey(const UA_ByteString privateKey,
+                         const UA_ByteString password,
+                         UA_ByteString *outDerKey);
+
 _UA_END_DECLS
 
 #endif /* UA_PLUGIN_PKI_H_ */

plugins/crypto/mbedtls/ua_pki_mbedtls.c

@@ -732,92 +732,58 @@ UA_CertificateVerification_CertFolders(UA_CertificateVerification *cv,
 #endif
 
 UA_StatusCode
-UA_PKI_decryptPemWithPassword(UA_ByteString privateKey, UA_ByteString *derOutput,
+UA_PKI_decryptPrivateKey(const UA_ByteString privateKey,
-                                  UA_PKI_PrivateKeyPasswordCallback passwordCallback,
+                         const UA_ByteString password,
-                                     void *callbackContext)
+                         UA_ByteString *outDerKey) {
-{
+    if(!outDerKey)
-    if (!derOutput) {
+        return UA_STATUSCODE_BADINTERNALERROR;
-        return UA_STATUSCODE_BADINVALIDARGUMENT;
-    }
 
-    if (!passwordCallback ||
+    /* Already in DER format -> return verbatim */
-       // Magic number for DER encoded keys
+    if(privateKey.length > 1 && privateKey.data[0] == 0x30 && privateKey.data[1] == 0x82)
-       (privateKey.length > 1 && privateKey.data[0] == 0x30 && privateKey.data[1] == 0x82)) {
+        return UA_ByteString_copy(&privateKey, outDerKey);
-        UA_ByteString_copy(&privateKey, derOutput);
-         return UA_STATUSCODE_GOOD;
-    }
 
+    /* Create a null-terminated string */
     UA_ByteString nullTerminatedKey = UA_mbedTLS_CopyDataFormatAware(&privateKey);
+    if(nullTerminatedKey.length != privateKey.length + 1)
+        return UA_STATUSCODE_BADOUTOFMEMORY;
 
-    mbedtls_pk_context key;
+    /* Create the private-key context */
-    mbedtls_pk_init(&key);
+    mbedtls_pk_context ctx;
-    UA_Boolean loadSuccess = false;
+    mbedtls_pk_init(&ctx);
-
 #if MBEDTLS_VERSION_NUMBER >= 0x02060000 && MBEDTLS_VERSION_NUMBER < 0x03000000
-    int err = mbedtls_pk_parse_key(&key, nullTerminatedKey.data, nullTerminatedKey.length, NULL, 0);
+    int err = mbedtls_pk_parse_key(&ctx, nullTerminatedKey.data,
+                                   nullTerminatedKey.length,
+                                   password.data, password.length);
 #else
     mbedtls_entropy_context entropy;
     mbedtls_entropy_init(&entropy);
-    int err = mbedtls_pk_parse_key(&key, nullTerminatedKey.data, nullTerminatedKey.length, NULL, 0, mbedtls_entropy_func, &entropy);
+    int err = mbedtls_pk_parse_key(&ctx, nullTerminatedKey.data,
-#endif
+                                   nullTerminatedKey.length,
-
+                                   password.data, password.length,
-    if (err) {
+                                   mbedtls_entropy_func, &entropy);
-        if (err == MBEDTLS_ERR_PK_PASSWORD_REQUIRED) {
-            UA_PrivateKeyPasswordState state = UA_PRIVATEKEYPASSWORDSTATE_INITIAL;
-             UA_Boolean doRetry = false;
-
-             do {
-                doRetry = false;
-                UA_ByteString password = passwordCallback( state, &doRetry, callbackContext);
-#if MBEDTLS_VERSION_NUMBER >= 0x02060000 && MBEDTLS_VERSION_NUMBER < 0x03000000
-                err = mbedtls_pk_parse_key(&key, nullTerminatedKey.data, nullTerminatedKey.length, password.data, password.length);
-#else
-                err = mbedtls_pk_parse_key(&key, nullTerminatedKey.data, nullTerminatedKey.length, password.data, password.length,
-                                           mbedtls_entropy_func, &entropy);
-#endif
-                UA_ByteString_clear(&password);
-
-                if (!err) {
-                    loadSuccess = true;
-                    break;
-                }
-
-                state = UA_PRIVATEKEYPASSWORDSTATE_WRONG;
-             } while (doRetry);
-        } else {
-             /* If this happens, there is something wrong with the PK file */
-#if MBEDTLS_VERSION_NUMBER < 0x02060000 || MBEDTLS_VERSION_NUMBER >= 0x03000000
-             mbedtls_entropy_free(&entropy);
-#endif
-             return UA_STATUSCODE_BADINVALIDARGUMENT;
-        }
-    } else {
-        loadSuccess = true;
-    }
-
-    UA_ByteString_clear(&nullTerminatedKey);
-
-#if MBEDTLS_VERSION_NUMBER < 0x02060000 || MBEDTLS_VERSION_NUMBER >= 0x03000000
     mbedtls_entropy_free(&entropy);
 #endif
-
+    UA_ByteString_clear(&nullTerminatedKey);
-    if (loadSuccess) {
+    if(err != 0) {
-        unsigned char buffer[16000];
+        mbedtls_pk_free(&ctx);
-        memset(buffer, 0, sizeof(buffer));
+        return UA_STATUSCODE_BADSECURITYCHECKSFAILED;
-        err = mbedtls_pk_write_key_der(&key, buffer, sizeof(buffer));
-        mbedtls_pk_free(&key);
-
-        if (err > 0) {
-            UA_ByteString temp = UA_BYTESTRING_NULL;
-            temp.length = err;
-            temp.data = buffer + sizeof(buffer) - err;
-            UA_ByteString_copy(&temp, derOutput);
-        }
-
-        return err > 0 ? UA_STATUSCODE_GOOD : UA_STATUSCODE_BADINTERNALERROR;
     }
 
-    return UA_STATUSCODE_BADINTERNALERROR;
+    /* Write the DER-encoded key into a local buffer */
+    unsigned char buf[2 << 13];
+    size_t pos = (size_t)mbedtls_pk_write_key_der(&ctx, buf, sizeof(buf));
+
+    /* Allocate memory */
+    UA_StatusCode res = UA_ByteString_allocBuffer(outDerKey, pos);
+    if(res != UA_STATUSCODE_GOOD) {
+        mbedtls_pk_free(&ctx);
+        return res;
+    }
+
+    /* Copy to the output */
+    memcpy(outDerKey->data, &buf[sizeof(buf) - pos], pos);
+    mbedtls_pk_free(&ctx);
+    return UA_STATUSCODE_GOOD;
 }
 
 #endif

plugins/crypto/openssl/ua_pki_openssl.c

@@ -834,95 +834,46 @@ UA_CertificateVerification_CertFolders(UA_CertificateVerification * cv,
 }
 #endif
 
-struct PrivateKeyPasswordCallbackUserData {
+static int
-    UA_PKI_PrivateKeyPasswordCallback callback;
+privateKeyPasswordCallback(char *buf, int size, int rwflag, void *userdata) {
-    void *callbackContext;
-    UA_PrivateKeyPasswordState state;
-    UA_Boolean retry;
-    UA_Boolean callbackWasCalled;
-};
-
-static int PrivateKeyPasswordCallback(char *buf, int size, int rwflag, void *userdata) {
     (void) rwflag;
-
+    UA_ByteString *pw = (UA_ByteString*)userdata;
-    struct PrivateKeyPasswordCallbackUserData *context = (struct PrivateKeyPasswordCallbackUserData *) userdata;
+    if(pw->length <= (size_t)size)
-
+        memcpy(buf, pw->data, pw->length);
-    size_t passwordSize = 0;
+    return (int)pw->length;
-
-    if (context && context->callback) {
-        UA_String password = context->callback(context->state, &context->retry, context->callbackContext);
-        context->callbackWasCalled = true;
-
-        if (password.length) {
-            if (password.length <= (size_t) size) {
-                memcpy(buf, password.data, password.length);
-                passwordSize = password.length;
-            }
-            UA_String_clear(&password);
-        }
-    }
-
-    return (int) passwordSize;
 }
 
-UA_StatusCode UA_PKI_decryptPemWithPassword(UA_ByteString privateKey, UA_ByteString *derOutput,
+UA_StatusCode
-                                     UA_PKI_PrivateKeyPasswordCallback passwordCallback,
+UA_PKI_decryptPrivateKey(const UA_ByteString privateKey,
-                                     void *callbackContext)
+                         const UA_ByteString password,
-{
+                         UA_ByteString *outDerKey) {
-    if (!derOutput) {
+    if(!outDerKey)
-        return UA_STATUSCODE_BADINVALIDARGUMENT;
+        return UA_STATUSCODE_BADINTERNALERROR;
-    }
 
-    if (!passwordCallback ||
+    /* Already in DER format -> return verbatim */
-       // Magic number for DER encoded keys
+    if(privateKey.length > 1 && privateKey.data[0] == 0x30 && privateKey.data[1] == 0x82)
-       (privateKey.length > 1 && privateKey.data[0] == 0x30 && privateKey.data[1] == 0x82)) {
+        return UA_ByteString_copy(&privateKey, outDerKey);
-        UA_ByteString_copy(&privateKey, derOutput);
 
-        return UA_STATUSCODE_GOOD;
+    /* Decrypt */
-    }
+    BIO *bio = BIO_new_mem_buf((void*)privateKey.data, (int)privateKey.length);
+    EVP_PKEY *pkey = PEM_read_bio_PrivateKey(bio, NULL,
+                                             privateKeyPasswordCallback,
+                                             (void*)(uintptr_t)&password);
+    BIO_free(bio);
+    if(!pkey)
+        return UA_STATUSCODE_BADSECURITYCHECKSFAILED;
 
-    struct PrivateKeyPasswordCallbackUserData userData;
+    /* Write DER encoded, allocates the new memory */
-    userData.callback = passwordCallback;
+    unsigned char *data = NULL;
-    userData.callbackContext = callbackContext;
+    const int numBytes = i2d_PrivateKey(pkey, &data);
-    userData.retry = false;
+    EVP_PKEY_free(pkey);
-    userData.state = UA_PRIVATEKEYPASSWORDSTATE_INITIAL;
+    if(!data)
+        return UA_STATUSCODE_BADOUTOFMEMORY;
 
-    do {
+    /* Move to the output */
-        userData.retry = false;
+    outDerKey->data = data;
-        userData.callbackWasCalled = false;
+    outDerKey->length = (size_t)numBytes;
-
+    return UA_STATUSCODE_GOOD;
-        EVP_PKEY *pkey = NULL;
-
-        BIO *bio = NULL;
-        bio = BIO_new_mem_buf((void *) privateKey.data, (int) privateKey.length);
-        pkey = PEM_read_bio_PrivateKey(bio, NULL, PrivateKeyPasswordCallback, &userData);
-
-        BIO_free(bio);
-
-        if (pkey) {
-            unsigned char *data = NULL;
-            const int numBytes = i2d_PrivateKey(pkey, &data);
-
-            EVP_PKEY_free(pkey);
-
-            if (numBytes > 0) {
-                derOutput->length = (size_t)numBytes;
-                derOutput->data = data;
-                return UA_STATUSCODE_GOOD;
-            }
-
-            return UA_STATUSCODE_BADINTERNALERROR;
-        }
-
-        if (!userData.callbackWasCalled) {
-            /* If this happens, there is something wrong with the PK file */
-            return UA_STATUSCODE_BADINVALIDARGUMENT;
-        }
-
-        userData.state = UA_PRIVATEKEYPASSWORDSTATE_WRONG;
-    } while(userData.retry);
-
-    return UA_STATUSCODE_BADINTERNALERROR;
 }
 
 #endif  /* end of defined(UA_ENABLE_ENCRYPTION_OPENSSL) || defined(UA_ENABLE_ENCRYPTION_LIBRESSL) */

plugins/include/open62541/plugin/pki_default.h

@@ -47,16 +47,6 @@ UA_CertificateVerification_CertFolders(UA_CertificateVerification *cv,
 #endif
 #endif
 
-/* Attempts to load privateKey and writes a DER encoded private key to derOutput.
- * If the key is PEM encoded and password protected, the password callback
- * is called to get the password to decrypt the private key.
- * A DER private key input is just copied without any loading operation involved.
- * The content of derOutput must be cleared after use. */
-UA_EXPORT UA_StatusCode
-UA_PKI_decryptPemWithPassword(UA_ByteString privateKey, UA_ByteString *derOutput,
-                              UA_PKI_PrivateKeyPasswordCallback passwordCallback,
-                              void *callbackContext);
-
 #endif
 
 _UA_END_DECLS

plugins/ua_config_default.c

@@ -24,6 +24,7 @@
 
 #include "../deps/mp_printf.h"
 
+#include <stdio.h>
 #ifdef _WIN32
 # include <winsock2.h>
 #else
@@ -45,6 +46,29 @@ UA_DURATIONRANGE(UA_Duration min, UA_Duration max) {
     return range;
 }
 
+/* Request the private key password from stdin if no callback is defined */
+#ifdef UA_ENABLE_ENCRYPTION
+static UA_StatusCode
+readPrivateKeyPassword(UA_ByteString *password) {
+    /* Read from stdin */
+    char buf[256];
+    fputs("Private key requires a password. Enter and press return: ", stdout);
+    char *s = fgets(buf, 256, stdin);
+    if(!s)
+        return UA_STATUSCODE_BADINTERNALERROR;
+
+    /* Get rid of any trailing \n */
+    size_t len = strlen(buf);
+    if(len == 0)
+        return UA_STATUSCODE_BADINTERNALERROR;
+    if(buf[len-1] == '\n')
+        buf[len-1] = 0;
+
+    *password = UA_BYTESTRING_ALLOC(buf);
+    return UA_STATUSCODE_GOOD;
+}
+#endif
+
 UA_Server *
 UA_Server_new(void) {
     UA_ServerConfig config;
@@ -1144,6 +1168,7 @@ UA_ClientConfig_setDefault(UA_ClientConfig *config) {
 }
 
 #ifdef UA_ENABLE_ENCRYPTION
+
 UA_StatusCode
 UA_ClientConfig_setDefaultEncryption(UA_ClientConfig *config,
                                      UA_ByteString localCertificate, UA_ByteString privateKey,
@@ -1167,13 +1192,28 @@ UA_ClientConfig_setDefaultEncryption(UA_ClientConfig *config,
         return UA_STATUSCODE_BADOUTOFMEMORY;
     config->securityPolicies = sp;
 
+    /* Load the private key and convert to the DER format. Use an empty password
+     * on the first try -- maybe the key does not require a password. */
     UA_ByteString decryptedPrivateKey = UA_BYTESTRING_NULL;
-    const UA_StatusCode keySuccess = UA_PKI_decryptPemWithPassword(privateKey, &decryptedPrivateKey,
+    UA_ByteString keyPassword = UA_BYTESTRING_NULL;
-                                                                config->privateKeyPasswordCallback,
+    UA_StatusCode keySuccess = UA_PKI_decryptPrivateKey(privateKey, keyPassword,
-                                                                config->privateKeyPasswordCallbackContext);
+                                                        &decryptedPrivateKey);
 
-    if (keySuccess != UA_STATUSCODE_GOOD)
+    /* Get the password and decrypt. An application might want to loop / retry
-        return UA_STATUSCODE_BADSECURITYCHECKSFAILED;
+     * here to allow users to correct their entry. */
+    if(keySuccess != UA_STATUSCODE_GOOD) {
+        if(config->privateKeyPasswordCallback)
+            keySuccess = config->privateKeyPasswordCallback(config, &keyPassword);
+        else
+            keySuccess = readPrivateKeyPassword(&keyPassword);
+        if(keySuccess != UA_STATUSCODE_GOOD)
+            return keySuccess;
+        keySuccess = UA_PKI_decryptPrivateKey(privateKey, keyPassword, &decryptedPrivateKey);
+        memset(keyPassword.data, 0, keyPassword.length);
+        UA_ByteString_clear(&keyPassword);
+    }
+    if(keySuccess != UA_STATUSCODE_GOOD)
+        return keySuccess;
 
     retval = UA_SecurityPolicy_Basic128Rsa15(&config->securityPolicies[config->securityPoliciesSize],
                                              localCertificate, decryptedPrivateKey, config->logging);

src/client/ua_client.c

@@ -188,7 +188,6 @@ UA_ClientConfig_clear(UA_ClientConfig *config) {
 
 #ifdef UA_ENABLE_ENCRYPTION
     config->privateKeyPasswordCallback = NULL;
-    config->privateKeyPasswordCallbackContext = NULL;
 #endif
 }
 

tests/encryption/check_encryption_key_password.c

@@ -26,17 +26,10 @@
 #include "testing_networklayers.h"
 #include "thread_wrapper.h"
 
-static int numAttempts = 0;
+static UA_StatusCode
-static UA_String privateKeyPasswordCallback(UA_PrivateKeyPasswordState state, bool *doRetry, void *context)
+privateKeyPasswordCallback(UA_ClientConfig *cc, UA_ByteString *password) {
-{
+    *password = UA_STRING_ALLOC("pass1234");
-    if (numAttempts++ == 0) {
+    return UA_STATUSCODE_GOOD;
-        ck_assert(state == UA_PRIVATEKEYPASSWORDSTATE_INITIAL);
-        *doRetry = true;
-        return UA_STRING_ALLOC("invalid");
-    }
-
-    ck_assert(state == UA_PRIVATEKEYPASSWORDSTATE_WRONG);
-    return UA_STRING_ALLOC("pass1234");
 }
 
 UA_Server *server;
@@ -143,9 +136,10 @@ START_TEST(encryption_connect_pem) {
     client = UA_Client_new();
     UA_ClientConfig *cc = UA_Client_getConfig(client);
     cc->privateKeyPasswordCallback = privateKeyPasswordCallback;
-    UA_ClientConfig_setDefaultEncryption(cc, certificate, privateKey,
+    retval = UA_ClientConfig_setDefaultEncryption(cc, certificate, privateKey,
-                                         trustList, trustListSize,
+                                                  trustList, trustListSize,
-                                         revocationList, revocationListSize);
+                                                  revocationList, revocationListSize);
+    ck_assert_uint_eq(retval, UA_STATUSCODE_GOOD);
     cc->certificateVerification.clear(&cc->certificateVerification);
     UA_CertificateVerification_AcceptAll(&cc->certificateVerification);
     cc->securityPolicyUri =
@@ -160,8 +154,6 @@ START_TEST(encryption_connect_pem) {
     retval = UA_Client_connect(client, "opc.tcp://localhost:4840");
     ck_assert_uint_eq(retval, UA_STATUSCODE_GOOD);
 
-    ck_assert_int_eq(numAttempts, 2);
-
     UA_Variant val;
     UA_Variant_init(&val);
     UA_NodeId nodeId = UA_NODEID_NUMERIC(0, UA_NS0ID_SERVER_SERVERSTATUS_STATE);