From: Alan T. DeKok <aland@freeradius.org>
Date: Thu, 11 Dec 2025 01:41:07 +0000 (-0500)
Subject: Rewrite fr_pair_list_afrom_substr()
X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=01ecadaa42d0c9c1a22a3fe92eeced1047441cfb;p=thirdparty%2Ffreeradius-server.git

Rewrite fr_pair_list_afrom_substr()

and update tests to match.

It now properly handles aliases, groups, etc.  The parsing has
been significantly re-worked.  All of the old-style strings should
still be allowed, except for ones which had never made sense.

And since aliases, groups, etc. are now handled properly, the
fr_pair_t parenting is also correct.  So we can start enabling the
checks on parenting.

Which should help us remove the last vestiges of "flat" attributes
---

diff --git a/src/lib/util/pair_legacy.c b/src/lib/util/pair_legacy.c
index d3e2f4d2f9c..335c8b01547 100644
--- a/src/lib/util/pair_legacy.c
+++ b/src/lib/util/pair_legacy.c
@@ -57,7 +57,7 @@ static fr_sbuff_term_t const 	bareword_terminals =
 
 static fr_table_num_sorted_t const pair_assignment_op_table[] = {
 	{ L("+="),	T_OP_ADD_EQ		},
-	{ L(":="),	T_OP_EQ			},
+	{ L(":="),	T_OP_SET       		},
 	{ L("="),	T_OP_EQ			},
 };
 static ssize_t pair_assignment_op_table_len = NUM_ELEMENTS(pair_assignment_op_table);
@@ -140,7 +140,73 @@ static ssize_t fr_pair_value_from_substr(fr_pair_t *vp, fr_sbuff_t *in, UNUSED b
 	return slen + ((quote != 0) << 1);
 }
 
+/** Our version of a DA stack.
+ *
+ *	@todo - add in whether or not we added / created the vp?  maybe an edit list?
+ *	and then we can clean up the unknown DAs, simply by talloc freeing the edit list.
+ */
+typedef struct {
+	int depth;
+	fr_dict_attr_t const	*da[FR_DICT_MAX_TLV_STACK];	//!< parent for parsing
+	fr_pair_t		*vp[FR_DICT_MAX_TLV_STACK];	//!< which VP we have created or found
+} legacy_da_stack_t;
+
 /**  Parse a #fr_pair_list_t from a substring
+ *
+ *	Syntax: ([raw.]|.)<name>[<.name>] op [(cast)] value...
+ *
+ *	A "raw" prefix creates a raw attribute, which allows us to encode raw data which might be invalid for
+ *	the given data type.  Or if a "(cast)" is given, the value is parsed as the specified data type.  Note
+ *	that casts can only be to a "leaf" data type, and not to a structural type such as "tlv", "group",
+ *	"struct", etc.  The "(cast)" syntax can only be used for "raw" attributes, and not for attributes
+ *	which are known.  The "name" can be either a known attribute, or a numerical OID.  Either way, the
+ *	final attribute which is created is marked as "raw" or "unknown", and is encoded via the "raw" rules,
+ *	and not as the known data type.
+ *
+ *	If the first name begins with ".", then it is a _relative_ name.  The attribute is created in the
+ *	context of the most recently created "structural" data type.
+ *
+ *	TBD - we have to determine what the heck that means...
+ *
+ *	The "name" can be one or more names from the input dictionary.  The names must be known, as numerical
+ *	OIDs can only be used when the "raw" prefix is used.
+ *
+ *	If there are multiple names (e.g. "foo.bar.baz"), then only the last name can be a "leaf" data
+ *	type.  All of the intermediate names must be "structural" data types.
+ *
+ *	Depending on the input arguments, the operator can be a comparison operator (==, <=, etc.).  Or, else
+ *	it can be an assignment operator (=, +=).  The "=" operator is used to assign, and the "+=" operator
+ *	is used to append.  No other assignment operators are permitted.  Note that "+=" cannot be used with
+ *	relative names (i.e. where the name begins with ".")
+ *
+ *	The "value" can either be a "leaf" data type (e.g. number, IP address, etc.) or for "structural" data
+ *	types it can be a sub-list.  A sub-list is a set of attribute assignments which are surrounded by
+ *	curly brackets "{...}".  When a sub-list is specified, the contents must be either children of the
+ *	parent attribute (for "tlv", "struct"), or children referenced by a "group", or internal attributes.
+ *
+ *	If an intermediate "name" is an ALIAS, then the attributes are created / used as if all intermediate
+ *	names were specified.  i.e. ALIAS is a short-cut for names (think "soft link), but it does not change
+ *	the hierarchy for normal attributes.
+ *
+ *
+ *	Examples
+ *	--------
+ *
+ *	Name = value
+ *		Leaf attributes.
+ *		The value MUST be parsed as the leaf data type.
+ *
+ *	Name = { children }
+ *		Structural attributes.
+ *		The children MUST be children of the parent.
+ *		OR the children can be from the "internal" dictionary.
+ *		OR for type 'group', children of the group reference (usually the dictionary root)
+ *
+ *	raw.Name = 0xabcdef
+ *		Raw attributes.
+ *		The value MUST be a hex string.
+ *
+ *	raw.Name = { children }
  *
  * @param[in] root	where we start parsing from
  * @param[in,out] relative where we left off, or where we should continue from
@@ -149,13 +215,12 @@ static ssize_t fr_pair_value_from_substr(fr_pair_t *vp, fr_sbuff_t *in, UNUSED b
  *	- <0 on error
  *	- 0 on no input
  *	- >0 on how many bytes of input we read
- *
  */
 fr_slen_t fr_pair_list_afrom_substr(fr_pair_parse_t const *root, fr_pair_parse_t *relative,
 				    fr_sbuff_t *in)
 {
 	int			i, components;
-	bool			raw;
+	bool			raw, was_unknown;
 	bool			was_relative = false;
 	bool			append;
 	bool			keep_going;
@@ -163,12 +228,13 @@ fr_slen_t fr_pair_list_afrom_substr(fr_pair_parse_t const *root, fr_pair_parse_t
 	fr_token_t		op;
 	fr_slen_t		slen;
 	fr_pair_t		*vp;
-	fr_dict_attr_t const	*internal = NULL;
-	fr_sbuff_marker_t	lhs_m, rhs_m;
+	fr_pair_parse_t		my;
+	fr_sbuff_marker_t	lhs_m, op_m, rhs_m;
 	fr_sbuff_t		our_in = FR_SBUFF(in);
+	legacy_da_stack_t      	da_stack = {};
 
 	if (unlikely(!root->ctx)) {
-		fr_strerror_const("Missing ctx fr_pair_parse_t");
+		fr_strerror_const("Missing input context (fr_pair_parse_t)");
 		return -1;
 	}
 
@@ -182,26 +248,50 @@ fr_slen_t fr_pair_list_afrom_substr(fr_pair_parse_t const *root, fr_pair_parse_t
 		return -1;
 	}
 
-	if (fr_dict_internal()) internal = fr_dict_root(fr_dict_internal());
-	if (internal == root->da) internal = NULL;
+	fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
 	if (fr_sbuff_remaining(&our_in) == 0) return 0;
 
+	/*
+	 *	Boot strap the relative references from the root.
+	 *
+	 *	The comparison operations are only used for internal tests, and should not be used by
+	 *	administrators.  So we disallow them, unless the destination list is empty.  This check
+	 *	prevents them from being used in administrative policies.
+	 */
+	if (!relative->da) {
+		if (root->allow_compare && !fr_pair_list_empty(root->list)) {
+			fr_strerror_const("Attribute comparisons can only be used when the destination list is empty");
+			return -1;
+		}
+
+		*relative = *root;
+	}
+
+#define CLEAN_DA_STACK do { if (was_unknown) {		\
+	for (i = 1; i < da_stack.depth; i++) {		\
+		fr_dict_attr_unknown_free(&da_stack.da[i]); \
+	} } } while (0)	
+
+
 redo:
