port: add sha256 API #664

szczys · 2024-10-19T22:01:01Z

Add an API for generating a sha256 hash that can be used to validate an OTA component.

Resolves https://github.com/golioth/firmware-issue-tracker/issues/717

github-actions · 2024-10-19T22:10:07Z

Visit the preview URL for this PR (updated for commit 9c4dd63):

https://golioth-firmware-sdk-doxygen-dev--pr664-szczys-sha256-z40limll.web.app

_{(expires Fri, 22 Nov 2024 20:47:49 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a9993e61697a3983f3479e468bcb0b616f9a0578}

szczys · 2024-10-19T22:40:35Z

clang-format seems to want to indent everything after after #define <whatever> line until the next in golioth_sys.h. I think this failure is incorrect. This does not fail locally for me. I have clang-format 18.1.5 installed via pip so I tried updating the workflow to v18 but the failure persists.

szczys · 2024-10-21T15:47:10Z

include/golioth/golioth_sys.h

@@ -121,6 +121,70 @@ void golioth_sys_thread_destroy(golioth_sys_thread_t thread);
 #define golioth_sys_rand() rand()
 #endif



When #666 merges I think we should guard the following function declarations so they are only available when CONFIG_GOLIOTH_OTA is selected.

The same goes for the definitions in each platform file.

sam-golioth · 2024-10-21T18:03:42Z

port/utils/hex.c

+    return 0;
+}
+
+size_t hex2bin(const char *hex, size_t hexlen, uint8_t *buf, size_t buflen)


It's really unfortunate that the manifest gives us a text string instead of a binary string for the SHA. In a manifest v2, I'd like to see this be binary, to reduce the payload size as well as the amount of computation that needs to be done on device.

Thinking about this some more, I think it would make sense to convert the hash to binary as we download it, and store it as binary in the manifest struct. A user of the SDK would have no reason to expect a hash in ASCII format, it's just an artifact of the cloud API, and we shouldn't let that implementation detail dictate the Firmware API.

Great idea, I've added a commit to store the hash as a binary array so it's ready for comparison.

sam-golioth · 2024-10-21T18:04:03Z

port/utils/hex.h

+ *
+ *  @return Zero on success or (negative) error code otherwise.
+ */
+int char2hex(char c, uint8_t *x);


Do we actually need this function to be public?

No, this is an error. Thanks for catching.

sam-golioth · 2024-10-21T18:06:28Z

port/utils/hex.c

