[Opinion] Hardcoded TLV format is not a good long term choice

[chipweinberger]

Thanks for answering my previous questions =)

I just wanted to share this idea and why I ended up choosing against the custom TLV format. Instead, I store certs in a generic KLV format, that I already implemented for other purposes in my code base.

As alluded to in #2, I'm not a fan of using a custom hard coded TLV format. Personally, It's a deal-breaker.

Why?

• hard coded type values are not future proof, and annoying to change
• one-off formats increase code size
• one-off formats increase code complexity
• it is not extensible by 3rd parties. I can't add a lot of my own fields.
• custom formats often die quickly, I want code to last. I don't want it to be deprecated.
• its more annoying to document, because the format changes as types are added
• types are not human readable

Instead, we really should:

• move the format to its own *.c an *.h file
• document it, so its not as proprietary
• use a keyLen,key,dataLen,data format, so we don't need hardcoded type values
• it should be generic and generally useful
• it should support versioning
• consider making it an ESP-IDF component

The generic KLV format I use looks like this:

Edit: also worth considering using DER format.

[EKLV]         (4 bytes) // magic value, "espKeyLengthValue"
[versionMajor] (1 bytes)
[versionMinor] (1 bytes)
[totalItemsLen] (8 bytes) // total items length in bytes

// item 1
[keyLenLen] (1 byte)    // keyLen size is configurable
[keyLen]    (X bytes)
[key]       (X bytes)
[padLen]     (2 bytes)   // for aligning 'data'
[pad]        (X bytes)
[dataLenLen] (1 byte)    // dataLen size is configurable
[dataLen]    (X bytes)
[data]       (X bytes)

// item 2
[keyLenLen] (1 byte) 
[keyLen]    (X bytes)
[key]       (X bytes)
[padLen]     (2 bytes) 
[pad]        (X bytes)
[dataLenLen] (1 byte) 
[dataLen]    (X bytes)
[data]       (X bytes)

...repeat...

The TLV format currently used here is a deal-breaker for me.

[chipweinberger]

Here is my implementation of a slightly simpler version w/ fixed 4-byte maximum lengths.

Just a starting point if you consider bringing the TLV structure into its own component...

pd_klv.h

struct pd_buf_t {
    uint8_t* bytes;
    uint32_t len;
};

typedef struct pd_buf_t pd_buf_t;

struct pd_klv_item_t {
    char* key;
    pd_buf_t data;
    uint32_t padLen;
    struct pd_klv_item_t* next;
};

typedef struct pd_klv_item_t pd_klv_item_t;

struct pd_klv_t {
    uint8_t verMajor;
    uint8_t verMinor;
    pd_klv_item_t* itemList;
};

typedef struct pd_klv_t pd_klv_t;

pd_klv_t pd_klv_create(void);

// note: this makes a copy of the data, which is owned by the klv
void pd_klv_update(pd_klv_t* klv, const char* key, pd_buf_t data);

void pd_klv_remove(pd_klv_t* klv, const char* key);

pd_klv_item_t* pd_klv_item(pd_klv_t klv, const char* key);

bool pd_klv_key_exists(pd_klv_t klv, const char* key);

// free all data
void pd_klv_free(pd_klv_t klv);

pd_klv_serialization.h

pd_klv_t pd_klv_read_from_buffer(pd_buf_t buf, bool* error);

pd_buf_t pd_klv_write_to_buffer(pd_klv_t klv);

All In Single File

//
//  main.c
//  c-code-playground
//
//  Created by Chip Weinberger on 11/4/22.
//  License: Public Domain
//
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <stdint.h>
#include <stdbool.h>

struct pd_buf_t {
    uint8_t* bytes;
    uint32_t len;
};

typedef struct pd_buf_t pd_buf_t;

pd_buf_t pd_buf_deep_copy(pd_buf_t buf)
{
    uint8_t* bytes2 = (uint8_t*) calloc(buf.len, 1);
    memcpy(bytes2, buf.bytes, buf.len);

    pd_buf_t buf2 = {
        .bytes = bytes2,
        .len = buf.len
    };

    return buf2;
}

