Improved image, keygen, sign, test lib messages. Polish

[gojimmypi]

This PR improves the diagnostic messages and error checking for various executable app code (image, keygen, sign).

Also polishes for code readability, adding return results, moved arg processing to separate function, and other improvements.

Note: test-lib now returns non-zero error code for errors, not just failure text.

No key functionality changes.

Was failing because of image size difference, perhaps due to new static inline int key_hash_ok(int id, uint8_t* digest) and other checks. See size adjustments in 6527db7

[danielinux]

This could be a git clean so we don't have to maintain the list

[gojimmypi]

The helpful thing about the explicit clean is that is also reminds of the generated files, spread over multiple directories. This is the thing that tripped me up at the beginning more than anything else.

Note the brute-force clean also removed the test-app directory and related source files.

There's always the potential that one of the files such as target.h ends up included in someone's repo.

I hear you on the list maintenance though: perhaps move that to a common script?

If I document and possible move the operation to a script, would you consider keeping?

[danielinux]

please replace with make distclean. We don't need all those here. Thanks.

[danielinux]

Doesn't this break normal sha256? Why is this only done in Sha256? The purpose of this new option is unclear/undocumented.

[gojimmypi]

This is something I bumped into when targeting other platforms. My goal is to make errors more obvious when plugging wolfBoot into other projects - particularly those using their own build process.

The proposed WOLFBOOT_KEYHASH_HAS_RESULT breadcrumb ensures the wolfboot.h was included with WOLFBOOT_HASH_SHA256 defined, and that during the key_sha256() all of the expected hashing config defines are there.

Why is this only done in Sha256?

I added only for sha256 at this time to determine interest; if I improve the documentation and add to other signature types, would you be willing to accept in either in this PR and/or a future one?

Doesn't this break normal sha256?

More specific? It seems to be working as desired. I'd like to capture other cases & add to tests.

[danielinux]

I personally don't like this at all, even if you would name it properly with the DEBUG all in capitals it's still looks unnecessarily complex.

Compile time checks should be in headers, so we are sure they do not produce code.

Why would key_sha256() return -1 when this is not defined?

Please find a better place to validate compile time mismatch than the key

How can this work:

#ifndef WOLFBOOT_KEYHASH_HAS_RESULT
return -1;
#endif

If (as per default) the option is not enabled? Is this check misplaced?

What's the difference between WOLFBOOT_KEYHASH_HAS_RET later on and WOLFBOOT_KEYHASH_HAS_RESULT ?

Where are the configuration options to enable those with the normal Makefile buiild? any documentation?

[gojimmypi]

My prior explanation was poor.

This change was in the context of #614

Note that I updated this key_sha256 to instead return an int result:

static void key_sha256(uint8_t key_slot, uint8_t *hash)

As I didn't update all of the key hash functions, so I needed to know which returned a value, and which did not.

The WOLFBOOT_KEYHASH_HAS_RESULT is to answer the question:

"Does the key hash return a value, or is it a void function". See the new key_hash_ok function:

static inline int key_hash_ok(int id, uint8_t* digest)
{
#ifdef WOLFBOOT_KEYHASH_HAS_RET
    return key_hash(id, digest);
#else
    key_hash(id, digest);
    return 0;
#endif
}

For instance, as the key_sha384 is still a void function in this PR:

static void key_sha384(uint8_t key_slot, uint8_t *hash)

If any of these fail, the caller doesn't know:

    wc_InitSha384(&sha384_ctx);
    wc_Sha384Update(&sha384_ctx, pubkey, (word32)pubkey_sz);
    wc_Sha384Final(&sha384_ctx, hash);
    wc_Sha384Free(&sha384_ctx);

This code then forces an error, as there's a known return value, but the expected breadcrumb not found:

#ifndef WOLFBOOT_KEYHASH_HAS_RESULT
    wolfBoot_printf("This hash result must define WOLFBOOT_KEYHASH_HAS_RESULT");
    return -1;
#endif

If you are in favor of returning a result for all key_[hash] functions, I can update them all and omit the WOLFBOOT_KEYHASH_HAS_RESULT check.

[gojimmypi]

Converting to draft while I address code review: remove new key_hash_ok and WOLFBOOT_KEYHASH_HAS_RESULT; have all hash algos return values.