+    }
+    else
+    {
+        return GOLIOTH_ERR_INVALID_FORMAT;


This is a positive value, which will cause the check on lines 47, 59, and 65 to fail. We should just leave this as -EINVAL, there's no reason to use golioth_status.h here, errno.h is part of the standard library and should be available on all platforms.

sam-golioth · 2024-10-21T18:10:18Z

port/zephyr/golioth_sys_zephyr.c

+struct golioth_hash
+{
+    mbedtls_sha256_context sha256_ctx;
+};


Why wrap this in another struct?

Originally I thought I would need more than just the context and didn't update this when that wasn't the case. I've changed to directly assign the struct point to the golioth typedef.

sam-golioth · 2024-10-21T18:17:26Z

include/golioth/golioth_sys.h

+/// Call on an existing context to begin generating a new sha256 hash.
+///
+/// @param sha_ctx A sha256 context handle
+void golioth_sys_sha256_init(golioth_sys_sha256_t sha_ctx);


I'm not sure we need both create()and init(). We don't need to provide a super-flexible, general API. We just need to provide hooks into the functionality that our SDK needs, ideally while minimizing the amount of work that a user of the SDK would need to do in order to port the SDK to another platform.

sam-golioth · 2024-10-21T18:18:19Z

include/golioth/golioth_sys.h

+/// Frees memory allocated to a sha256 context.
+///
+/// @param sha_ctx A sha256 context handle
+void golioth_sys_sha256_free(golioth_sys_sha256_t sha_ctx);


APIs should match semantically:

*_create() should be paired with *_destroy()

*_alloc() should be paired with *_free()

sam-golioth · 2024-10-21T18:18:54Z

include/golioth/golioth_sys.h

+/// @return GOLIOTH_OK On success
+/// @return GOLIOTH_ERR_FAIL On failure
+enum golioth_status golioth_sys_sha256_update(golioth_sys_sha256_t sha_ctx,
+                                              uint8_t *input,


We can make input const.

sam-golioth · 2024-10-22T16:50:07Z

port/zephyr/golioth_sys_zephyr.c

+    mbedtls_sha256_context sha256_ctx;
+};
+
+golioth_sys_sha256_t golioth_sys_sha256_create(void)


I'm back and forth on whether to use a typedef here, a straight void *, or a struct golioth_hash * with the incomplete definition in the header and the concrete definition in the source file.

I also went back and forth when considering your feedback. I decided to keep with the typedef. It uses the same memory as a void* but does a better job off indicating intent.

Add an API to manage sha256. This hash calculation is needed to verify OTA component integrity based on the hash supplied by the server in the OTA manifest. Signed-off-by: Mike Szczys <[email protected]>

Add two functions of hex.c from the Zephyr tree for use in converting OTA hex string sha256 hash to a binary array. Expose the hex2bin() function needed to compare against locally-calculated sha256 of downloaded assets. Signed-off-by: Mike Szczys <[email protected]>

Signed-off-by: Mike Szczys <[email protected]>

github-actions · 2024-11-15T20:36:25Z

Code Coverage

Package	Line Rate	Branch Rate	Health
include.golioth	75%	50%	✔
port.linux	44%	21%	❌
port.utils	58%	46%	➖
port.zephyr	46%	18%	❌
src	69%	32%	➖
Summary	66% (2518 / 3814)	31% (1085 / 3507)	➖

codecov · 2024-11-15T20:36:34Z

Codecov Report

Attention: Patch coverage is 11.49425% with 77 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
port/linux/golioth_sys_linux.c	0.00%	30 Missing ⚠️
port/zephyr/golioth_sys_zephyr.c	0.00%	30 Missing ⚠️
port/utils/hex.c	34.61%	11 Missing and 6 partials ⚠️

Files with missing lines	Coverage Δ
src/ota.c	`57.60% <100.00%> (ø)`
port/utils/hex.c	`34.61% <34.61%> (ø)`
port/linux/golioth_sys_linux.c	`45.81% <0.00%> (-11.91%)`	⬇️
port/zephyr/golioth_sys_zephyr.c	`53.47% <0.00%> (-14.08%)`	⬇️

... and 3 files with indirect coverage changes

Component hash is received from the server as a hex string. Convert it to a binary array when decoding the manifest so that it's ready to compare to a locally generated hash. Signed-off-by: Mike Szczys <[email protected]>

szczys force-pushed the szczys/sha256-api branch 2 times, most recently from fcdf4db to cae526c Compare October 19, 2024 22:09

szczys force-pushed the szczys/sha256-api branch 3 times, most recently from 4a25ae3 to 5417aff Compare October 19, 2024 22:34

szczys requested review from mniestroj and sam-golioth October 21, 2024 15:22

szczys commented Oct 21, 2024

View reviewed changes

sam-golioth reviewed Oct 22, 2024

View reviewed changes

szczys force-pushed the szczys/sha256-api branch from 5417aff to 270e269 Compare October 23, 2024 20:37

szczys added 4 commits October 23, 2024 15:44

golioth_sys: add sha256 API

611b67f

Add an API to manage sha256. This hash calculation is needed to verify OTA component integrity based on the hash supplied by the server in the OTA manifest. Signed-off-by: Mike Szczys <[email protected]>

port: zephyr: implement sha256 API

81cad76

Signed-off-by: Mike Szczys <[email protected]>

port: freertos: implement sha256 API

338de80

Signed-off-by: Mike Szczys <[email protected]>

szczys force-pushed the szczys/sha256-api branch from 270e269 to 35364fa Compare October 23, 2024 20:50

port: linux: implement sha256 API

7af79bb

Signed-off-by: Mike Szczys <[email protected]>

szczys force-pushed the szczys/sha256-api branch from 35364fa to 7af79bb Compare October 23, 2024 20:53

szczys requested a review from sam-golioth October 23, 2024 21:21

szczys force-pushed the szczys/sha256-api branch from 2d14017 to 7b79cdf Compare November 15, 2024 20:25

ota: store component hash as binary array

9c4dd63

Component hash is received from the server as a hex string. Convert it to a binary array when decoding the manifest so that it's ready to compare to a locally generated hash. Signed-off-by: Mike Szczys <[email protected]>

szczys force-pushed the szczys/sha256-api branch from 7b79cdf to 9c4dd63 Compare November 15, 2024 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

port: add sha256 API #664

port: add sha256 API #664

szczys commented Oct 19, 2024 •

edited

Loading

github-actions bot commented Oct 19, 2024 •

edited

Loading

szczys commented Oct 19, 2024

szczys Oct 21, 2024

sam-golioth Oct 21, 2024

sam-golioth Nov 7, 2024

szczys Nov 15, 2024

sam-golioth Oct 21, 2024

szczys Oct 23, 2024

sam-golioth Oct 21, 2024

sam-golioth Oct 21, 2024

szczys Oct 23, 2024

sam-golioth Oct 21, 2024

sam-golioth Oct 21, 2024

sam-golioth Oct 21, 2024

sam-golioth Oct 22, 2024

szczys Oct 23, 2024

github-actions bot commented Nov 15, 2024

codecov bot commented Nov 15, 2024

		@@ -121,6 +121,70 @@ void golioth_sys_thread_destroy(golioth_sys_thread_t thread);
		#define golioth_sys_rand() rand()
		#endif

port: add sha256 API #664

Are you sure you want to change the base?

port: add sha256 API #664

Conversation

szczys commented Oct 19, 2024 • edited Loading

github-actions bot commented Oct 19, 2024 • edited Loading

szczys commented Oct 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 15, 2024

Code Coverage

codecov bot commented Nov 15, 2024

Codecov Report

szczys commented Oct 19, 2024 •

edited

Loading

github-actions bot commented Oct 19, 2024 •

edited

Loading