unsigned long strbuflen(const char* str){
    return strlen(str) + 1;
}

/*

 Jamcorder-keyLengthValue:

 A simple data structure for storing raw bytes on disk.

    [JKLV]         (4 bytes) // magic value
    [versionMajor] (1 bytes)
    [versionMinor] (1 bytes)
    [totalItemsLen](4 bytes) // total length of all items, in bytes
    
    // item 1
    [keyLen]       (4 bytes)
    [key]          (X bytes)
    [padLen]       (4 bytes) // for 'data' alignment
    [pad]          (X bytes)
    [dataLen]      (4 bytes)
    [data]         (X bytes)
    
    // item 2
    [keyLen]       (4 bytes)
    [key]          (X bytes)
    [padLen]       (4 bytes) // for 'data' alignment
    [pad]          (X bytes)
    [dataLen]      (4 bytes)
    [data]         (X bytes)

..... repeat ....

*/

#define MAGIC_SIZE 4
#define VER_MAJ_SIZE 1
#define VER_MIN_SIZE 1
#define TOTAL_OBJ_LENGTH_SIZE 4

#define KEY_LEN_SIZE 4
#define PAD_LEN_SIZE 4
#define DATA_LEN_SIZE 4

struct pd_klv_item_t {
    char* key;
    pd_buf_t data;
    uint32_t padLen;
    struct pd_klv_item_t* next;
};

typedef struct pd_klv_item_t pd_klv_item_t;

struct pd_klv_t {
    uint8_t verMajor;
    uint8_t verMinor;
    pd_klv_item_t* itemList;
};

typedef struct pd_klv_t pd_klv_t;

pd_klv_t pd_klv_create(void);

// note: this makes a copy of the data, which is owned by the klv
void pd_klv_update(pd_klv_t* klv, const char* key, pd_buf_t data);

void pd_klv_remove(pd_klv_t* klv, const char* key);

pd_klv_item_t* pd_klv_item(pd_klv_t klv, const char* key);

bool pd_klv_key_exists(pd_klv_t klv, const char* key);

// free all data
void pd_klv_free(pd_klv_t klv);

pd_klv_t pd_klv_read_from_buffer(pd_buf_t buf, bool* error);

pd_buf_t pd_klv_write_to_buffer(pd_klv_t klv);


pd_klv_t pd_klv_create()
{
    pd_klv_t k = {
        .verMajor = 1,
        .verMinor = 0,
        .itemList = NULL
    };
    return k;
}

void pd_klv_update(pd_klv_t* klv, const char* key, pd_buf_t data)
{
    pd_klv_item_t* cur = klv->itemList;
    
    bool found = false;

    while (cur) {

        // replace current data
        if (strcmp(cur->key, key) == 0) {
            found = true;
            break;
        }

        cur = cur->next;
    }
    
    if (found) {
        
        // free existing data
        free(cur->data.bytes);

        // make local copy
        pd_buf_t dataCopy = pd_buf_deep_copy(data);

        // replace
        cur->data = dataCopy;
        
    } else {

        // make data local copy
        pd_buf_t dataCopy = pd_buf_deep_copy(data);

        // make key local copy
        char* keyCopy = (char*) calloc(strbuflen(key), 1);
        memcpy(keyCopy, key, strbuflen(key));

        // make new item
        pd_klv_item_t* item = (pd_klv_item_t*) calloc(sizeof(pd_klv_item_t), 1);
        item->data = dataCopy;
        item->key = keyCopy;
        item->padLen = 0;
        item->next = NULL;

        // place at begining of list
        item->next = klv->itemList;
        klv->itemList = item;
    }
}

void pd_klv_remove(pd_klv_t* klv, const char* key)
{
    pd_klv_item_t* prev = NULL;
    pd_klv_item_t* cur = klv->itemList;

    while (cur) {

        // remove current
        if (strcmp(cur->key, key) == 0) {

            if (prev) {
                prev->next = cur->next;
            } else {
                klv->itemList = cur->next;
            }

            // free
            free(cur->data.bytes);
            free(cur->key);

            break;
        }

        prev = cur;
        cur = cur->next;
    }

    return;
}

