sign and validate with public keys (OpenSSL, mbedtls) #68

wgtdkp · 2019-05-30T12:24:15Z

This PR has two main changes:

expose the COSE_sign0_sign and COSE_Sign0_validate API with public keys;
kills all warnings;

For the first change: While using COSE_sign0_sign API in real applications, it is usual that we have the public key in format of DER or PEM encoded bytes which is parsed into mbedtls/openssl key structures. As the signing and validation functions are implemented with mbedtls/openssl, it would be much more convenient if user can sign/validate a message with those keys directly. And more, it reduces resources when applied in embedded systems.

For the second change: many applications force no warnings. As a library, COSE-C should also not emit any warnings. This does no harm at least.

coveralls · 2019-05-30T12:33:32Z

Coverage decreased (-2.7%) to 92.567% when pulling 6c43757 on wgtdkp:master into d6f46a3 on cose-wg:master.

coveralls · 2019-05-30T12:33:34Z

Coverage decreased (-2.7%) to 92.567% when pulling 6c43757 on wgtdkp:master into d6f46a3 on cose-wg:master.

sbertin-telular · 2019-05-30T12:36:13Z

src/Recipient.c

@@ -251,6 +251,10 @@ bool _COSE_Recipient_decrypt(COSE_RecipientInfo * pRecip, COSE_RecipientInfo * p
 	int cbitKeyX = 0;
 	byte rgbKey[256 / 8];

+	UNUSED(rgbKey);


This is confusing. Why would a local variable exist if it is unused. Probably need an #ifdef around the declaration instead.

sbertin-telular · 2019-05-30T12:38:55Z

src/Recipient.c

@@ -867,6 +874,8 @@ byte * _COSE_RecipientInfo_generateKey(COSE_RecipientInfo * pRecipient, int algI
 	const cn_cbor * pK;
 	byte *pbSecret = NULL;

+	UNUSED(algIn);


algIn is not unused in all cases. I'd like to see a clearer indication of when it is used and when it is not.

usages of algIn are all included in #if ... #endif preprocessor directives. algIn will never be used if none of those macros is defined. To make an unused indication, we need to write:

#if !USE_Direct_HKDF_HMAC_SHA_256 && !USE_Direct_HKDF_HMAC_SHA_512 && \ !USE_Direct_HKDF_AES_128 && !USE_Direct_HKDF_AES_256 && \ !USE_ECDH_ES_HKDF_256 && !USE_ECDH_ES_HKDF_512 && \ !USE_ECDH_SS_HKDF_256 && !USE_ECDH_SS_HKDF_512 UNUSED(algIn); #endif

Do we truly wants this everywhere?

I think the UNUSED() macro should be understood as MAYBE_UNUSED(). Or I can rename it like that.

I like the idea of a MAYBE_UNUSED(). I think that is more clear. Keep UNUSED() for parameters that are truly never used.

sbertin-telular · 2019-05-30T12:42:24Z

include/cose.h

+		struct ec_key_st *key;
+		int group;
+	} eckey_t;
+#endif // USE_MBED_TLS


Need #includes to get struct mbedtls_ecp_keypair of struct ec_key_st defined.

Since we are using struct mbedtls_ecp_keypair and struct ec_key_st by just pointers, they are not necessary be defined, but only need to be declared (forward declaration). by writing typedef struct mbedtls_ecp_keypair eckey_t;, the mbedtls_ecp_keypair structure got itself declared.

sbertin-telular · 2019-05-30T12:59:57Z

Adding a function to parse the public key in format of DER or PEM encoded bytes could allow the application to not care if the underlying encryption library is mbedtls or openssl.

jimsch · 2019-05-30T23:00:51Z

What happens when I want to add RSA to the library?

wgtdkp · 2019-05-31T05:34:29Z

Adding a function to parse the public key in format of DER or PEM encoded bytes could allow the application to not care if the underlying encryption library is mbedtls or openssl.

@sbertin-telular
I have been struggled to choose passing encoded bytes or the key structure. I didn't choose the encoded bytes because it needs parsing the key at each validation or signing. The public key is pre-shared which means it won't change frequently. Parsing the encoded keys every time could be a cost as I am using it in embedded system.

But I agree that passing encoded bytes hides the implementation, I can switch to this if you prefer it.

BTW, how about also replacing those cn_cbor* parameters by encoded cbor bytes?

wgtdkp · 2019-05-31T05:48:24Z

What happens when I want to add RSA to the library?

@jimsch
The APIs affected by this PR are only signing and validation functions, it seems RFC 8152 defines only EC keys for signing purpose. But to address the extensibility in case of COSE is going to add more algorithms, the mbedtls_pk_context of mbedtls can be used to replace the mbedtls_ecp_keypair.
Not sure if there is a general key representation in OpenSSL. Maybe a better solution is to passing keys by encoded bytes as @sbertin-telular suggested and my consideration is listed in the reply.

sbertin-telular · 2019-05-31T11:50:40Z

Adding a function to parse the public key in format of DER or PEM encoded bytes could allow the application to not care if the underlying encryption library is mbedtls or openssl.

I have been struggled to choose passing encoded bytes or the key structure.
@wgtdkp
I wasn't suggesting passing the encoded bytes to the validation or signing, but adding an additional function that would take the encoded bytes and decode to the key structure. The key structure could still be passed for validation or signing. This is simply so the application doesn't need to use openssl or mbedtls directly.

sbertin-telular · 2019-05-31T12:18:38Z

BTW, how about also replacing those cn_cbor* parameters by encoded cbor bytes?
@wgtdkp
I think hiding the CBOR implementation used would require major API changes. There are many places where cn_cbor* is returned also. I'm not sure it would be worth doing that. If it was done it should be a separate change.

wgtdkp · 2019-05-31T15:18:00Z

Adding a function to parse the public key in format of DER or PEM encoded bytes could allow the application to not care if the underlying encryption library is mbedtls or openssl.

I have been struggled to choose passing encoded bytes or the key structure.
@wgtdkp
I wasn't suggesting passing the encoded bytes to the validation or signing, but adding an additional function that would take the encoded bytes and decode to the key structure. The key structure could still be passed for validation or signing. This is simply so the application doesn't need to use openssl or mbedtls directly.

@sbertin-telular
Ok, I got it. That will be addressed in next commit.
How about providing both passing key structure and passing encoded bytes APIs? The second one will be just parsing the encoded key and calling the first API. User can choose don't care about implementation by passing in encoded bytes and can make better performance by passing in pre-parsed key structures.

sbertin-telular · 2019-05-31T15:21:41Z

Ok, I got it. How about providing both passing key structure and passing encoded bytes APIs? The second one will be just parsing the encoded key and calling the first API. User can choose don't care about implementation by passing in encoded bytes and can make better performance by passing in pre-parsed key structures.

Sounds fine to me. @jimsch what do you think? Ultimately it is your call.

jimsch · 2019-05-31T15:51:33Z

Trying to address everything in one comment:

RFC 8230 added RSA to the set of algorithms that COSE supports so the definitions do exist. There are some trusted modules that exist that only do RSA so there may be some desire to do this in the future. One also needs to think about issues such as the hash signature algorithm that is being defined in https://datatracker.ietf.org/doc/draft-ietf-cose-hash-algs.

One of the things I did not do in this library that I did do for the other two libraries is to define a new COSE_Key object structure. One can then create the COSE_Key from either encoded bytes, a DER object or even an underlying key object. One could create a COSE_Key from either an mbedtls key or from an openssl key directly. This provides the type of efficiency that I think is being considered both because the same key can be shared across multiple layers and because it can be kept around long term. Exposing different functions ifdef-ed by library usage works just fine for me.

I would probably not start with the same cbor library if I was doing this today, but that is life. I think that one needs to allow for returning the CBOR object rather than the encoded bytes because there are a number of situations such as CWTs where one object becomes nested in the next level up and doing an encoded/decode to do that seems wasteful.

jimsch · 2019-05-31T15:53:40Z

Top of my head design - needs some thoughts
struct {
int key type;
union {
cn_cbor * decoded cose key;
#ifdef MBEDTLS
mbedtls * mbed key;
#endif
}

and so forth.

wgtdkp · 2019-05-31T17:49:21Z

Top of my head design - needs some thoughts
struct {
int key type;
union {
cn_cbor * decoded cose key;
#ifdef MBEDTLS
mbedtls * mbed key;
#endif
}

and so forth.

I used COSE-JAVA also and it is nice to have a java PublicKey object converted to the OneKey.
But defining a new COSE_Key structure leads to great change of the APIs and the implementation. I think it would be better to do it in another separate PR.

gocarlos · 2020-04-16T07:56:26Z

what is the status of this MR? @wgtdkp

wgtdkp · 2020-04-16T08:45:37Z

what is the status of this MR? @wgtdkp

@gocarlos This PR, I think, is not perfect to be merged. The problems is that a public / private key encoded as a COSE key (CBOR format) cannot be easily constructed. For my usage, mbedtls keys are exclusively used.

While we should not change current API, so I added APIs like COSE_Sign0_Sign_eckey to accept mbedtls or OpenSSL public keys. But it is also not ideal, becuase we need to double the API number.

I think the best way is to have two functions that convert mbedtls and OpenSSL keys to cose keys in CBOR format. For example, the COSE-JAVA project has a OneKey contructor that converts Java Public / Private Key to COSE key:
https://github.com/cose-wg/COSE-JAVA/blob/0385f31bfcfb2793abb9d6a6560acf7f9a405269/src/main/java/COSE/OneKey.java#L68

gocarlos · 2020-04-16T20:14:42Z

I think the best way is to have two functions that convert mbedtls and OpenSSL keys to cose keys in CBOR format.

is this something you intend to contribute in a foreseeable future?

wgtdkp · 2020-04-17T03:25:29Z

is this something you intend to contribute in a foreseeable future?

Yes. Actually the convertion from mbedtls public key to COSE key has already been done in OT Commissioner:
https://github.com/openthread/ot-commissioner/blob/53cf7f3f5280804b9dff24e1e730a88cee9c30c7/src/library/cose.cpp#L248-L314

But I didn't get a chance to make this back to the master branch here. @gocarlos I am appreciate if you'd like to help to make it happen earlier, or I will handle this in next weeks.

jimsch · 2020-05-22T16:59:52Z

PR #120 provides this functionality for ECDSA

upgrade to support mbedtls 3.x

sign and validate using public key; kill warnings;

6c43757

sbertin-telular reviewed May 30, 2019

View reviewed changes

add definition of USE_MBED_TLS

059e8f4

wgtdkp mentioned this pull request Apr 16, 2020

update to upstream cose-c openthread/ot-commissioner#70

Open

gocarlos mentioned this pull request Apr 22, 2020

feat: add eckey_t #93

Open

wgtdkp added 2 commits April 2, 2024 19:51

upgrade to support mbedtls 3.x

d26157b

Merge pull request #1 from wgtdkp/update-mbedtls

9401d58

upgrade to support mbedtls 3.x

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sign and validate with public keys (OpenSSL, mbedtls) #68

sign and validate with public keys (OpenSSL, mbedtls) #68

wgtdkp commented May 30, 2019

coveralls commented May 30, 2019

coveralls commented May 30, 2019

sbertin-telular May 30, 2019

sbertin-telular May 30, 2019

wgtdkp May 31, 2019

sbertin-telular May 31, 2019

sbertin-telular May 30, 2019

wgtdkp May 31, 2019

sbertin-telular commented May 30, 2019

jimsch commented May 30, 2019

wgtdkp commented May 31, 2019

wgtdkp commented May 31, 2019 •

edited

Loading

sbertin-telular commented May 31, 2019

sbertin-telular commented May 31, 2019

wgtdkp commented May 31, 2019 •

edited

Loading

sbertin-telular commented May 31, 2019

jimsch commented May 31, 2019

jimsch commented May 31, 2019

wgtdkp commented May 31, 2019

gocarlos commented Apr 16, 2020 •

edited

Loading

wgtdkp commented Apr 16, 2020

gocarlos commented Apr 16, 2020

wgtdkp commented Apr 17, 2020

jimsch commented May 22, 2020

sign and validate with public keys (OpenSSL, mbedtls) #68

Are you sure you want to change the base?

sign and validate with public keys (OpenSSL, mbedtls) #68

Conversation

wgtdkp commented May 30, 2019

coveralls commented May 30, 2019

coveralls commented May 30, 2019

sbertin-telular May 30, 2019

Choose a reason for hiding this comment

sbertin-telular May 30, 2019

Choose a reason for hiding this comment

wgtdkp May 31, 2019

Choose a reason for hiding this comment

sbertin-telular May 31, 2019

Choose a reason for hiding this comment

sbertin-telular May 30, 2019

Choose a reason for hiding this comment

wgtdkp May 31, 2019

Choose a reason for hiding this comment

sbertin-telular commented May 30, 2019

jimsch commented May 30, 2019

wgtdkp commented May 31, 2019

wgtdkp commented May 31, 2019 • edited Loading

sbertin-telular commented May 31, 2019

sbertin-telular commented May 31, 2019

wgtdkp commented May 31, 2019 • edited Loading

sbertin-telular commented May 31, 2019

jimsch commented May 31, 2019

jimsch commented May 31, 2019

wgtdkp commented May 31, 2019

gocarlos commented Apr 16, 2020 • edited Loading

wgtdkp commented Apr 16, 2020

gocarlos commented Apr 16, 2020

wgtdkp commented Apr 17, 2020

jimsch commented May 22, 2020

wgtdkp commented May 31, 2019 •

edited

Loading

wgtdkp commented May 31, 2019 •

edited

Loading

gocarlos commented Apr 16, 2020 •

edited

Loading