refactor: Use structs for extended public/secret keys.

[iphydf]

This change is 

[codecov]

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (a1e999f) 73.00% compared to head (d671110) 73.04%.

❗ Current head d671110 differs from pull request most recent head 021db70. Consider uploading reports for the commit 021db70 to get more accurate results

Files	Patch %	Lines
toxcore/group_chats.c	96.20%	3 Missing ⚠️
toxcore/crypto_core_pack.c	92.59%	2 Missing ⚠️
toxcore/group_connection.c	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2672      +/-   ##
==========================================
+ Coverage   73.00%   73.04%   +0.03%     
==========================================
  Files         148      149       +1     
  Lines       30486    30516      +30     
==========================================
+ Hits        22257    22289      +32     
+ Misses       8229     8227       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nurupo]

toxcore/group_chats.c line 403 at r2 (raw file):

        GC_Connection *gconn = get_gc_connection(chat, 0);
        assert(gconn != nullptr);
        gconn->addr.public_key = *ext_public_key;

This is a bit unusual. Had to double-check if this does a shallow copy or a full copy. Turns out it's a full copy.

[nurupo]

toxcore/group_chats.c line 7453 at r2 (raw file):

           get_sig_pk(&chat->shared_state.founder_public_key), SIG_PUBLIC_KEY_SIZE);
    memcpy(chat->moderation.self_public_sig_key, get_sig_pk(&chat->self_public_key), SIG_PUBLIC_KEY_SIZE);
    memcpy(chat->moderation.self_secret_sig_key, get_sig_sk(&chat->self_secret_key), SIG_SECRET_KEY_SIZE);

Nice typo fix

[nurupo]