pd_klv_item_t* pd_klv_item(pd_klv_t klv, const char* key)
{
    pd_klv_item_t* cur = klv.itemList;

    while (cur) {

        if (strcmp(cur->key, key) == 0){
            break;
        }

        cur = cur->next;
    }

    return cur;
}

bool pd_klv_key_exists(pd_klv_t klv, const char* key)
{
    return pd_klv_item(klv, key) != NULL;
}


void pd_klv_free(pd_klv_t klv)
{
    pd_klv_item_t* cur = klv.itemList;

    while (cur) {

        pd_klv_item_t* next = cur->next; 

        // free key & value
        free(cur->data.bytes);
        free(cur->key);

        // free item
        free(cur);

        // next
        cur = next;
    }
}


//
// Serialization Read
//

static uint32_t readLittleEndianU32(uint8_t* data)
{
    uint32_t n = 0;
    n |= data[0] <<  0;
    n |= data[1] <<  8;
    n |= data[2] << 16;
    n |= data[3] << 24;
    return n;
}

static uint32_t hdrLen()
{
    return
        MAGIC_SIZE +
        VER_MAJ_SIZE +
        VER_MIN_SIZE +
        TOTAL_OBJ_LENGTH_SIZE;
}

static uint32_t itemLen(uint32_t keyLen, uint32_t padLen, uint32_t dataLen)
{
    return
        KEY_LEN_SIZE +
        keyLen +
        PAD_LEN_SIZE +
        padLen +
        DATA_LEN_SIZE +
        dataLen;
}


static pd_klv_item_t parseItem(uint8_t* data, int32_t bytesLeft, uint32_t* bytesRead, bool* error)
{
    *error = true;

    // allocations
    pd_klv_item_t item;
    memset(&item, 0, sizeof(pd_klv_item_t));

    pd_buf_t bufs[3];
    memset(bufs, 0, sizeof(pd_buf_t) * 3);

    {
    // read keyLen, padLen, & dataLen
    for (int i = 0; i < 3; i++){

        if (bytesLeft < 4) {
            *error = true;
            printf("ran out of bytes for length. idx %u", i);
            goto end;
        }

        // read length
        bufs[i].len = readLittleEndianU32(data + *bytesRead);
        bufs[i].bytes = (data + *bytesRead) + 4;

        bytesLeft -= 4;
        *bytesRead += 4;

        // missing key?
        if (i == 0 && bufs[i].len == 0) {
            *error = true;
            printf("missing key");
            goto end;
        }

        // missing data?
        if (i == 2 && bufs[i].len == 0) {
            *error = true;
            printf("missing data");
            goto end;
        }

        // enough data?
        if (bytesLeft < bufs[i].len) {
            *error = true;
            printf("not enough bytes for data. idx %u", i);
            goto end;
        }

        bytesLeft -= bufs[i].len;
        *bytesRead += bufs[i].len;
    }

    // copy key & make sure null terminated
    item.key = (char*) calloc(bufs[0].len + 1, 1);
    memcpy(item.key, bufs[0].bytes, bufs[0].len);

    // copy data
    item.data = pd_buf_deep_copy(bufs[2]);

    *error = false;
    }
end:

    if (*error){
        // free key & data
        free(bufs[0].bytes);
        free(bufs[1].bytes);
    }

    return item;
}


