auth: move PSK handling to samples #406

sam-golioth · 2023-07-05T19:22:10Z

move system settings into samples
add PSK support to hardcoded_credentials module
remove PSK handling in system_client
update samples to use the same auth for consistency

github-actions · 2023-07-05T19:23:17Z

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

https://golioth-zephyr-sdk-doxygen-dev--pr406-psk-refactor-ilew35wg.web.app

_{(expires Thu, 10 Aug 2023 22:03:17 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a389eefadf4b4b68a539327b3459dd66c142cf49}

mniestroj

This could use some commit splitting, as it is really hard to connect the dots of all the changes that happen here.

samples/settings/src/main.c

samples/common/Kconfig

samples/hello/README.rst

mniestroj · 2023-07-11T16:54:01Z

samples/hello/README.rst

@@ -184,6 +228,8 @@ This is the output from the serial console:
 Responses to Hello messages are printed above as a hexdump of "Hello mark". This
 means that communication with Golioth is working.

+.. _authentication methods: https://docs.golioth.io/firmware/zephyr-device-sdk/authentication/


No strong opinion here... but I'm a bit afraid of circular referencing (docs are written based this SDK, while here we start to reference those docs), since this page is strictly Golioth Zephyr SDK (this repo) related. It might quickly get outdated.

Good call out.

samples/hello/prj.conf

mniestroj · 2023-07-20T14:27:27Z

samples/settings/prj.conf

-CONFIG_SETTINGS_RUNTIME=y
-CONFIG_GOLIOTH_SAMPLE_SETTINGS_AUTOLOAD=y
-CONFIG_GOLIOTH_SAMPLE_SETTINGS_SHELL=y
-


Some time ago there was a request to do exactly the opposite - enable storing PSK-ID/PSK in settings subsystem, so that the DFU sample can be flashed on multiple devices as is and then do a DFU of the fleet (which might consist of 2 devices, but still). I see the advantage of using hardcoded credentials (consistence as you mention in commit message), but in the end there are PROs and CONs.

@szczys any opinion on that? It is likely that devrel team was requesting "non-hardcoded" credentials for this sample.

enable storing PSK-ID/PSK in settings subsystem, so that the DFU sample can be flashed on multiple devices as is and then do a DFU of the fleet

Yes, this is how precompiled binaries for the zephyr-training, thingy91demo, and all the reference designs currently work. We're using PSK auth but it's not hardcoded into the firmware.

Hardcoded PSKs are just the default. As part of this PR, there's a new overlay config file in samples/common that can enable runtime PSKs via the settings subsystem for any of our samples, e.g.:
west build -p -b esp32 samples/dfu -- -DOVERLAY_CONFIG="../common/runtime_psk.conf"

So this should give us the best of both worlds, while maintaining consistency among our samples. All the samples use hardcoded PSKs by default, and all can be changed to use runtime PSKs and the settings shell with a single build argument. This is documented in the READMEs for each of the samples now, e.g.: https://github.com/golioth/golioth-zephyr-sdk/pull/406/files#diff-40c1e6da1ab07e4c146dae9c4f8ecf4c2dd4afdf703d11f21d10dc6b2e4898c3R46

samples/common/hardcoded_credentials.c

net/golioth/Kconfig

mniestroj · 2023-07-20T17:56:25Z

net/golioth/Kconfig

-config GOLIOTH_SYSTEM_SETTINGS
-	bool "Load credentials from persistent settings"
-	default y
-	depends on GOLIOTH_AUTH_METHOD_PSK
-	depends on SETTINGS
-	help
-	  When selected, Golioth credentials will be loaded from settings
-	  subsystem.
-
-if GOLIOTH_SYSTEM_SETTINGS
-


There were good and bad things about it.

Bad:

coupled too much with core SDK functionality

Good:

IMO this was bringing value to customers, who wanted to quickly setup downstream project and use some persisent storage for credentials

By moving it into samples/common/ we actuall remove any persistent storage support "out of the box". IMO we should decouple it from core SDK (as it is done in this commit), but put it under Kconfig option, that will let users select that functionality for their downstream applications.

Whatever we decide, there should be some rationale in commit message WHY we do it.

CC @golioth/developer-relations

Most notable in my mind is persistent storage for WiFI credentials. Currently we use the shell to set to those in the storage partition.

What do you think about creating something like a util or snippets folder to hold things that could be useful for both our samples and customer applications, but that we don't want to include as part of the "core" SDK?

I'm fine with providing things like the setting shell as an option for users, but I want to be intentional and clear about where we're being opinionated, and where we're just showing the user one way among many of doing things.

Upstream Zephyr has now support for connection/network manager with some functionalities implemented like management of WiFi (or other medium type) interface. Such "generic" functionalities make most sense to be included as part of Zephyr, since those are not cloud-provider (Golioth) specific. We should make use of those as much as possible, so that:
a) we don't maintain too much utilities that are not directly related to Golioth as a platform
b) it makes less friction for users that want to integrate their existing (or under development) app into Golioth

