[PATCH 1/2] key: Add basic keystore support

[PATCH 1/2] key: Add basic keystore support

[Mat Martineau]

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

---
 Makefile.am |   6 ++-
 ell/ell.h   |   1 +
 ell/key.c   | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ell/key.h   |  58 +++++++++++++++++++++++
 4 files changed, 218 insertions(+), 2 deletions(-)
 create mode 100644 ell/key.c
 create mode 100644 ell/key.h

diff --git a/Makefile.am b/Makefile.am
index 55ab8e6..e5a8f94 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -41,7 +41,8 @@ pkginclude_HEADERS = ell/ell.h \
 			ell/base64.h \
 			ell/pem.h \
 			ell/tls.h \
-			ell/uuid.h
+			ell/uuid.h \
+			ell/key.h
 
 lib_LTLIBRARIES = ell/libell.la
 
@@ -88,7 +89,8 @@ ell_libell_la_SOURCES = $(linux_headers) \
 			ell/tls-private.h \
 			ell/tls.c \
 			ell/tls-record.c \
-			ell/uuid.c
+			ell/uuid.c \
+			ell/key.c
 
 ell_libell_la_LDFLAGS = -no-undefined \
 			-version-info $(ELL_CURRENT):$(ELL_REVISION):$(ELL_AGE)
diff --git a/ell/ell.h b/ell/ell.h
index 8cec756..390743f 100644
--- a/ell/ell.h
+++ b/ell/ell.h
@@ -43,6 +43,7 @@
 #include <ell/pem.h>
 #include <ell/tls.h>
 #include <ell/uuid.h>
+#include <ell/key.h>
 
 #include <ell/netlink.h>
 #include <ell/genl.h>
diff --git a/ell/key.c b/ell/key.c
new file mode 100644
index 0000000..b6a1303
--- /dev/null
+++ b/ell/key.c
@@ -0,0 +1,155 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2016  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#define _GNU_SOURCE
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <linux/keyctl.h>
+
+#include "private.h"
+#include "util.h"
+#include "key.h"
+
+//DEBUG
+#include <stdio.h>
+
+#define is_valid_type(type) ((type) <= L_KEY_DH_PRIVATE)
+
+static int keyring_base;
+
+struct l_key {
+	int type;
+	long serial;
+};
+
+static long kernel_add_key(char *type, char *description,
+				const void *payload, size_t len, long keyring)
+{
+	return syscall(__NR_add_key, type, description, payload, len, keyring);
+}
+
+static long kernel_read_key(long serial, const void *payload, size_t len)
+{
+	return syscall(__NR_keyctl, KEYCTL_READ, serial, payload, len);
+}
+
+static long kernel_update_key(long serial, const void *payload, size_t len)
+{
+	return syscall(__NR_keyctl, KEYCTL_UPDATE, serial, payload, len);
+}
+
+static long kernel_revoke_key(long serial)
+{
+	return syscall(__NR_keyctl, KEYCTL_REVOKE, serial);
+}
+
+static bool setup_keyring_base(void)
+{
+	keyring_base = kernel_add_key("keyring", "ell", 0, 0,
+					KEY_SPEC_THREAD_KEYRING);
+
+	if (keyring_base <= 0) {
+		keyring_base = 0;
+		return false;
+	}
+
+	return true;
+}
+
+LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
+					size_t payload_length)
+{
+	struct l_key *key;
+
+	if (unlikely(!payload))
+		return NULL;
+
+	if (unlikely(!is_valid_type(type)))
+		return NULL;
+
+	if (!keyring_base && !setup_keyring_base()) {
+		return NULL;
+	}
+
+	key = l_new(struct l_key, 1);
+	key->type = type;
+	key->serial = kernel_add_key("user", "testing", payload, payload_length,
+					keyring_base);
+
+	if (key->serial < 0) {
+		l_free(key);
+		key = NULL;
+	}
+
+	return key;
+}
+
+LIB_EXPORT void l_key_free(struct l_key *key)
+{
+	if (unlikely(!key))
+		return;
+
+	kernel_revoke_key(key->serial);
+
+	l_free(key);
+}
+
+LIB_EXPORT bool l_key_set_payload(struct l_key *key, void *payload, size_t len)
+{
+	long error;
+
+	if (unlikely(!key))
+		return false;
+
+	error = kernel_update_key(key->serial, payload, len);
+
+	return error == 0;
+}
+
+LIB_EXPORT bool l_key_get_payload(struct l_key *key, void *payload, size_t *len)
+{
+	long copied;
+
+	if (unlikely(!key))
+		return false;
+
+	copied = kernel_read_key(key->serial, payload, *len);
+
+	if (copied < 0 || (size_t)copied > *len)
+		return false;
+
+	*len = copied;
+	return true;
+}
+
+LIB_EXPORT bool l_key_get_type(struct l_key *key, enum l_key_type *type)
+{
+	if (unlikely(!key))
+		return false;
+
+	*type = key->type;
+	return true;
+}
diff --git a/ell/key.h b/ell/key.h
new file mode 100644
index 0000000..c8b6c28
--- /dev/null
+++ b/ell/key.h
@@ -0,0 +1,58 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2016  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifndef __ELL_KEY_H
+#define __ELL_KEY_H
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+#include <stddef.h>
+#include <stdbool.h>
+
+struct l_key;
+
+enum l_key_type {
+	L_KEY_USER = 0,
+	L_KEY_ASYMMETRIC_PUBLIC,
+	L_KEY_ASYMMETRIC_PRIVATE,
+	L_KEY_DH_PUBLIC,
+	L_KEY_DH_PRIVATE
+};
+
+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);
+
+bool l_key_set_payload(struct l_key *key, void *payload, size_t len);
+
+bool l_key_get_payload(struct l_key *key, void *payload, size_t *len);
+
+bool l_key_get_type(struct l_key *key, enum l_key_type *type);
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* __ELL_KEY_H */
-- 
2.7.2

