Skip to contentSkip to conversation

pem: refactor to expose pem iterator privately

PATCH Applied James Prestwood 2 participants 5 messages Oct 3, 2019

lore
Share & tools
lore
#0 lore raw

PATCH v5 1/3 pem: refactor to expose pem iterator privately

This refactors pem.c to expose a new internal API, pem_next. This API will allow iteration/parsing of PEM(s). Other APIs in pem.c were updated to use pem_next.

pem_next behaves similar to pem_load_buffer, except it does not do any base64 decoding. Instead it just returns the start of the base64 data and the caller must decode it. This allows simple PEM iteration loops. Strictly for pem.c usage we decode the base64 string in the loop, but settings.c will need this for parsing PEMs from settings files and returning the PEM as a full string, including the start/end tags.

---
 ell/pem-private.h | 32 +++++++++++++++++++
 ell/pem.c         | 78 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 82 insertions(+), 28 deletions(-)
 create mode 100644 ell/pem-private.h

-v5:
* Fixed incorrect commit description (return start of base64 data)
* Removed len out parameter in pem_next, endp can be used instead
* Fixed base64_len off by one error.
* Fixed misleading comment, and removed unused error handling code

ell/pem-private.h new file +32 -0
new file mode 100644
index 0000000..1cf7ee5
@@ -0,0 +1,32 @@
+/*
+ *
+ *  Embedded Linux library
+ *
+ *  Copyright (C) 2019  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 <sys/types.h>
+
+const char *pem_next(const void *buf, size_t buf_len, char **type_label,
+				size_t *base64_len,
+				const char **endp, bool strict);
ell/pem.c +50 -28

148 lines not on this page · raw message

Signature · 1 line
2.17.1
#1 lore raw

PATCH v5 2/3 settings: introduce extended groups concept

This patch introduces a new extended format for settings files. This new format allows raw data to be provided under a given group rather than key/value pairs. A group using an extended format will contain both the extended type and group name in the format, for example:

[@pem(a)certificate]

Where 'pem' is the type and 'certificate' is the name. The type of format requires an extension specific parser implementation in settings as the parser must know when to start/stop parsing data. This allows for any textual characters to be present after the group is declared, including special characters like [] (its completely up to the parser to decide whats valid and what is not).

Three new l_settings APIs were added:

l_settings_has_embedded_group
l_settings_get_embedded_groups
l_settings_get_embedded_value

In this patch, the 'pem' extension type is also introduced, and for now is the only extended type. The pem extended type expects a base64 encoded PEM (or list of PEMs) after the group. The pem parser will stop after the last PEM is found. Examples can be found in unit/test-settings.c

---
 ell/ell.sym    |   3 +
 ell/settings.c | 234 ++++++++++++++++++++++++++++++++++++++++++++++---
 ell/settings.h |   7 ++
 3 files changed, 233 insertions(+), 11 deletions(-)

-v5:
* Updated to use endp instead of len out parameter

ell/ell.sym +3 -0
index e02f0fe..32d94f5 100644
@@ -425,6 +425,9 @@ global:
 	l_settings_set_float;
 	l_settings_remove_group;
 	l_settings_remove_key;
+	l_settings_has_embedded_group;
+	l_settings_get_embedded_groups;
+	l_settings_get_embedded_value;
 	/* signal */
 	l_signal_create;
 	l_signal_remove;
ell/settings.c +223 -11

301 lines not on this page · raw message

ell/settings.h +7 -0
index 0da9f55..e4203af 100644
@@ -123,6 +123,13 @@ bool l_settings_remove_key(struct l_settings *settings, const char *group_name,
 				const char *key);
 bool l_settings_remove_group(struct l_settings *settings,
 				const char *group_name);
+
+char **l_settings_get_embedded_groups(struct l_settings *settings);
+bool l_settings_has_embedded_group(struct l_settings *settings,
+					const char *group);
+const char *l_settings_get_embedded_value(struct l_settings *settings,
+						const char *group_name,
+						const char **out_type);
 #ifdef __cplusplus
 }
 #endif
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH v5 2/3] settings: introduce extended groups concept

Hi James,

Quoted 62 lines · James Prestwood

This patch introduces a new extended format for settings files. This new format allows raw data to be provided under a given group rather than key/value pairs. A group using an extended format will contain both the extended type and group name in the format, for example:

[@pem(a)certificate]