In other words, it would be great to provide Golioth SDK users some utilities, even for downstream applications. But in those cases it makes more sense to upstream such feature and let it be improved by larger community. Development of such feature (if it is well-thought) should take the same time in both cases. So in most cases some example fits best in samples/common/ IMO.

In case of PSK-ID/PSK TLS credentials, I think that the ideal situation would be to provide a "TLS credentials storage which utilizes Zephyr settings subsystem". The only thing that would be Golioth-specific is the TLS ID which is used to reference those credentials. This TLS ID is already configured in Kconfig. The rest of it make sense to be put in Zephyr upstream. We can create something temporarily here in SDK until a "proper" implementation is merged to Zephyr, so that we don't introduce any latency between development and being able to use it.

Settings shell is one example why above strategy with upstreaming would be beneficial. First a settings shell was created in Golioth SDK. Shortly after that a different implementation (but with similar functionality in mind) was merged to Zephyr. Then there was a conflict between Kconfig symbols. It was solved (by prepending "our" service with GOLIOTH_ in Kconfig option name). Right now there is still a conflict in shell module, so either one is visible or the other, since both commands are named settings.

I like this a lot! I agree, upstreaming is the way to go.

I filed https://github.com/golioth/firmware-issue-tracker/issues/183 and https://github.com/golioth/firmware-issue-tracker/issues/184 for some of the work that you mentioned, feel free to edit/add/delete.

We can create something temporarily here in SDK until a "proper" implementation is merged to Zephyr

Where should that live? Still samples/common or do we make another folder for things that are in the process of being upstreamed?

mniestroj

LGTM, in terms of code changes.