[PATCH 2/2] key: Add keystore unit test

[Mat Martineau]

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

---
 .gitignore      |  1 +
 Makefile.am     |  5 +++-
 unit/test-key.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 93 insertions(+), 1 deletion(-)
 create mode 100644 unit/test-key.c

diff --git a/.gitignore b/.gitignore
index 8a20ffe..6e35752 100644
--- a/.gitignore
+++ b/.gitignore
@@ -52,6 +52,7 @@ unit/test-base64
 unit/test-tls
 unit/test-pem
 unit/test-uuid
+unit/test-key
 unit/*.log
 unit/*.trs
 examples/dbus-service
diff --git a/Makefile.am b/Makefile.am
index e5a8f94..256b163 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -131,7 +131,8 @@ unit_tests = unit/test-unit \
 			unit/test-base64 \
 			unit/test-pem \
 			unit/test-tls \
-			unit/test-uuid
+			unit/test-uuid \
+			unit/test-key
 
 dbus_tests = unit/test-kdbus \
 			unit/test-dbus \
@@ -215,6 +216,8 @@ unit_test_tls_LDADD = ell/libell-private.la
 
 unit_test_uuid_LDADD = ell/libell-private.la
 
+unit_test_key_LDADD = ell/libell-private.la
+
 if MAINTAINER_MODE
 noinst_LTLIBRARIES += unit/example-plugin.la
 endif
diff --git a/unit/test-key.c b/unit/test-key.c
new file mode 100644
index 0000000..a803166
--- /dev/null
+++ b/unit/test-key.c
@@ -0,0 +1,88 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2016  Intel Corporation. All rights reserved.
+ *
+ *  This library is free software; you can redistribute it and/or
+ *  modify it under the terms of the GNU Lesser General Public
+ *  License as published by the Free Software Foundation; either
+ *  version 2.1 of the License, or (at your option) any later version.
+ *
+ *  This library is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  Lesser General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Lesser General Public
+ *  License along with this library; if not, write to the Free Software
+ *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <assert.h>
+
+#include <ell/ell.h>
+
+#define KEY1_STR "This key has exactly _32_ bytes!"
+#define KEY1_LEN (strlen(KEY1_STR))
+#define KEY2_STR "This key is longer than 32 bytes, just to be different."
+#define KEY2_LEN (strlen(KEY2_STR))
+
+static void test_unsupported(const void *data)
+{
+	struct l_key *key;
+
+	key = l_key_new(42, KEY1_STR, KEY1_LEN);
+	assert(!key);
+}
+
+static void test_user(const void *data)
+{
+	struct l_key *key;
+	bool ok;
+	char buf[64] = { 0 };
+	size_t len;
+
+	assert(KEY1_LEN < KEY2_LEN);
+	assert(KEY2_LEN < sizeof(buf));
+
+	key = l_key_new(L_KEY_USER, KEY1_STR, KEY1_LEN);
+	assert(key);
+
+	len = KEY1_LEN - 1;
+	ok = l_key_get_payload(key, buf, &len);
+	assert(!ok);
+
+	len = sizeof(buf);
+	ok = l_key_get_payload(key, buf, &len);
+	assert(ok);
+	assert(len == KEY1_LEN);
+	assert(!strcmp(buf, KEY1_STR));
+
+	ok = l_key_set_payload(key, KEY2_STR, KEY2_LEN);
+	assert(ok);
+
+	len = sizeof(buf);
+	ok = l_key_get_payload(key, buf, &len);
+	assert(ok);
+	assert(len == KEY2_LEN);
+	assert(!strcmp(buf, KEY2_STR));
+
+	l_key_free(key);
+}
+
+int main(int argc, char *argv[])
+{
+	l_test_init(&argc, &argv);
+
+	l_test_add("unsupported", test_unsupported, NULL);
+
+	l_test_add("user key", test_user, NULL);
+
+	return l_test_run();
+}
-- 
2.7.2

Re: [PATCH 1/2] key: Add basic keystore support

[Denis Kenzior]

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

Hi Mat,

On 03/01/2016 06:23 PM, Mat Martineau wrote:
> ---
>   Makefile.am |   6 ++-
>   ell/ell.h   |   1 +
>   ell/key.c   | 155 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/key.h   |  58 +++++++++++++++++++++++
>   4 files changed, 218 insertions(+), 2 deletions(-)
>   create mode 100644 ell/key.c
>   create mode 100644 ell/key.h
>

<snip>

> diff --git a/ell/key.c b/ell/key.c
> new file mode 100644
> index 0000000..b6a1303
> --- /dev/null
> +++ b/ell/key.c
> @@ -0,0 +1,155 @@
> +/*
> + *
> + *  Embedded Linux library
> + *
> + *  Copyright (C) 2016  Intel Corporation. All rights reserved.
> + *
> + *  This library is free software; you can redistribute it and/or
> + *  modify it under the terms of the GNU Lesser General Public
> + *  License as published by the Free Software Foundation; either
> + *  version 2.1 of the License, or (at your option) any later version.
> + *
> + *  This library is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  Lesser General Public License for more details.
> + *
> + *  You should have received a copy of the GNU Lesser General Public
> + *  License along with this library; if not, write to the Free Software
> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +#include <linux/keyctl.h>
> +
> +#include "private.h"
> +#include "util.h"
> +#include "key.h"
> +
> +//DEBUG
> +#include <stdio.h>

Still needed?

> +
> +#define is_valid_type(type) ((type) <= L_KEY_DH_PRIVATE)
> +
> +static int keyring_base;
> +
> +struct l_key {
> +	int type;
> +	long serial;
> +};
> +
> +static long kernel_add_key(char *type, char *description,
> +				const void *payload, size_t len, long keyring)
> +{
> +	return syscall(__NR_add_key, type, description, payload, len, keyring);
> +}
> +
> +static long kernel_read_key(long serial, const void *payload, size_t len)
> +{
> +	return syscall(__NR_keyctl, KEYCTL_READ, serial, payload, len);
> +}
> +
> +static long kernel_update_key(long serial, const void *payload, size_t len)
> +{
> +	return syscall(__NR_keyctl, KEYCTL_UPDATE, serial, payload, len);
> +}
> +
> +static long kernel_revoke_key(long serial)
> +{
> +	return syscall(__NR_keyctl, KEYCTL_REVOKE, serial);
> +}
> +
> +static bool setup_keyring_base(void)
> +{
> +	keyring_base = kernel_add_key("keyring", "ell", 0, 0,
> +					KEY_SPEC_THREAD_KEYRING);
> +

This is fine for now, but I think we might want to expose the concept of 
keyrings in the future.

> +	if (keyring_base <= 0) {
> +		keyring_base = 0;
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void *payload,
> +					size_t payload_length)
> +{
> +	struct l_key *key;
> +
> +	if (unlikely(!payload))
> +		return NULL;
> +
> +	if (unlikely(!is_valid_type(type)))
> +		return NULL;
> +
> +	if (!keyring_base && !setup_keyring_base()) {
> +		return NULL;
> +	}
> +
> +	key = l_new(struct l_key, 1);
> +	key->type = type;
> +	key->serial = kernel_add_key("user", "testing", payload, payload_length,
> +					keyring_base);
> +
> +	if (key->serial < 0) {
> +		l_free(key);
> +		key = NULL;
> +	}
> +
> +	return key;
> +}
> +
> +LIB_EXPORT void l_key_free(struct l_key *key)
> +{
> +	if (unlikely(!key))
> +		return;
> +
> +	kernel_revoke_key(key->serial);
> +
> +	l_free(key);
> +}
> +
> +LIB_EXPORT bool l_key_set_payload(struct l_key *key, void *payload, size_t len)

I'm not sure I like the name.  Perhaps l_key_update?

> +{
> +	long error;
> +
> +	if (unlikely(!key))
> +		return false;
> +
> +	error = kernel_update_key(key->serial, payload, len);
> +
> +	return error == 0;
> +}
> +
> +LIB_EXPORT bool l_key_get_payload(struct l_key *key, void *payload, size_t *len)

Maybe l_key_extract, l_key_extract_raw_bytes, l_key_get_raw_bytes?

> +{
> +	long copied;
> +
> +	if (unlikely(!key))
> +		return false;
> +
> +	copied = kernel_read_key(key->serial, payload, *len);
> +
> +	if (copied < 0 || (size_t)copied > *len)
> +		return false;

So what is the behavior of the kernel syscall here?  How can copied ever 
be greater than len?

Another issue is whether userspace has any way of obtaining information 
on how large the payload is.  E.g. what size len to pass in.  Should we 
track the key size or extract it out somehow?

> +
> +	*len = copied;
> +	return true;
> +}
> +
> +LIB_EXPORT bool l_key_get_type(struct l_key *key, enum l_key_type *type)

We avoided having to deal with this type of situation where an enum 
needs to be extracted back out of an instance.  Some may like doing 
something like:

enum l_key_type l_key_get_type(struct l_key *key) better.  However, 
doing it this way would require us to introduce an _INVALID enum value. 
  Not sure which one is better/worse.

I'm okay with the approach taken in the patch, but maybe others have 
thoughts on this?  We'd need to standardize on something.

> +{
> +	if (unlikely(!key))
> +		return false;
> +
> +	*type = key->type;
> +	return true;
> +}

<snip>

Regards,
-Denis

Re: [PATCH 1/2] key: Add basic keystore support

[Mat Martineau]

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


Denis,

On Wed, 2 Mar 2016, Denis Kenzior wrote:

> Hi Mat,
>
> On 03/01/2016 06:23 PM, Mat Martineau wrote:
>> ---
>>   Makefile.am |   6 ++-
>>   ell/ell.h   |   1 +
>>   ell/key.c   | 155 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>   ell/key.h   |  58 +++++++++++++++++++++++
>>   4 files changed, 218 insertions(+), 2 deletions(-)
>>   create mode 100644 ell/key.c
>>   create mode 100644 ell/key.h
>> 
>
> <snip>
>
>> diff --git a/ell/key.c b/ell/key.c
>> new file mode 100644
>> index 0000000..b6a1303
>> --- /dev/null
>> +++ b/ell/key.c
>> @@ -0,0 +1,155 @@
>> +/*
>> + *
>> + *  Embedded Linux library
>> + *
>> + *  Copyright (C) 2016  Intel Corporation. All rights reserved.
>> + *
>> + *  This library is free software; you can redistribute it and/or
>> + *  modify it under the terms of the GNU Lesser General Public
>> + *  License as published by the Free Software Foundation; either
>> + *  version 2.1 of the License, or (at your option) any later version.
>> + *
>> + *  This library is distributed in the hope that it will be useful,
>> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>> + *  Lesser General Public License for more details.
>> + *
>> + *  You should have received a copy of the GNU Lesser General Public
>> + *  License along with this library; if not, write to the Free Software
>> + *  Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 
>> USA
>> + *
>> + */
>> +
>> +#ifdef HAVE_CONFIG_H
>> +#include <config.h>
>> +#endif
>> +
>> +#define _GNU_SOURCE
>> +#include <unistd.h>
>> +#include <sys/syscall.h>
>> +#include <linux/keyctl.h>
>> +
>> +#include "private.h"
>> +#include "util.h"
>> +#include "key.h"
>> +
>> +//DEBUG
>> +#include <stdio.h>
>
> Still needed?

