From: YASUOKA Masahiko 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 Hello, On Thu, 18 Sep 2025 20:29:44 +0200 Denis Fondras 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; > >