-	append = true;
 	raw = false;
 	raw_type = FR_TYPE_NULL;
 	relative->last_char = 0;
+	was_unknown = false;
 	vp = NULL;
 
-	fr_sbuff_adv_past_whitespace(&our_in, SIZE_MAX, NULL);
+	fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
 	/*
-	 *	Relative attributes start from the input list / parent.
+	 *	STEP 1: Figure out if we have relative or absolute attributes.
 	 *
 	 *	Absolute attributes start from the root list / parent.
+	 *	Or, when there is no previous relative setting.
+	 *
+	 *	Relative attributes start from the input list / parent.
 	 *
-	 *	Once we decide where we are coming from, all subsequent operations are on the "relative"
+	 *	Once we decide where we start parsing from, all subsequent operations are on the "relative"
 	 *	structure.
 	 */
 	if (!fr_sbuff_next_if_char(&our_in, '.')) {
@@ -211,7 +301,7 @@ redo:
 		was_relative = false;
 
 		/*
-		 *	Be nice to people who expect to see '&' everywhere.
+		 *	Be nice to people who expect to use '&' everywhere.
 		 */
 		(void) fr_sbuff_next_if_char(&our_in, '&');
 
@@ -219,16 +309,51 @@ redo:
 		 *	Raw attributes can only be at our root.
 		 *
 		 *	"raw.foo" means that SOME component of the OID is raw.  But the starting bits might be known.
+		 *
+		 *	Raw attributes cannot be created in the internal namespace.  But an internal group can
+		 *	contain raw protocol attributes.
 		 */
 		if (fr_sbuff_is_str_literal(&our_in, "raw.")) {
-			raw = true;
 			fr_sbuff_advance(&our_in, 4);
+			goto is_raw;
 		}
-	} else if (!relative->ctx || !relative->da || !relative->list) {
-		fr_strerror_const("The '.Attribute' syntax can only be used if the previous attribute is structural, and the line ends with ','");
-		return -1;
+
+	} else if (relative->da->flags.is_root) {
+		fr_strerror_const("The '.Attribute' syntax cannot be used at the root of a dictionary");
+
+	error:
+		CLEAN_DA_STACK;
+		return fr_sbuff_error(&our_in);
+
+	} else if (relative->da->type == FR_TYPE_GROUP) {
+		fr_strerror_printf("The '.Attribute' syntax cannot be used with parent %s of data type 'group'",
+				   relative->da->name);
+		goto error;
+
 	} else {
+		fr_assert(relative->ctx);
+		fr_assert(relative->list);
+
 		was_relative = true;
+		append = true;
+	}
+
+	/*
+	 *	If the input root is an unknown attribute, then forbid internal ones, and force everything
+	 *	else to be raw, too.
+	 */
+	if (relative->da->flags.is_unknown) {
+	is_raw:
+		raw = true;
+	}
+
+	/*
+	 *	Raw internal attributes don't make sense.  An internal group can contain raw protocol
+	 *	attributes, but the group is not raw.
+	 */
+	if (raw && relative->da->flags.internal) {
+		fr_strerror_const("Cannot create internal attributes which are 'raw'");
+		goto error;
 	}
 
 	/*
@@ -237,6 +362,8 @@ redo:
 	fr_sbuff_marker(&lhs_m, &our_in);
 
 	/*
+	 *	STEP 2: Find and check the operator.
+	 *
 	 *	Skip over the attribute name.  We need to get the operator _before_ creating the VPs.
 	 */
 	components = 0;
@@ -248,426 +375,751 @@ redo:
 	/*
 	 *	Couldn't find anything.
 	 */
-	if (!components) {
-		fr_strerror_const("Empty input");
-		return 0;
-	}
+	if (!components) goto done;
 
-	fr_sbuff_adv_past_whitespace(&our_in, SIZE_MAX, NULL);
+	fr_sbuff_marker(&op_m, &our_in);
+	fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
 	/*
 	 *	Look for the operator.
 	 */
 	if (relative->allow_compare) {
 		fr_sbuff_out_by_longest_prefix(&slen, &op, pair_comparison_op_table, &our_in, T_INVALID);
+		if (op == T_INVALID) {
+			fr_strerror_const("Expecting operator");
+			goto error;
+		}
+
 	} else {
+		/*
+		 *	@todo - handle different operators ala v3?
+		 *	What is the difference between ":=" and "="?  Perhaps nothing?
+		 */
 		fr_sbuff_out_by_longest_prefix(&slen, &op, pair_assignment_op_table, &our_in, T_INVALID);
-	}
-	if (op == T_INVALID) {
-		fr_strerror_const("Expecting operator");
-		return fr_sbuff_error(&our_in);
+		if (op == T_INVALID) {
+			fr_strerror_const("Expecting operator");
+			goto error;
+		}
+
+		/*
+		 *	+= means "append"
+		 *	:= menas "don't append".
+		 */
+		if (op != T_OP_EQ) {
+			if (was_relative) {
+				fr_strerror_printf("The '.Attribute' syntax cannot be used along with the '%s' operator",
+						   fr_tokens[op]);
+				goto error;
+			}
+		}
+
+		if (op == T_OP_ADD_EQ) {
+			append = true;
+		}
+
+		if (op == T_OP_SET) {
+			append = false;
+		}
+
+		op = T_OP_EQ;
 	}
 
 	/*
-	 *	Skip past whitespace, and set a marker at the RHS.  Then reset the input to the LHS attribute
-	 *	name, so that we can go back and parse / create the attributes.
+	 *	Check the character after the operator.  This check is only necessary to produce better error
+	 *	messages.  i.e. We allow "=", but the user enters "==".
 	 */