toxcore/group_pack.c line 123 at r2 (raw file):

    uint8_t founder_public_key[EXT_PUBLIC_KEY_SIZE];
    if (!bin_unpack_bin_fixed(bu, founder_public_key, EXT_PUBLIC_KEY_SIZE)) {

Why not do something like this instead of using a temporary and memcpy()?

 if (!bin_unpack_bin_fixed(bu, chat->shared_state.founder_public_key.enc, CRYPTO_PUBLIC_KEY_SIZE)
        || !bin_unpack_bin_fixed(bu, chat->shared_state.founder_public_key.sig, CRYPTO_SIGN_PUBLIC_KEY_SIZE)) {

Is it because a partial unpack is undesired?

(Cursed: add a bool bin_unpack_bin_fixed(Bin_Unpack *bu, uint8_t *data, uint32_t data_length, ...) variadic function variant that will accept multiple data+data_length tuples, internally allocating a temporary array of the appropriate data_length1+data_length2...+data_lengthN size, essentially doing an all or nothing bin unpack into data1, data2, ... dataN)

If you must use a temporary, maybe it would make sense to write a dedicated function for this? Something like:

non_null()
static bool bin_unpack_ext_public_key(Bin_Unpack *bu, Extended_Public_Key ext_public_key)
{
    uint8_t tmp_key[EXT_PUBLIC_KEY_SIZE];
    if (!bin_unpack_bin_fixed(bu, tmp_key, EXT_PUBLIC_KEY_SIZE)) {
        return false;
    }
    memcpy(ext_public_key.enc, tmp_key, CRYPTO_PUBLIC_KEY_SIZE);
    memcpy(ext_public_key.sig, tmp_key + CRYPTO_PUBLIC_KEY_SIZE, CRYPTO_SIGN_PUBLIC_KEY_SIZE);
    return true;
}

non_null()
static bool bin_unpack_ext_secret_key(Bin_Unpack *bu, Extended_Secret_Key ext_secret_key)
{
    uint8_t tmp_key[EXT_SECRET_KEY_SIZE];
    if (!bin_unpack_bin_fixed(bu, tmp_key, EXT_SECRET_KEY_SIZE)) {
        return false;
    }
    memcpy(ext_secret_key.enc, tmp_key, CRYPTO_SECRET_KEY_SIZE);
    memcpy(ext_secret_key.sig, tmp_key + CRYPTO_SECRET_KEY_SIZE, CRYPTO_SIGN_SECRET_KEY_SIZE);
    return true;
}

[nurupo]

Don't necessary want to block / don't necessary request code changes, the PR is fine, but just want to make sure my comments are read, as otherwise someone else might approve the PR and it would all go poof -- automerged and probably unread.

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

[iphydf]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

---

toxcore/group_chats.c line 403 at r2 (raw file):

Previously, nurupo wrote…

This is a bit unusual. Had to double-check if this does a shallow copy or a full copy. Turns out it's a full copy.

What should I change to make a struct assignment through = more usual?

---

toxcore/group_chats.c line 7453 at r2 (raw file):

Previously, nurupo wrote…

Nice typo fix

Everything is better with structs :).

---

toxcore/group_pack.c line 123 at r2 (raw file):

Previously, nurupo wrote…

Why not do something like this instead of using a temporary and memcpy()?

 if (!bin_unpack_bin_fixed(bu, chat->shared_state.founder_public_key.enc, CRYPTO_PUBLIC_KEY_SIZE)
        || !bin_unpack_bin_fixed(bu, chat->shared_state.founder_public_key.sig, CRYPTO_SIGN_PUBLIC_KEY_SIZE)) {

Is it because a partial unpack is undesired?

(Cursed: add a bool bin_unpack_bin_fixed(Bin_Unpack *bu, uint8_t *data, uint32_t data_length, ...) variadic function variant that will accept multiple data+data_length tuples, internally allocating a temporary array of the appropriate data_length1+data_length2...+data_lengthN size, essentially doing an all or nothing bin unpack into data1, data2, ... dataN)

If you must use a temporary, maybe it would make sense to write a dedicated function for this? Something like:

non_null()
static bool bin_unpack_ext_public_key(Bin_Unpack *bu, Extended_Public_Key ext_public_key)
{
    uint8_t tmp_key[EXT_PUBLIC_KEY_SIZE];
    if (!bin_unpack_bin_fixed(bu, tmp_key, EXT_PUBLIC_KEY_SIZE)) {
        return false;
    }
    memcpy(ext_public_key.enc, tmp_key, CRYPTO_PUBLIC_KEY_SIZE);
    memcpy(ext_public_key.sig, tmp_key + CRYPTO_PUBLIC_KEY_SIZE, CRYPTO_SIGN_PUBLIC_KEY_SIZE);
    return true;
}

non_null()
static bool bin_unpack_ext_secret_key(Bin_Unpack *bu, Extended_Secret_Key ext_secret_key)
{
    uint8_t tmp_key[EXT_SECRET_KEY_SIZE];
    if (!bin_unpack_bin_fixed(bu, tmp_key, EXT_SECRET_KEY_SIZE)) {
        return false;
    }
    memcpy(ext_secret_key.enc, tmp_key, CRYPTO_SECRET_KEY_SIZE);
    memcpy(ext_secret_key.sig, tmp_key + CRYPTO_SECRET_KEY_SIZE, CRYPTO_SIGN_SECRET_KEY_SIZE);
    return true;
}

Done, moved to crypto_core_pack module. This had a rather large fallout, so there is a lot more to review now.

[nurupo]

Reviewed 11 of 26 files at r3, 20 of 20 files at r4, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

---

toxcore/crypto_core.c line 219 at r3 (raw file):

                             const Sign_Public_Key *public_key)
{
    return crypto_sign_verify_detached(signature, message, message_length, public_key->data) == 0;

Would making crypto_sign_*() take Sign_Public_Key and Sign_Secret_Key argument types be the next step? Probably in a separate PR though.

EDIT: was looking at r3 when writing that, since r4 there are no more Sign_Public_Key and Sign_Secret_Key. Still leaving the comment as it might be something to consider.

---

toxcore/crypto_core_pack.c line 18 at r3 (raw file):

    uint8_t ext_key[EXT_PUBLIC_KEY_SIZE];
    memcpy(ext_key, key->enc.data, CRYPTO_PUBLIC_KEY_SIZE);
    memcpy(&ext_key[CRYPTO_PUBLIC_KEY_SIZE], key->sig.data, CRYPTO_SIGN_PUBLIC_KEY_SIZE);

What do you think about adding static asserts making sure CRYPTO_[PUBLIC|SECRET]_KEY_SIZE + CRYPTO_SIGN_[PUBLIC|SECRET]_KEY_SIZE == EXT_[PUBLIC|SECRET]_KEY_SIZE?
Good idea? Unnecessary?

---

toxcore/crypto_core_pack.c line 25 at r3 (raw file):

bool pack_extended_secret_key(const Extended_Secret_Key *key, Bin_Pack *bp)
{
    uint8_t ext_key[EXT_SECRET_KEY_SIZE];

Are we okay with leaving secret keys on the stack? Was #2661 somehow special because it was the gc session key?

---

toxcore/crypto_core_pack.c line 48 at r3 (raw file):

bool unpack_extended_secret_key(Extended_Secret_Key *key, Bin_Unpack *bu)
{
    uint8_t ext_key[EXT_SECRET_KEY_SIZE];

Are we okay with leaving secret keys on the stack? Was #2661 somehow special because it was the gc session key?

---

toxcore/group_chats.c line 403 at r2 (raw file):

Previously, iphydf wrote…

What should I change to make a struct assignment through = more usual?

Nothing, was just a blunter on my part. Not used to dealing with fixed-sized arrays in structs, so automatically thought of them as dynamic arrays, in which case the assignment would just assign the array pointers since that's all the struct would contain. But with the fixed-size arrays the struct actually contains all the array data, not just pointers, so the assignment would overwrite the entire array data, which is what we want.

[iphydf]

Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @nurupo)

---

toxcore/crypto_core.c line 219 at r3 (raw file):

Previously, nurupo wrote…

Would making crypto_sign_*() take Sign_Public_Key and Sign_Secret_Key argument types be the next step? Probably in a separate PR though.

EDIT: was looking at r3 when writing that, since r4 there are no more Sign_Public_Key and Sign_Secret_Key. Still leaving the comment as it might be something to consider.

Those are libsodium functions, so no. But: #2680

---

toxcore/crypto_core_pack.c line 18 at r3 (raw file):

Previously, nurupo wrote…

What do you think about adding static asserts making sure CRYPTO_[PUBLIC|SECRET]_KEY_SIZE + CRYPTO_SIGN_[PUBLIC|SECRET]_KEY_SIZE == EXT_[PUBLIC|SECRET]_KEY_SIZE?
Good idea? Unnecessary?

Done.

---

toxcore/crypto_core_pack.c line 25 at r3 (raw file):

Previously, nurupo wrote…

Are we okay with leaving secret keys on the stack? Was #2661 somehow special because it was the gc session key?

We're not, good call. I've added memzero to both secret and public key (not strictly necessary, but the symmetry is nice and doesn't harm much (just a little bit of performance that doesn't matter here).

---

toxcore/crypto_core_pack.c line 48 at r3 (raw file):

Previously, nurupo wrote…

Are we okay with leaving secret keys on the stack? Was #2661 somehow special because it was the gc session key?

Done.

---

toxcore/group_chats.c line 403 at r2 (raw file):

Previously, nurupo wrote…

Nothing, was just a blunter on my part. Not used to dealing with fixed-sized arrays in structs, so automatically thought of them as dynamic arrays, in which case the assignment would just assign the array pointers since that's all the struct would contain. But with the fixed-size arrays the struct actually contains all the array data, not just pointers, so the assignment would overwrite the entire array data, which is what we want.

Yup :) it's really great, because this way we also don't get automatic pointer conversions (decays), making all the code much more explicit.

[nurupo]

toxcore/crypto_core_pack.c line 25 at r3 (raw file):

Previously, iphydf wrote…

We're not, good call. I've added memzero to both secret and public key (not strictly necessary, but the symmetry is nice and doesn't harm much (just a little bit of performance that doesn't matter here).

Eh, that's really unnecessary for the public key and the functions don't have to be symmetric.

[nurupo]

Reviewed 10 of 10 files at r5, all commit messages.
Reviewable status: complete! 1 of 1 approvals obtained

[nurupo]

Reviewed 3 of 4 files at r6, all commit messages.
Reviewable status: 1 change requests, 0 of 1 approvals obtained (waiting on @iphydf)

---

toxcore/crypto_core_pack.c line 17 at r6 (raw file):

bool pack_extended_public_key(const Extended_Public_Key *key, Bin_Pack *bp)
{
    uint8_t ext_key[EXT_PUBLIC_KEY_SIZE];

Maybe make it uint8_t ext_key[sizeof(ext_key)]; then too? Since that's how much data we are memcpy()'ing into it.

---

toxcore/crypto_core_pack.c line 28 at r6 (raw file):

bool pack_extended_secret_key(const Extended_Secret_Key *key, Bin_Pack *bp)
{
    uint8_t ext_key[EXT_SECRET_KEY_SIZE];

Same comment as above

---

toxcore/crypto_core_pack.c line 41 at r6 (raw file):

bool unpack_extended_public_key(Extended_Public_Key *key, Bin_Unpack *bu)
{
    uint8_t ext_key[EXT_PUBLIC_KEY_SIZE];

Same comment as above

---

toxcore/crypto_core_pack.c line 55 at r6 (raw file):

bool unpack_extended_secret_key(Extended_Secret_Key *key, Bin_Unpack *bu)
{
    uint8_t ext_key[EXT_SECRET_KEY_SIZE];

Same comment as above

[nurupo]

toxcore/crypto_core_pack.c line 17 at r6 (raw file):

Previously, nurupo wrote…

Maybe make it uint8_t ext_key[sizeof(ext_key)]; then too? Since that's how much data we are memcpy()'ing into it.

Typo, I meant uint8_t ext_key[sizeof(key)]; not uint8_t ext_key[sizeof(ext_key)];, but I guess that wouldn't make much sense either.

[nurupo]

Reviewed 1 of 4 files at r6.
Reviewable status: complete! 1 of 1 approvals obtained

[nurupo]

toxcore/crypto_core_pack.c line 17 at r6 (raw file):

Previously, nurupo wrote…

Typo, I meant uint8_t ext_key[sizeof(key)]; not uint8_t ext_key[sizeof(ext_key)];, but I guess that wouldn't make much sense either.

LGTM

[iphydf]

Reviewable status: complete! 1 of 1 approvals obtained

---

toxcore/crypto_core_pack.c line 17 at r6 (raw file):

Previously, nurupo wrote…

LGTM

I intentionally left this one to be the macro constant. sizeof(some_struct) is not what we want here.