No, I meant to remove that.

>> +
>> +#define is_valid_type(type) ((type) <= L_KEY_DH_PRIVATE)
>> +
>> +static int keyring_base;
>> +
>> +struct l_key {
>> +	int type;
>> +	long serial;
>> +};
>> +
>> +static long kernel_add_key(char *type, char *description,
>> +				const void *payload, size_t len, long 
>> keyring)
>> +{
>> +	return syscall(__NR_add_key, type, description, payload, len, 
>> keyring);
>> +}
>> +
>> +static long kernel_read_key(long serial, const void *payload, size_t len)
>> +{
>> +	return syscall(__NR_keyctl, KEYCTL_READ, serial, payload, len);
>> +}
>> +
>> +static long kernel_update_key(long serial, const void *payload, size_t 
>> len)
>> +{
>> +	return syscall(__NR_keyctl, KEYCTL_UPDATE, serial, payload, len);
>> +}
>> +
>> +static long kernel_revoke_key(long serial)
>> +{
>> +	return syscall(__NR_keyctl, KEYCTL_REVOKE, serial);
>> +}
>> +
>> +static bool setup_keyring_base(void)
>> +{
>> +	keyring_base = kernel_add_key("keyring", "ell", 0, 0,
>> +					KEY_SPEC_THREAD_KEYRING);
>> +
>
> This is fine for now, but I think we might want to expose the concept of 
> keyrings in the future.
>

Yes, I'm planning on that.

>> +	if (keyring_base <= 0) {
>> +		keyring_base = 0;
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +LIB_EXPORT struct l_key *l_key_new(enum l_key_type type, const void 
>> *payload,
>> +					size_t payload_length)
>> +{
>> +	struct l_key *key;
>> +
>> +	if (unlikely(!payload))
>> +		return NULL;
>> +
>> +	if (unlikely(!is_valid_type(type)))
>> +		return NULL;
>> +
>> +	if (!keyring_base && !setup_keyring_base()) {
>> +		return NULL;
>> +	}
>> +
>> +	key = l_new(struct l_key, 1);
>> +	key->type = type;
>> +	key->serial = kernel_add_key("user", "testing", payload, 
>> payload_length,
>> +					keyring_base);
>> +
>> +	if (key->serial < 0) {
>> +		l_free(key);
>> +		key = NULL;
>> +	}
>> +
>> +	return key;
>> +}
>> +
>> +LIB_EXPORT void l_key_free(struct l_key *key)
>> +{
>> +	if (unlikely(!key))
>> +		return;
>> +
>> +	kernel_revoke_key(key->serial);
>> +
>> +	l_free(key);
>> +}
>> +
>> +LIB_EXPORT bool l_key_set_payload(struct l_key *key, void *payload, size_t 
>> len)
>
> I'm not sure I like the name.  Perhaps l_key_update?

Ok. That also aligns with the syscall terminology.

>> +{
>> +	long error;
>> +
>> +	if (unlikely(!key))
>> +		return false;
>> +
>> +	error = kernel_update_key(key->serial, payload, len);
>> +
>> +	return error == 0;
>> +}
>> +
>> +LIB_EXPORT bool l_key_get_payload(struct l_key *key, void *payload, size_t 
>> *len)
>
> Maybe l_key_extract, l_key_extract_raw_bytes, l_key_get_raw_bytes?
>

I like l_key_extract

>> +{
>> +	long copied;
>> +
>> +	if (unlikely(!key))
>> +		return false;
>> +
>> +	copied = kernel_read_key(key->serial, payload, *len);
>> +
>> +	if (copied < 0 || (size_t)copied > *len)
>> +		return false;
>
> So what is the behavior of the kernel syscall here?  How can copied ever be 
> greater than len?
>

The syscall return value is the total length of the key, which may be 
larger than the buffer passed to the kernel. I will change "copied" to 
"keylen".

> Another issue is whether userspace has any way of obtaining information on 
> how large the payload is.  E.g. what size len to pass in.  Should we track 
> the key size or extract it out somehow?
>

The KEYCTL_READ syscall lets us get the length without copying anything if 
a zero-length buffer is passed in. I think it's best to expose that 
(l_key_get_size()), since there may be keys that we ask the kernel to 
generate for us.

>> +
>> +	*len = copied;
>> +	return true;
>> +}
>> +
>> +LIB_EXPORT bool l_key_get_type(struct l_key *key, enum l_key_type *type)
>
> We avoided having to deal with this type of situation where an enum needs to 
> be extracted back out of an instance.  Some may like doing something like:
>
> enum l_key_type l_key_get_type(struct l_key *key) better.  However, doing it 
> this way would require us to introduce an _INVALID enum value.  Not sure 
> which one is better/worse.
>

We think very much alike here! I considered both options but I didn't see 
other examples of the _INVALID approach, so I went with the bool return to 
handle the error case and figured we could discuss the alternatives.

Using an _INVALID enum would be less awkward in the common case, where the 
caller would check to see if the type matched an expected value. Returning 
a bool means the caller has to check both the boolean return value and (if 
that is 'true') the retrieved type.

The use case I envisioned is that l_keys will be passed to other APIs, 
like cipher. The specific cipher implementation would check the expected 
key type before trying to use it. If we instead opt to skip that kind of 
check and let the cipher operation fail if they key isn't applicable, then 
we can remove l_key_type altogether.

> I'm okay with the approach taken in the patch, but maybe others have thoughts 
> on this?  We'd need to standardize on something.
>
>> +{
>> +	if (unlikely(!key))
>> +		return false;
>> +
>> +	*type = key->type;
>> +	return true;
>> +}
>
> <snip>

--
Mat Martineau
Intel OTC

Re: [PATCH 1/2] key: Add basic keystore support

[Denis Kenzior]

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

Hi Mat,

<snip>

 >>> +
>>> +    copied = kernel_read_key(key->serial, payload, *len);
>>> +
>>> +    if (copied < 0 || (size_t)copied > *len)
>>> +        return false;
>>
>> So what is the behavior of the kernel syscall here?  How can copied
>> ever be greater than len?
>>
>
> The syscall return value is the total length of the key, which may be
> larger than the buffer passed to the kernel. I will change "copied" to
> "keylen".
>

Sounds good.

>> Another issue is whether userspace has any way of obtaining
>> information on how large the payload is.  E.g. what size len to pass
>> in.  Should we track the key size or extract it out somehow?
>>
>
> The KEYCTL_READ syscall lets us get the length without copying anything
> if a zero-length buffer is passed in. I think it's best to expose that
> (l_key_get_size()), since there may be keys that we ask the kernel to
> generate for us.
>

Yes, I think something like this is needed.

Are there kernel keyctl internal enumerations for key types as well? 
E.g. how do we know that a certificate loaded by keyctl is a private key 
vs a public key?

>>> +
>>> +    *len = copied;
>>> +    return true;
>>> +}
>>> +
>>> +LIB_EXPORT bool l_key_get_type(struct l_key *key, enum l_key_type
>>> *type)
>>
>> We avoided having to deal with this type of situation where an enum
>> needs to be extracted back out of an instance.  Some may like doing
>> something like:
>>
>> enum l_key_type l_key_get_type(struct l_key *key) better.  However,
>> doing it this way would require us to introduce an _INVALID enum
>> value.  Not sure which one is better/worse.
>>
>
> We think very much alike here! I considered both options but I didn't
> see other examples of the _INVALID approach, so I went with the bool
> return to handle the error case and figured we could discuss the
> alternatives.
>
> Using an _INVALID enum would be less awkward in the common case, where
> the caller would check to see if the type matched an expected value.
> Returning a bool means the caller has to check both the boolean return
> value and (if that is 'true') the retrieved type.
>
> The use case I envisioned is that l_keys will be passed to other APIs,
> like cipher. The specific cipher implementation would check the expected
> key type before trying to use it. If we instead opt to skip that kind of
> check and let the cipher operation fail if they key isn't applicable,
> then we can remove l_key_type altogether.

So looking at the key_type enumeration again, I'm still a bit unsure of 
the real-life usage.

E.g. for Diffie-Hellman, we want something like:

l_getrandom(key, size);
private = l_key_new(L_KEY_DH_PRIVATE, key, size);
prime = l_key_new(??, prime, size);
generator = l_key_new(??, generator, size);
public = l_key_new_dh_public(generator, private, prime);

peer_public = l_key_new(L_KEY_DH_PUBLIC, peer_key, peer_key_size);
shared_secret = l_key_new_dh_shared(peer_public, private, prime);

Note that the implementation of dh_shared and dh_public are 
(mathematically) the same, just the expected input key types might be 
different.

What key type do we use for prime and generator?
Do we really need to make the distinction between private / public 
types?  If the types are dropped above, not much of value is lost...

Regards,
-Denis

Re: [PATCH 1/2] key: Add basic keystore support

[Mat Martineau]

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


On Wed, 2 Mar 2016, Denis Kenzior wrote:

<snip>

>>> Another issue is whether userspace has any way of obtaining
>>> information on how large the payload is.  E.g. what size len to pass
>>> in.  Should we track the key size or extract it out somehow?
>>> 
>> 
>> The KEYCTL_READ syscall lets us get the length without copying anything
>> if a zero-length buffer is passed in. I think it's best to expose that
>> (l_key_get_size()), since there may be keys that we ask the kernel to
>> generate for us.
>> 
>
> Yes, I think something like this is needed.
>
> Are there kernel keyctl internal enumerations for key types as well? E.g. how 
> do we know that a certificate loaded by keyctl is a private key vs a public 
> key?
>

Asymmetric keys have a "subtype", but there's only one right now (x509). 
When an asymmetric key is created, it has a list of parsers to attempt, 
and a successful parser will set the subtype appropriately. After the key 
is created, KEYCTL_DESCRIBE will return a string which contains the key 
type and subtype information.

User keys are just a blob.

This leads me to think that the type should be retrieved from the kernel 
dynamically, similar to how the size is handled.

>>>> +
>>>> +    *len = copied;
>>>> +    return true;
>>>> +}
>>>> +
>>>> +LIB_EXPORT bool l_key_get_type(struct l_key *key, enum l_key_type
>>>> *type)
>>> 
>>> We avoided having to deal with this type of situation where an enum
>>> needs to be extracted back out of an instance.  Some may like doing
>>> something like:
>>> 
>>> enum l_key_type l_key_get_type(struct l_key *key) better.  However,
>>> doing it this way would require us to introduce an _INVALID enum
>>> value.  Not sure which one is better/worse.
>>> 
>> 
>> We think very much alike here! I considered both options but I didn't
>> see other examples of the _INVALID approach, so I went with the bool
>> return to handle the error case and figured we could discuss the
>> alternatives.
>> 
>> Using an _INVALID enum would be less awkward in the common case, where
>> the caller would check to see if the type matched an expected value.
>> Returning a bool means the caller has to check both the boolean return
>> value and (if that is 'true') the retrieved type.
>> 
>> The use case I envisioned is that l_keys will be passed to other APIs,
>> like cipher. The specific cipher implementation would check the expected
>> key type before trying to use it. If we instead opt to skip that kind of
>> check and let the cipher operation fail if they key isn't applicable,
>> then we can remove l_key_type altogether.
>
> So looking at the key_type enumeration again, I'm still a bit unsure of the 
> real-life usage.
>
> E.g. for Diffie-Hellman, we want something like:
>
> l_getrandom(key, size);
> private = l_key_new(L_KEY_DH_PRIVATE, key, size);
> prime = l_key_new(??, prime, size);
> generator = l_key_new(??, generator, size);
> public = l_key_new_dh_public(generator, private, prime);
>
> peer_public = l_key_new(L_KEY_DH_PUBLIC, peer_key, peer_key_size);
> shared_secret = l_key_new_dh_shared(peer_public, private, prime);
>
> Note that the implementation of dh_shared and dh_public are (mathematically) 
> the same, just the expected input key types might be different.

