Move hardcoded certificates from Golioth subsystem into samples #398

sam-golioth · 2023-06-15T01:31:06Z

This PR removes the management of TLS certificates from the Golioth library. Clients are now responsible for setting TLS certificate credentials in the TLS Credential subsystem before starting the Golioth system, as well as persisting them on the device. In order to facilitate prototyping and experimentation, a new hardcoded_credentials module has been added to samples/common to provide the same functionality that used to be baked directly into the SDK. This gives application developers the flexibility to implement their own provisioning and storage mechanisms, while maintaining the same low-effort experience for users of the samples.

Note that this doesn't actually provide any facility for provisioning certificates, it only removes the parts of the SDK that prevented a customer from implementing provisioning themselves. In a future PR, I'll add a sample that shows one way a developer could provision certificates onto the device. This PR was getting larger than I initially expected, so I decided to break it into parts.

Note too that the new hardcoded_credentials module is also where I plan to put hardcoded PSK credentials for the samples after we remove that from the SDK. Bringing the hardcoded credentials code up into the samples should make it more obvious to users when they need to set credentials in Kconfig vs provisioned, as well as make the SDK itself cleaner.

github-actions · 2023-06-15T01:32:37Z

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

https://golioth-zephyr-sdk-doxygen-dev--pr398-certificate-prov-egk9u7x2.web.app

_{(expires Wed, 28 Jun 2023 17:07:23 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: a389eefadf4b4b68a539327b3459dd66c142cf49}

sam-golioth · 2023-06-15T01:39:15Z

net/golioth/CMakeLists.txt

-  foreach(type IN ITEMS ca crt key)
-    string(TOUPPER ${type} type_upper)
-    set(path ${CONFIG_GOLIOTH_SYSTEM_CLIENT_${type_upper}_PATH})
+  set(path ${CONFIG_GOLIOTH_SYSTEM_CLIENT_CA_PATH})


The CA cert stays hardcoded in the Golioth library for now because it is the same across the entire fleet. Eventually, we'll need a way to provision this certificate, but that is part of a larger story around rolling the CA certificate that is out of scope of these changes.

mniestroj

I like the direction this is going to, i.e. decoupling "system client" from TLS credentials.

We need something similar for PSK auth as well, so that user (or in our case samples/ implementation) will be responsible for setting credentials.

s/libray/library/ typo in commit message.

It looks like those 2 commits are not bisectable, so probably they should be squashed.

samples/common/hardcoded_credentials.c

mniestroj · 2023-06-15T07:18:33Z

net/golioth/system_client.c

+	int ret = 0;
 	if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_PSK)) {
 		golioth_settings_check_credentials();
+	} else if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_CERT)) {
+		ret = golioth_check_cert_credentials();
+	}
+
+	if (ret == 0) {
+		k_sem_give(&sys_client_started);
+	} else {
+		LOG_WRN("Error loading TLS credentials, golioth system client was not started");


Suggested change

int ret = 0;

if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_PSK)) {

golioth_settings_check_credentials();

} else if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_CERT)) {

ret = golioth_check_cert_credentials();

}

if (ret == 0) {

k_sem_give(&sys_client_started);

} else {

LOG_WRN("Error loading TLS credentials, golioth system client was not started");

int err = 0;

if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_PSK)) {

golioth_settings_check_credentials();

} else if (IS_ENABLED(CONFIG_GOLIOTH_AUTH_METHOD_CERT)) {

err = golioth_check_cert_credentials();

}