pd_klv_t pd_klv_read_from_buffer(pd_buf_t buf, bool* error)
{
    *error = true;

    pd_klv_t klv = {
        .verMajor = 1,
        .verMinor = 0,
        .itemList = NULL
    };

    // check minimum size
    if (buf.len < hdrLen()){
        *error = true;
        printf("bufLen (%u) smaller than min size (%u)", buf.len, hdrLen());
        return klv;
    }

    // check magic
    if (buf.bytes[0] != 'J' ||
        buf.bytes[1] != 'K' ||
        buf.bytes[2] != 'L' ||
        buf.bytes[3] != 'V')
    {
        *error = true;
        printf("missing 'JKLV', found 0x%08x", *((uint32_t*)buf.bytes));
        return klv;
    }

    // check version
    if (buf.bytes[4] != 1 ||
        buf.bytes[5] != 0)
    {
        *error = true;
        printf("JKLV unsupported version %u.%u (must be 1.0)",
            buf.bytes[4], buf.bytes[5]);
        return klv;
    }

    uint32_t left = buf.len - hdrLen();
    
    uint8_t* nextItem = buf.bytes + hdrLen();

    while (left) {

        uint32_t read = 0;
        pd_klv_item_t item = parseItem(nextItem, left, &read, error);
        if (*error){
            *error = true;
            printf("parseItem() failed");
            return klv;
        }
        
        nextItem += read;
        
        // update left
        if (read >= left) {
            left = 0;
        } else {
            left -= read;
        }

        // move to heap
        pd_klv_item_t* itemHeap = (pd_klv_item_t*) malloc(sizeof(pd_klv_item_t));
        memcpy(itemHeap, &item, sizeof(pd_klv_item_t));

        // put on front of linked list
        itemHeap->next = klv.itemList;
        klv.itemList = itemHeap;
    }

    return klv;
}

//
// Serialization Write
//

static void writeLittleEndianU32(uint8_t* data, uint32_t n)
{
    data[0] = n >>  0 & 0xFF;
    data[1] = n >>  8 & 0xFF;
    data[2] = n >> 16 & 0xFF;
    data[3] = n >> 24 & 0xFF;
}

static uint32_t writeHdr(uint8_t* hdr, uint32_t totalItemsLen)
{
    uint32_t b = 0;

    // set magic
    hdr[b] = 'J'; b++;
    hdr[b] = 'K'; b++;
    hdr[b] = 'L'; b++;
    hdr[b] = 'V'; b++;

    // set version
    hdr[b] = 1; b++; // major
    hdr[b] = 0; b++; // minor

    // set totalItemsLen
    writeLittleEndianU32(&hdr[b], totalItemsLen);
    b += 4;

    return b;
}

static uint32_t writeItem(uint8_t* obj, const char* key, pd_buf_t data)
{
    uint32_t keyLen = (uint32_t) strbuflen(key);
    uint32_t padLen = 0; // currently unused
    uint32_t dataLen = data.len;

    uint32_t b = 0;

    // keyLen (little endian)
    writeLittleEndianU32(&obj[b], keyLen);
    b += 4;

    // key
    memcpy(&obj[b], key, keyLen);
    b += keyLen;

    // padLen
    writeLittleEndianU32(&obj[b], padLen);
    b += 4;

    // pad
    memset(&obj[b], 0, padLen);
    b += padLen;

    // dataLen
    writeLittleEndianU32(&obj[b], dataLen);
    b += 4;

    // data
    memcpy(&obj[b], data.bytes, dataLen);
    b += dataLen;

    return b;
}

static uint32_t totalItemsLen(pd_klv_t klv)
{
    pd_klv_item_t* cur = klv.itemList;

    uint32_t total = 0;

    while (cur) {

        uint32_t keyLen = (uint32_t) strbuflen(cur->key);
        uint32_t padLen = 0;
        uint32_t dataLen = cur->data.len;

        total += itemLen(keyLen, padLen, dataLen);

        cur = cur->next;
    }

    return total;
}

pd_buf_t pd_klv_write_to_buffer(pd_klv_t klv)
{
    uint32_t itemsLen = totalItemsLen(klv);
    uint32_t len = hdrLen() + itemsLen;

    // alloc
    pd_buf_t buf = {
        .bytes = (uint8_t*) calloc(len, 1),
        .len = len,
    };

    uint32_t wrote = 0;

    // write header
    wrote += writeHdr(buf.bytes, itemsLen);

    pd_klv_item_t* cur = klv.itemList;

    while (cur) {

        // write object
        wrote += writeItem(buf.bytes + wrote, cur->key, cur->data);

        cur = cur->next;
    }

    return buf;
}


//
// Test
//

pd_buf_t ones(void)
{
    pd_buf_t ones = {
        .len = 1024,
        .bytes = calloc(1024, 1),
    };
    memset(ones.bytes, 1, 1024);
    return ones;
}

