joelguittet / mender-esp32-example

Mender MCU example running on ESP32 using ESP-IDF platform
MIT License
14 stars 6 forks source link

Condition never true in mender_api_perform_authentication #12

Closed Piocky closed 1 month ago

Piocky commented 1 month ago

Hi @joelguittet,

Yes it's me again :) This time it's not really an issue, but more a remark. I was looking to add an info to the identity of the device (we use a "Serial Number" field instead of the mac address to quickly find our device on Mender server). I found where to add it, but was surprise not to see it on server side. I just found out that it's because even if I don't have a tenant token, the else condition from the following code is never fired:

    /* Format payload */
    if (NULL != mender_api_config.tenant_token) {
        if (NULL
            == (payload = (char *)malloc(strlen("{ \"id_data\": \"{ \\\"mac\\\": \\\"\\\"}\", \"pubkey\": \"\", \"tenant_token\": \"\" }")
                                         + strlen(mender_api_config.mac_address) + strlen(public_key_pem) + strlen(mender_api_config.tenant_token) + 1))) {
            mender_log_error("Unable to allocate memory");
            ret = MENDER_FAIL;
            goto END;
        }
        sprintf(payload,
                "{ \"id_data\": \"{ \\\"mac\\\": \\\"%s\\\"}\", \"pubkey\": \"%s\", \"tenant_token\": \"%s\" }",
                mender_api_config.mac_address,
                public_key_pem,
                mender_api_config.tenant_token);
    } else {
        if (NULL
            == (payload = (char *)malloc(strlen("{ \"id_data\": \"{ \"Serial Number\": \"\"}\", \"pubkey\": \"\" }") + strlen(mender_api_config.serial_number)
                                         + strlen(public_key_pem) + 1))) {
            mender_log_error("Unable to allocate memory");
            ret = MENDER_FAIL;
            goto END;
        }
        sprintf(payload, "{ \"id_data\": \"{ \"Serial Number\": \"%s\"}\", \"pubkey\": \"%s\" }", mender_api_config.serial_number, public_key_pem);
    }

And it's simply because the tenant token initialized as NULL is then replace with "". So I'm not sure what would be the best, either adding a check box in the Kconfig, or completing the condition with != NULL & != "", so just wanted to open the discussion :)

joelguittet commented 1 month ago

Hello @Piocky Good point. This is a real issue yes, but should not be fixed here. I will convert to a real issue/pr asap in mender-mcu-client repo. Joel

joelguittet commented 1 month ago

Note I don't see the link between your serial number and the token. Do you fill the mac address with your serial ? Nothing prohibit that on the client, I just wonder if something else is required to be added in the code at the api level ... ?

Piocky commented 1 month ago

No there is no link between the token and my serial number, it's just that in my case I don't have tenant token, so instead of changing both conditions, I just wanted to make the change on the one I thought was triggered, that how I found this "bug". Now I changed for this, and it works well:

    /* Format payload */
    if (NULL != mender_api_config.tenant_token) {
        if (NULL
            == (payload = (char *)malloc(strlen("{ \"id_data\": \"{ \\\"mac\\\": \\\"\\\", \\\"Serial Number\\\": \\\"\\\"}\", \"pubkey\": \"\", \"tenant_token\": \"\" }")
                                         + strlen(mender_api_config.mac_address) + strlen(mender_api_config.serial_number) + strlen(public_key_pem) + strlen(mender_api_config.tenant_token) + 1))) {
            mender_log_error("Unable to allocate memory");
            ret = MENDER_FAIL;
            goto END;
        }
        sprintf(payload,
                "{ \"id_data\": \"{ \\\"mac\\\": \\\"%s\\\", \\\"Serial Number\\\": \\\"%s\\\"}\", \"pubkey\": \"%s\", \"tenant_token\": \"%s\" }",
                mender_api_config.mac_address,
                mender_api_config.serial_number,
                public_key_pem,
                mender_api_config.tenant_token);
    } else {
        if (NULL
            == (payload = (char *)malloc(strlen("{ \"id_data\": \"{ \\\"mac\\\": \\\"\\\", \\\"Serial Number\\\": \\\"\\\"}\", \"pubkey\": \"\" }")
                                         + strlen(mender_api_config.mac_address) + strlen(mender_api_config.serial_number) + strlen(public_key_pem) + 1))) {
            mender_log_error("Unable to allocate memory");
            ret = MENDER_FAIL;
            goto END;
        }
        sprintf(payload,
                "{ \"id_data\": \"{ \\\"mac\\\": \\\"%s\\\", \\\"Serial Number\\\": \\\"%s\\\"}\", \"pubkey\": \"%s\" }",
                mender_api_config.mac_address,
                mender_api_config.serial_number,
                public_key_pem);
    }

I kept the mac address, but my main identity key is the Serial Number in our case, I sort the devices by it.

joelguittet commented 1 month ago

Hi I m on it for the fix. However the serial number stuff is specific to you, I guess you have a patch on the server as well to use it ? Why not using inventory addon to set the serial ? By the way it's fine like that too probably :-) Joel

joelguittet commented 1 month ago

@Piocky should be now fixed, please checkout the develop branch and update the submodule to retrieve the modification. Tell me if this is ok for you so we can close the issue after that. Note there are several modification of the main.c file due to upgrades on the master branch of the client in progress to prepare 0.9.0.

joelguittet commented 1 month ago

@Piocky for your information another person asked me about the identity content so I should probably do something in the client for that, will permit you to add your serial properly :-)

Piocky commented 1 month ago

Hi I m on it for the fix. However the serial number stuff is specific to you, I guess you have a patch on the server as well to use it ? Why not using inventory addon to set the serial ? By the way it's fine like that too probably :-) Joel

Yes it's specific to us indeed, I wasn't waiting for you to add it to the fix of course :) But if there was a possibility to add customized fields to the identity it could be great! In our case, it's way more convenient to sort the devices with the serial number, instead of the Mac address, that's why we need to add this to the identity, and not only the inventory.

joelguittet commented 1 month ago

@Piocky the custom identity is now available, was not a big deal. https://github.com/joelguittet/mender-mcu-client/pull/49 and example on the develop branch here (modification of client init): https://github.com/joelguittet/mender-esp32-example/blob/develop/main/main.c#L369-L370

Piocky commented 1 month ago

@joelguittet that's perfect! Thank you so much for your efficiency :)

joelguittet commented 1 month ago

@Piocky hello So, is the tenant token issue solved for you so that we can close this issue ? Thanks for the feedback Joel

Piocky commented 1 month ago

Yes it is, you can close this issue :)