[PATCH 1/5] util: Remove semicolons in single-statement macros

[PATCH 1/5] util: Remove semicolons in single-statement macros

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 882 bytes --]

Including semicolons in L_AUTO_CLEANUP_VAR and L_AUTO_FREE_VAR caused
gcc to emit errors related to mixed declarations and code when there
were multiple L_AUTO_* lines or declarations following an L_AUTO_ macro.
---
 ell/util.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ell/util.h b/ell/util.h
index 92e7c8a..8d1050e 100644
--- a/ell/util.h
+++ b/ell/util.h
@@ -160,10 +160,10 @@ static inline void l_put_be64(uint64_t val, void *ptr)
 }
 
 #define L_AUTO_CLEANUP_VAR(vartype,varname,destroy) \
-	vartype varname __attribute__((cleanup(destroy)));
+	vartype varname __attribute__((cleanup(destroy)))
 
 #define L_AUTO_FREE_VAR(vartype,varname) \
-	vartype varname __attribute__((cleanup(auto_free)));
+	vartype varname __attribute__((cleanup(auto_free)))
 
 #define L_ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
 
-- 
2.10.1

[PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 3493 bytes --]

Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is
useful to let the kernel keep a key linked even if ELL isn't directly
keeping track of that key anymore - for example, a keyring of trusted
keys can be used for validation without keeping l_key objects around for
every single key in that keyring. The kernel will clean up the kernel
key objects when there are no more references to them whether or not we
explicitly revoke from userspace.
---
 ell/key.c | 14 ++++++++++----
 ell/key.h |  4 ++--
 ell/tls.c |  6 +++---
 3 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/ell/key.c b/ell/key.c
index 4cf2307..4225271 100644
--- a/ell/key.c
+++ b/ell/key.c
@@ -276,12 +276,15 @@ LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
 	return key;
 }
 
-LIB_EXPORT void l_key_free(struct l_key *key)
+LIB_EXPORT void l_key_free(struct l_key *key, bool revoke)
 {
 	if (unlikely(!key))
 		return;
 
-	kernel_revoke_key(key->serial);
+	if (revoke)
+		kernel_revoke_key(key->serial);
+	else
+		kernel_unlink_key(key->serial, internal_keyring);
 
 	l_free(key);
 }
@@ -693,12 +696,15 @@ LIB_EXPORT struct l_keyring *l_keyring_new(enum l_keyring_type type,
 	return keyring;
 }
 
-LIB_EXPORT void l_keyring_free(struct l_keyring *keyring)
+LIB_EXPORT void l_keyring_free(struct l_keyring *keyring, bool revoke)
 {
 	if (unlikely(!keyring))
 		return;
 
-	kernel_revoke_key(keyring->serial);
+	if (revoke)
+		kernel_revoke_key(keyring->serial);
+	else
+		kernel_unlink_key(keyring->serial, internal_keyring);
 
 	l_free(keyring);
 }
