@henningw commented on this pull request.
Thanks again for the update and sorry for the delay.
I have done a review and there are certain things that should to be addressed
in my opinion. The module code looks a bit unstructured, as it was created by
some means of refactoring, conversion or with the help of an AI tool maybe.
Summary:
- declare variables at the beginning of your functions
- remove the module name from all log statements prefix
- have a closer look to your error handling when you call external functions
- smaller logical issues in certain functions
- use PKG memory instead of system memory
- smaller docs and cmake adjustments
I have added notes to this points, but due to the number of places I did not
marked all.
Did I understand it correctly that your module is doing a full curl
initialization for every API call? Probably it will not matter much, as the
latency of the blockchain will be much larger anyway, but I noticed it.
> + * Module initialization function prototype
+ */
+static int mod_init(void);
+
+/*
+ * Child initialization function prototype
+ */
+static int child_init(int rank);
+
+/*
+ * Configuration initialization from environment variables
+ */
+static void init_config_from_env(void);
+
+/* Module parameters - can be overridden by environment variables */
+char *authentication_rpc_url = NULL;
This variables should be prefixed by the module name, or made static.
> @@ -754,7 +754,7 @@ set(MODULE_GROUP_PACKAGE_GROUPS
# Add group names to available group and provide "ALL_PACKAGED" as well
# for easier packaging using components
-list(APPEND AVAILABLE_GROUPS ALL_PACKAGED KMINI ${MODULE_GROUP_PACKAGE_GROUPS})
This change, and also the other changes below looks odd. Usually you need only
to add the module name to the MOD_LIST_UTILS, as show above. Maybe there were
from a different, older copy of the cmake files in the repository?
> + LM_ERR("cannot bind auth api\n");
+ return -1;
+ }
+
+ /* Initialize curl globally */
+ if (curl_global_init(CURL_GLOBAL_DEFAULT) != CURLE_OK) {
+ LM_ERR("failed to initialize curl globally\n");
+ return -1;
+ }
+
+ /* Initialize configuration from environment variables */
+ init_config_from_env();
+
+ if (contract_debug_mode) {
+ LM_INFO("Web3Auth initialized:\n");
+ LM_INFO(" Authentication RPC URL: %s\n", authentication_rpc_url
Usually we don't use additional white-space in front of the logging statements.
This will not work anyway e.g. for json log output anyway. Please remove the
trailing whitespace here and below.
> + : "(using default)");
+ LM_INFO(" ENS RPC URL: %s\n",
+ ens_rpc_url ? ens_rpc_url : "(using default)");
+ LM_INFO(" Debug: %s\n", contract_debug_mode ? "enabled" : "disabled");
+ LM_INFO(" Timeout: %d seconds\n", rpc_timeout);
+ }
+
+ return 0;
+}
+
+/*
+ * Child initialization function
+ */
+static int child_init(int rank) {
+ if (contract_debug_mode) {
+ LM_INFO("Web3Auth child %d initialized\n", rank);
You can remove the module name here and also below from the logging. Kamailio
will automatically prefix the module name to the log statement.
> +/*
+ * Module destroy function
+ */
+static void destroy(void) {
+ LM_INFO("Web3 Authentication module shutting down\n");
+
+ /* Cleanup curl */
+ curl_global_cleanup();
+}
+
+/*
+ * Configuration initialization from environment variables
+ * Environment variables override config file parameters,
+ * defaults are only used if neither config file nor env vars are set
+ */
+static void init_config_from_env(void) {
Interesting choice using environment variables, not that common. But as will
probably not used from the majority of installations, nothing generally against
it. The kamailio core has support for environment variables in the
configuration, so you could also rely on that.
> +static void destroy(void) {
+ LM_INFO("Web3 Authentication module shutting down\n");
+
+ /* Cleanup curl */
+ curl_global_cleanup();
+}
+
+/*
+ * Configuration initialization from environment variables
+ * Environment variables override config file parameters,
+ * defaults are only used if neither config file nor env vars are set
+ */
+static void init_config_from_env(void) {
+ char *env_authentication_rpc_url = getenv("AUTHENTICATION_RPC_URL");
+ if (env_authentication_rpc_url) {
+ authentication_rpc_url = strdup(env_authentication_rpc_url);
Please use PKG memory for internal storage inside a kamailio module if its not
shared between processes. There are also special str utility functions, to copy
a kamailio str to new memory. This applies also to the strdup below.
> + smethod = msg->first_line.u.request.method;
+ }
+
+ return web3_digest_authenticate(msg, &srealm, HDR_PROXYAUTH_T, &smethod);
+}
+
+/*
+ * Fixup function for authentication functions
+ */
+static int fixup_web3_auth(void **param, int param_no) {
+ if (strlen((char *)*param) <= 0) {
+ LM_ERR("empty parameter %d not allowed\n", param_no);
+ return -1;
+ }
+
+ switch (param_no) {
Not sure what the purpose of this switch statement is.
> @@ -0,0 +1,4 @@
+kamailio_module(
The file should be replaced with:
```
file(GLOB MODULE_SOURCES "*.c")
add_library(${module_name} SHARED ${MODULE_SOURCES})
find_package(CURL REQUIRED)
target_compile_definitions(${module_name} PRIVATE KAMAILIO_MOD_INTERFACE)
target_link_libraries(${module_name} PRIVATE CURL::libcurl)
```
according to Xenofon.
> @@ -0,0 +1,331 @@
+/* sha3 - an implementation of Secure Hash Algorithm 3 (Keccak).
After the merge we probably should think about moving this to the core/crypto,
where other similar functions are placed. Was this modified for your module or
is this the original upstream version?
> +#include "../../core/parser/parse_to.h"
+#include "../../core/parser/parse_uri.h"
+#include "../../modules/auth/api.h"
+#include "auth_web3_mod.h"
+#include "keccak256.h"
+#include <curl/curl.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+
+/**
+ * Convert hex string to bytes
+ */
+int hex_to_bytes(const char *hex_str, unsigned char *bytes, int max_bytes) {
If this is only used in this file, please make it static.
> +
+/**
+ * Convert hex string to bytes
+ */
+int hex_to_bytes(const char *hex_str, unsigned char *bytes, int max_bytes) {
+ int len = strlen(hex_str);
+ if (len % 2 != 0)
+ return -1; // Invalid hex string
+
+ int byte_len = len / 2;
+ if (byte_len > max_bytes)
+ return -1; // Too many bytes
+
+ for (int i = 0; i < byte_len; i++) {
+ char hex_byte[3] = {hex_str[i * 2], hex_str[i * 2 + 1], '\0'};
+ bytes[i] = (unsigned char)strtol(hex_byte, NULL, 16);
strtol can fail and return LONG_MIN or LONG_MAX. This will probably cause an
array out of bound access.
> +
+ for (int i = 0; i < byte_len; i++) {
+ char hex_byte[3] = {hex_str[i * 2], hex_str[i * 2 + 1], '\0'};
+ bytes[i] = (unsigned char)strtol(hex_byte, NULL, 16);
+ }
+
+ return byte_len;
+}
+
+/**
+ * Curl callback function for Web3 RPC responses
+ */
+size_t web3_curl_callback(void *contents, size_t size, size_t nmemb,
+ struct Web3ResponseData *data) {
+ size_t realsize = size * nmemb;
+ char *ptr = realloc(data->memory, data->size + realsize + 1);
Please use PKG memory functions.
> +/**
+ * Helper function to make blockchain calls for ENS and Oasis queries
+ * Now accepts rpc_url parameter to support different networks
+ */
+static int web3_blockchain_call(const char *rpc_url, const char *to_address,
+ const char *data, char *result_buffer,
+ size_t buffer_size) {
+ CURL *curl;
+ CURLcode res;
+ struct Web3ResponseData web3_response = {0};
+ struct curl_slist *headers = NULL;
+ int result = -1;
+
+ curl = curl_easy_init();
+ if (!curl) {
+ LM_ERR("Web3Auth: Failed to initialize curl for blockchain call\n");
Remove module name here and below
> +
+ if (contract_debug_mode) {
+ LM_INFO("Web3Auth: Authenticating user=%s, realm=%.*s\n", username_str,
+ cred->realm.len, cred->realm.s);
+ LM_INFO("Web3Auth: User provided response=%.*s\n", cred->response.len,
+ cred->response.s);
+ }
+
+ curl = curl_easy_init();
+ if (!curl) {
+ LM_ERR("Web3Auth: Failed to initialize curl\n");
+ return NOT_AUTHENTICATED;
+ }
+
+ // Extract parameters from SIP message context
+ char realm_str[256], method_str[16], uri_str[256], nonce_str[256],
Please move variable definition to the function header, as some compilers will
report errors for this.
> + pos += snprintf(call_data + pos, total_len * 2 + 1 - pos, "%064lx",
> offset7);
+
+ // String 1: username - length + padded data
+ pos += snprintf(call_data + pos, total_len * 2 + 1 - pos, "%064lx", len1);
+ for (size_t i = 0; i < len1; i++) {
+ pos += snprintf(call_data + pos, total_len * 2 + 1 - pos, "%02x",
+ (unsigned char)username_str[i]);
+ }
+ for (size_t i = len1 * 2; i < padded_len1 * 2; i++) {
+ call_data[pos++] = '0';
+ }
+
+ // String 2: realm - length + padded data
+ pos += snprintf(call_data + pos, total_len * 2 + 1 - pos, "%064lx", len2);
+ for (size_t i = 0; i < len2; i++) {
+ pos += snprintf(call_data + pos, total_len * 2 + 1 - pos, "%02x",
If one of this snprintf above or below runs on an error, it will return a
negative number and mess up all this logic.
> +
+ // Calculate string lengths
+ size_t len1 = strlen(username_str), len2 = strlen(realm_str),
+ len3 = strlen(method_str);
+ size_t len4 = strlen(uri_str), len5 = strlen(nonce_str);
+
+ // Calculate padded lengths (round up to 32-byte boundaries)
+ size_t padded_len1 = ((len1 + 31) / 32) * 32;
+ size_t padded_len2 = ((len2 + 31) / 32) * 32;
+ size_t padded_len3 = ((len3 + 31) / 32) * 32;
+ size_t padded_len4 = ((len4 + 31) / 32) * 32;
+ size_t padded_len5 = ((len5 + 31) / 32) * 32;
+ size_t padded_len7 = ((actual_byte_len + 31) / 32) * 32; // For response
bytes
+
+ // Calculate offsets (selector + 7 offset words = 0xE0 for first string)
+ size_t offset1 = 0xE0;
What happened to offset6 and padded_len6? If you don't need len1 etc.., just
put the strlen inside the offset might be easier to understand.
Please declare your variables at the beginning of the function.
> +
> "{\"jsonrpc\":\"2.0\",\"method\":\"eth_call\",\"params\":[{\"to\":"
+ "\"%s\",\"data\":\"0x%s\"},\"latest\"],\"id\":1}",
+ authentication_contract_address, call_data);
+
+ if (contract_debug_mode) {
+ const char *algo_name = (algo == 0) ? "MD5"
+ : (algo == 1) ? "SHA-256"
+ : "SHA-512";
+ LM_INFO("Web3Auth: Algorithm: %s (%d)\n", algo_name, algo);
+ LM_INFO("Web3Auth: Calling authenticateUser with payload: %s\n", payload);
+ }
+
+ curl_easy_setopt(curl, CURLOPT_URL, authentication_rpc_url);
+ curl_easy_setopt(curl, CURLOPT_POSTFIELDS, payload);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, web3_curl_callback);
+ curl_easy_setopt(curl, CURLOPT_WRITEDATA, &web3_response);
Not an curl expert, but where is this memory allocated? It probably should be
allocated in PKG mem, and then also pkg_free called later on.
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4328#pullrequestreview-3298875341
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4328/review/[email protected]>_______________________________________________
Kamailio - Development Mailing List -- [email protected]
To unsubscribe send an email to [email protected]
Important: keep the mailing list in the recipients, do not reply only to the
sender!
