net: lib: tls_credentials: Add CSR Generation #71242

glarsennordic · 2024-04-08T22:06:21Z

This PR contributes a number of commits which add CSR generation to the tls credentials subsystem.

Add optional tls_credential_csr function to the tls_credentials module, allowing users to request a private/public keypair be generated, stored (specifically the private key), and used to generate a CSR with the requested distinguished name / subject. Add an implementation of tls_credential_csr to the trusted credentials backend, and also apply a couple minor related refactors. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

Improve how the tls_credentials shell handls printing large blocks of data. Move credentials buffer dumping into its own static function. Add block prefix and block end strings to make parsing block data output easier. Prefix is printed before each line of block data, block end is printed on new line after block data has finished printing. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

Adds `cred csr` command to the TLS Credentials Shell, exposing the tls_credential_csr function. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

Add automated tests for tls_credential_csr. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

glarsennordic · 2024-04-08T22:08:41Z

There are some minor mistakes I am already aware of, which I will point out in a self-review and fix the next time I do a deep rebase.

The bigger issue is that the final commit, Test CSR Generation is not working at present. When I attempt on-board execution, it hangs.

There also probably need to be some docs changes reflecting the new command and the new block data output.

I am posting this draft now so that the CSR generation code can be reviewed.

jayteemo · 2024-04-08T22:49:00Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+	}
+
+	/* Destroy temporary PSA key if it was created */
+	if (key_created) {


instead of having a separate variable, would checking key_id != PSA_KEY_ID_NULL be sufficient ?

You're right, that should work, good catch

jayteemo · 2024-04-08T22:49:10Z

subsys/net/lib/tls_credentials/Kconfig

@@ -35,6 +36,12 @@ config TLS_CREDENTIALS_BACKEND_PROTECTED_STORAGE

 endchoice

+config TLS_CREDENTIAL_CSR
+	bool "Enable CSR and private/public keypair generation"


should this be marked EXPERIMENTAL until opaque keys can be used?
currently, the private key material is made available in non-secure memory.
this should be noted in an obvious place.

I think it should yes, I will do so

jayteemo · 2024-04-08T22:52:07Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+	mbedtls_pk_context pk_ctx;
+	mbedtls_x509write_csr writer;
+	void* cred_start = 0;
+	size_t csr_max = *csr_len;


should csr_len be checked for NULL?

Can't hurt to do so, good catch

jayteemo · 2024-04-08T23:05:06Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+	mbedtls_pk_context pk_ctx;
+	mbedtls_x509write_csr writer;
+	void* cred_start = 0;
+	size_t csr_max = *csr_len;


this could be const size_t csr_max = *csr_len;

jayteemo · 2024-04-08T23:19:26Z

include/zephyr/net/tls_credentials.h

@@ -124,6 +124,37 @@ int tls_credential_get(sec_tag_t tag, enum tls_credential_type type,
 */
 int tls_credential_delete(sec_tag_t tag, enum tls_credential_type type);

+/**
+ * @brief Create client private key and certificate signing request (CSR).


since this function does 2 things, any consideration for:

change to tls_credential_generate_key - with optional CSR or public key output
and/or
allow tls_credential_csr to use existing credentials

The thought has definitely crossed my mind.

My initial thought had been to add only tls_credentials_csr and then later add a tls_credential_keygen function which only performs keygen.

But you make a very good point; Users may want tls_credentials_csr to support using a pre-existing private key, so long as the credentials backend can handle it.

It will be some extra work, but I'll go ahead and implement tls_credential_keygen, and give tls_credentials_csr an option to use an existing PK sectag rather than generate one. I think you are correct that this would be cleaner, and it will save us headache down the line if we implement it now.

jayteemo · 2024-04-08T23:31:45Z

include/zephyr/net/tls_credentials.h

+/**
+ * @brief Create client private key and certificate signing request (CSR).
+ *
+ * @details This function generates a public/private keypair, and then signs a corresponding PKCS#10


add that SECP256R1 is used for key generation.

or add a config struct to allow user to specify key type/size along with CSR info such as country, organization, etc. (instead of only the char *dn parameter).

add that SECP256R1 is used for key generation.

The thought I had was that it could simply be up to the storage back-end which key type is used for generating the CSR.

or add a config struct to allow user to specify key type/size along with CSR info such as country, organization, etc. (instead of only the char *dn parameter).

The dn parameter does allow a full distinguished name to be specified, so country, organization, etc are all possible. I'd rather avoid re-inventing a struct representation for those values when MbedTLS already accepts them quite naturally as a string.

As for key-type customization, I am struggling to see a way to do this cleanly. There are a lot of concerns to balance:

If we need key type customization, we probably need it in command line too, so the system has to be compatible with that

We should, IMO, try to keep the tls_credentials module API agnostic of the cryptography backend being used (such as MbedTLS). So we cannot simply wrap MbedTLS types (very easily)

Others have expressed interest in expanding the TLS Credentials module to allow multiple simultaneous backends / APIs to create additional backends.

I suspect the cleanest way to handle this would probably be to allow a custom keytype to be requested via an optional string argument (alongside the dn argument). It would be up to the back-end to interpret/parse the keytype string, but this is probably the most versatile / interoperable option possible. I'd be willing to implement that in this PR.

Either that, or we will need to develop a full backend options customization system (similar to what sockets provide), and that, I think, should be a separate PR.

What do you think of the strings solution? @jayteemo

And @rlubos what are your thoughts on all of this as well?

The dn parameter does allow a full distinguished name to be specified, so country, organization, etc are all possible.

Ah ok, I didn't realize that accepted everything 👍🏻

I am referring to this.
Either indicate in the function header what it does or allow the user to specify their own key parameters.

Yeah, I understood you -- That section of code you highlight is in the secure credentials backend. While this is currently the only backend which supports CSR generation, it is conceivable that there might be other backends which also support CSR generation eventually.

Therefore, if we are letting the backends be in charge of what key types they support generating, it doesn't make sense to specify that in the header file (which describes the abstract API all backends conform to)

Allowing users to specify a desired key type for the back-end to consider does make some sense, But if we're going to do it, I think it will either need to come as part of another PR, or be a simple string argument which the backend is responsible for interpreting

I'm only asking that you pick one option:
Either document that only ecc 256 keys are supported or allow the user to specify.

Maybe the documenting should be done in kconfig, maybe with an option?

Either document that only ecc 256 keys are supported

I could document it in the back-end for which it is true, sure.

But I cannot document it in tls_credentials.h since which key-types are supported depends on the back-end in use.

or allow the user to specify.

The problem with that is that it is complicated to do cleanly. I was asking your opinion on using a pure string to allow this.

But I have decided after some research and thought that a simple enum listing a subset of the IANA supported groups is probably sufficient for this purpose.

But I cannot document it in tls_credentials.h since which key-types are supported depends on the back-end in use.

i understand, i suggested using kconfig.

a simple enum listing a subset of the IANA supported groups is probably sufficient for this purpose.

sounds good.

I'm not really an expert in the area of key generation, but from the API perspective I think it'd make sense to provide an option to select key type (or parameters?) instead of hardcoding this within the backend. If the backed does not support requested params, it could simply reject the request.

jayteemo · 2024-04-08T23:38:44Z

subsys/net/lib/tls_credentials/Kconfig

@@ -21,6 +21,7 @@ choice TLS_CREDENTIALS_BACKEND

 config TLS_CREDENTIALS_BACKEND_VOLATILE
 	bool "TLS credentials management volatile backend"
+	depends on !TLS_CREDENTIAL_CSR


should this dependency be moved to the TLS_CREDENTIAL_CSR option?
as in, TLS_CREDENTIAL_CSR depends on !TLS_CREDENTIALS_BACKEND_VOLATILE
or maybe better depends on TLS_CREDENTIALS_BACKEND_PROTECTED_STORAGE

Personally, I think it makes more sense for the backends to specify which features they support, rather than for the features to specify which backends support them

In other words, I don't so much see this as "TLS_CREDENTIAL_CSR requires TLS_CREDENTIALS_BACKEND_PROTECTED_STORAGE", but rather, "TLS_CREDENTIALS_BACKEND_VOLATILE does not support the optional TLS_CREDENTIAL_CSR feature"

Ok.
Use whatever the standard is for zephyr... I don't know what that is 🙈

glarsennordic · 2024-04-08T22:09:32Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

@@ -14,6 +14,9 @@
 #include "tls_internal.h"
 #include "tls_credentials_digest_raw.h"

+/* This lets check which MbedTLS features are enabled */


Suggested change

/* This lets check which MbedTLS features are enabled */

/* This lets us check which MbedTLS features are enabled */

glarsennordic · 2024-04-08T22:09:41Z

subsys/net/lib/tls_credentials/tls_credentials_digest_raw.c

-#include CONFIG_MBEDTLS_CFG_FILE
-#endif /* CONFIG_MBEDTLS_CFG_FILE */
-#endif /* CONFIG_MBEDTLS */
+/* This lets check which MbedTLS features are enabled */


Suggested change

/* This lets check which MbedTLS features are enabled */

/* This lets us check which MbedTLS features are enabled */

glarsennordic · 2024-04-08T22:09:50Z

subsys/net/lib/tls_credentials/tls_credentials_mbedtls_config.h

@@ -0,0 +1,24 @@
+/*
+ * Copyright (c) 2023 Nordic Semiconductor ASA


Update copyright

glarsennordic · 2024-04-08T22:10:14Z

subsys/net/lib/tls_credentials/tls_credentials_shell.c

+		return -EBADF;
+	}
+	return 0;
+	//TODO: Allow a customizable dump prefix.


Remove this TODO, its already done

rlubos · 2024-04-12T10:38:59Z

include/zephyr/net/tls_credentials.h

+/**
+ * @brief Create client private key and certificate signing request (CSR).
+ *
+ * @details This function generates a public/private keypair, and then signs a corresponding PKCS#10


I'm not really an expert in the area of key generation, but from the API perspective I think it'd make sense to provide an option to select key type (or parameters?) instead of hardcoding this within the backend. If the backed does not support requested params, it could simply reject the request.

rlubos · 2024-04-12T10:40:39Z

subsys/net/lib/tls_credentials/tls_credentials_mbedtls_config.h

+ * @brief Internal helper file which allows support for MbedTLS features to be checked easily.
+ */
+
+#ifndef __TLS_CREDEMTIALS_MBEDTLS_CONFIG_H


Typo, CREDEMTIALS

rlubos · 2024-04-12T10:44:15Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+#if defined(CONFIG_MBEDTLS) && defined(MBEDTLS_USE_PSA_CRYPTO) && defined(MBEDTLS_X509_CSR_WRITE_C)
+
+#include <psa/crypto.h>
+#include "mbedtls/pk.h"


#include <mbedtls/pk.h>, same below

rlubos · 2024-04-12T10:45:00Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+int tls_credential_csr(sec_tag_t tag, char *dn, void *csr, size_t *csr_len)
+{
+	int ret = 0;
+


I don't think we need those empty lines between variable definitons.

rlubos · 2024-04-12T10:47:57Z

subsys/net/lib/tls_credentials/tls_credentials_shell.c

+						"NULL-terminated, but a NULL-terminated "
+							"format was specified.\n");


Something weird with indentation here.

rlubos · 2024-04-12T10:50:29Z

subsys/net/lib/tls_credentials/tls_credentials_trusted.c

+	if (mbed_status) {
+		LOG_ERR("Failed to set up opaque private key. Status: %d", mbed_status);
+		ret = -EFAULT;
+		mbedtls_pk_free(&pk_ctx);


Isn't this done already after the cleanup label?

rlubos · 2024-04-12T10:55:15Z

tests/net/lib/tls_credentials/prj_trusted_tfm_csr.conf

@@ -0,0 +1,14 @@
+
+# Same as prj_trusted_tfm, but enable CSR generation.


This must've been overlooked in previous case, but since it's an overlay file it would be good to stick to common overlay file format (overlay-trusted-tfm-csr.conf).

rlubos · 2024-04-12T10:57:02Z

@carlescufi Any idea who else from the upstream could be interested in reviewing this?

github-actions · 2024-06-12T00:29:40Z

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

glarsennordic added 4 commits April 8, 2024 14:59

net: lib: tls_credentials: Add CSR Generation Command to shell

f167cee

Adds `cred csr` command to the TLS Credentials Shell, exposing the tls_credential_csr function. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

net: lib: tls_credentials: Test CSR Generation

a88895e

Add automated tests for tls_credential_csr. Signed-off-by: Georges Oates_Larsen <georges.larsen@nordicsemi.no>

glarsennordic requested a review from rlubos April 8, 2024 22:06

jayteemo reviewed Apr 8, 2024

View reviewed changes

glarsennordic commented Apr 8, 2024

View reviewed changes

rlubos reviewed Apr 12, 2024

View reviewed changes

github-actions bot added the Stale label Jun 12, 2024

	/* This lets check which MbedTLS features are enabled */
	/* This lets us check which MbedTLS features are enabled */

		@@ -0,0 +1,24 @@
		/*
		* Copyright (c) 2023 Nordic Semiconductor ASA

		"NULL-terminated, but a NULL-terminated "
		"format was specified.\n");

		@@ -0,0 +1,14 @@

		# Same as prj_trusted_tfm, but enable CSR generation.

net: lib: tls_credentials: Add CSR Generation #71242

Are you sure you want to change the base?

net: lib: tls_credentials: Add CSR Generation #71242

Conversation

glarsennordic commented Apr 8, 2024

glarsennordic commented Apr 8, 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

glarsennordic Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glarsennordic Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glarsennordic Apr 9, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glarsennordic Apr 9, 2024 • edited

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

glarsennordic Apr 9, 2024 • edited

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

rlubos commented Apr 12, 2024

github-actions bot commented Jun 12, 2024

glarsennordic Apr 9, 2024 •

edited

glarsennordic Apr 9, 2024 •

edited

glarsennordic Apr 9, 2024 •

edited

glarsennordic Apr 9, 2024 •

edited

glarsennordic Apr 9, 2024 •

edited