My current DH implementation in the kernel has separate keyctl commands 
for deriving the public key and shared secret (with the internal 
implementation sharing the common math and key handling), but I'm going to 
change that to just one KEYCTL_DH_COMPUTE command. The user then gets the 
public key by passing in the generator key id, and the shared secret by 
passing in the peer public key.

>
> What key type do we use for prime and generator?
> Do we really need to make the distinction between private / public types?  If 
> the types are dropped above, not much of value is lost...

On the kernel side, they're all "user" keys 
(prime/generator/public/private), since the asymmetric type only handles 
x509 format.

There isn't much value in tracking the key types for DH. The motivation 
has more to do with asymmetric ciphers. When creating a new key in the 
kernel, a type string must be specified ("user", "asymmetric", etc.), and 
the enumerations would map to the appropriate kernel key type string. 
However, it's sufficient to have more general types like L_KEY_RAW and 
L_KEY_ASYMMETRIC. Sound like a better approach?


Mat

Re: [PATCH 1/2] key: Add basic keystore support

[Denis Kenzior]

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

Hi Mat,

 >> Are there kernel keyctl internal enumerations for key types as well?
>> E.g. how do we know that a certificate loaded by keyctl is a private
>> key vs a public key?
>>
>
> Asymmetric keys have a "subtype", but there's only one right now (x509).
> When an asymmetric key is created, it has a list of parsers to attempt,
> and a successful parser will set the subtype appropriately. After the
> key is created, KEYCTL_DESCRIBE will return a string which contains the
> key type and subtype information.
>
> User keys are just a blob.
>
> This leads me to think that the type should be retrieved from the kernel
> dynamically, similar to how the size is handled.
>