diff --git a/ell/key.h b/ell/key.h
index e7036c6..0c84000 100644
--- a/ell/key.h
+++ b/ell/key.h
@@ -54,7 +54,7 @@ enum l_key_cipher_type {
 struct l_key *l_key_new(enum l_key_type type, const void *payload,
 			size_t payload_length);
 
-void l_key_free(struct l_key *key);
+void l_key_free(struct l_key *key, bool revoke);
 
 bool l_key_update(struct l_key *key, const void *payload, size_t len);
 
@@ -91,7 +91,7 @@ bool l_key_verify(struct l_key *key, enum l_key_cipher_type cipher,
 struct l_keyring *l_keyring_new(enum l_keyring_type type,
 				const struct l_keyring *trust);
 
-void l_keyring_free(struct l_keyring *keyring);
+void l_keyring_free(struct l_keyring *keyring, bool revoke);
 
 bool l_keyring_link(struct l_keyring *keyring, const struct l_key *key);
 
diff --git a/ell/tls.c b/ell/tls.c
index bc4c210..a89b2db 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -135,7 +135,7 @@ static void tls_reset_handshake(struct l_tls *tls)
 	memset(tls->pending.key_block, 0, sizeof(tls->pending.key_block));
 
 	l_free(tls->peer_cert);
-	l_key_free(tls->peer_pubkey);
+	l_key_free(tls->peer_pubkey, true);
 
 	tls->peer_cert = NULL;
 	tls->peer_pubkey = NULL;
@@ -2211,7 +2211,7 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
 	}
 
 	if (tls->priv_key) {
-		l_key_free(tls->priv_key);
+		l_key_free(tls->priv_key, true);
 		tls->priv_key = NULL;
 		tls->priv_key_size = 0;
 	}
@@ -2234,7 +2234,7 @@ LIB_EXPORT bool l_tls_set_auth_data(struct l_tls *tls, const char *cert_path,
 		if (!l_key_get_info(tls->priv_key, L_KEY_RSA_PKCS1_V1_5,
 					L_CHECKSUM_NONE, &tls->priv_key_size,
 					&is_public) || is_public) {
-			l_key_free(tls->priv_key);
+			l_key_free(tls->priv_key, true);
 			tls->priv_key = NULL;
 			tls->priv_key_size = 0;
 			return false;
-- 
2.10.1

[PATCH 3/5] unit: Update for new l_key_free/l_keyring_free revoke parameter

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2154 bytes --]

---
 unit/test-key.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/unit/test-key.c b/unit/test-key.c
index 1195da4..124e2ce 100644
--- a/unit/test-key.c
+++ b/unit/test-key.c
@@ -83,7 +83,7 @@ static void test_user(const void *data)
 	assert(len == KEY2_LEN);
 	assert(!strcmp(buf, KEY2_STR));
 
-	l_key_free(key);
+	l_key_free(key, true);
 }
 
 struct dh_test_vector {
@@ -231,7 +231,7 @@ static void testkey_from_string(const char *keystr, struct testkey *tk)
 
 static void testkey_free_contents(struct testkey *tk)
 {
-	l_key_free(tk->key);
+	l_key_free(tk->key, true);
 	l_free(tk->bytes);
 }
 
@@ -358,11 +358,11 @@ static void test_simple_keyring(const void *data)
 	success = l_keyring_link(ring, key2);
 	assert(success);
 
-	l_key_free(key1);
+	l_key_free(key1, true);
 	success = l_keyring_unlink(ring, key2);
 	assert(success);
-	l_keyring_free(ring);
-	l_key_free(key2);
+	l_keyring_free(ring, true);
+	l_key_free(key2, true);
 }
 
 static void test_trusted_keyring(const void *data)
@@ -400,10 +400,10 @@ static void test_trusted_keyring(const void *data)
 	success = l_keyring_link(ring, key);
 	assert(success);
 
-	l_keyring_free(trust);
-	l_keyring_free(ring);
-	l_key_free(cakey);
-	l_key_free(key);
+	l_keyring_free(trust, true);
+	l_keyring_free(ring, true);
+	l_key_free(cakey, true);
+	l_key_free(key, true);
 	l_free(cacert);
 	l_free(cert);
 }
@@ -457,10 +457,10 @@ static void test_trust_chain(const void *data)
 	success = l_keyring_link(ring, key);
 	assert(success);
 
-	l_keyring_free(trust);
-	l_keyring_free(ring);
-	l_key_free(cakey);
-	l_key_free(key);
+	l_keyring_free(trust, true);
+	l_keyring_free(ring, true);
+	l_key_free(cakey, true);
+	l_key_free(key, true);
 	l_free(cacert);
 	l_free(cert);
 }
@@ -638,8 +638,8 @@ static void test_key_crypto(const void *data)
 				strlen(plaintext), sizeof(ciphertext));
 	assert(!success);
 
-	l_key_free(key);
-	l_key_free(pubkey);
+	l_key_free(key, true);
+	l_key_free(pubkey, true);
 	l_free(cert);
 	l_free(pubcert);
 }
-- 
2.10.1

[PATCH 4/5] tls: Validate cert chain using l_keyring

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1829 bytes --]

---
 ell/tls.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/ell/tls.c b/ell/tls.c
