6:44 p.m.
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
6:44 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
6:47 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
6:55 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
7:07 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
8:34 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
8:39 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
8:45 p.m.
New subject: [PATCH 2/5] key: Make key/keychain revocation optional when freeing
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
6:44 p.m.
New subject: [PATCH 3/5] unit: Update for new l_key_free/l_keyring_free revoke parameter
---
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
6:44 p.m.
New subject: [PATCH 4/5] tls: Validate cert chain using l_keyring
---
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
6:44 p.m.
New subject: [PATCH 5/5] unit: Fix memory leak in trust chain test
---
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
7:06 p.m.
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
1828
days inactive
1828
days old
11 comments
3 participants
participants (3)
-
Denis Kenzior -
Marcel Holtmann -
Mat Martineau