if (err) {

LOG_WRN("Error loading TLS credentials, golioth system client was not started");

} else {

k_sem_give(&sys_client_started);

I prefer to have the happy path first, and error handling in the else clause. I'll update to the err naming to match the rest of the file.

mniestroj · 2023-06-15T07:19:56Z

net/golioth/system_client.c

+				     TLS_CREDENTIAL_SERVER_CERTIFICATE, NULL, &cred_len);
+	if (-ENOENT == err) {
+		LOG_WRN("Certificate authentication configured, but no client certificate found");
+		goto finish;


Suggested change

goto finish;

return err;

I prefer to have a single return point from each function. In a small function like this, it doesn't make much difference, but in larger functions it helps to make the code more readable (subjective) and reduces the likelihood of bugs arising from improper cleanup (objective).

mniestroj · 2023-06-15T07:20:06Z

net/golioth/system_client.c

+				 TLS_CREDENTIAL_PRIVATE_KEY, NULL, &cred_len);
+	if (-ENOENT == err) {
+		LOG_WRN("Certificate authentication configured, but no private key found");
+		goto finish;


Suggested change

goto finish;

return err;

mniestroj · 2023-06-15T07:21:25Z

net/golioth/system_client.c

+	size_t cred_len = 0;
+	int err = tls_credential_get(CONFIG_GOLIOTH_SYSTEM_CLIENT_CREDENTIALS_TAG,
+				     TLS_CREDENTIAL_SERVER_CERTIFICATE, NULL, &cred_len);
+	if (-ENOENT == err) {


Suggested change

if (-ENOENT == err) {

if (err == -ENOENT) {

And how about other error codes?

This was put in specifically to guard against pion/dtls#529 (spent about 30 minutes confused before I remembered that one). I don't think there's value in checking other error codes. Right now, the only other one that is specified in the API is -EACCESS. This isn't actually returned anywhere in the implementation, but I imagine it's there in the case that a future TLS Credentials subsystem implementation uses a secure storage backend, which could allow writing keys but not reading them, or only allow reading keys from secure code.

net/golioth/system_client.c

sam-golioth · 2023-06-15T19:15:33Z

We need something similar for PSK auth as well, so that user (or in our case samples/ implementation) will be responsible for setting credentials.

Yup. In the PR description I talk about using the same hardcoded_credentials module to handle PSKs. It will be done as part of https://github.com/golioth/firmware-issue-tracker/issues/145

net/golioth/CMakeLists.txt

Clients are now responsible for setting certificates in the TLS Credential subsystem before starting Golioth samples: add support for hardcoded certs in hello sample The hello sample now shows a trivial example of loading certificates from files selected at build time. This does not demonstrate provisioning, but is the simplest way to show clients how to load certificates into the appropriate storage module so that the TLS library can find and use them. It also maintains the same ease-of-use for samples and experimentation. Signed-off-by: Sam Friedman <[email protected]>

szczys

LGTM. I tested this on an ESP32 and it works great!

Changes addressed

sam-golioth requested review from szczys and mniestroj June 15, 2023 01:31

sam-golioth force-pushed the certificate_provisioning_support branch from ba9dc5b to d567d8b Compare June 15, 2023 01:36

sam-golioth commented Jun 15, 2023

View reviewed changes

sam-golioth force-pushed the certificate_provisioning_support branch from d567d8b to cbca3fb Compare June 15, 2023 01:50

sam-golioth changed the title ~~Certificate provisioning support~~ Move hardcoded certificates from Golioth subsystem into samples Jun 15, 2023

sam-golioth force-pushed the certificate_provisioning_support branch from cbca3fb to c581432 Compare June 15, 2023 02:13

sam-golioth marked this pull request as ready for review June 15, 2023 02:18

mniestroj previously requested changes Jun 15, 2023

View reviewed changes

sam-golioth force-pushed the certificate_provisioning_support branch from c581432 to 3e386eb Compare June 15, 2023 19:39

sam-golioth requested a review from mniestroj June 15, 2023 19:40

szczys suggested changes Jun 20, 2023

View reviewed changes

net/golioth/CMakeLists.txt Outdated Show resolved Hide resolved

sam-golioth force-pushed the certificate_provisioning_support branch from 3e386eb to c3db7a1 Compare June 21, 2023 16:05

sam-golioth requested a review from szczys June 21, 2023 16:05

sam-golioth force-pushed the certificate_provisioning_support branch from c3db7a1 to bee50d8 Compare June 21, 2023 17:06

szczys approved these changes Jun 21, 2023

View reviewed changes

sam-golioth merged commit e36c3ae into main Jun 23, 2023

sam-golioth deleted the certificate_provisioning_support branch June 23, 2023 19:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move hardcoded certificates from Golioth subsystem into samples #398

Move hardcoded certificates from Golioth subsystem into samples #398

sam-golioth commented Jun 15, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading

sam-golioth Jun 15, 2023

mniestroj left a comment

mniestroj Jun 15, 2023

sam-golioth Jun 15, 2023

mniestroj Jun 15, 2023

sam-golioth Jun 15, 2023

mniestroj Jun 15, 2023

mniestroj Jun 15, 2023

sam-golioth Jun 15, 2023

sam-golioth commented Jun 15, 2023

szczys left a comment

Move hardcoded certificates from Golioth subsystem into samples #398

Move hardcoded certificates from Golioth subsystem into samples #398

Conversation

sam-golioth commented Jun 15, 2023 • edited Loading

github-actions bot commented Jun 15, 2023 • edited Loading

Choose a reason for hiding this comment

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 commented Jun 15, 2023

szczys left a comment

Choose a reason for hiding this comment

sam-golioth commented Jun 15, 2023 •

edited

Loading

github-actions bot commented Jun 15, 2023 •

edited

Loading