index a89b2db..55b3b1a 100644
--- a/ell/tls.c
+++ b/ell/tls.c
@@ -2464,10 +2464,63 @@ static const struct pkcs1_encryption_oid {
 	},
 };
 
+static void tls_key_cleanup(struct l_key **p)
+{
+	l_key_free(*p, false);
+}
+
+static bool tls_cert_verify_with_keyring(struct tls_cert *cert,
+						struct l_keyring *ring)
+{
+	if (!cert)
+		return true;
+
+	if (tls_cert_verify_with_keyring(cert->issuer, ring)) {
+		L_AUTO_CLEANUP_VAR(struct l_key *, key, tls_key_cleanup);
+
+		key = l_key_new(L_KEY_RSA, cert->asn1, cert->size);
+		if (!key)
+			return false;
+
+		return l_keyring_link(ring, key);
+	}
+
+	return false;
+}
+
+static void tls_keyring_cleanup(struct l_keyring **p)
+{
+	l_keyring_free(*p, true);
+}
+
 bool tls_cert_verify_certchain(struct tls_cert *certchain,
 				struct tls_cert *ca_cert)
 {
-	return true;
+	L_AUTO_CLEANUP_VAR(struct l_keyring *, ca_ring, tls_keyring_cleanup);
+	L_AUTO_CLEANUP_VAR(struct l_keyring *, verify_ring,
+				tls_keyring_cleanup);
+
+	ca_ring = NULL;
+	verify_ring = NULL;
+
+	if (ca_cert) {
+		L_AUTO_CLEANUP_VAR(struct l_key *, ca_key, tls_key_cleanup);
+		ca_key = NULL;
+
+		ca_ring = l_keyring_new(L_KEYRING_SIMPLE, NULL);
+		if (!ca_ring)
+			return false;
+
+		ca_key = l_key_new(L_KEY_RSA, ca_cert->asn1, ca_cert->size);
+		if (!ca_key || !l_keyring_link(ca_ring, ca_key))
+			return false;
+	}
+
+	verify_ring = l_keyring_new(L_KEYRING_TRUSTED_ASYM_CHAIN, ca_ring);
+	if (!verify_ring)
+		return false;
+
+	return tls_cert_verify_with_keyring(certchain, verify_ring);
 }
 
 void tls_cert_free_certchain(struct tls_cert *cert)
-- 
2.10.1

[PATCH 5/5] unit: Fix memory leak in trust chain test

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 484 bytes --]

---
 unit/test-key.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/unit/test-key.c b/unit/test-key.c
index 124e2ce..e4d0ba4 100644
--- a/unit/test-key.c
+++ b/unit/test-key.c
@@ -460,8 +460,10 @@ static void test_trust_chain(const void *data)
 	l_keyring_free(trust, true);
 	l_keyring_free(ring, true);
 	l_key_free(cakey, true);
+	l_key_free(intkey, true);
 	l_key_free(key, true);
 	l_free(cacert);
+	l_free(intcert);
 	l_free(cert);
 }
 
-- 
2.10.1

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]


Denis -

On Mon, 24 Oct 2016, Mat Martineau wrote:

> Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is

"unlinks them from any keyring" is what I meant. Can you fix that up 
before committing?

> useful to let the kernel keep a key linked even if ELL isn't directly
> keeping track of that key anymore - for example, a keyring of trusted
> keys can be used for validation without keeping l_key objects around for
> every single key in that keyring. The kernel will clean up the kernel
> key objects when there are no more references to them whether or not we
> explicitly revoke from userspace.
> ---
> ell/key.c | 14 ++++++++++----
> ell/key.h |  4 ++--
> ell/tls.c |  6 +++---
> 3 files changed, 15 insertions(+), 9 deletions(-)

--
Mat Martineau
Intel OTC

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1509 bytes --]

Hi Mat,

> Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is
> useful to let the kernel keep a key linked even if ELL isn't directly
> keeping track of that key anymore - for example, a keyring of trusted
> keys can be used for validation without keeping l_key objects around for
> every single key in that keyring. The kernel will clean up the kernel
> key objects when there are no more references to them whether or not we
> explicitly revoke from userspace.
> ---
> ell/key.c | 14 ++++++++++----
> ell/key.h |  4 ++--
> ell/tls.c |  6 +++---
> 3 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/ell/key.c b/ell/key.c
> index 4cf2307..4225271 100644
> --- a/ell/key.c
> +++ b/ell/key.c
> @@ -276,12 +276,15 @@ LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
> 	return key;
> }
> 
> -LIB_EXPORT void l_key_free(struct l_key *key)
> +LIB_EXPORT void l_key_free(struct l_key *key, bool revoke)
> {

I am not a huge fan of glueing on booleans to simple functions like l_key_free. They should be doing one thing and one thing only. Frankly later on nobody remembers what the difference between l_key_free(x, true) and l_key_free(x, false) was. It becomes to cumbersome to read and remember in application code.

So why not use l_key_free(x) and then introduce l_key_revoke_and_free(x). Or just simply go with l_key_revoke(x) and have the API description be clear that it also frees the key.

Regards

Marcel

Re: [PATCH 1/5] util: Remove semicolons in single-statement macros

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 408 bytes --]

Hi Mat,

On 10/24/2016 01:44 PM, Mat Martineau wrote:
> Including semicolons in L_AUTO_CLEANUP_VAR and L_AUTO_FREE_VAR caused
> gcc to emit errors related to mixed declarations and code when there
> were multiple L_AUTO_* lines or declarations following an L_AUTO_ macro.
> ---
>   ell/util.h | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>

Applied, thanks.

Regards,
-Denis

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1710 bytes --]

Hi Mat,

