Feature/der encoder decoder by ethan-thompson · Pull Request #5497 · FreeRADIUS/freeradius-server

ethan-thompson · 2025-01-21T20:00:11Z

Wrote an encoder and decoder for DER. This adds the ability to decoder DER encoded X509 certificates and CSR's into dictionary attributes, as well as encode into DER.

src/protocols/der/base.c

alandekok · 2025-01-21T20:13:51Z

src/protocols/der/base.c

+ * @copyright 2024 Inkbridge Networks SAS.
+ */
+
+#include "include/build.h"


nits: we usually put #include "foo" after all of the common includes. that way any location definitions don't affect the other header files.

alandekok · 2025-01-21T20:14:17Z

src/protocols/der/base.c

+ */
+
+#include "include/build.h"
+#include "der.h"


#include <freeradius-devel/util/table.h> etc.

alandekok · 2025-01-21T20:14:58Z

src/protocols/der/base.c

+};
+
+fr_der_tag_constructed_t tag_labels[] = {
+	[FR_DER_TAG_BOOLEAN]	      = FR_DER_TAG_PRIMATIVE,


PRIMATIVE? Is that a DER thing, or is it PRIMITIVE?

alandekok · 2025-01-21T20:16:02Z

src/protocols/der/base.c

+{
+	fr_der_attr_flags_t *flags = fr_dict_attr_ext(*da_p, FR_DICT_ATTR_EXT_PROTOCOL_SPECIFIC);
+
+	flags->tagnum = (uint8_t)atoi(value);


do we want sanity checks here for overflow, or for bad formats?

Admins make mistakes. An error of "you screwed up" is usually better than silently doing the wrong thing.

alandekok · 2025-01-21T20:18:09Z

src/protocols/der/base.c

+static int dict_flag_set_of(fr_dict_attr_t **da_p, char const *value, UNUSED fr_dict_flag_parser_rule_t const *rules)
+{
+	static fr_table_num_sorted_t const table[] = {
+		{ L("bitstring"), FR_DER_TAG_BITSTRING },


with repeated tables like this, it may be useful to put them into a common macro? depending on whether or not the C preprocessor likes the L() macro inside another macro :(

alandekok · 2025-01-21T20:18:46Z

src/protocols/der/base.c

+}
+
+static fr_dict_flag_parser_t const der_flags[] = {
+						   { L("class"), { .func = dict_flag_class } },


whitespace / indentation

alandekok · 2025-01-21T20:19:24Z

src/protocols/der/base.c

+
+	if (fr_der_flag_is_sequence_of(da->parent) || fr_der_flag_is_set_of(da->parent)) {
+		static fr_table_num_sorted_t const table[] = {
+			{ L("bitstring"), FR_DER_TAG_BITSTRING },


repeated repetition... :(

alandekok · 2025-01-21T20:19:49Z

src/protocols/der/der.h

@@ -0,0 +1,171 @@
+#include "include/build.h"


use <freeradius-devel/...

alandekok · 2025-01-21T20:20:26Z

src/protocols/der/der.h

+#include "include/build.h"
+#include "lib/util/types.h"
+#include <freeradius-devel/util/dict.h>
+#include <stdbool.h>


stdbool and stdint should already be included by the types.h header

alandekok · 2025-01-21T20:21:23Z

src/protocols/der/der.h

+ */
+static bool *fr_type_to_der_tags[] = {
+	[FR_TYPE_MAX] = NULL,
+	[FR_TYPE_BOOL] = (bool []){[FR_DER_TAG_BOOLEAN] = true, [FR_DER_TAG_INTEGER] = true, [FR_DER_TAG_NULL] = true, [FR_DER_TAG_MAX] = false},


maybe put line breaks after the , which can make the lines shorter, and a bit easier to read.

alandekok · 2025-01-21T20:23:24Z

src/protocols/der/der.h

+
+static inline CC_HINT(always_inline) fr_der_tag_num_t fr_type_to_der_tag_default(fr_type_t type)
+{
+	return fr_type_to_der_tag_defaults[type];


what about data types like FR_TYPE_IPADDR which aren't listed in the above table? The C compiler will initialize the array entry to 0. Is there a FR_DER_TAG_INVALID which is defined as 0 ?

alandekok · 2025-01-21T20:23:58Z

src/protocols/der/der.h

+#define DER_BOOLEAN_TRUE 0xff	 //!< DER encoded boolean true value.
+
+typedef struct {
+	uint8_t tagnum;


maybe format these so that the names are further to the right, and then all lined up vertically. that makes it easier to read.

alandekok · 2025-01-21T20:24:07Z

src/protocols/der/der.h

+}
+
+#define fr_der_flag_tagnum(_da) 	(fr_der_attr_flags(_da)->tagnum)
+#define fr_der_flag_class(_da) 	(fr_der_attr_flags(_da)->class)


whitespace nits :)

alandekok · 2025-01-21T20:25:02Z

src/protocols/der/all.mk

+TARGET		:= libfreeradius-der$(L)
+
+SOURCES		:= base.c \
+		   decode.c \


maybe include push this commit until after the encode.c and decode.c files have been added. otherwise the build is broken.

alandekok · 2025-01-21T20:28:02Z

src/protocols/der/decode.c

+ * @copyright 2024 Arran Cudbard-Bell (a.cudbardb@freeradius.org)
+ * @copyright 2024 Inkbridge Networks SAS.
+ */
+#include "include/build.h"


header name / "" versus <>

I'll stop commenting here. just take a pass through the code for these, and also stdbool etc. likely most of the std* includes can be dropped.

alandekok · 2025-01-21T20:28:19Z

src/protocols/der/decode.c

+#include <freeradius-devel/util/time.h>
+#include <freeradius-devel/util/value.h>
+#include <stdlib.h>
+#include <time.h>


util/time.h already includes time.h

alandekok · 2025-01-21T20:29:52Z

src/protocols/der/decode.c

+
+typedef ssize_t (*fr_der_decode_oid_t)(uint64_t subidentifier, void *uctx, bool is_last);
+
+static ssize_t fr_der_decode_oid(fr_pair_list_t *out, fr_dbuff_t *in, fr_der_decode_oid_t func, void *uctx);


maybe add CC_HINT(nonnull) for the function prototypes

alandekok · 2025-01-21T20:30:42Z

src/protocols/der/decode.c

+	[UINT8_MAX] = { .constructed = FR_DER_TAG_PRIMATIVE, .decode = NULL },
+};
+
+static int decode_test_ctx(void **out, TALLOC_CTX *ctx, UNUSED fr_dict_t const *dict)


move the various test_ctx stuff to the bottom of the file, where it's located for the other protocols

alandekok · 2025-01-21T20:31:14Z

src/protocols/der/decode.c

+	}
+
+	/*
+	 * 	ISO/IEC 8825-1:2021


comments and references to standards make me happy.

alandekok · 2025-01-21T20:32:01Z

src/protocols/der/decode.c

+		return -1;
+	}
+
+	if (unlikely(val != DER_BOOLEAN_FALSE && val != DER_BOOLEAN_TRUE)) {


brackets around things. It's not required by C, but it cam avoid typos

if ((a == b) || (c == d))

alandekok · 2025-01-21T20:32:48Z

src/protocols/der/decode.c

+
+	vp = fr_pair_afrom_da(ctx, parent);
+	if (unlikely(vp == NULL)) {
+		fr_strerror_const("Out of memory for boolean pair");


just Out of memory is fine, like all of the other uses. arguably we could have a macro for this somewhere else in the code.

alandekok · 2025-01-21T20:35:24Z

src/protocols/der/decode.c

+}
+
+typedef struct {
+	TALLOC_CTX	     *ctx; /**< Allocation context */


comments can be aligned vertically, and use //!< which is more consistent with the rest of the code.

alandekok · 2025-01-21T20:39:36Z

src/protocols/der/decode.c

+	char	  *str	  = NULL;
+
+	static bool const allowed_chars[] = {
+		[' '] = true, ['\''] = true, ['('] = true, [')'] = true, ['+'] = true,	     [','] = true, ['-'] = true,


format as 4 per line, which makes it a bit easier to read

alandekok · 2025-01-21T20:40:55Z

src/protocols/der/decode.c

+	char	  *str	  = NULL;
+
+	static bool const allowed_chars[] = {
+		[0x08] = true, [0x0A] = true, [0x0C] = true,	  [0x0D] = true, [0x0E] = true, [0x0F] = true,


see src/lib/util/token.c. GCC and clang support ranges, which makes these tables a lot easier

/* * This is fine. Don't complain. */ #ifdef __clang__ #pragma clang diagnostic ignored "-Wgnu-designator" #endif ... [ 0 ... T_HASH ] = '?', /* GCC extension for range initialization, also allowed by clang */

alandekok · 2025-01-21T20:42:00Z

src/protocols/der/decode.c

+		/*
+		 *	We have a multi-byte tag
+		 *
+		 *	Note: Mutli-byte tags would mean having a tag number that is greater than 30 (0x1E) (since tag


alandekok · 2025-01-21T20:43:10Z

share/dictionary/der/dictionary

+PROTOCOL	DER		11354911
+BEGIN-PROTOCOL DER
+
+$INCLUDE dictionary.test


probably don't want test dictionaries in the default build.

the src/tests/unit stuff allows for local dictionaries to be loaded

alandekok · 2025-01-21T20:44:05Z

share/dictionary/der/dictionary.common

+DEFINE GeneralName      choice
+BEGIN GeneralName
+
+ATTRIBUTE otherName     0       sequence option=0


run ./scripts/dict/format.dl share/dictionary/der/dictionary*

for consistent formatting

alandekok · 2025-01-27T18:53:26Z

src/tests/unit/protocols/der/dictionary.test

+# Copyright (C) 2025 The FreeRADIUS Server project and contributors
+# This work is licensed under CC-BY version 4.0 https://creativecommons.org/licenses/by/4.0
+# Version $Id$
+DEFINE	Certificate-Extensions				x509_extensions ref=OID-Tree


run the dicts through scripts/dict/format.pl. I like pretty things

That was the format after having run the format.pl script on the dictionaries

alandekok · 2025-01-27T19:05:12Z

share/dictionary/der/dictionary

+#
+
+PROTOCOL	DER		11354911
+BEGIN-PROTOCOL DER


Delete those two lines, and replace them with

BEGIN PROTOCOL DER 11354911

I think the server supports 64-bit protocol numbers? I haven't checked recently.

what's the significance of 11354911 ?

I will update that line. There was no significance in 11354911. I believe @arr2036 may have used this number to avoid collision

arr2036 · 2025-01-28T18:54:33Z

src/protocols/der/decode.c

+	fr_dbuff_t our_in = FR_DBUFF(in);
+	uint8_t	   val;
+
+	size_t len = fr_dbuff_remaining(&our_in);


You should check the return code of fr_dbuff_out instead of manually checking the length

arr2036 · 2025-01-28T19:00:31Z

Default value flag should produce a value box. Using a DEFAULT enum value is counterintuitive.

arr2036 · 2025-01-28T19:21:25Z

Where possible you should still remove fr_dbuff_remaining where fr_dbuff_out would perform the same check

arr2036 · 2025-01-28T19:33:15Z

Have some bad news ... is a GCC extension, so probably wants to be converted to the explicit range

arr2036 · 2025-01-28T19:36:48Z

Even with the artificial windows, it's probably better to use fr_dbuff_extend_lowat and check the return values. That lets the artificial "end" point to unrealised data.

Signed-off-by: ethan-thompson <ethan.thompson@networkradius.com>

alandekok · 2025-02-13T13:37:46Z

src/protocols/der/encode.c

+		fr_dbuff_in(dbuff, (uint8_t)((slen) >> ((len_len - i - 1) * 8)));
+	}
+
+	fr_dbuff_set(dbuff, fr_dbuff_current(length_start) + len_len + 1 + slen);


This doesn't seem right? The function encodes the length, and then skips over the length + the data which will be encoded?

and almost all calls to this function do

...fr_der_encode_len(&our_dbuff, &length_start, fr_dbuff_behind(&length_start) - 1)

except for one...

alandekok · 2025-02-13T13:40:03Z

src/protocols/der/encode.c

+		 */
+		fr_dict_enum_value_t const *evp;
+
+		evp = fr_dict_enum_by_name(vp->da, "DEFAULT", strlen("DEFAULT"));


I'll leave this in for now, but it could be in the dict_ext stuff?

I made similar comments on the review call. It can just be a flag.