libradius(3) IPv6 support

[pjd]

Summary

Implement IPv6 support in libradius.

As a bonus if a hostname is given to rad_add_server_bindto() function and this hostname resolves to multiple IP addresses we will create a rad_server for each IP address.

[bz]

This feels like a 15 year old patch I've seen before.

Also can we add #ifdefs for INET and INET6 for WITH_INET_SUPPORT and WITH_INET6_SUPPORT?

[pjd]

You are probably talking about this one: https://markmail.org/message/e3zkukh7oggutrvw
I've tried it, but it is very out-dated and has some bugs. It also keeps server on a TAILQ, which is nice, but I don't think it should be combined. The nice thing about that patch was to add multiple servers when a hostname resolves to multiple IP addresses, which I also implemented.

[pjd]

• Allow for optional compilation without IPv4 or IPv6 support.
• Make it possible to use IPv6 addresses in configuration file.
• Call getaddrinfo(3) with flags set to AI_ADDRCONFIG.

[oshogbo]

radlib.c

398

Maybe:

sa = NULL;

if (bindto != NULL) {
...
}

453

I just wonder if we should add some somewhere check that at least one INET or INET6 was given?

497

Extra space.
Maybe assert the snprintf output?

534

Shouldn't we gen some warning about that?
Like above.

662

Maybe simplify that:

if (pp == NULL) {
   generr()
}

[rest of code without else]

699

Maybe we want to check here also the range of port?
We shouldnt also errno check only if *portp is equals to 0 ?

701

I don't thing we should retake the errno here.

1022

Style: POS_LENGTH + 1

[oshogbo]

radlib.c

699

*We should check errno only if *portp is equals to 0.

[pjd]

radlib.c

398

I prefer the way it is now, so sa is not set twice.

497

Space fixed. assert() seems excessive.

534

No. This is not an error if host resolves to too many IPs. It is enough if we add one new server. The rest will be added if there is room.

662

Good idea.

699

errno shouldn't change if the function succeeds.

701

Correct, thanks.

1022

I don't want to change this style yet. I'll actually revert merging those two lines for now.

[pjd]

Revert style changes.
Remove #ifdef INET checks from radlib.h, it prevents some consumers from compiling.
Implement some of oshogbo suggestions.

[hrs]

radlib.c

206

A comparison of sa_family is redundant if applying memcmp() to whole of the struct sockaddr_storage.

419

You can obtain a struct sockaddr for INADDR_ANY by getaddrinfo(NULL, "radacct", &(struct addrinfo){.ai_flags = AI_PASSIVE}, &res) for both INET and INET6 depending on .ai_family. It is guaranteed that a single struct addrinfo will return when an AF is specified in .ai_family.

As described below, I think "bindto" variable can be struct addrinfo instead of struct sockaddr to make the code simpler.

495

Is there any reason to keep "port" as int? If this API is brand-new and there is no calculation by using the port number, const char *port is easier to handle. After all, "int port" has to be converted into (char *) for getaddrinfo(3) and then getaddrinfo(3) converts back it to uint16_t for (struct sockaddr)->sin{,6}_port.

593

This function can be simply replaced with getaddrinfo(3). getaddrinfo(str, "0", &(struct addrinfo){.ai_flags = AI_NUMERICSERV}, &res) should do the same job in a transport-independent fashion.

getaddrinfo(3) is the universal tool to get a struct sockaddr from address information. However, I recommend to directly use struct addrinfo for "bindto" because it includes all of the information required for both bind(2) and socket(2). There is no need to take care of struct sockaddr_storage directly in that case though freeaddrinfo(3) is required to free the allocated memory space at some point.

700

I have no strong opinion and this is just a comment, but I personally think that conversion of the port number to int and validation of the port range are redundant. If this is required, a call of getaddrinfo (here again) is sufficient. Checking if pp points a string and it is a valid port number can be done by getaddrinfo(NULL, pp, &(struct addrinfo){.ai_flags = AI_NUMERICSERV}, &res) == 0. It is quite light and does not involve either any lookup of DNS or /etc/services. Whether the port number is 0 or not can be tested by converting ai_addr by getnameinfo() with NI_NUMERICSERV and comparing the result with "0" since it canonicalizes the expression. While these may be a overkill, putting similar validation checks by using a magic number such as 65535 in many places is something we should avoid.

[pjd]

radlib.c

419

What would be your opinion about changing bindto to 'const char *'? This would be consistent with 'host' and I think in majority of cases the consumer would have string anyway. I'd still like to keep port as 'int' or even 'unsigned it', this for me sounds like a more common type for port.

495

Ip refer int, or even unsigned int for port, because this is how I expect consumer of this API will have the port. This is how we keep port in our product. On the other hand, I'm thinking about changing bindto to string and would love to hear your thoughts on this.

[• cmcdonald_netgate.com]

radlib.c

701

port should be *portp here, port is undefined