Where 'pem' is the type and 'certificate' is the name. The type of format requires an extension specific parser implementation in settings as the parser must know when to start/stop parsing data. This allows for any textual characters to be present after the group is declared, including special characters like [] (its completely up to the parser to decide whats valid and what is not).

Three new l_settings APIs were added:

l_settings_has_embedded_group
l_settings_get_embedded_groups
l_settings_get_embedded_value

In this patch, the 'pem' extension type is also introduced, and for now is the only extended type. The pem extended type expects a base64 encoded PEM (or list of PEMs) after the group. The pem parser will stop after the last PEM is found. Examples can be found in unit/test-settings.c

---
  ell/ell.sym    |   3 +
  ell/settings.c | 234 ++++++++++++++++++++++++++++++++++++++++++++++---
  ell/settings.h |   7 ++
  3 files changed, 233 insertions(+), 11 deletions(-)
(quoted diff) +0 -1
-v5:
  * Updated to use endp instead of len out parameter
ell/ell.sym +3 -0
index e02f0fe..32d94f5 100644
@@ -425,6 +425,9 @@ global:
  	l_settings_set_float;
  	l_settings_remove_group;
  	l_settings_remove_key;
+	l_settings_has_embedded_group;
+	l_settings_get_embedded_groups;
+	l_settings_get_embedded_value;
  	/* signal */
  	l_signal_create;
  	l_signal_remove;
ell/settings.c +1 -0
index 98f0cfd..457d3ec 100644
@@ -48,15 +48,23 @@
  #include "settings.h"
  #include "private.h"
  #include "missing.h"
+#include "pem-private.h"
 
  struct setting_data {
  	char *key;
  	char *value;
  };
 

ell/settings.c line 67

Quoted 10 lines
ell/settings.c +7 -0
+struct embedded_group_data {
+	char type[32];
+	size_t len;
+	char data[0];
+};
+
  struct group_data {
  	char *name;
  	struct l_queue *settings;
+	struct embedded_group_data *embedded_data;

I would store these in a separate queue from the other groups

Quoted 17 lines
ell/settings.c +5 -1
  };
 
  struct l_settings {
@@ -81,7 +89,11 @@ static void group_destroy(void *data)
  	struct group_data *group = data;
 
  	l_free(group->name);
-	l_queue_destroy(group->settings, setting_destroy);
+
+	if (group->settings)
+		l_queue_destroy(group->settings, setting_destroy);
+	else
+		l_free(group->embedded_data);
 
  	l_free(group);
  }

ell/settings.c static char *escape_value(const char *value) · line 239

Quoted 6 lines
ell/settings.c +3 -0
@@ -222,6 +234,128 @@ static char *escape_value(const char *value)
  	return ret;
  }
 
+static ssize_t parse_pem(const char *data, size_t len)
+{
+	const char *ptr, *end;

One variable declaration per line please

Quoted 41 lines
ell/settings.c +41 -0
+	size_t count = 0;
+
+	ptr = data;
+	end = data + len;
+
+	while (ptr && ptr < end) {
+		const char *pem_start = ptr;
+
+		if (!pem_next(ptr, len, NULL, NULL, &ptr, true)) {
+			if (ptr)
+				return -EINVAL;
+
+			break;
+		}
+
+		len -= ptr - pem_start;
+		count += ptr - pem_start;
+	}
+
+	return count;
+}
+
+struct group_extension {
+	char *name;
+	ssize_t (*parse)(const char *data, size_t len);
+};
+
+static struct group_extension pem_extension = {
+	.name = "pem",
+	.parse = parse_pem,
+};
+
+struct group_extension *extensions[] = {
+	&pem_extension,
+	NULL
+};
+
+static struct group_extension *find_group_extension(const char *type)
+{
+	unsigned int i;
+

ell/settings.c static char *escape_value(const char *value) · line 290

Quoted 10 lines
ell/settings.c +10 -0
+	for (i = 0; extensions[i]; i++) {
+		if (!strcmp(type, extensions[i]->name))
+			return extensions[i];
+	}
+
+	return NULL;
+}
+
+static ssize_t parse_embedded_group(struct l_settings *setting, const char *data,
+					size_t line_len, size_t len)

So you decided not to use the line variable at all in the signature? How would you report syntax errors via l_util_debug?

ell/settings.c static char *escape_value(const char *value) · line 294