pd_buf_t twos(void)
{
    pd_buf_t twos = {
        .len = 1024,
        .bytes = calloc(1024, 1),
    };
    memset(twos.bytes, 2, 1024);
    return twos;
}

pd_buf_t threes(void)
{
    pd_buf_t threes = {
        .len = 1024,
        .bytes = calloc(1024, 1),
    };
    memset(threes.bytes, 3, 1024);
    return threes;
}

pd_buf_t fours(void)
{
    pd_buf_t fours = {
        .len = 1024,
        .bytes = calloc(1024, 1),
    };
    memset(fours.bytes, 4, 1024);
    return fours;
}

#define PDMIN(x, y) (((x) < (y)) ? (x) : (y))

void pd_klv_print(pd_klv_t klv)
{
    printf("====== KLV =======\n");
    printf("klv: verMajor %u\n", klv.verMajor);
    printf("klv: verMinor %u\n", klv.verMinor);

    pd_klv_item_t* cur = klv.itemList;

    uint32_t i = 0;

    while (cur) {

        char temp[51];
        for(int i = 0; i < PDMIN(cur->data.len,25); i++) {
            snprintf(temp+(i*2), 51-(i*2), "%02x", cur->data.bytes[i]);
        }

        printf("klv: [%u] key: %s\n", i, cur->key);
        printf("klv: [%u] data: (%u bytes) 0x%s...\n", i, cur->data.len, temp);

        i++;
        cur = cur->next;
    }
    printf("=================\n");
}

int main(int argc, const char * argv[]) {
    // insert code here...
    printf("Hello, World!\n");
    
    pd_klv_t klv = pd_klv_create();
    pd_klv_print(klv);
    
    pd_klv_update(&klv, "abcdefg", ones());
    pd_klv_print(klv);
    
    pd_klv_update(&klv, "abcdefg", twos());
    pd_klv_print(klv);
    
    pd_klv_update(&klv, "bbbbbbbbb", threes());
    pd_klv_print(klv);
    
    pd_klv_remove(&klv, "abcdefg");
    pd_klv_print(klv);
    
    pd_klv_update(&klv, "iuiuiuiu", fours());
    pd_klv_print(klv);
    
    pd_buf_t buf = pd_klv_write_to_buffer(klv);
    
    bool error = false;
    klv = pd_klv_read_from_buffer(buf, &error);
    pd_klv_print(klv);
    
    if (error){
        printf("error\n");
    }
    
    pd_klv_free(klv);
    
    return 0;
}

[AdityaHPatwardhan]

Hi @chipweinberger Thanks for your opinion. and thanks for the detailed reasoning.
As for why we replace NVS, I think as already mentioned in #2 The major advantage here is that the content need not be copied into the RAM for future use.
This works best where DS peripheral is involved in which the ciphertext itself is 1200 bytes. After adding the size of certs the total heap saved comes around 2KB.

I have seen your implementation and it looks interesting. But I think it is more generic and it also supports updating an existing KLV.

As for the flash format, Our main objective was to make it simple, and more efficient.
The esp_secure_cert partition is designed to store more like the read only data for the device. This partition should contain only the data which is to be stored once ( e.g. private keys, ciphertext, device cert, ca cert) and then it stays there for the life-time of the device. So I think any changes in that data by the running application are not recommended. As of now we do not support writing to the esp_secure_cert partition but in future we may support appending the data to the existing esp_secure_cert partition. but it is assumed that once the data is written it is not to be updated.

While designing a generic flash format, I think there are many aspects that need to be looked at. However, our current goal here was to tailor the flash format specific to our needs.

I agree that adding a header that contains the version would be a good idea. I will discuss it and see if we can incorporate it.
Also, I agree that a detailed documentation is needed for the user to understand how to use it.

While I give you my explanation as to why we designed it in such a way, we would really take your comments into consideration.

Thanks,
Aditya

[chipweinberger]

Thanks for the thoughtful answer.

Yes, I think if the format was documented and moved to it's own .h and .c file, that would go a long way towards the "longevity" of the format.

If this repo changes, the .h and .c files will be all a user needs to copy into own code.

[AdityaHPatwardhan]

I see, that would be helpful as well. But not sure how many users would use this customised flash format with their own projects.