@henningw commented on this pull request.
Thanks again. Besides the conflict in the PR (probably because of the merging
of the auth_web3 module), I have added a bunch of review comments. Mostly
related to error logging. error handling, name space topics and some
recommendations about core function usage.
> +#include <stddef.h>
+
+// Configuration constants
+#define MAX_ENS_LENGTH 256
+#define MAX_ADDRESS_LENGTH 43 // 0x + 40 hex chars + null terminator
+#define MAX_SIGNATURE_LENGTH 132 // 0x + 130 hex chars + null terminator
+#define MAX_UUID_LENGTH 64
+#define MAX_RESPONSE_SIZE 4096
+
+// Return codes
+#define ARNACON_SUCCESS 200
+#define ARNACON_FAILURE 404
+#define ARNACON_ERROR -1
+
+// Structure for HTTP response data
+struct ResponseData {
This (commonly named) structures should be prefixed by the module name, see
also below.
> + * @param ens ENS name
+ * @param registry_address ENS Registry contract address
+ * @param rpc_url RPC endpoint URL
+ * @param debug Enable debug logging
+ * @return 1 if user exists, 0 if not, -1 on error
+ */
+int arnacon_core_user_exists(const char *ens, const char *registry_address,
+ const char *rpc_url, int debug);
+
+/**
+ * Compute ENS namehash for a given domain name
+ * @param name The ENS domain name
+ * @param hash_hex Output buffer for the hex-encoded hash (65 bytes minimum)
+ * @return 0 on success, -1 on error
+ */
+int compute_namehash(const char *name, char *hash_hex);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + * @param name The ENS domain name
+ * @param hash_hex Output buffer for the hex-encoded hash (65 bytes minimum)
+ * @return 0 on success, -1 on error
+ */
+int compute_namehash(const char *name, char *hash_hex);
+
+/**
+ * Make a blockchain RPC call to get ENS owner with dynamic Name Wrapper
detection
+ * @param ens_name The ENS domain name
+ * @param owner_address Output buffer for the owner address (43 bytes minimum)
+ * @param registry_address ENS Registry contract address
+ * @param rpc_url RPC endpoint URL
+ * @param debug Enable debug logging
+ * @return 0 on success, -1 on error
+ */
+int get_ens_owner(const char *ens_name, char *owner_address,
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + * @param owner_address Output buffer for the owner address (43 bytes
> minimum)
+ * @param registry_address ENS Registry contract address
+ * @param rpc_url RPC endpoint URL
+ * @param debug Enable debug logging
+ * @return 0 on success, -1 on error
+ */
+int get_ens_owner(const char *ens_name, char *owner_address,
+ const char *registry_address, const char *rpc_url, int debug);
+
+/**
+ * Parse X-Data parameter to extract UUID and timestamp
+ * @param x_data The X-Data string
+ * @param parsed_data Output structure with parsed data
+ * @return 0 on success, -1 on error
+ */
+int parse_x_data(const char *x_data, struct XData *parsed_data);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> +
+/**
+ * Parse X-Data parameter to extract UUID and timestamp
+ * @param x_data The X-Data string
+ * @param parsed_data Output structure with parsed data
+ * @return 0 on success, -1 on error
+ */
+int parse_x_data(const char *x_data, struct XData *parsed_data);
+
+/**
+ * Validate timestamp against current time
+ * @param timestamp The timestamp to validate
+ * @param max_age_seconds Maximum age in seconds
+ * @return 0 if valid, -1 if expired
+ */
+int validate_timestamp(uint64_t timestamp, int max_age_seconds);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + * Validate timestamp against current time
+ * @param timestamp The timestamp to validate
+ * @param max_age_seconds Maximum age in seconds
+ * @return 0 if valid, -1 if expired
+ */
+int validate_timestamp(uint64_t timestamp, int max_age_seconds);
+
+/**
+ * Recover Ethereum address from ECDSA signature
+ * @param message The signed message
+ * @param signature The signature (hex string starting with 0x)
+ * @param recovered_address Output buffer for recovered address (43 bytes
minimum)
+ * @param debug Enable debug logging
+ * @return 0 on success, -1 on error
+ */
+int recover_ethereum_address(const char *message, const char *signature,
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + * @param signature The signature (hex string starting with 0x)
+ * @param recovered_address Output buffer for recovered address (43 bytes
minimum)
+ * @param debug Enable debug logging
+ * @return 0 on success, -1 on error
+ */
+int recover_ethereum_address(const char *message, const char *signature,
+ char *recovered_address, int debug);
+
+/**
+ * Convert hex string to bytes
+ * @param hex_str Input hex string
+ * @param bytes Output byte array
+ * @param max_bytes Maximum number of bytes to convert
+ * @return Number of bytes converted, -1 on error
+ */
+int hex_to_bytes(const char *hex_str, unsigned char *bytes, size_t max_bytes);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> +/**
+ * Convert hex string to bytes
+ * @param hex_str Input hex string
+ * @param bytes Output byte array
+ * @param max_bytes Maximum number of bytes to convert
+ * @return Number of bytes converted, -1 on error
+ */
+int hex_to_bytes(const char *hex_str, unsigned char *bytes, size_t max_bytes);
+
+/**
+ * Convert bytes to hex string
+ * @param bytes Input byte array
+ * @param len Length of byte array
+ * @param hex_str Output hex string buffer
+ */
+void bytes_to_hex(const unsigned char *bytes, size_t len, char *hex_str);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> +
+/**
+ * Convert bytes to hex string
+ * @param bytes Input byte array
+ * @param len Length of byte array
+ * @param hex_str Output hex string buffer
+ */
+void bytes_to_hex(const unsigned char *bytes, size_t len, char *hex_str);
+
+/**
+ * Normalize Ethereum address (convert to lowercase and ensure 0x prefix)
+ * @param address Input address
+ * @param normalized Output normalized address (43 bytes minimum)
+ * @return 0 on success, -1 on error
+ */
+int normalize_ethereum_address(const char *address, char *normalized);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + * Normalize Ethereum address (convert to lowercase and ensure 0x prefix)
+ * @param address Input address
+ * @param normalized Output normalized address (43 bytes minimum)
+ * @return 0 on success, -1 on error
+ */
+int normalize_ethereum_address(const char *address, char *normalized);
+
+/**
+ * cURL callback function for collecting response data
+ * @param contents Response data
+ * @param size Size of each element
+ * @param nmemb Number of elements
+ * @param userp User data (ResponseData structure)
+ * @return Number of bytes processed
+ */
+size_t response_write_callback(void *contents, size_t size, size_t nmemb,
struct ResponseData *response);
Does this needs to be exported in the header file, or could it be made static
in the c implementation?
> + */
+void arnacon_core_cleanup(void) {
+ curl_global_cleanup();
+ LM_INFO("Core authentication system cleaned up");
+}
+
+/**
+ * cURL callback function for collecting response data
+ */
+size_t response_write_callback(void *contents, size_t size, size_t nmemb,
struct ResponseData *response) {
+ size_t realsize;
+ char *ptr;
+
+ realsize = size * nmemb;
+
+ if (response->memory == NULL) {
This is using another logic as the auth_web3 module. I think this logic is
probably better, just noticed it. The auth_web3 module is also not logging any
errors here.
> + }
+
+ bytes[i] = (unsigned char)val;
+ }
+
+ return (int)byte_len;
+}
+
+/**
+ * Convert bytes to hex string
+ */
+void bytes_to_hex(const unsigned char *bytes, size_t len, char *hex_str) {
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ sprintf(hex_str + 2 * i, "%02x", bytes[i]);
Error checks?
> + bytes[i] = (unsigned char)val;
+ }
+
+ return (int)byte_len;
+}
+
+/**
+ * Convert bytes to hex string
+ */
+void bytes_to_hex(const unsigned char *bytes, size_t len, char *hex_str) {
+ size_t i;
+
+ for (i = 0; i < len; i++) {
+ sprintf(hex_str + 2 * i, "%02x", bytes[i]);
+ }
+ hex_str[2 * len] = '\0';
Are we sure that the hex_str is actually that large, as there are no input
checks?
> + LM_DBG("Checking if contract is NameWrapper: %s", contract_address);
+ }
+
+ curl_easy_setopt(curl, CURLOPT_URL, rpc_url);
+ curl_easy_setopt(curl, CURLOPT_POSTFIELDS, payload);
+ curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, response_write_callback);
+ curl_easy_setopt(curl, CURLOPT_WRITEDATA, &response);
+ curl_easy_setopt(curl, CURLOPT_TIMEOUT, 30L);
+
+ headers = curl_slist_append(NULL, "Content-Type: application/json");
+ curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers);
+
+ res = curl_easy_perform(curl);
+
+ if (res != CURLE_OK) {
+ if (debug) {
Wondering if we should log an error in any case, maybe without additional
information? Otherwise the user will get no feedback that the curl request
failed?
> +
+ headers = curl_slist_append(NULL, "Content-Type: application/json");
+ curl_easy_setopt(curl, CURLOPT_HTTPHEADER, headers);
+
+ res = curl_easy_perform(curl);
+
+ if (res != CURLE_OK) {
+ if (debug) {
+ LM_DBG("curl_easy_perform() failed: %s (contract may not have
name() function)", curl_easy_strerror(res));
+ }
+ goto cleanup;
+ }
+
+ if (!response.memory) {
+ if (debug) {
+ LM_DBG("No response from blockchain");
Similar as above, maybe we should return an error? In the
make_blockchain_call(..) we are having this logic, maybe its not important for
this function? This is present in other checks below as well.
> +static int try_recover_with_recovery_id(secp256k1_context *ctx, const
> unsigned char *sig_bytes,
+ const unsigned char *message_hash, int
recovery_id,
+ char *recovered_address, int debug) {
+ secp256k1_ecdsa_recoverable_signature recoverable_sig;
+ secp256k1_pubkey pubkey;
+ unsigned char pubkey_bytes[65];
+ size_t pubkey_len;
+ SHA3_CTX addr_ctx;
+ unsigned char addr_hash[32];
+ int ret;
+
+ (void)debug; /* Unused parameter */
+
+ /* Create recoverable signature */
+ if (!secp256k1_ecdsa_recoverable_signature_parse_compact(ctx,
&recoverable_sig, sig_bytes, recovery_id)) {
+ return -1;
Error logging? Also see below
> +
+ (void)debug; /* Unused parameter */
+
+ /* Create recoverable signature */
+ if (!secp256k1_ecdsa_recoverable_signature_parse_compact(ctx,
&recoverable_sig, sig_bytes, recovery_id)) {
+ return -1;
+ }
+
+ /* Recover public key */
+ if (!secp256k1_ecdsa_recover(ctx, &pubkey, &recoverable_sig,
message_hash)) {
+ return -1;
+ }
+
+ /* Serialize public key (uncompressed format) */
+ pubkey_len = sizeof(pubkey_bytes);
+ secp256k1_ec_pubkey_serialize(ctx, pubkey_bytes, &pubkey_len, &pubkey,
SECP256K1_EC_UNCOMPRESSED);
Error checking?
> +
+ LM_ERR("Failed to recover address with any recovery ID");
+ secp256k1_context_destroy(ctx);
+ return -1;
+}
+
+/**
+ * Check if a string is a valid Ethereum address (0x + 40 hex chars)
+ */
+static int is_ethereum_address(const char *str) {
+ int i;
+
+ if (!str) return 0;
+
+ /* Check for 0x prefix */
+ if (strncmp(str, "0x", 2) != 0 && strncmp(str, "0X", 2) != 0) {
Error logging? Also see below
> +
+ /* Step 6: Compare addresses */
+ if (debug) {
+ LM_DBG("=== Address Comparison ===");
+ LM_DBG("ENS owner address: %s", normalized_ens_address);
+ LM_DBG("Recovered address: %s", normalized_recovered_address);
+ }
+
+ if (strcmp(normalized_ens_address, normalized_recovered_address) == 0) {
+ if (debug) {
+ LM_DBG("Authentication SUCCESS: Addresses match!");
+ }
+ return ARNACON_SUCCESS;
+ } else {
+ if (debug) {
+ LM_DBG("Authentication FAILED: Addresses do not match");
Error logging?
> +}
+
+/**
+ * Module destroy function
+ */
+static void destroy(void)
+{
+ LM_INFO("Shutting down");
+ arnacon_core_cleanup();
+}
+
+/**
+ * Extract string parameter from Kamailio function call
+ * Handles both literal strings and pseudo-variables
+ */
+static int get_str_param(struct sip_msg *msg, char *param, str *result)
Why is this necessary? If I am not mistaken, there are functions to just print
a string from the cfg, regardless if its a literal string or contains PVs.
> + }
+}
+
+/**
+ * Extract X-Data and X-Sign headers from SIP message
+ */
+static int extract_auth_headers(struct sip_msg *msg, str *x_data, str *x_sign)
+{
+ struct hdr_field *hdr;
+
+ /* Look for X-Data and X-Sign headers */
+ for (hdr = msg->headers; hdr; hdr = hdr->next) {
+ if (hdr->type == HDR_OTHER_T) {
+ if (hdr->name.len == 6 && strncasecmp(hdr->name.s, "X-Data", 6) ==
0) {
+ *x_data = hdr->body;
+ /* Trim whitespace */
Consider using trim_len from core/ut.h, its used from other modules in similar
situations
> + for (hdr = msg->headers; hdr; hdr = hdr->next) {
+ if (hdr->type == HDR_OTHER_T) {
+ if (hdr->name.len == 6 && strncasecmp(hdr->name.s, "X-Data", 6) ==
0) {
+ *x_data = hdr->body;
+ /* Trim whitespace */
+ while (x_data->len > 0 && (x_data->s[0] == ' ' || x_data->s[0]
== '\t')) {
+ x_data->s++;
+ x_data->len--;
+ }
+ while (x_data->len > 0 && (x_data->s[x_data->len-1] == ' ' ||
x_data->s[x_data->len-1] == '\t' || x_data->s[x_data->len-1] == '\r' ||
x_data->s[x_data->len-1] == '\n')) {
+ x_data->len--;
+ }
+ }
+ else if (hdr->name.len == 6 && strncasecmp(hdr->name.s, "X-Sign",
6) == 0) {
+ *x_sign = hdr->body;
+ /* Trim whitespace */
Consider using trim_len from core/ut.h - its used from other modules in similar
situations
--
Reply to this email directly or view it on GitHub:
https://github.com/kamailio/kamailio/pull/4470#pullrequestreview-3572646188
You are receiving this because you are subscribed to this thread.
Message ID: <kamailio/kamailio/pull/4470/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!