Quoted 4 lines
ell/settings.c +4 -0
+{
+	struct group_data *group;
+	struct group_extension *ext;
+	const char *ptr1, *ptr2;

One variable declaration per line please

Also, maybe more descriptive names are in order? E.g. type and name?

ell/settings.c static char *escape_value(const char *value) · line 299

Quoted 5 lines
ell/settings.c +5 -0
+	L_AUTO_FREE_VAR(char *, type) = NULL;
+	char *name;
+	ssize_t bytes;
+
+	/* Must be at least [@a(a)b]\n */

Isn't newline stripped by now?

ell/settings.c static char *escape_value(const char *value) · line 308

Quoted 9 lines
ell/settings.c +9 -0
+	if (line_len < 7)
+		return -EINVAL;
+
+	/* caller checked data[1] == '@' */
+	ptr1 = data + 1;
+
+	ptr2 = ptr1 + 1;
+
+	while (*ptr2 != '@' && (size_t)(ptr2 - data) < line_len)

Why the cast?

ell/settings.c static char *escape_value(const char *value) · line 309

Quoted 1 line
ell/settings.c +1 -0
+		ptr2++;

memchr?

ell/settings.c static char *escape_value(const char *value) · line 314

Quoted 5 lines
ell/settings.c +5 -0
+
+	if (*ptr2 != '@')
+		return -EINVAL;
+
+	type = l_strndup(ptr1 + 1, ptr2 - ptr1 - 1);

Don't you want to validate type is ascii printable and less than 31 bytes? Also, why do you strdup here?

ell/settings.c static char *escape_value(const char *value) · line 325

Quoted 11 lines
ell/settings.c +11 -0
+
+	ptr1 = ptr2;
+
+	while (*ptr1 != ']' && (size_t)(ptr1 - data) < line_len)
+		ptr1++;
+
+	if (*ptr1 != ']')
+		return -EINVAL;
+
+	if (ptr1[1] != '\n')
+		return -EINVAL;

This looks fishy... There should be no newlines included in the line length. Have you run this through valgrind?

ell/settings.c static char *escape_value(const char *value) · line 338

Quoted 13 lines
ell/settings.c +13 -0
+
+	if (ptr1 - ptr2 - 1 < 1)
+		return -EINVAL;
+
+	name = l_strndup(ptr2 + 1, ptr1 - ptr2 - 1);
+
+	ext = find_group_extension(type);
+	if (!ext)
+		goto free_name;
+
+	bytes = ext->parse(ptr1 + 2, len - line_len);
+	if (bytes < 0)
+		goto free_name;

Ah okay, so you're combining parsing the group name and the contents into one function. I would play with actually breaking this into two steps. Perhaps:

struct group_extension *parse_embedded_group(settings, data, len, line, char **out_name);

ell/settings.c static char *escape_value(const char *value) · line 344

Quoted 6 lines
ell/settings.c +6 -0
+
+	group = l_new(struct group_data, 1);
+
+	group->name = name;
+	group->embedded_data = l_malloc(sizeof(struct embedded_group_data) +
+						bytes + 1);

Why +1? Shouldn't all our utilities handle non-null terminated data?

ell/settings.c static char *escape_value(const char *value) · line 345

Quoted 1 line
ell/settings.c +1 -0
+	strcpy(group->embedded_data->type, type);

Overflow?

Quoted 16 lines
ell/settings.c +13 -0
+	group->embedded_data->len = bytes;
+	memcpy(group->embedded_data->data, ptr1 + 2, bytes);
+	group->embedded_data->data[bytes] = '\0';
+
+	l_queue_push_tail(setting->groups, group);
+
+	return bytes;
+
+free_name:
+	l_free(name);
+	return -EINVAL;
+}
+
  static bool parse_group(struct l_settings *settings, const char *data,
  			size_t len, size_t line)
  {

<snip>

Quoted 14 lines
ell/settings.c +4 -2
@@ -552,24 +708,24 @@ static void gather_groups(void *data, void *user_data)
  	struct group_data *group_data = data;
  	struct gather_data *gather = user_data;
 
-	gather->v[gather->cur++] = l_strdup(group_data->name);
+	if (!group_data->settings)
+		return;
+
+	gather->v = l_strv_append(gather->v, group_data->name);
  }
 
  LIB_EXPORT char **l_settings_get_groups(const struct l_settings *settings)
  {
-	char **ret;

ell/settings.c static void gather_groups(void *data, void *user_data) · line 725

Quoted 10 lines
ell/settings.c +1 -3
  	struct gather_data gather;
 
  	if (unlikely(!settings))
  		return NULL;
 
-	ret = l_new(char *, l_queue_length(settings->groups) + 1);
-	gather.v = ret;
-	gather.cur = 0;
+	gather.v = l_strv_new();
 

So honestly I'm not a huge fan of using l_strv_append for anything as it causes a realloc. I would argue it'd be far easier to store embedded groups and groups separately.

Quoted 38 lines
ell/settings.c +28 -1
  	l_queue_foreach(settings->groups, gather_groups, &gather);
 
-	return ret;
+	return gather.v;
  }
 
  LIB_EXPORT bool l_settings_has_group(const struct l_settings *settings,
@@ -1176,3 +1332,59 @@ LIB_EXPORT bool l_settings_remove_key(struct l_settings *settings,
 
  	return true;
  }
+
+static void gather_embedded_groups(void *data, void *user_data)
+{
+	struct group_data *group_data = data;
+	struct gather_data *gather = user_data;
+
+	if (!group_data->embedded_data)
+		return;
+
+	gather->v = l_strv_append(gather->v, group_data->name);
+}
+
+LIB_EXPORT char **l_settings_get_embedded_groups(struct l_settings *settings)
+{
+	struct gather_data gather;
+
+	if (unlikely(!settings))
+		return NULL;
+
+	gather.v = l_strv_new();
+
+	l_queue_foreach(settings->groups, gather_embedded_groups, &gather);
+
+	return gather.v;
+}
+
+LIB_EXPORT bool l_settings_has_embedded_group(struct l_settings *settings,

ell/settings.c LIB_EXPORT bool l_settings_remove_key(struct l_settings *settings, · line 1371

Quoted 10 lines
ell/settings.c +10 -0
+						const char *group)
+{
+	struct group_data *group_data;
+
+	if (unlikely(!settings))
+		return false;
+
+	group_data = l_queue_find(settings->groups, group_match, group);
+
+	return group_data->embedded_data != NULL;

What if group_data is NULL?

Quoted 6 lines
ell/settings.c +6 -0
+}
+
+LIB_EXPORT const char *l_settings_get_embedded_value(struct l_settings *settings,
+							const char *group_name,
+							const char **out_type)
+{

ell/settings.c LIB_EXPORT bool l_settings_remove_key(struct l_settings *settings, · line 1387

Quoted 10 lines
ell/settings.c +10 -0
+	struct group_data *group;
+
+	if (unlikely(!settings))
+		return false;
+
+	group = l_queue_find(settings->groups, group_match, group_name);
+	if (!group || !group->embedded_data)
+		return NULL;
+
+	*out_type = group->embedded_data->type;

Might want to check out_type is !NULL

ell/settings.c LIB_EXPORT bool l_settings_remove_key(struct l_settings *settings, · line 1390

Quoted 3 lines
ell/settings.c +3 -0
+
+	return group->embedded_data->data;
+}

Regards,
-Denis

#3 lore raw

PATCH v5 3/3 unit/test-settings: add pem extended type tests

In reply to James Prestwood
---
 unit/settings.test   |  28 +++++
 unit/test-settings.c | 238 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 264 insertions(+), 2 deletions(-)
unit/settings.test +28 -0
index 3884f7d..5f2d4f8 100644
@@ -20,3 +20,31 @@ StringList=Foo,Bar,Baz
 StringListEmpty=
 StringListOne=FooBarBaz
 StringWithSpaces=Bar B Q
+
+[@pem(a)certificate]
+-----BEGIN CERTIFICATE-----
+MIIEajCCA1KgAwIBAgIUKIOXQXEyHZsOFw/+ihDBNGTQnWUwDQYJKoZIhvcNAQEL
+BQAweDE1MDMGA1UECgwsSW50ZXJuYXRpb25hbCBVbmlvbiBvZiBFeGFtcGxlIE9y
+Z2FuaXphdGlvbnMxHzAdBgNVBAMMFkNlcnRpZmljYXRlIGlzc3VlciBndXkxHjAc
+BgkqhkiG9w0BCQEWD2NhQG1haWwuZXhhbXBsZTAeFw0xOTA5MTYxNzEyNThaFw00
+NzAyMDExNzEyNThaMHgxNTAzBgNVBAoMLEludGVybmF0aW9uYWwgVW5pb24gb2Yg
+RXhhbXBsZSBPcmdhbml6YXRpb25zMR8wHQYDVQQDDBZDZXJ0aWZpY2F0ZSBpc3N1
+ZXIgZ3V5MR4wHAYJKoZIhvcNAQkBFg9jYUBtYWlsLmV4YW1wbGUwggEiMA0GCSqG
+SIb3DQEBAQUAA4IBDwAwggEKAoIBAQCjovj3aq26sAQ0k5vD/BVp40p0lhz1+Wet
+1EcdQa1arVIca9nhfvoHfJAmK+zzqQLbvI0/e2if4X6OKf41g7w7VaYS9qv5jZZ0
+v/7aL6PUa2F7C9HG/vuIII/dRvP2uQ43PLxeTeZyj7bBUB9xCFCpzB+7AZuUuH0H
+ABaC9CAGZWImBY5NUXST7E/BsvqU80KJglDovcabthvwoekji9DC/wwISLE1e9cO
+A9IB0Co0mA1ME6wzrawmuTzxUw9BsmEhbKhFGBRwIrrq0r4GvDmeMFiZjXv+I0vq
+wSCyRtgoeBmyemqIEgiN4Z23V7ps3dbYF/tw96Zj7rd5gtjY9VSdAgMBAAGjgesw
+gegwDwYDVR0TBAgwBgEB/wIBADAdBgNVHQ4EFgQUnvllvvxy17VqqR0FfgS4NwzX
+jScwgbUGA1UdIwSBrTCBqoAUnvllvvxy17VqqR0FfgS4NwzXjSehfKR6MHgxNTAz
+BgNVBAoMLEludGVybmF0aW9uYWwgVW5pb24gb2YgRXhhbXBsZSBPcmdhbml6YXRp
+b25zMR8wHQYDVQQDDBZDZXJ0aWZpY2F0ZSBpc3N1ZXIgZ3V5MR4wHAYJKoZIhvcN
+AQkBFg9jYUBtYWlsLmV4YW1wbGWCFCiDl0FxMh2bDhcP/ooQwTRk0J1lMA0GCSqG
+SIb3DQEBCwUAA4IBAQBROAyWfQyKXQ007U6ctgihHbg/lsfEEfeNPG+QRVt8/e53
+4fH6scuY9bW7CZQSdiBo178ITHrIOo2CuFMa0ysnW3V1M9/s0dUYjBHYdpTEEQ+d
+tgm1uRLiTsYeBtueRItEmZU6JjgmvAH8i1UqI0e5iYlfnovPmftpqIwRH7k7A9kS
+SehC9QkkrnIttDEoeYTGhLOJu1Fx2cwAodce6VNgz/k1zIXY5Tprg440zrCwc+th
+MpX48F31ggg8Wd5N6Xg1nricGwL8K90ts6xvwF1WwKsg6BeYdyC0eYBqQ41MA/7P
+DK3OGM6cC5tbQGWaIT0Q407GJBGpaijDicA2YqlK
+-----END CERTIFICATE-----
unit/test-settings.c +236 -2

274 lines not on this page · raw message

Signature · 1 line
2.17.1
#4 lore raw
In reply to James Prestwood

Hi James,

Quoted 15 lines · James Prestwood

This refactors pem.c to expose a new internal API, pem_next. This API will allow iteration/parsing of PEM(s). Other APIs in pem.c were updated to use pem_next.

pem_next behaves similar to pem_load_buffer, except it does not do any base64 decoding. Instead it just returns the start of the base64 data and the caller must decode it. This allows simple PEM iteration loops. Strictly for pem.c usage we decode the base64 string in the loop, but settings.c will need this for parsing PEMs from settings files and returning the PEM as a full string, including the start/end tags.

---
  ell/pem-private.h | 32 +++++++++++++++++++
  ell/pem.c         | 78 ++++++++++++++++++++++++++++++-----------------
  2 files changed, 82 insertions(+), 28 deletions(-)
Quoted 7 lines

create mode 100644 ell/pem-private.h

-v5:
* Fixed incorrect commit description (return start of base64 data)
* Removed len out parameter in pem_next, endp can be used instead
* Fixed base64_len off by one error.
* Fixed misleading comment, and removed unused error handling code

Applied, thanks.

Regards,
-Denis