Index | Thread | Search

From:
YASUOKA Masahiko <yasuoka@openbsd.org>
Subject:
Re: radiusd_standard(8): add attributes to responses
To:
denis@openbsd.org
Cc:
tech@openbsd.org
Date:
Sat, 20 Sep 2025 14:52:45 +0900

Download raw body.

Thread
Hello,

On Thu, 18 Sep 2025 20:29:44 +0200
Denis Fondras <denis@openbsd.org> wrote:
> My provider requires additional attributes when authentifying
> users.
> The add-response-attribute configuration directive permits that.

I like this idea.

I'd like to know this use case correctly.  I suppose the additional
attributes is needed for the *requests* if they are required when
authenticating in general.  But the diff is to add the attributes to
the *responses*.  I'd like you to let me know what I am missing.

I think it's better to have both add-request-attribute and
add-response-attribute either way.


Also, I'd like to make it possible to configure any types other than
pre-defined.  But I don't have a good way to give the type of the
value, other than a tricky one like below.

  set add-response-attribute 8 ip:192.168.0.1
  set add-response-attribute 12 int:1400



I wrote some comments inline.

> My additional config in radiud.conf :
> ```
> module add-attributes "/usr/libexec/radiusd/radiusd_standard" {
>         set add-response-attribute Tunnel-Type 3
>         set add-response-attribute Tunnel-Medium-Type 1
>         set add-response-attribute Tunnel-Server-Endpoint 192.0.2.1
> }
> 
> authenticate *@myrealm by file decorate-by strip-realm add-attributes
> ```
> 
> Denis
> 
> Index: radiusd_standard.8
> ===================================================================
> RCS file: /cvs/src/usr.sbin/radiusd/radiusd_standard.8,v
> diff -u -p -r1.4 radiusd_standard.8
> --- radiusd_standard.8	4 Aug 2024 03:56:57 -0000	1.4
> +++ radiusd_standard.8	18 Sep 2025 18:17:08 -0000
> @@ -63,6 +63,19 @@ To specify a vendor attribute,
>  specify the Vendor-Id
>  in a decimal number for
>  .Ar vendor .
> +.Pp
> +.It Cm add-response-attribute Oo Ar vendor Oc Ar type Oc Ar value
> +Add the specified attributes to response messages of Access-Request.
> +Specify
> +.Ar type
> +of the attribute in a decimal number or supported description
> +(Tunnel-Type, Tunnel-Medium-Type and Tunnel-Server-Endpoint)
> +followed by the
> +.Ar value .
> +To specify a vendor attribute,
> +specify the Vendor-Id
> +in a decimal number for
> +.Ar vendor .
>  .El
>  .Sh FILES
>  .Bl -tag -width "/usr/libexec/radiusd/radiusd_standard" -compact
> Index: radiusd_standard.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/radiusd/radiusd_standard.c,v
> diff -u -p -r1.6 radiusd_standard.c
> --- radiusd_standard.c	2 Jul 2024 00:33:51 -0000	1.6
> +++ radiusd_standard.c	18 Sep 2025 18:17:08 -0000
> @@ -37,10 +37,15 @@
>  TAILQ_HEAD(attrs,attr);
>  
>  struct attr {
> -	uint8_t			 type;
> -	uint32_t		 vendor;
> -	uint32_t		 vtype;
> -	TAILQ_ENTRY(attr)	 next;
> +	uint8_t			type;
> +	uint32_t		vendor;
> +	uint32_t		vtype;
> +	union {
> +		uint32_t	numeric;
> +		char		string[255];
> +	} value;
> +	uint32_t		len;
> +	TAILQ_ENTRY(attr)	next;
>  };
>  
>  struct module_standard {
> @@ -49,6 +54,7 @@ struct module_standard {
>  	bool			 strip_nt_domain;
>  	struct attrs		 remove_reqattrs;
>  	struct attrs		 remove_resattrs;
> +	struct attrs		 add_resattrs;
>  };
>  
>  struct radius_const_str {
> @@ -80,6 +86,50 @@ static struct radius_const_str
>  		 acct_status_type_consts[], acct_authentic_consts[],
>  		 terminate_cause_consts[], tunnel_medium_type_consts[];
>  
> +static int	parse_strip_atmark(char * const *, int,
> +		    struct module_standard *);
> +static int	parse_strip_ntdom(char * const *, int,
> +		    struct module_standard *);
> +static int	parse_rm_reqattr(char * const *, int,
> +		    struct module_standard *);
> +static int	parse_rm_resattr(char * const *, int,
> +		    struct module_standard *);
> +static int	parse_add_attr(char * const *, int, struct module_standard *);
> +static int	parse_args(char * const *, struct attr *);
> +
> +struct command {
> +	char *name;
> +	int minargs;
> +	int maxargs;
> +	int (*func)(char * const *, int, struct module_standard *);
> +} commands[] = {
> +	{ "strip-atmark-realm", 1, 1, parse_strip_atmark },
> +	{ "strip-nt-domain", 1, 1, parse_strip_ntdom },
> +	{ "remove-request-attribute", 1, 2, parse_rm_reqattr },
> +	{ "remove-response-attribute", 1, 2, parse_rm_resattr },
> +	{ "add-response-attribute", 2, 3, parse_add_attr },
> +	{ "_", 0, 0, NULL },
> +	{ NULL, 0, 0, NULL },
> +};
> +
> +enum attribute_types {
> +	T_NULL = 0,
> +	T_NUM,
> +	T_STRING
> +};
> +
> +struct radius_attribute_type {
> +	char *name; 

There is an extra space at the end.

> +	u_int8_t val;
> +	enum attribute_types type;
> +	u_int8_t len;
> +} types[] = {
> +	{"Tunnel-Type", 64, T_NUM, 4},
> +	{"Tunnel-Medium-Type", 65, T_NUM, 4},
> +	{"Tunnel-Server-Endpoint", 67, T_STRING, 255},
> +	{NULL, 0, T_NULL, 0}
> +};
> +
>  int
>  main(int argc, char *argv[])
>  {
> @@ -95,6 +145,7 @@ main(int argc, char *argv[])
>  	memset(&module_standard, 0, sizeof(module_standard));
>  	TAILQ_INIT(&module_standard.remove_reqattrs);
>  	TAILQ_INIT(&module_standard.remove_resattrs);
> +	TAILQ_INIT(&module_standard.add_resattrs);
>  
>  	if ((module_standard.base = module_create(
>  	    STDIN_FILENO, &module_standard, &handlers)) == NULL)
> @@ -120,96 +171,237 @@ main(int argc, char *argv[])
>  		TAILQ_REMOVE(&module_standard.remove_resattrs, attr, next);
>  		freezero(attr, sizeof(struct attr));
>  	}
> +	while ((attr = TAILQ_FIRST(&module_standard.add_resattrs)) != NULL) {
> +		TAILQ_REMOVE(&module_standard.add_resattrs, attr, next);
> +		freezero(attr, sizeof(struct attr));
> +	}
>  
>  	exit(EXIT_SUCCESS);
>  }
>  
> +static int
> +parse_args(char * const *argv, struct attr *attr)
> +{
> +	struct radius_attribute_type	*att;
> +	const char			*errstr;
> +
> +	attr->type = strtonum(argv[0], 0, 255, &errstr);
> +	if (errstr) {
> +		/* Then assume it is a string */
> +		if (strlcpy(attr->value.string, argv[0], 255) >= 255)
> +			return (-1);
> +
> +		for (att = types; att->name; att++)
> +			if (strcmp(attr->value.string, att->name) == 0)
> +				break;
> +		if (att->name)
> +			attr->type = att->val;
> +	}
> +
> +	for (att = types; att->name; att++)
> +		if (att->val == attr->type)
> +			break;
> +
> +	if (att->name == NULL)
> +		return (-1);
> +
> +	if (att->type == T_NUM) {
> +		attr->value.numeric = strtonum(argv[1], 0, UINT32_MAX, &errstr);
> +		if (errstr)
> +			return (-1);
> +	} else if (att->type == T_STRING) {
> +		if (strlcpy(attr->value.string, argv[1], 255) >= 255)
> +			return (-1);
> +	}
> +
> +	return (0);
> +}
> +
> +static enum attribute_types
> +get_attribute_type(u_int8_t value)
> +{
> +	struct radius_attribute_type	*att;
> +
> +	for (att = types; att->name; att++)
> +		if (att->val == value)
> +			return att->type;
> +
> +	return T_NULL;
> +}
> +
> +static uint32_t
> +get_attribute_len(u_int8_t value)
> +{
> +	struct radius_attribute_type	*att;
> +
> +	for (att = types; att->name; att++)
> +		if (att->val == value)
> +			return (att->len);
> +
> +	return 0;
> +}
> +
> +static int
> +parse_strip_atmark(char * const *argv, int argc, struct module_standard *module)
> +{
> +	if (strcmp(argv[0], "true") == 0)
> +		module->strip_atmark_realm = true;
> +	else if (strcmp(argv[0], "false") == 0)
> +		module->strip_atmark_realm = false;
> +	else
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int
> +parse_strip_ntdom(char * const *argv, int argc, struct module_standard *module)
> +{
> +	if (strcmp(argv[0], "true") == 0)
> +		module->strip_nt_domain = true;
> +	else if (strcmp(argv[0], "false") == 0)
> +		module->strip_nt_domain = false;
> +	else
> +		return -1;
> +
> +	return 0;
> +}
> +
> +static int
> +parse_rm_reqattr(char * const *argv, int argc, struct module_standard *module)
> +{
> +	struct attr	*attr;
> +	const char	*errstr;
> +
> +	if ((attr = calloc(1, sizeof(struct attr))) == NULL) {
> +		return -1;
> +	}
> +	if (argc == 1) {
> +		attr->type = strtonum(argv[0], 0, 255, &errstr);
> +		if (errstr == NULL &&
> +		    attr->type != RADIUS_TYPE_VENDOR_SPECIFIC) {
> +			TAILQ_INSERT_TAIL(&module->remove_reqattrs,
> +			    attr, next);
> +			attr = NULL;
> +		}
> +	} else {
> +		attr->vtype = RADIUS_TYPE_VENDOR_SPECIFIC;
> +		attr->vendor = strtonum(argv[0], 0, UINT32_MAX, &errstr);
> +		if (errstr == NULL)
> +			attr->type = strtonum(argv[1], 0, 255, &errstr);

The diff is doing

  attr->vtype = RADIUS_TYPE_VENDOR_SPECIFIC
  attr->type = (vendor type)
  attr->vendor = (vendor id)

But the previous was doing

  attr->type = RADIUS_TYPE_VENDOR_SPECIFIC
  attr->vendor = (vendor id)
  attr->vtype = (vendor type)

I'd like to keep the previous because the RADIUS_TYPE_VENDOR_SPECIFIC
is the value for "Type" field in the RADIUS attribute when it's a
vendor specific attribute and "vtype" is to represent the "Vendor
type" field in the RADIUS vendor specific attribute.

> +		if (errstr == NULL) {
> +			TAILQ_INSERT_TAIL(&module->remove_reqattrs, attr,
> +			    next);
> +			attr = NULL;
> +		}
> +	}
> +	freezero(attr, sizeof(struct attr));
> +	if (errstr == NULL)
> +		return 0;
> +	else
> +		return -1;
> +}
> +
> +static int
> +parse_rm_resattr(char * const *argv, int argc, struct module_standard *module)
> +{
> +	struct attr	*attr;
> +	const char	*errstr;
> +
> +	if ((attr = calloc(1, sizeof(struct attr))) == NULL) {
> +		return -1;
> +	}
> +	if (argc == 1) {
> +		attr->type = strtonum(argv[0], 0, 255, &errstr);
> +		if (errstr == NULL &&
> +		    attr->type != RADIUS_TYPE_VENDOR_SPECIFIC) {
> +			TAILQ_INSERT_TAIL(&module->remove_resattrs,
> +			    attr, next);
> +			attr = NULL;
> +		}
> +	} else {
> +		attr->vtype = RADIUS_TYPE_VENDOR_SPECIFIC;
> +		attr->vendor = strtonum(argv[0], 0, UINT32_MAX, &errstr);
> +		if (errstr == NULL)
> +			attr->type = strtonum(argv[1], 0, 255, &errstr);
> +		if (errstr == NULL) {
> +			TAILQ_INSERT_TAIL(&module->remove_resattrs, attr,
> +			    next);
> +			attr = NULL;
> +		}
> +	}
> +	freezero(attr, sizeof(struct attr));
> +	if (errstr == NULL)
> +		return 0;
> +	else
> +		return -1;
> +}
> +
> +static int
> +parse_add_attr(char * const *argv, int argc, struct module_standard *module)
> +{
> +	struct attr	*attr;
> +	const char	*errstr;
> +
> +	if ((attr = calloc(1, sizeof(struct attr))) == NULL) {
> +		return -1;
> +	}
> +
> +	if (argc == 3) {
> +		attr->vtype = RADIUS_TYPE_VENDOR_SPECIFIC;
> +		attr->vendor = strtonum(argv[0], 0, UINT32_MAX, &errstr);
> +		if (errstr != NULL) {
> +			freezero(attr, sizeof(struct attr));
> +			return -1;
> +		}
> +		argv++;
> +	}
> +
> +	if (parse_args(argv, attr) != 0) {
> +		freezero(attr, sizeof(struct attr));
> +		return -1;
> +	}
> +
> +	TAILQ_INSERT_TAIL(&module->add_resattrs, attr,
> +	    next);
> +
> +	return 0;
> +}
> +
>  static void
>  module_standard_config_set(void *ctx, const char *name, int argc,
>      char * const * argv)
>  {
>  	struct module_standard	*module = ctx;
> -	struct attr		*attr;
> -	const char		*errmsg = "none";
> -	const char		*errstr;
> +	struct command		*cmd;
>  
> -	if (strcmp(name, "strip-atmark-realm") == 0) {
> -		SYNTAX_ASSERT(argc == 1,
> -		    "`strip-atmark-realm' must have only one argment");
> -		if (strcmp(argv[0], "true") == 0)
> -			module->strip_atmark_realm = true;
> -		else if (strcmp(argv[0], "false") == 0)
> -			module->strip_atmark_realm = false;
> -		else
> -			SYNTAX_ASSERT(0,
> -			    "`strip-atmark-realm' must `true' or `false'");
> -	} else if (strcmp(name, "strip-nt-domain") == 0) {
> -		SYNTAX_ASSERT(argc == 1,
> -		    "`strip-nt-domain' must have only one argment");
> -		if (strcmp(argv[0], "true") == 0)
> -			module->strip_nt_domain = true;
> -		else if (strcmp(argv[0], "false") == 0)
> -			module->strip_nt_domain = false;
> -		else
> -			SYNTAX_ASSERT(0,
> -			    "`strip-nt-domain' must `true' or `false'");
> -	} else if (strcmp(name, "remove-request-attribute") == 0 ||
> -	    strcmp(name, "remove-response-attribute") == 0) {
> -		struct attrs		*attrs;
> -
> -		if (strcmp(name, "remove-request-attribute") == 0) {
> -			SYNTAX_ASSERT(argc == 1 || argc == 2,
> -			    "`remove-request-attribute' must have one or two "
> -			    "argment");
> -			attrs = &module->remove_reqattrs;
> -		} else {
> -			SYNTAX_ASSERT(argc == 1 || argc == 2,
> -			    "`remove-response-attribute' must have one or two "
> -			    "argment");
> -			attrs = &module->remove_resattrs;
> -		}
> -		if ((attr = calloc(1, sizeof(struct attr))) == NULL) {
> -			module_send_message(module->base, IMSG_NG,
> -			    "Out of memory: %s", strerror(errno));
> -		}
> -		if (argc == 1) {
> -			attr->type = strtonum(argv[0], 0, 255, &errstr);
> -			if (errstr == NULL &&
> -			    attr->type != RADIUS_TYPE_VENDOR_SPECIFIC) {
> -				TAILQ_INSERT_TAIL(attrs, attr, next);
> -				attr = NULL;
> -			}
> -		} else {
> -			attr->type = RADIUS_TYPE_VENDOR_SPECIFIC;
> -			attr->vendor = strtonum(argv[0], 0, UINT32_MAX,
> -			    &errstr);
> -			if (errstr == NULL)
> -				attr->vtype = strtonum(argv[1], 0, 255,
> -				    &errstr);
> -			if (errstr == NULL) {
> -				TAILQ_INSERT_TAIL(attrs, attr, next);
> -				attr = NULL;
> -			}
> -		}
> -		freezero(attr, sizeof(struct attr));
> -		if (strcmp(name, "remove-request-attribute") == 0)
> -			SYNTAX_ASSERT(attr == NULL,
> -			    "wrong number for `remove-request-attribute`");
> -		else
> -			SYNTAX_ASSERT(attr == NULL,
> -			    "wrong number for `remove-response-attribute`");
> -	} else if (strncmp(name, "_", 1) == 0)
> -		/* nothing */; /* ignore all internal messages */
> -	else {
> +	for (cmd = commands; cmd->name; cmd++)
> +		if (strncmp(cmd->name, name, strlen(cmd->name)) == 0)
> +			break;
> +
> +	if (cmd->name == NULL) {
>  		module_send_message(module->base, IMSG_NG,
>  		    "Unknown config parameter name `%s'", name);
>  		return;
>  	}
> -	module_send_message(module->base, IMSG_OK, NULL);
> -	return;
>  
> - syntax_error:
> -	module_send_message(module->base, IMSG_NG, "%s", errmsg);
> +	if (argc < cmd->minargs || argc > cmd->maxargs) {
> +		module_send_message(module->base, IMSG_NG,
> +		    "Wrong number of parameters for `%s'", cmd->name);
> +		return;
> +	}
> +
> +	if (cmd->func == NULL)
> +		goto finish;
> +
> +	if (cmd->func(argv, argc, module) == -1) {
> +		module_send_message(module->base, IMSG_NG,
> +		    "Invalid attribute for `%s'", cmd->name);
> +		return;
> +	}
> +
> +finish:
> +	module_send_message(module->base, IMSG_OK, NULL);
>  }
>  
>  /* request message decoration */
> @@ -268,7 +460,7 @@ module_standard_reqdeco(void *ctx, u_int
>  			radius_del_attr_all(radpkt, attr->type);
>  		else
>  			radius_del_vs_attr_all(radpkt, attr->vendor,
> -			    attr->vtype);
> +			    attr->type);
>  	}
>  	if (radpkt == NULL) {
>  		pkt = NULL;
> @@ -294,7 +486,65 @@ module_standard_resdeco(void *ctx, u_int
>  	struct module_standard	*module = ctx;
>  	RADIUS_PACKET		*radres = NULL;
>  	struct attr		*attr;
> +	uint32_t		 numeric;
> +	int			 putres = 0;
>  
> +	TAILQ_FOREACH(attr, &module->add_resattrs, next) {
> +		if (radres == NULL &&
> +		    (radres = radius_convert_packet(res, reslen)) == NULL) {
> +			syslog(LOG_ERR,
> +			    "%s: radius_convert_packet() failed: %m", __func__);
> +			module_stop(module->base);
> +			return;
> +		}
> +		if (attr->vtype == RADIUS_TYPE_VENDOR_SPECIFIC) {
> +			switch (get_attribute_type(attr->type)) {
> +			case T_NUM:
> +				numeric = htonl(attr->value.numeric);
> +				putres = radius_put_vs_raw_attr_cat(radres,
> +				    attr->vendor, attr->type,
> +				    &numeric,
> +				    get_attribute_len(attr->type));
> +				break;
> +			case T_STRING:
> +				putres = radius_put_vs_raw_attr_cat(radres,
> +				    attr->vendor, attr->type,
> +				    attr->value.string,
> +				    strlen(attr->value.string));
> +				break;
> +			default:
> +				syslog(LOG_ERR,
> +				    "%s: unknown attribute type", __func__);
> +				module_stop(module->base);
> +				return;
> +			}
> +		} else {
> +			switch (get_attribute_type(attr->type)) {
> +			case T_NUM:
> +				numeric = htonl(attr->value.numeric);
> +				putres = radius_put_raw_attr_cat(radres,
> +				    attr->type, &numeric,
> +				    get_attribute_len(attr->type));
> +				break;
> +			case T_STRING:
> +				putres = radius_put_raw_attr_cat(radres,
> +				    attr->type, attr->value.string,
> +				    strlen(attr->value.string));
> +				break;
> +			default:
> +				syslog(LOG_ERR,
> +				    "%s: unknown attribute type", __func__);
> +				module_stop(module->base);
> +				return;
> +			}
> +		}
> +		if (putres == -1) {
> +			syslog(LOG_ERR,
> +			    "%s: radius_put_*_raw_attr_cat failed: %m", __func__);
> +			module_stop(module->base);
> +			return;
> +		}
> +	}
>  	TAILQ_FOREACH(attr, &module->remove_resattrs, next) {
>  		if (radres == NULL &&
>  		    (radres = radius_convert_packet(res, reslen)) == NULL) {
> @@ -303,11 +553,11 @@ module_standard_resdeco(void *ctx, u_int
>  			module_stop(module->base);
>  			return;
>  		}
> -		if (attr->type != RADIUS_TYPE_VENDOR_SPECIFIC)
> -			radius_del_attr_all(radres, attr->type);
> -		else
> +		if (attr->vtype == RADIUS_TYPE_VENDOR_SPECIFIC)
>  			radius_del_vs_attr_all(radres, attr->vendor,
> -			    attr->vtype);
> +			    attr->type);
> +		else
> +			radius_del_attr_all(radres, attr->type);
>  	}
>  	if (radres == NULL) {
>  		res = NULL;
> 
>