My thinking as well.  Last I looked the keyctl 'types' were a bit weird. 
  So we may need to do some additional work here.

>
> My current DH implementation in the kernel has separate keyctl commands
> for deriving the public key and shared secret (with the internal
> implementation sharing the common math and key handling), but I'm going
> to change that to just one KEYCTL_DH_COMPUTE command. The user then gets
> the public key by passing in the generator key id, and the shared secret
> by passing in the peer public key.

Yep, I think that's a good way of doing it.  Lets see what the kernel 
guys think :)

>
>>
>> What key type do we use for prime and generator?
>> Do we really need to make the distinction between private / public
>> types?  If the types are dropped above, not much of value is lost...
>
> On the kernel side, they're all "user" keys
> (prime/generator/public/private), since the asymmetric type only handles
> x509 format.
>
> There isn't much value in tracking the key types for DH. The motivation
> has more to do with asymmetric ciphers. When creating a new key in the
> kernel, a type string must be specified ("user", "asymmetric", etc.),
> and the enumerations would map to the appropriate kernel key type
> string. However, it's sufficient to have more general types like
> L_KEY_RAW and L_KEY_ASYMMETRIC. Sound like a better approach?
>

Yep, I think so.  We can also postpone adding the key type for now until 
our understanding of the asymmetric key types and key type extraction 
from the kernel are a bit less fuzzy.

Regards,
-Denis