-	fr_sbuff_adv_past_whitespace(&our_in, SIZE_MAX, NULL);
+	{
+		uint8_t c = fr_sbuff_char(&our_in, '\0');
+		static const bool invalid[UINT8_MAX + 1] = {
+			['!'] = true, ['#'] = true, ['$'] = true, ['*'] = true,
+			['+'] = true, ['-'] = true, ['/'] = true, ['<'] = true,
+			['='] = true, ['>'] = true, ['?'] = true, ['|'] = true,
+			['~'] = true,
+		};
 
-	fr_sbuff_marker(&rhs_m, &our_in);
+		if (c && invalid[c]) {
+			fr_strerror_printf("Invalid character '%c' after operator '%s'",
+					   (char) c, fr_tokens[op]);
+			goto error;
+		}
+	}
 
 	/*
-	 *	If the value of the attribute is 0x..., then we always force the raw type to be octets, even
-	 *	if the attribute is named and known.  e.g. raw.Framed-IP-Address = 0x01
+	 *	Skip past whitespace, and set a marker at the RHS value.  We do a quick peek at the value, to
+	 *	set the data type of the RHS.  This allows us to parse raw TLVs.
+	 */
+	fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
+
+	/*
+	 *	STEP 3: Try to guess the data type for "raw" attributes.
+	 *
+	 *	If the attribute is raw, and the value of the attribute is 0x..., then we always force the raw
+	 *	type to be octets, even if the attribute is named and known.  e.g. raw.Framed-IP-Address =
+	 *	0x01.
 	 *
 	 *	OR if the attribute is entirely unknown (and not a raw version of a known one), then we allow a
-	 *	cast to set the data type.
+	 *	cast which sets the data type.
 	 */
 	if (raw) {
 		if (fr_sbuff_is_str_literal(&our_in, "0x")) {
 			raw_type = FR_TYPE_OCTETS;
 
 		} else if (fr_sbuff_next_if_char(&our_in, '(')) {
-			fr_sbuff_marker_t m;
-
-			fr_sbuff_marker(&m, &our_in);
+			fr_sbuff_marker(&rhs_m, &our_in);
 
 			fr_sbuff_out_by_longest_prefix(&slen, &raw_type, fr_type_table, &our_in, FR_TYPE_NULL);
-			if ((raw_type == FR_TYPE_NULL) || !fr_type_is_leaf(raw_type)) {
+
+			/*
+			 *	The input has to be a real (non-NULL) leaf.  The input shouldn't be cast to a
+			 *	TLV.  Instead, the value should just start with '{'.
+			 */
+			if (!fr_type_is_leaf(raw_type)) {
 				fr_sbuff_set(&our_in, &rhs_m);
 				fr_strerror_const("Invalid data type in cast");
-				return fr_sbuff_error(&our_in);
+				goto error;
 			}
 
 			if (!fr_sbuff_next_if_char(&our_in, ')')) {
 				fr_strerror_const("Missing ')' in cast");
-				return fr_sbuff_error(&our_in);
+				goto error;
 			}
 
-			fr_sbuff_adv_past_whitespace(&our_in, SIZE_MAX, NULL);
-			fr_sbuff_marker(&rhs_m, &our_in);
+			fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
 		} else if (fr_sbuff_is_char(&our_in, '{')) {
+			/*
+			 *	Raw attributes default to data type TLV.
+			 */
 			raw_type = FR_TYPE_TLV;
+			append = false;
 		}
 	}
 
+	fr_sbuff_marker(&rhs_m, &our_in);
+
 	fr_sbuff_set(&our_in, &lhs_m);
 
 	/*
-	 *	Parse each OID component, creating pairs along the way.
+	 *	That we know the data type, parse each OID component.  We build the DA stack from top to bottom.
+	 *
+	 *	0 is our relative root.  1..N are the DAs that we find or create.
 	 */
-	i = 1;
-	do {
+	da_stack = (legacy_da_stack_t) {
+		.da = {
+			[0] = relative->da,
+		},
+		.depth = 1,
+	};
+
+	/*
+	 *	STEP 4: Re-parse the attributes, building up the da_stack of #fr_dict_attr_t that we will be
+	 *	using as parents.
+	 */
+	for (i = 1; i <= components; i++, da_stack.depth++) {
 		fr_dict_attr_err_t	err;
 		fr_dict_attr_t const	*da = NULL;
 		fr_dict_attr_t const	*da_unknown = NULL;
+		fr_dict_attr_t const	*parent;
+		fr_dict_attr_t const	*ref;
+		fr_type_t		unknown_type;
+
+		if (da_stack.depth >= FR_DICT_MAX_TLV_STACK) {
+			fr_strerror_printf("Attributes are nested too deeply at \"%.*s\"",
+					   (int) fr_sbuff_diff(&op_m, &lhs_m), fr_sbuff_current(&lhs_m));
+			goto error;
+		}
+
+		fr_sbuff_marker(&lhs_m, &our_in);
+
+		/*
+		 *	The fr_pair_t parent might be a group, in which case the fr_dict_attr_t parent will be
+		 *	different.
+		 */
+		parent = da_stack.da[da_stack.depth - 1];
+		if (parent->type == FR_TYPE_GROUP) {
+			parent = fr_dict_attr_ref(parent);
+			fr_assert(parent != NULL);
+		}
+
+		/*
+		 *	Once we parse a completely unknown attribute, all of the rest of them have to be
+		 *	unknown, too.  We cannot allow unknown TLVs to contain internal attributes, for
+		 *	example.
+		 */
+		if (was_unknown) {
+			goto alloc_unknown;
+		}
+
+		/*
+		 *	Look up the name (or number).  If it's found, life is easy.  Otherwise, we jump
+		 *	through a bunch of hoops to see if we are changing dictionaries, or creating a raw OID
+		 *	from a number, etc.
+		 */
+		slen = fr_dict_oid_component(&err, &da, parent, &our_in, &bareword_terminals);
+		if (err != FR_DICT_ATTR_OK) {
+			/*
+			 *	We were looking in the internal dictionary.  Maybe this attribute is instead
+			 *	in the protocol dictionary?
+			 */
+			if ((relative->da->dict == relative->internal) && relative->dict) {
+				fr_assert(relative->dict != relative->internal);
 
-		slen = fr_dict_oid_component(&err, &da, relative->da, &our_in, &bareword_terminals);
-		if (err == FR_DICT_ATTR_NOTFOUND) {
-			if (raw) {
-				fr_pair_t *parent_vp;
+				/*
+				 *	Internal groups can be used to cache protocol data.  Internal
+				 *	structural attributes cannot.
+				 *
+				 *	@todo - this restriction makes sense, but maybe people want to do that
+				 *	anyways?
+				 */
+				if (relative->da->type != FR_TYPE_GROUP) {
+					fr_strerror_printf("Internal attribute '%s' of data type '%s' cannot contain protocol attributes",
+							   relative->da->name, fr_type_to_str(relative->da->type));
+					goto error;
+				}
 
+				slen = fr_dict_oid_component(&err, &da, fr_dict_root(relative->dict), &our_in, &bareword_terminals);
+				if (err == FR_DICT_ATTR_OK) {
+					ref = fr_dict_root(relative->dict);
+					goto found;
+				}
+			}
+
+			/*
+			 *	Try to parse the name from the internal namespace first, as this is the most
+			 *	likely case.  Plus, if we parse the OIDs second, the errors for unknown
+			 *	attributes mention the protocol dictionary, and not the internal one.
+			 *
+			 *	Raw attributes also cannot be created in the internal dictionary space.
+			 */
+			if (!raw && relative->internal) {
 				/*
-				 *	We looked up raw.FOO, and FOO wasn't found.  The component must be a number.
+				 *	If the current dictionary isn't internal, then look up the attribute
+				 *	in the internal dictionary.
+				 *
+				 *	Buf if the current dictionary is internal, AND the internal type is
+				 *	GROUP, AND we we have a protocol dictionary, then allow an internal
+				 *	group to contain protocol attributes.
 				 */
-				if (!fr_sbuff_is_digit(&our_in)) goto notfound;
+				if (parent->dict != relative->internal) {
+					ref = fr_dict_root(relative->internal);
 
-				if (raw_type == FR_TYPE_NULL) {
-					raw_type = FR_TYPE_OCTETS;
+				} else if ((da_stack.da[da_stack.depth - 1]->type == FR_TYPE_GROUP) && (root->da->dict != root->internal)) {
+					ref = fr_dict_root(root->da->dict);
 
-				} else if (raw_type == FR_TYPE_TLV) {
+				} else {
 					/*
-					 *	Reset the type based on the parent.
+					 *	Otherwise we are already in the internal dictionary, and the
+					 *	attribute was not found.  So don't search for it again in the
+					 *	internal dictionary.  And because we're in the internal
+					 *	dictionary, we don't allow raw attributes.
 					 */
-					if (relative->da->type == FR_TYPE_VSA) {
-						raw_type = FR_TYPE_VENDOR;
-					}
+					goto notfound;
 				}
 
-				slen = fr_dict_attr_unknown_afrom_oid_substr(root->ctx, &da_unknown, relative->da, &our_in, raw_type);
-				if (slen < 0) return fr_sbuff_error(&our_in) + slen;
-
-				fr_assert(da_unknown);
-
-				/*
-				 *	Append from the root list, starting at the root depth.
-				 */
-				vp = fr_pair_afrom_da_depth_nested(root->ctx, root->list, da_unknown,
-								   root->da->depth);
-				fr_dict_attr_unknown_free(&da_unknown);
+				slen = fr_dict_oid_component(&err, &da, ref, &our_in, &bareword_terminals);
+				if (err == FR_DICT_ATTR_OK) {
+					goto found;
+				}
 
-				if (!vp) return fr_sbuff_error(&our_in);
+				goto notfound;
+			}
 
-				parent_vp = fr_pair_parent(vp);
-				fr_assert(parent_vp);
+			/*
+			 *	We didn't find anything, that's an error.
+			 */
+			if (!raw) {
+			notfound:
+				fr_strerror_printf("Unknown attribute \"%.*s\" for parent \"%s\"",
+						   (int) fr_sbuff_diff(&op_m, &our_in), fr_sbuff_current(&our_in),
+						   da_stack.da[da_stack.depth - 1]->name);
+				goto error;
+			}
 
-				/*
-				 *	@todo - check parent_vp->da, too?  But we have to handle the case of
-				 *	groups, changing dictionaries, etc.
-				 */
-				PAIR_ALLOCED(vp);
-				fr_pair_verify(__FILE__, __LINE__, NULL, &parent_vp->vp_group, vp, false);
+		alloc_unknown:
+			/*
+			 *	We looked up raw.FOO, and FOO wasn't found.  See if we can still parse it.
+			 */
+			if (da_stack.da[da_stack.depth - 1]->type == FR_TYPE_GROUP) {
+				fr_strerror_printf("Cannot create 'raw' children in attribute %s of data type 'group'",
+						   da_stack.da[da_stack.depth - 1]->name);
+				goto error;
+			}
 
-				/*
-				 *	The above function MAY have jumped ahead a few levels.  Ensure
-				 *	that the relative structure is set correctly for the parent,
-				 *	but only if the parent changed.
-				 */
-				if (relative->da != vp->da->parent) {
+			/*
+			 *	Unknown attributes must be 'raw.1234'.
+			 */
+			if (!fr_sbuff_is_digit(&our_in)) {
+				goto notfound;
+			}
 
-					relative->ctx = parent_vp;
-					relative->da = parent_vp->da;
-					relative->list = &parent_vp->vp_group;
+			/*
+			 *	Figure out the data type for unknown attributes.  Intermediate attributes are
+			 *	structural.  Only the final attribute is forced to "raw_type".
+			 */
+			if (i < components) {
+				if (parent->type == FR_TYPE_VSA) {
+					unknown_type = FR_TYPE_VENDOR;
+				} else {
+					unknown_type = FR_TYPE_TLV;
 				}
 
+			} else if (raw_type == FR_TYPE_NULL) {
+				unknown_type = FR_TYPE_OCTETS;
+
+			} else if ((raw_type == FR_TYPE_TLV) && (parent->type == FR_TYPE_VSA)) {
 				/*
-				 *	update the new relative information for the current VP, which
-				 *	may be structural, or a key field.
+				 *	We had previously parsed a known VSA, but this component is
+				 *	perhaps a numerical OID.  Set the data type to VENDOR, so that
+				 *	the hierachy is correct.
 				 */
-				fr_assert(!fr_sbuff_is_char(&our_in, '.')); /* be sure the loop exits */
-				goto update_relative;
+				unknown_type = FR_TYPE_VENDOR;
+
+			} else {
+				unknown_type = raw_type;
 			}
 
-			if (internal) {
-				slen = fr_dict_oid_component(&err, &da, internal, &our_in, &bareword_terminals);
+			da_unknown = fr_dict_attr_unknown_afrom_oid(root->ctx, parent, &our_in, unknown_type);
+			if (!da_unknown) goto error;
+
+			da = da_unknown;
+			was_unknown = true;
+
+			goto next;
+		} /* huge block of "we didn't find a known attribute" */
+
+		/*
+		 *	We found the component.  It MIGHT be an ALIAS which jumps down a few levels.  Or, it
+		 *	might be a group which jumps back to the dictionary root.  Or it may suddenly be an
+		 *	internal attribute.
+		 *
+		 *	For an ALIAS, we need to add intermediate nodes up to the parent.
+		 *
+		 *	For a GROUP, we need to add nodes up to the ref of the group.
+		 *
+		 *	For internal attributes, we need to add nodes up to the root of the internal
+		 *	dictionary.
+		 */
+		if (da->parent != parent) {
+			int j, diff;
+			fr_dict_attr_t const *up;
+
+			ref = parent;
+
+		found:
+			fr_assert(fr_dict_attr_common_parent(ref, da, true) == ref);
+
+			diff = da->depth - ref->depth;
+			fr_assert(diff >= 1);
+
+			diff--;
+
+			if ((da_stack.depth + diff) >= FR_DICT_MAX_TLV_STACK) {
+				fr_strerror_printf("Attributes are nested too deeply at \"%.*s\"",
+						   (int) fr_sbuff_diff(&op_m, &lhs_m), fr_sbuff_current(&lhs_m));
+				goto error;
 			}
 
-		} else if (raw && fr_type_is_structural(da->type) && (raw_type != FR_TYPE_OCTETS)) {
 			/*
-			 *	We were asked to do a "raw" thing, but we found a known attribute matching
-			 *	that description.
-			 *
-			 *	@todo - this is only allowed because we can't distinguish between "raw.1" and
-			 *	"raw.User-Name".
+			 *	Go back up the da_stack, setting the parent.
 			 */
-			raw = false;
-			raw_type = FR_TYPE_NULL;
-		}
+			up = da;
+			for (j = da_stack.depth + diff; j >= da_stack.depth; j--) {
+				da_stack.da[j] = up;
+				up = up->parent;
+			}
 
-		if (err != FR_DICT_ATTR_OK) {
-		notfound:
-			fr_sbuff_marker(&rhs_m, &our_in);
-			fr_sbuff_adv_past_allowed(&our_in, SIZE_MAX, fr_dict_attr_allowed_chars, NULL);
+			for (j = da_stack.depth; j <= da_stack.depth + diff; j++) {
+				fr_assert(da_stack.da[j] != NULL);
+			}
 
-			fr_strerror_printf("Unknown attribute \"%.*s\" for parent \"%s\"",
-					   (int) fr_sbuff_diff(&our_in, &rhs_m), fr_sbuff_current(&rhs_m),
-					   relative->da->name);
-			return fr_sbuff_error(&our_in);
+			/*
+			 *	Record that we've added more attributes to the da_stack.
+			 */
+			da_stack.depth += diff;
 		}
-		fr_assert(da != NULL);
 
-#if 0
+	next:
 		/*
-		 *	@todo - If we're at the root, then aliases can cause us to jump over intermediate
-		 *	attributes.  In which case we have to create the intermediate attributes, too.
+		 *	Limit the data types that we can parse.  This check is mainly to get better error
+		 *	messages.
 		 */
-		if (relative->da) {
-			if (relative->da->flags.is_root) {
-				fr_assert(da->depth == 1);
+		switch (da->type) {
+		case FR_TYPE_GROUP:
+			if (raw && (raw_type != FR_TYPE_OCTETS)) {
+				fr_strerror_printf("Cannot create 'raw' attributes for data type '%s'", fr_type_to_str(da->type));
+				goto error;
 			}
+			break;
+
+		case FR_TYPE_STRUCTURAL_EXCEPT_GROUP:
+		case FR_TYPE_LEAF:
+			break;
+
+		default:
+			fr_strerror_printf("Invalid data type '%s'", fr_type_to_str(da->type));
+			goto error;
 		}
-#endif
 
 		/*
-		 *	Intermediate components are always found / created.  The final component is
-		 *	always appended, no matter the operator.
+		 *	Everything until the last component must end with a '.', because otherwise there would
+		 *	be no next component.
 		 */
 		if (i < components) {
-			if (append) {
-				vp = fr_pair_find_last_by_da(relative->list, NULL, da);
-				if (!vp) {
-					if (fr_pair_append_by_da(relative->ctx, &vp, relative->list, da) < 0) {
-						return fr_sbuff_error(&our_in);
-					}
-					PAIR_ALLOCED(vp);
-				}
-			} else {
-				vp = fr_pair_afrom_da(relative->ctx, da);
-				if (!vp) return fr_sbuff_error(&our_in);
-
-				PAIR_ALLOCED(vp);
-				fr_pair_append(relative->list, vp);
+			if (!fr_sbuff_next_if_char(&our_in, '.')) {
+				fr_strerror_printf("Missing '.' at \"%.*s\"",
+						   (int) fr_sbuff_diff(&op_m, &lhs_m), fr_sbuff_current(&lhs_m));
+				goto error;
 			}
 
 			/*
-			 *	We had a raw type and we're passing
-			 *	raw octets to it.  We don't care if
-			 *	its structural or anything else.  Just
-			 *	create the raw attribute.
+			 *	Leaf attributes cannot appear in the middle of the OID list.
 			 */
-		} else if (raw_type != FR_TYPE_NULL) {
+			if (fr_type_is_leaf(da->type)) {
+				if (fr_dict_attr_is_key_field(da)) {
+					fr_strerror_printf("Please remove the reference to key field '%s' from the input string",
+							   da->name);
+				} else {
+					fr_strerror_printf("Leaf attribute '%s' cannot have children", da->name);
+				}
+
+				goto error;
+			}
+
+		} else if (raw && !da->flags.is_unknown) {
 			/*
+			 *	Only the last component can be raw.  If the attribute we found isn't unknown,
+			 *	then create an unknown DA from the known one.
+			 *
 			 *	We have parsed the full OID tree, *and* found a known attribute.  e.g. raw.Vendor-Specific = ...
 			 *
 			 *	For some reason, we allow: raw.Vendor-Specific = { ... }
 			 *
 			 *	But this is what we really want: raw.Vendor-Specific = 0xabcdef
 			 */
-			fr_assert(!da_unknown);
-
 			if ((raw_type != FR_TYPE_OCTETS) && (raw_type != da->type)) {
+				/*
+				 *	@todo - because it breaks a lot of the encoders.
+				 */
 				fr_strerror_printf("Cannot create raw attribute %s which changes data type from %s to %s",
 						   da->name, fr_type_to_str(da->type), fr_type_to_str(raw_type));
-				return fr_sbuff_error(&our_in);
+				fr_sbuff_set(&our_in, &lhs_m);
+				goto error;
 			}
 
-			/*
-			 *	If we're parsing raw octets, create a raw octets attribute.
-			 *
-			 *	Otherwise create one of type 'tlv', and then parse the children.
-			 */
-			if (raw_type == FR_TYPE_OCTETS) {
-				da_unknown = fr_dict_attr_unknown_raw_afrom_da(root->ctx, da);
-			} else {
-				da_unknown = fr_dict_attr_unknown_afrom_da(root->ctx, da);
-			}
-			if (!da_unknown) return fr_sbuff_error(&our_in);
-
-			if (fr_pair_append_by_da(relative->ctx, &vp, relative->list, da_unknown) < 0) {
-				fr_dict_attr_unknown_free(&da_unknown);
-				return fr_sbuff_error(&our_in);
-			}
-			PAIR_ALLOCED(vp);
+			da_unknown = fr_dict_attr_unknown_alloc(root->ctx, da, raw_type);
+			if (!da_unknown) goto error;
 
-			fr_dict_attr_unknown_free(&da_unknown);
+			da = da_unknown;
+			was_unknown = true;
+		}
 
-			/*
-			 *	Just create the leaf attribute.
-			 */
-		} else if (da->parent->type == FR_TYPE_STRUCT) {
-			fr_pair_t *tail = fr_pair_list_tail(relative->list);
+		da_stack.da[da_stack.depth] = da;
+	}
 
-			/*
-			 *	If the structure member is _less_ than the last one, go create a new structure
-			 *	in the grandparent.
-			 */
-			if (tail && (tail->da->attr >= da->attr) && !da->flags.array) {
-				fr_pair_t *parent_vp, *grand_vp;
+	/*
+	 *	at least [0]=root, [1]=da, [2]=NULL
+	 */
+	if (da_stack.depth <= 1) {
+		fr_strerror_const("Internal sanity check failed on depth 1");
+		return fr_sbuff_error(&our_in);
+	}
 
-				parent_vp = fr_pair_list_parent(relative->list);
-				if (!parent_vp) goto leaf;
+	if (da_stack.depth <= components) {
+		fr_strerror_const("Internal sanity check failed on depth 2");
+		return fr_sbuff_error(&our_in);
+	}
 
-				fr_assert(da->parent == parent_vp->da);
+	/*
+	 *	STEP 5: Reset the parser to the value, and double-check if it's what we expect.
+	 */
+	fr_sbuff_set(&our_in, &rhs_m);
 
-				grand_vp = fr_pair_parent(parent_vp);
-				if (!grand_vp) goto leaf;
+	if (fr_type_is_structural(da_stack.da[da_stack.depth - 1]->type)) {
+		if (!fr_sbuff_is_char(&our_in, '{')) {
+			fr_strerror_printf("Group list for %s MUST start with '{'", da_stack.da[da_stack.depth - 1]->name);
+			goto error;
+		}
 
-				/*
-				 *	Create a new parent in the context of the grandparent.
-				 */
-				if (fr_pair_append_by_da(grand_vp, &vp, &grand_vp->vp_group, parent_vp->da) < 0) {
-					return fr_sbuff_error(&our_in);
-				}
-				PAIR_ALLOCED(vp);
+		/*
+		 *	The fr_pair_validate() function doesn't support operators for structural attributes,
+		 *	so we forbid them here.
+		 */
+		if (relative->allow_compare && (op != T_OP_EQ) && (op != T_OP_CMP_EQ)) {
+			fr_strerror_printf("Structural attribute '%s' must use '=' or '==' for comparisons",
+					   da_stack.da[da_stack.depth - 1]->name);
+			goto error;
+		}
 
-				relative->ctx = vp;
-				fr_assert(relative->da == vp->da);
-				relative->list = &vp->vp_group;
-			}
+		/*
+		 *	If we have "foo = { ... }", then we just create the attribute.
+		 */
+		if (components == 1) append = (op != T_OP_EQ);
+	}
 
-			goto leaf;
-		} else {
-		leaf:
-			vp = fr_pair_afrom_da_depth_nested(relative->ctx, relative->list, da,
-							   relative->da->depth);
-			if (!vp) return fr_sbuff_error(&our_in);
-			PAIR_ALLOCED(vp);
-		}
+	/*
+	 *	STEP 6: Use the da_stack to either find or add intermediate #fr_pair_t.
+	 */
+	my = *relative;
+	for (i = 1; i < da_stack.depth; i++) {
+		fr_dict_attr_t const *da;
 
-		fr_assert(vp != NULL);
+		da = da_stack.da[i];
 
-	update_relative:
 		/*
-		 *	Reset the parsing to the new namespace if necessary.
+		 *	When we have a full path that contains MEMBERs of a STRUCT, we need to check ordering.
+		 *	The children MUST be added in order.  If we see a child that is out of order, then
+		 *	that means we need to start a new parent STRUCT.
 		 */
-		switch (vp->vp_type) {
-		case FR_TYPE_STRUCTURAL_EXCEPT_GROUP:
-			relative->ctx = vp;
-			relative->da = vp->da;
-			relative->list = &vp->vp_group;
-			break;
+		if ((da->parent->type == FR_TYPE_STRUCT) && (i > 1)) {
+			fr_assert(da_stack.da[i - 1] == da->parent);
+			fr_assert(da_stack.vp[i - 1] != NULL);
+			fr_assert(my.ctx == da_stack.vp[i - 1]);
 
 			/*
-			 *	Groups reset the namespace to the da referenced by the group.
-			 *
-			 *	Internal groups get their namespace to the root namespace.
+			 *	@todo - cache the last previous child that we added?  Or maybe the DA of the
+			 *	last child?
 			 */
-		case FR_TYPE_GROUP:
-			relative->ctx = vp;
-			relative->da = fr_dict_attr_ref(vp->da);
-			if (relative->da == internal) {
-				relative->da = fr_dict_root(root->da->dict);
+			for (vp = fr_pair_list_tail(my.list);
+			     vp != NULL;
+			     vp = fr_pair_list_prev(my.list, vp)) {
+				if (!vp->da->flags.internal) break;
 			}
-			fr_assert(relative->da != NULL);
-			relative->list = &vp->vp_group;
-			break;
 
-		default:
-			/*
-			 *	A leaf attribute MUST be the last one in the list.  We can no longer have
-			 *	"key" fields which contain children.
-			 */
-			if (fr_sbuff_is_char(&our_in, '.')) {
-				if (fr_dict_attr_is_key_field(da)) {
-					fr_strerror_printf("Please remove the reference to key field '%s' from the input string",
-							   da->name);
+			if (vp && (vp->da->attr > da->attr)) {
+				fr_pair_t *parent = da_stack.vp[i - 2];
+
+				if (parent) {
+					if (fr_pair_append_by_da(parent, &vp, &parent->vp_group, da->parent) < 0) {
+						goto error;
+					}
 				} else {
-					fr_strerror_printf("Leaf attribute '%s' cannot have children", da->name);
+					if (fr_pair_append_by_da(root->ctx, &vp, root->list, da->parent) < 0) {
+						goto error;
+					}
 				}
 
-				return fr_sbuff_error(&our_in);
+				vp->op = T_OP_EQ;
+				PAIR_ALLOCED(vp);
+				my.ctx = vp;
+				my.list = &vp->vp_group;
+			}
+		}
+
+		/*
+		 *	Everything up to the last entry must be structural.
+		 *
+		 *	The last entry may be structural, or else it might be a leaf.
+		 */
+		if (fr_type_is_structural(da->type)) {
+			if (append) {
+				vp = fr_pair_find_last_by_da(my.list, NULL, da);
+				if (vp) goto update_relative;
+			}
+
+			if (fr_pair_append_by_da(my.ctx, &vp, my.list, da) < 0) {
+				goto error;
 			}
-			break;
 
-		case FR_TYPE_INTERNAL:
-			fr_strerror_printf("Cannot parse internal data type %s", fr_type_to_str(vp->vp_type));
-			return fr_sbuff_error(&our_in);
+			vp->op = T_OP_EQ;
+			PAIR_ALLOCED(vp);
+
+		update_relative:
+			da_stack.vp[i] = vp;
+
+			my.ctx = vp;
+			my.da = vp->da;
+			my.list = &vp->vp_group;
+			continue;
 		}
 
-		i++;
-	} while (fr_sbuff_next_if_char(&our_in, '.'));
+		/*
+		 *	We're finally at the leaf attribute, which must be the last attribute.
+		 */
+		fr_assert(i == (da_stack.depth - 1));
 
-	if (relative->allow_compare) {
+		vp = fr_pair_afrom_da(my.ctx, da);
+		if (!vp) goto error;
+
+		PAIR_ALLOCED(vp);
 		vp->op = op;
-	} else {
-		vp->op = T_OP_EQ;
+		da_stack.vp[i] = vp;
 	}
 
 	/*
-	 *	Reset the parser to the RHS so that we can parse the value.
+	 *	Intermediate nodes always use the operator '='.  The final one uses the assigned operator.
 	 */
-	fr_sbuff_set(&our_in, &rhs_m);
+	fr_assert(vp != NULL);
+	fr_assert(vp->op != T_INVALID);
 
 	/*
-	 *	The RHS is a list, go parse the nested attributes.
+	 *	STEP 7: Parse the value, recursing if necessary.
+	 *
+	 *	@todo - do all kinds of cleanups if anything fails.  TBH, this really needs the edit lists,
+	 *	and that might be a bit much overhead for this code.
 	 */
-	if (fr_sbuff_next_if_char(&our_in, '{')) {
+	if (fr_type_is_structural(vp->da->type)) {
 		fr_pair_parse_t child = (fr_pair_parse_t) {
 			.allow_compare = root->allow_compare,
+			.dict = root->dict,
+			.internal = root->internal,
 		};
 
-		if (!fr_type_is_structural(vp->vp_type)) {
-			fr_strerror_printf("Cannot assign list to leaf data type %s for attribute %s",
-				fr_type_to_str(vp->vp_type), vp->da->name);
-			return fr_sbuff_error(&our_in);
+		if (!fr_sbuff_next_if_char(&our_in, '{')) {
+			fr_strerror_printf("Child list for %s MUST start with '{'", vp->da->name);
+			goto error;
 		}
 
+		fr_assert(my.ctx == vp);
+		my.allow_compare = root->allow_compare;
+		my.end_of_list = true;
+
 		while (true) {
-			fr_sbuff_adv_past_whitespace(&our_in, SIZE_MAX, NULL);
+			fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
 			if (fr_sbuff_is_char(&our_in, '}')) {
 				break;
 			}
 
-			slen = fr_pair_list_afrom_substr(relative, &child, &our_in);
+			slen = fr_pair_list_afrom_substr(&my, &child, &our_in);
 			if (!slen) break;
 
-			if (slen < 0) return fr_sbuff_error(&our_in) + slen;
+			if (slen < 0) goto error;
 		}
 
 		if (!fr_sbuff_next_if_char(&our_in, '}')) {
 			fr_strerror_const("Failed to end list with '}'");
-			return fr_sbuff_error(&our_in);
+			goto error;
 		}
 
-		goto done;
-	}
+		/*
+		 *	This structure was the last thing we parsed.  The next thing starts from here.
+		 */
+		*relative = my;
 
-	if (fr_type_is_structural(vp->vp_type)) {
-		fr_strerror_printf("Group list for %s MUST start with '{'", vp->da->name);
-		return fr_sbuff_error(&our_in);
+	} else {
+		slen = fr_pair_value_from_substr(vp, &our_in, relative->tainted);
+		if (slen <= 0) goto error;
+
+		fr_pair_append(my.list, vp);
 	}
 
-	slen = fr_pair_value_from_substr(vp, &our_in, relative->tainted);
-	if (slen <= 0) return fr_sbuff_error(&our_in) + slen;
+	PAIR_VERIFY(vp);
+
+	CLEAN_DA_STACK;
+
+	fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
 
-done:
 	/*
-	 *	Now that we have a value, verify the full VP.
+	 *	STEP 8: See if we're done, or if we need to stop parsing this #fr_pair_t.
+	 *
+	 *	Allow a limited set of characters after a value.
+	 *
+	 *	It can be "," OR "CRLF" OR ",CRLF".  But not anything else.
 	 */
-	PAIR_VERIFY(vp);
-
 	keep_going = false;
 	if (fr_sbuff_next_if_char(&our_in, ',')) {
+		fr_sbuff_adv_past_blank(&our_in, SIZE_MAX, NULL);
+
 		keep_going = true;
 		relative->last_char = ',';
 	}
 
+	/*
+	 *	We hit the end of the parent list.  There's no need to update "relative", we just return, and
+	 *	let the caller end the list.
+	 *
+	 *	Note that we allow trailing commas:  Foo = { Bar = Baz, }
+	 *
+	 *	We don't care about any trailing data.
+	 */
+	if (relative->end_of_list && fr_sbuff_is_char(&our_in, '}')) {
+		relative->last_char = '\0';
+		goto done;
+	}
+
 	if (relative->allow_crlf) {
 		size_t len;
 
 		len = fr_sbuff_adv_past_allowed(&our_in, SIZE_MAX, sbuff_char_line_endings, NULL);
 		if (len) {
-			keep_going |= true;
+			keep_going = true;
 			if (!relative->last_char) relative->last_char = '\n';
 		}
 	}
 
-	keep_going &= ((fr_sbuff_remaining(&our_in) > 0) || (fr_sbuff_extend(&our_in) > 0));
+	/*
+	 *	This is mainly for the detail file reader.  We allow zeros as end of "attr op value".  But we
+	 *	also treat zeros as "don't keep going".
+	 */
+	if (relative->allow_zeros) {
+		while (fr_sbuff_next_if_char(&our_in, '\0')) {
+			/* nothing */
+		}
+
+		goto done;
+	}
+
+	/*
+	 *	There's no more input, we're done.  Any next attributes will cause the input to be parsed from
+	 *	the root again.
+	 */
+	(void) fr_sbuff_extend(&our_in);
+	if (!fr_sbuff_remaining(&our_in)) goto done;
+
+	/*
+	 *	STEP 9: If we need to keep going, then set up the relative references based on what we've
+	 *	done, and go back to start over again.
+	 *
+	 *	The caller is responsible for checking whether or not we have too much data.
+	 */
+	if (keep_going) {
+		/*
+		 *	Update the relative list for parsing the next pair.
+		 */
+		if (fr_type_is_leaf(vp->da->type)) {
+			fr_pair_t *parent;
+
+			parent = fr_pair_parent(vp);
+			if (!parent) {
+				*relative = *root;
+
+			} else {
+				relative->ctx = parent;
+				relative->da = parent->da;
+				relative->list = &parent->vp_group;
+			}
+
+		} else {
+			relative->ctx = vp;
+			relative->da = vp->da;
+			relative->list = &vp->vp_group;
+		}
+
+		goto redo;
+	}
+
+	/*
+	 *	STEP 10: Complain if we have unexpected input.
+	 *
+	 *	We have more input, BUT we didn't have a comma or CRLF to explicitly finish the last pair we
+	 *	read.  That's a problem.
+	 */
+	if (!relative->last_char) {
+		size_t remaining;
+
+		remaining = fr_sbuff_remaining(&our_in);
 
-	if (keep_going) goto redo;
+		if (remaining > 20) remaining = 20;
 
+		fr_strerror_printf("Unexpected text '%.*s ...' after value",
+				   (int) remaining, fr_sbuff_current(&our_in));
+		return fr_sbuff_error(&our_in); /* da_stack has already been cleaned */
+	}
+
+done:
+	/*
+	 *	STEP 11: Finally done.
+	 */
 	FR_SBUFF_SET_RETURN(in, &our_in);
 }
 
@@ -700,6 +1152,8 @@ int fr_pair_list_afrom_file(TALLOC_CTX *ctx, fr_dict_t const *dict, fr_pair_list
 		.ctx = ctx,
 		.da = fr_dict_root(dict),
 		.list = &tmp_list,
+		.dict = dict,
+		.internal = fr_dict_internal(),
 		.allow_crlf = true,
 		.allow_compare = true,
 	};
diff --git a/src/lib/util/pair_legacy.h b/src/lib/util/pair_legacy.h
index 12f323f9d07..fb449f694ea 100644
--- a/src/lib/util/pair_legacy.h
+++ b/src/lib/util/pair_legacy.h
@@ -49,8 +49,10 @@ typedef struct fr_pair_parse_s {
 
 	bool			allow_compare;	//!< allow comparison operators
 	bool			allow_crlf;	//!< allow CRLF, and treat like comma
+	bool			allow_zeros;	//!< allow '\0' as end of attribute
 	bool			tainted;	//!< source is tainted
 	char			last_char;	//!< last character we read - ',', '\n', or 0 for EOF
+	bool			end_of_list;	//!< do we expect an end of list '}' character?
 } fr_pair_parse_t;
 
 fr_slen_t fr_pair_list_afrom_substr(fr_pair_parse_t const *root, fr_pair_parse_t *relative,
diff --git a/src/listen/detail/proto_detail.c b/src/listen/detail/proto_detail.c
index 48295fee621..ecf0755dd84 100644
--- a/src/listen/detail/proto_detail.c
+++ b/src/listen/detail/proto_detail.c
@@ -221,7 +221,7 @@ static int mod_decode(void const *instance, request_t *request, uint8_t *const d
 	fr_pair_list_t		tmp_list;
 	fr_dcursor_t		cursor;
 	time_t			timestamp = 0;
-	fr_pair_parse_t root, relative;
+	fr_pair_parse_t		root, relative;
 
 	RHEXDUMP3(data, data_len, "proto_detail decode packet");
 
@@ -263,6 +263,8 @@ static int mod_decode(void const *instance, request_t *request, uint8_t *const d
 	 *	Parse each individual line.
 	 */
 	while (p < end) {
+		fr_slen_t slen;
+
 		/*
 		 *	Each record begins with a zero byte.  If the
 		 *	next byte is also zero, that's the end of
@@ -355,16 +357,24 @@ static int mod_decode(void const *instance, request_t *request, uint8_t *const d
 			.list = &tmp_list,
 			.dict = request->proto_dict,
 			.internal = fr_dict_internal(),
+			.allow_zeros = true,
 		};
 		relative = (fr_pair_parse_t) { };
 
-		if ((fr_pair_list_afrom_substr(&root, &relative,
-					       &FR_SBUFF_IN((char const *) p, (data + data_len) - p)) > 0) && !fr_pair_list_empty(&tmp_list)) {
+		slen = fr_pair_list_afrom_substr(&root, &relative,
+						 &FR_SBUFF_IN((char const *) p, (data + data_len) - p));
+		if (slen < 0) {
+			RPEDEBUG("Failed reading line");
+			vp = NULL;
+
+		} else if ((slen == 0) || fr_pair_list_empty(&tmp_list)) {
+			vp = NULL;
+			RWDEBUG("Ignoring line %d - %s", lineno, p);
+
+		} else {
+
 			vp = fr_pair_list_head(&tmp_list);
 			fr_pair_list_append(&request->request_pairs, &tmp_list);
-		} else {
-			vp = NULL;
-			RWDEBUG("Ignoring line %d - :%s", lineno, p);
 		}
 
 		/*
diff --git a/src/tests/unit/protocols/der/base.txt b/src/tests/unit/protocols/der/base.txt
index cde038488a1..8f8e832f2a6 100644
--- a/src/tests/unit/protocols/der/base.txt
+++ b/src/tests/unit/protocols/der/base.txt
@@ -911,10 +911,16 @@ encode-pair Test-GeneralNames = { iPAddress = 10.0.5.4 }
 match 30 06 87 04 0a 00 05 04
 
 #
-#  Ignore raw.FOO when we know the subtypes.
+#  Raw groups cannot be TLVs, they can only be octets
 #
 encode-pair raw.Test-GeneralNames = { dNSName = "foo.bar" }
-match 30 09 82 07 66 6f 6f 2e 62 61 72
+match Cannot create 'raw' attributes for data type 'group'
+
+#
+#  @todo - This shouldn't be raw data. It should be wrapped in data type 'octets'
+#
+encode-pair raw.Test-GeneralNames = 0xabcdef
+match ab cd ef
 
 proto-dictionary-root Test-GeneralNames
 
@@ -936,4 +942,4 @@ match c0 01 01 c1 01 02
 
 
 count
-match 560
+match 562
diff --git a/src/tests/unit/protocols/dhcpv6/client-id.txt b/src/tests/unit/protocols/dhcpv6/client-id.txt
index c5b87f54858..cfb986170bb 100644
--- a/src/tests/unit/protocols/dhcpv6/client-id.txt
+++ b/src/tests/unit/protocols/dhcpv6/client-id.txt
@@ -9,17 +9,17 @@ proto dhcpv6
 proto-dictionary dhcpv6
 fuzzer-out dhcpv6
 
-#
-#  This seems very wrong.
-#
 pair Client-ID.UUID.Value = 0xabcdef
-match Client-ID = { Value.UUID = { Value = 0xabcdef } }
+match Client-ID = { UUID = { Value = 0xabcdef } }
 
 encode-pair Client-ID.UUID.Value = 0xabcdef
-match fr_struct_to_network: Asked to encode UUID, but its parent Value is not the expected parent Client-ID
+match 00 01 00 12 00 04 ab cd ef 00 00 00 00 00 00 00 00 00 00 00 00 00
 
-#decode-pair -
-#match foo
+#
+#  The value is padded to 16 octets.
+#
+decode-pair -
+match Client-ID = { DUID = ::UUID, UUID = { Value = 0xabcdef00000000000000000000000000 } }
 
 count
-match 7
+match 9
diff --git a/src/tests/unit/protocols/dhcpv6/vendor.txt b/src/tests/unit/protocols/dhcpv6/vendor.txt
index 58c0db677fb..9e841db59f6 100644
--- a/src/tests/unit/protocols/dhcpv6/vendor.txt
+++ b/src/tests/unit/protocols/dhcpv6/vendor.txt
@@ -17,7 +17,7 @@ match 00 11 00 2a 00 00 19 7f 00 01 00 0a 4c 41 42 4f 4c 54 32 2d 6e 61 00 02 00
 decode-pair -
 match Vendor-Opts = { Nokia-SR = { WAN-Pool = "LABOLT2-na", PFX-Pool = "LABOLT2-pd", PFX-Len = 63, Reserved-NA-Len = 64 } }
 
-encode-pair Vendor-Opts.Nokia-SR = { WAN-Pool = "LABOLT2-na" }, Vendor-Opts.Nokia-SR = { PFX-Pool = "LABOLT2-pd" }, Vendor-Opts.Nokia-SR = { PFX-Len = 63 }, Vendor-Opts.Nokia-SR = { Reserved-NA-Len = 64 }
+encode-pair Vendor-Opts.Nokia-SR := { WAN-Pool = "LABOLT2-na" }, Vendor-Opts.Nokia-SR := { PFX-Pool = "LABOLT2-pd" }, Vendor-Opts.Nokia-SR := { PFX-Len = 63 }, Vendor-Opts.Nokia-SR := { Reserved-NA-Len = 64 }
 match 00 11 00 12 00 00 19 7f 00 01 00 0a 4c 41 42 4f 4c 54 32 2d 6e 61 00 11 00 12 00 00 19 7f 00 02 00 0a 4c 41 42 4f 4c 54 32 2d 70 64 00 11 00 09 00 00 19 7f 00 03 00 01 3f 00 11 00 09 00 00 19 7f 00 04 00 01 40
 
 decode-pair -
diff --git a/src/tests/unit/protocols/radius/wimax.txt b/src/tests/unit/protocols/radius/wimax.txt
index f328a1d5eba..ed3f6ae7cbf 100644
--- a/src/tests/unit/protocols/radius/wimax.txt
+++ b/src/tests/unit/protocols/radius/wimax.txt
@@ -287,13 +287,13 @@ decode-pair -
 match Vendor-Specific = { WiMAX = { Capability = { Release = "1.0" } } }
 
 #
-#  two VSAs in "flat" form, create two VSAs.
+#  two VSAs in "flat" form using ":=" creates two VSAs.
 #
-encode-pair Vendor-Specific.WiMAX.Capability = { Release = "1.0" }, Vendor-Specific.WiMAX.Capability = { Accounting-Capabilities = No-Accounting }
+encode-pair Vendor-Specific.WiMAX.Capability := { Release = "1.0" }, Vendor-Specific.WiMAX.Capability := { Accounting-Capabilities = No-Accounting }
 match 1a 0e 00 00 60 b5 01 08 00 01 05 31 2e 30 1a 0c 00 00 60 b5 01 06 00 02 03 00
 
 #
-#  Two VSAs in "nested" form create one VSA.
+#  Two VSAs in "nested" form creates one VSA.
 #
 encode-pair Vendor-Specific.WiMAX.Capability = { Release = "1.0", Accounting-Capabilities = No-Accounting }
 match 1a 11 00 00 60 b5 01 0b 00 01 05 31 2e 30 02 03 00