Would be good to have @golioth/developer-relations approval (doesn't have to be here).

sam-golioth · 2023-08-03T21:48:15Z

LGTM, in terms of code changes.

Would be good to have @golioth/developer-relations approval (doesn't have to be here).

Confirmed with @ChrisGammell

- Use SYS_INIT() to automatically set hardcoded credentials Signed-off-by: Sam Friedman <[email protected]>

Uses the hardcoded_credentials module in samples/common to load hardcoded PSKs. Signed-off-by: Sam Friedman <[email protected]>

Standardize auth across samples so that all samples use hardcoded PSK by default. Signed-off-by: Sam Friedman <[email protected]>

System settings (for runtime PSK setting) is moved to samples/common Signed-off-by: Sam Friedman <[email protected]>

Separate WiFi settings from PSK settings. It was non-obvious that enabling runtime PSK settings also enabled runtime WiFi settings. This change makes it so enabling runtime WiFi settings must be an explicit action on the part of the user. Signed-off-by: Sam Friedman <[email protected]>

Adding test coverage (build only) for enabling the PSK settings shell in the hello sample. Signed-off-by: Sam Friedman <[email protected]>

Updating the README for all of the samples to standardizze authentication information. All samples use hardcoded PSKs by default, and can be configured to use runtime PSKs or hardcoded certificates. Signed-off-by: Sam Friedman <[email protected]>

sam-golioth force-pushed the psk_refactor branch 11 times, most recently from c8f5899 to 94c9630 Compare July 6, 2023 21:53

sam-golioth requested review from mniestroj and szczys July 7, 2023 01:38

sam-golioth marked this pull request as ready for review July 7, 2023 01:38

mniestroj reviewed Jul 11, 2023

View reviewed changes

sam-golioth force-pushed the psk_refactor branch 2 times, most recently from 2fd3874 to f3a3d65 Compare July 16, 2023 20:20

sam-golioth requested a review from mniestroj July 16, 2023 20:23

sam-golioth force-pushed the psk_refactor branch 6 times, most recently from 637381e to 56b02f7 Compare July 19, 2023 20:45

mniestroj reviewed Jul 20, 2023

View reviewed changes

sam-golioth mentioned this pull request Jul 26, 2023

Zephyr: examples: add hello golioth/golioth-firmware-sdk#129

Merged

sam-golioth force-pushed the psk_refactor branch from 56b02f7 to 16fa412 Compare July 31, 2023 18:35

mniestroj approved these changes Aug 3, 2023

View reviewed changes

sam-golioth force-pushed the psk_refactor branch from 16fa412 to 4d31c3f Compare August 3, 2023 21:57

sam-golioth added 7 commits August 3, 2023 18:01

samples: auto set hardcoded_credentials

f710a16

- Use SYS_INIT() to automatically set hardcoded credentials Signed-off-by: Sam Friedman <[email protected]>

auth: Move hardcoded PSKs to samples

f1f5ad2

Uses the hardcoded_credentials module in samples/common to load hardcoded PSKs. Signed-off-by: Sam Friedman <[email protected]>

samples: Use hardcoded PSK by default

04ef856

Standardize auth across samples so that all samples use hardcoded PSK by default. Signed-off-by: Sam Friedman <[email protected]>

system settings: move system settings to samples

0de3360

System settings (for runtime PSK setting) is moved to samples/common Signed-off-by: Sam Friedman <[email protected]>

samples: add build test for runtime PSK settings

8f8866e

Adding test coverage (build only) for enabling the PSK settings shell in the hello sample. Signed-off-by: Sam Friedman <[email protected]>

samples: Update auth section in READMEs

d6c3234

Updating the README for all of the samples to standardizze authentication information. All samples use hardcoded PSKs by default, and can be configured to use runtime PSKs or hardcoded certificates. Signed-off-by: Sam Friedman <[email protected]>

sam-golioth force-pushed the psk_refactor branch from 4d31c3f to d6c3234 Compare August 3, 2023 22:02

sam-golioth merged commit 8f46b5e into main Aug 3, 2023
8 checks passed

mniestroj deleted the psk_refactor branch August 4, 2023 06:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: move PSK handling to samples #406

auth: move PSK handling to samples #406

sam-golioth commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 •

edited

Loading

mniestroj left a comment

mniestroj Jul 11, 2023

sam-golioth Jul 11, 2023

mniestroj Jul 20, 2023

szczys Jul 20, 2023

sam-golioth Jul 31, 2023

mniestroj Jul 20, 2023

szczys Jul 20, 2023

sam-golioth Jul 31, 2023 •

edited

Loading

mniestroj Aug 1, 2023

sam-golioth Aug 1, 2023

mniestroj left a comment

sam-golioth commented Aug 3, 2023

auth: move PSK handling to samples #406

auth: move PSK handling to samples #406

Conversation

sam-golioth commented Jul 5, 2023

github-actions bot commented Jul 5, 2023 • edited Loading

mniestroj left a 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

sam-golioth Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mniestroj left a comment

Choose a reason for hiding this comment

sam-golioth commented Aug 3, 2023

github-actions bot commented Jul 5, 2023 •

edited

Loading

sam-golioth Jul 31, 2023 •

edited

Loading