On 10/24/2016 01:55 PM, Marcel Holtmann wrote:
> Hi Mat,
>
>> Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is
>> useful to let the kernel keep a key linked even if ELL isn't directly
>> keeping track of that key anymore - for example, a keyring of trusted
>> keys can be used for validation without keeping l_key objects around for
>> every single key in that keyring. The kernel will clean up the kernel
>> key objects when there are no more references to them whether or not we
>> explicitly revoke from userspace.
>> ---
>> ell/key.c | 14 ++++++++++----
>> ell/key.h |  4 ++--
>> ell/tls.c |  6 +++---
>> 3 files changed, 15 insertions(+), 9 deletions(-)
>>
>> diff --git a/ell/key.c b/ell/key.c
>> index 4cf2307..4225271 100644
>> --- a/ell/key.c
>> +++ b/ell/key.c
>> @@ -276,12 +276,15 @@ LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
>> 	return key;
>> }
>>
>> -LIB_EXPORT void l_key_free(struct l_key *key)
>> +LIB_EXPORT void l_key_free(struct l_key *key, bool revoke)
>> {
>
> I am not a huge fan of glueing on booleans to simple functions like l_key_free. They should be doing one thing and one thing only. Frankly later on nobody remembers what the difference between l_key_free(x, true) and l_key_free(x, false) was. It becomes to cumbersome to read and remember in application code.
>
> So why not use l_key_free(x) and then introduce l_key_revoke_and_free(x). Or just simply go with l_key_revoke(x) and have the API description be clear that it also frees the key.
>

+1.  Although we do have precedent for this in l_string_free, but that 
is mostly how GLib API was done.

Regards,
-Denis

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

On Mon, 24 Oct 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 10/24/2016 01:55 PM, Marcel Holtmann wrote:
>> Hi Mat,
>> 
>>> Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is
>>> useful to let the kernel keep a key linked even if ELL isn't directly
>>> keeping track of that key anymore - for example, a keyring of trusted
>>> keys can be used for validation without keeping l_key objects around for
>>> every single key in that keyring. The kernel will clean up the kernel
>>> key objects when there are no more references to them whether or not we
>>> explicitly revoke from userspace.
>>> ---
>>> ell/key.c | 14 ++++++++++----
>>> ell/key.h |  4 ++--
>>> ell/tls.c |  6 +++---
>>> 3 files changed, 15 insertions(+), 9 deletions(-)
>>> 
>>> diff --git a/ell/key.c b/ell/key.c
>>> index 4cf2307..4225271 100644
>>> --- a/ell/key.c
>>> +++ b/ell/key.c
>>> @@ -276,12 +276,15 @@ LIB_EXPORT struct l_key *l_key_new(enum l_key_type 
>>> type, const void *payload,
>>> 	return key;
>>> }
>>> 
>>> -LIB_EXPORT void l_key_free(struct l_key *key)
>>> +LIB_EXPORT void l_key_free(struct l_key *key, bool revoke)
>>> {
>> 
>> I am not a huge fan of glueing on booleans to simple functions like 
>> l_key_free. They should be doing one thing and one thing only. Frankly 
>> later on nobody remembers what the difference between l_key_free(x, true) 
>> and l_key_free(x, false) was. It becomes to cumbersome to read and remember 
>> in application code.
>> 
>> So why not use l_key_free(x) and then introduce l_key_revoke_and_free(x). 
>> Or just simply go with l_key_revoke(x) and have the API description be 
>> clear that it also frees the key.
>> 
>
> +1.  Although we do have precedent for this in l_string_free, but that is 
> mostly how GLib API was done.

Ok, I'll go with l_key_revoke_and_free(). It was l_string_free that sent 
me in the original add-a-flag direction, but that's also a function that 
has been a source of confusion.

--
Mat Martineau
Intel OTC

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Marcel Holtmann]

[-- Attachment #1: Type: text/plain, Size: 1975 bytes --]

Hi Mat,

>>>> Revoking keys (or keyrings) unlinks them any keyring. Sometimes it is
>>>> useful to let the kernel keep a key linked even if ELL isn't directly
>>>> keeping track of that key anymore - for example, a keyring of trusted
>>>> keys can be used for validation without keeping l_key objects around for
>>>> every single key in that keyring. The kernel will clean up the kernel
>>>> key objects when there are no more references to them whether or not we
>>>> explicitly revoke from userspace.
>>>> ---
>>>> ell/key.c | 14 ++++++++++----
>>>> ell/key.h |  4 ++--
>>>> ell/tls.c |  6 +++---
>>>> 3 files changed, 15 insertions(+), 9 deletions(-)
>>>> diff --git a/ell/key.c b/ell/key.c
>>>> index 4cf2307..4225271 100644
>>>> --- a/ell/key.c
>>>> +++ b/ell/key.c
>>>> @@ -276,12 +276,15 @@ LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
>>>> 	return key;
>>>> }
>>>> -LIB_EXPORT void l_key_free(struct l_key *key)
>>>> +LIB_EXPORT void l_key_free(struct l_key *key, bool revoke)
>>>> {
>>> I am not a huge fan of glueing on booleans to simple functions like l_key_free. They should be doing one thing and one thing only. Frankly later on nobody remembers what the difference between l_key_free(x, true) and l_key_free(x, false) was. It becomes to cumbersome to read and remember in application code.
>>> So why not use l_key_free(x) and then introduce l_key_revoke_and_free(x). Or just simply go with l_key_revoke(x) and have the API description be clear that it also frees the key.
>> 
>> +1.  Although we do have precedent for this in l_string_free, but that is mostly how GLib API was done.
> 
> Ok, I'll go with l_key_revoke_and_free(). It was l_string_free that sent me in the original add-a-flag direction, but that's also a function that has been a source of confusion.

we need an in depth API review and might even fix the l_string_free and replace it with two functions.

Regards

Marcel

Re: [PATCH 2/5] key: Make key/keychain revocation optional when freeing

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 567 bytes --]

Hi Mat,

 >> +1.  Although we do have precedent for this in l_string_free, but that
>> is mostly how GLib API was done.
>
> Ok, I'll go with l_key_revoke_and_free(). It was l_string_free that sent
> me in the original add-a-flag direction, but that's also a function that
> has been a source of confusion.
>

Yeah, blame GLib for that.  The point of ell was to make it easy to port 
GLib based projects, so I'm not sure if changing l_string_free is better 
or worse.

I would personally vote for l_key_free and l_key_free_norevoke.

Regards,
-Denis