PPP ifname enhanced #289

EasyNetDev · 2021-06-24T08:02:35Z

Added support for PPP custom interface name under Linux.
Closing #285 and #116.

Neustradamus · 2021-06-30T20:42:33Z

@paulusmack: What do you think?

EasyNetDev · 2021-07-01T11:42:53Z

I hope it will not take 2-3 years to try to merge the PR. 😂.

paulusmack · 2021-07-02T08:06:57Z

First off, the commit messages are quite inadequate and don't even have a signoff (I wonder how we lost the automatic check for the signoff). Please read Submitting-patches.md. The "PPP enhanced ifname" commit definitely needs at least 2 or 3 paragraphs explaining what you are doing as well as any useful information on why you implemented it the way you did.

Is there a reason for not supporting the new format on Solaris? Is it just that you don't know if regcomp etc. would be available on Solaris?

There are 3 big blocks of code added, of which two seem to be just about extracting the unit number (ifunit) that was put on. Why can't those two places just use 'ifunit' instead of having to do all that complicated parsing using regular expressions?

jkroonza

Whilst ongoing with this, how about %u to replace with the authenticated username? This would require some limiting such that if the usename results in names longer than 15 characters the overall interface name gets stripped down, eg, let's say I auth as "jkroon" (that's 6 chars), and ifname is specified as pppd-vpn-%u-%d and the unit number is 999 then that gives pppd-vpn-jkroon-999 (19 chars) that will need to be trimmed to pppd-vpn-jk-999, but if unit number is 1, then it's fine to use pppd-vpn-jkro-1 for example.

This does get quite complex, so really just a suggestion, this is of benefit as it stands either way and I support the idea regardless.

EasyNetDev · 2021-07-02T10:16:52Z

ifunit

First off, the commit messages are quite inadequate and don't even have a signoff (I wonder how we lost the automatic check for the signoff). Please read Submitting-patches.md. The "PPP enhanced ifname" commit definitely needs at least 2 or 3 paragraphs explaining what you are doing as well as any useful information on why you implemented it the way you did.

Is there a reason for not supporting the new format on Solaris? Is it just that you don't know if regcomp etc. would be available on Solaris?

There are 3 big blocks of code added, of which two seem to be just about extracting the unit number (ifunit) that was put on. Why can't those two places just use 'ifunit' instead of having to do all that complicated parsing using regular expressions?

Hi @paulusmack,

Where exactly is this part you are refering at? Is about radius.c? I don't get it.
I will try to add Signoff part of the patches. Regarding Solaris, I don't have any Solaris OS to test it and how is working. That's I said in a previous comment in other PR that I need help for this. BSD systems are not supporting interface renaming, as I know.

EasyNetDev · 2021-07-02T10:18:09Z

Whilst ongoing with this, how about %u to replace with the authenticated username? This would require some limiting such that if the usename results in names longer than 15 characters the overall interface name gets stripped down, eg, let's say I auth as "jkroon" (that's 6 chars), and ifname is specified as pppd-vpn-%u-%d and the unit number is 999 then that gives pppd-vpn-jkroon-999 (19 chars) that will need to be trimmed to pppd-vpn-jk-999, but if unit number is 1, then it's fine to use pppd-vpn-jkro-1 for example.

This does get quite complex, so really just a suggestion, this is of benefit as it stands either way and I support the idea regardless.

It was another discussing to add the main interface from where the connection is comming. Let's merge this patch and we can add new features for other thinks.

EasyNetDev · 2021-07-02T13:37:47Z

@paulusmack,

If you are talking about radius.c part, I could ask the same why in the first place we had this:

if (sscanf(ifname, "ppp%d", &port) == 1) {
..

What I did was to expand this sscanf to a more userfiendly regex to extract the port from the name of the interface. I think in radius.c you will not get ifunit here. This ifunit is allocated for each new interface connected. Afterwards you will lose this "ifunit" in radius.c.
When you are doing interims I don't think you will get "ifunit" anymore, just the name of the interface.

Could you please be more specific?

pali · 2021-07-02T14:32:59Z

... instead of having to do all that complicated parsing using regular expressions?

Yea... seems that usage of regular expression for implementing %fmt format is overkill and current implementation is not easily extendable for e.g. %u format like @jkroonza proposed.

Is there any particular reason why usage of regular expression was chosen instead of classic %fmt string processing like it is implemented in most printf-based function implementation? I'm just curious and asking as probably I do not see some important details...

And for me it looks like usage of e.g. ppp%d-wan does not as your code with regular expressions expects that format %d must be at the end of string only. Which seems like a big limitation when such powerful thing like regular expressions is used.

EasyNetDev · 2021-07-05T06:19:28Z

Hi @pali,

I'm not a very good in coding. I like doing stuff and I'm trying to add some features to the software in my knowledge limits of my coding. First implementation I did it was with strcmp. Then somebody came with the idea to use regex. I rewrote the code using regex.
The part with regex I used in radius.c, but I rewrote that part also. As I explained earlier, radius.c doesn't receive ifunit, is just getting the interface name and we have to extract somehow the unit id. ifunit is received only when the interface is created.
Taking in account that most of the firewalls have a wildcard usage, like * in nft and + in iptables, this is used in the end of the interface.
Using the numbering inside of the interface name, for me I don't see a benefit: very hard to match in firewall a specific type of connection, like "pppoe*", "pptp*", "l2tp*", etc. Maybe I'm wrong. I wasn't using such naming at all.
If you want something more elaborate, let's come with some codes. I got this idea from a patch that was able to "rename" the connection without using any ifunit in naming. I expanded the idea 2-3 years ago. Now I rewrote the code to not use strcmp and suddenly we need more options. In these 2-3 years why we didn't discuss about this? This patch stayed here without any reply for this period.
My first implementation was to try using a script in ppp/ifup.d. The limitation of this is radius.c couldn't find the new naming of the interface, then the interims failed to be send it to RADIUS and I got an disconnect from the RADIUS with message: peer is dead, beacause of leak of interims. Then I went in the code and I created this patch to be able to send the interims correctly to RADIUS. This applies for ISP or for VPN concentrator, not for regular users.
Honestly I'm curious who is using a naming of the interfaces like: gi0.10-wan, gi0.20-lan, or eth10-extern. More logical is to have lan10, wan1 .. etc.
Even Cisco or Juniper doesn't have using interface units inside of the interface name: Tunnnel100, Gi0/1, etc.

jkroonza · 2021-07-05T11:28:21Z

May it not be a better idea to get the unit number passed in then somehow?

I fully agree that naming the interfaces by function makes a lot of sense, eg, ppp-vpn-%d (for vpn clients), and ppp-red-%d (for external uplinks) which would greatly simplify firewalling since now we can match on ppp-vpn-+ in iptables/netfilter to match all VPN connections. If one could add the auth username too, I could take this further to say vpn-%.u-%d where the . could indicate trim this field if the resulting name is too long (>15 chars), or one could (conceivable for the moment) force %u to be trimmed. This does introduce corner cases for firewalling though ... eg, using jkroon (6 chars) and ppp-vpn-%u-%d (ppp-vpn-jkroon- is 15 chars, so depending on the unit number in use ... this can end up as ppp-vpn-jkroo-X, ppp-vpn-jkro-XX or ppp-vpn-jkr-XXX - or if we go unit number >= 1000 even shorter).

@EasyDevNet - I could also take a swing at this specific patch for you if you don't come right or prefer.

pali · 2021-07-05T12:44:25Z

Using the numbering inside of the interface name, for me I don't see a benefit: very hard to match in firewall a specific type of connection, like "pppoe*", "pptp*", "l2tp*", etc. Maybe I'm wrong. I wasn't using such naming at all.

In nftables you can specify wildcard * at any position in interface name, not only at the end of string.

Honestly I'm curious who is using a naming of the interfaces like: gi0.10-wan, gi0.20-lan, or eth10-extern. More logical is to have lan10, wan1 .. etc.

Well, sometimes I'm using it :-)

I'm not a very good in coding. I like doing stuff and I'm trying to add some features to the software in my knowledge limits of my coding.

I would suggest you to look at how to write %fmt-processing function in extendable-way if you have never done it before or have not see it. There is common pattern how to do it. E.g. ppp code has also one %fmt processing function, look:

ppp/pppd/utils.c

Lines 134 to 246 in 93fd8a0

    
           int 
        
           vslprintf(char *buf, int buflen, char *fmt, va_list args) 
        
           { 
        
               int c, i, n; 
        
               int width, prec, fillch; 
        
               int base, len, neg, quoted; 
        
               long lval = 0; 
        
               unsigned long val = 0; 
        
               char *str, *f, *buf0; 
        
               unsigned char *p; 
        
               char num[32]; 
        
               time_t t; 
        
               u_int32_t ip; 
        
               static char hexchars[] = "0123456789abcdef"; 
        
               struct buffer_info bufinfo; 
        
               int termch; 
        
               buf0 = buf; 
        
               --buflen; 
        
               while (buflen > 0) { 
        
           	for (f = fmt; *f != '%' && *f != 0; ++f) 
        
           	    ; 
        
           	if (f > fmt) { 
        
           	    len = f - fmt; 
        
           	    if (len > buflen) 
        
           		len = buflen; 
        
           	    memcpy(buf, fmt, len); 
        
           	    buf += len; 
        
           	    buflen -= len; 
        
           	    fmt = f; 
        
           	} 
        
           	if (*fmt == 0) 
        
           	    break; 
        
           	c = *++fmt; 
        
           	width = 0; 
        
           	prec = -1; 
        
           	fillch = ' '; 
        
           	if (c == '0') { 
        
           	    fillch = '0'; 
        
           	    c = *++fmt; 
        
           	} 
        
           	if (c == '*') { 
        
           	    width = va_arg(args, int); 
        
           	    c = *++fmt; 
        
           	} else { 
        
           	    while (isdigit(c)) { 
        
           		width = width * 10 + c - '0'; 
        
           		c = *++fmt; 
        
           	    } 
        
           	} 
        
           	if (c == '.') { 
        
           	    c = *++fmt; 
        
           	    if (c == '*') { 
        
           		prec = va_arg(args, int); 
        
           		c = *++fmt; 
        
           	    } else { 
        
           		prec = 0; 
        
           		while (isdigit(c)) { 
        
           		    prec = prec * 10 + c - '0'; 
        
           		    c = *++fmt; 
        
           		} 
        
           	    } 
        
           	} 
        
           	str = 0; 
        
           	base = 0; 
        
           	neg = 0; 
        
           	++fmt; 
        
           	switch (c) { 
        
           	case 'l': 
        
           	    c = *fmt++; 
        
           	    switch (c) { 
        
           	    case 'd': 
        
           		lval = va_arg(args, long); 
        
           		if (lval < 0) { 
        
           		    neg = 1; 
        
           		    val = -lval; 
        
           	        } else 
        
           		    val = lval; 
        
           		base = 10; 
        
           		break; 
        
           	    case 'u': 
        
           		val = va_arg(args, unsigned long); 
        
           		base = 10; 
        
           		break; 
        
           	    default: 
        
           		OUTCHAR('%'); 
        
           		OUTCHAR('l'); 
        
           		--fmt;		/* so %lz outputs %lz etc. */ 
        
           		continue; 
        
           	    } 
        
           	    break; 
        
           	case 'd': 
        
           	    i = va_arg(args, int); 
        
           	    if (i < 0) { 
        
           		neg = 1; 
        
           		val = -i; 
        
           	    } else 
        
           		val = i; 
        
           	    base = 10; 
        
           	    break; 
        
           	case 'u': 
        
           	    val = va_arg(args, unsigned int); 
        
           	    base = 10; 
        
           	    break; 
        
           	case 'o': 
        
           	    val = va_arg(args, unsigned int); 
        
           	    base = 8; 
        
           	    break; 
        
           	case 'x': 
        
           	case 'X': 
        
           	    val = va_arg(args, unsigned int); 
        
           	    base = 16; 
        
           	    break;

Basically you need to read input %fmt string from left to right and put read character into output buffer. When you find % character then process following character in switch. Based on it you put into output buffer either non-%-character or replacement for %d (in your case unit number). This switch can be easily extended for new functionality. E.g. @jkroonza can implement here case 'u' for username processing without need to rewrite whole code. After the switch can be common function to check that output buffer size is still in 15-character limit.

If needed, I can help with implementation (I have sent more patches to ppp). Just I do not want to have too many open pull requests :-) (I have there one big which seems to take more time to review).

EasyNetDev · 2021-07-05T14:30:41Z

Hi,

Then you need to think how you will implement in radius.c the part of the ifunit extraction.
Otherwise we will face the issue with interims packets which will not update anymore the RADIUS database.

pali · 2021-07-05T14:47:38Z

@EasyNetDev I'm looking at radius.c change. This parsing is used only in get_client_port() function which is used only for setting rstate.client_port and moreover marked as hack:

ppp/pppd/plugins/radius/radius.c

Lines 277 to 279 in 86936b6

    
               /* Hack... the "port" is the ppp interface number.  Should really be 
        
                  the tty */ 
        
               rstate.client_port = get_client_port(portnummap ? devnam : ifname);

Then rstate.client_port is used only as second argument for function rc_auth_using_server() which is used for setting PW_NAS_PORT. So it is used only for setting RFC2865 Radius NAS-Port attribute as defined in https://datatracker.ietf.org/doc/html/rfc2865#section-5.5

This Attribute indicates the physical port number of the NAS which is authenticating the user. It is only used in Access-Request packets. Note that this is using "port" in its sense of a physical connection on the NAS, not in the sense of a TCP or UDP port number. Either NAS-Port or NAS-Port-Type (61) or both SHOULD be present in an Access-Request packet, if the NAS differentiates among its ports.

ppp radius plugin has some configuration options how is RFC2865 Radius NAS-Port calculated:

ppp/pppd/plugins/radius/radius.c

Lines 64 to 72 in 86936b6

    
           static option_t Options[] = { 
        
               { "radius-config-file", o_string, &config_file }, 
        
               { "avpair", o_special, add_avp }, 
        
               { "map-to-ttyname", o_bool, &portnummap, 
        
           	"Set Radius NAS-Port attribute value via libradiusclient library", OPT_PRIO | 1 }, 
        
               { "map-to-ifname", o_bool, &portnummap, 
        
           	"Set Radius NAS-Port attribute to number as in interface name (Default)", OPT_PRIOSUB | 0 }, 
        
               { NULL } 
        
           };

So I think that for (default) option map-to-ifname, it should retrieve number directly from pppd (same number which you use for %d replacement) and not guessing it from interface name as obviously interface name itself does not have to contain it.

pali · 2021-07-05T14:54:37Z

Anyway, when setting NAS-Port it is suggested to set also appropriate NAS-Port-Type. Current list of all port types is managed by IANA: https://www.iana.org/assignments/radius-types/radius-types.xhtml#radius-types-13

pali · 2021-07-05T15:12:16Z

So in my opinion this code

ppp/pppd/plugins/radius/radius.c

Lines 277 to 279 in 86936b6

    
               /* Hack... the "port" is the ppp interface number.  Should really be 
        
                  the tty */ 
        
               rstate.client_port = get_client_port(portnummap ? devnam : ifname);

should contain something like this. First check type of connection:

if connect_tty() was successfully called and ttyfd (in tty.c) is valid then using serial tty device
if using serial tty device and using_pty is set then using virtual device
if using serial tty device and using_pty is not set then device is Sync if sync_serial set, otherwise is Async.

And then based on connection type set NAS-Port and NAS-Port-Type:

if using non-virtual serial tty device then function rc_map2id(devnam) should be used to retrieve NAS-Port and NAS-Port-Type should be set to Async or Sync (based sync_serial)
if using virtual serial tty device then NAS-Port-Type should be set to Virtual and NAS-Port probably does not make sense. Maybe NAS-Port can be set to pty id.
if not using serial tty device then some kind of pppd plugin is used (e.g. pppoatm, pppoe, pppol2tp) and plugin itself should provide appropriate NAS-Port and NAS-Port-Type -- if it does not then either do not set these attribute at all or fallback to Virtual NAS-Port-Type and do not set NAS-Port-Type at all (as it is not known).

Just this is my opinion how it should be set.

pali · 2021-07-05T15:27:52Z

Anyway, one very important thing! There are two numeric identifiers associated with every ppp network interface. One is PPP unit id (this is IIRC unique identifier for kernel lifetime associated only for ppp network interfaces in ascending order) and another one kernel network interface id (again unique in system across all network interfaces, not only ppp; this is visible in ip link output as the first number on line; starting from 1). And these two identifiers are different. There is kernel API to retrieve both numbers if you know interface name.

Also interface name may contain some number, lot of times people put them at the end, e.g. eth4 or ppp2. And this number is in most cases different from above two numbers.

So beware when talking about interface number, it may be PPP unit id, it may be kernel interface number or it may be number at the end of interface name.

(Not mentioning that there is also list of PPP channel numbers which may change during existence of ppp connection and which are and must be different than PPP unit id :D)

pali · 2021-07-06T23:18:26Z

@EasyNetDev: If you need some more information or help with something, let me know.

EasyNetDev · 2021-07-07T09:45:14Z

Anyway, one very important thing! There are two numeric identifiers associated with every ppp network interface. One is PPP unit id (this is IIRC unique identifier for kernel lifetime associated only for ppp network interfaces in ascending order) and another one kernel network interface id (again unique in system across all network interfaces, not only ppp; this is visible in ip link output as the first number on line; starting from 1). And these two identifiers are different. There is kernel API to retrieve both numbers if you know interface name.

Hi @pali,

From what I saw in the code, ppp ifunit is returned by ioctl when by the creation of the new PPP interface usign ioctl function, here:

static int make_ppp_unit(void)
{
        int x, flags;

        if (ppp_dev_fd >= 0) {
                dbglog("in make_ppp_unit, already had /dev/ppp open?");
                close(ppp_dev_fd);
        }
        ppp_dev_fd = open("/dev/ppp", O_RDWR);
        if (ppp_dev_fd < 0)
                fatal("Couldn't open /dev/ppp: %m");
        flags = fcntl(ppp_dev_fd, F_GETFL);
        if (flags == -1
            || fcntl(ppp_dev_fd, F_SETFL, flags | O_NONBLOCK) == -1)
                warn("Couldn't set /dev/ppp to nonblock: %m");

        ifunit = req_unit;
        x = ioctl(ppp_dev_fd, PPPIOCNEWUNIT, &ifunit);
        if (x < 0 && req_unit >= 0 && errno == EEXIST) {
                warn("Couldn't allocate PPP unit %d as it is already in use", req_unit);
                ifunit = -1;
                x = ioctl(ppp_dev_fd, PPPIOCNEWUNIT, &ifunit);
        }
        if (x < 0)
                error("Couldn't create new ppp unit: %m");

        if (x == 0 && req_ifname[0] != '\0') {
                struct ifreq ifr;
                char t[MAXIFNAMELEN];
                memset(&ifr, 0, sizeof(struct ifreq));
                slprintf(t, sizeof(t), "%s%d", PPP_DRV_NAME, ifunit);
                strlcpy(ifr.ifr_name, t, IF_NAMESIZE);
                strlcpy(ifr.ifr_newname, req_ifname, IF_NAMESIZE);
                x = ioctl(sock_fd, SIOCSIFNAME, &ifr);
                if (x < 0)
                    error("Couldn't rename interface %s to %s: %m", t, req_ifname);
                else
                    info("Renamed interface %s to %s", t, req_ifname);
        }

        return x;
}

So this is not the interface network id. Is just the PPP driver allocation id. That's why. when you sent to RADIUS the auth or post-auth info, this is the ID which is sent also, and not interface network id. More logical is this ID than the interface network id, because PPP can reallocate unsed IDs. Example, you have ppp0, ppp1, ppp2, ppp3. Then ppp1 is disconnected and then another client is connecting. PPP will allocate ppp1 again for the new client, but the interface network id will increase, obviously.

radius.c is very old file. The history of this file seems to be from 2002 or 2008.
My idea with regex in raidus.c is very simple: doesn't metter what you add as interface name in config file, the regex will search the exact string until %d. Then will extract the ifunit and is using for the rest of the code.
Even if I'm using "l2tp%d" which contains number inside of the interface, radius.c will extract without any problems "l2tp" part, because is defined in req_ifname. My aproche was a simple one, in my opinion.

Using %fmt I guess is working the same. I don't know what is the performance penalities between these 2 aproches.

If everybody thinks is a good idea to implement %fmt solution instead regex, we can give a try. Regex is alread in libc, no need to build a lot of code, again, beeing in my opinion the fastest end easieast solution to implement this.
Initially I done it with strcpy/strcmp. %fmt is going back to that solution.

jkroonza

There are two approaches in radius.c for the NAS-Port (which I think must be an int based on the freeradius dictionary: dictionary.rfc2865:ATTRIBUTE NAS-Port 5 integer).

In the case where we do have a serial interface (tty), eg, /dev/ttyS0 or the like, using NAS-Port == 0 along with some sensible NAS-Port-Type (Sync?) would be great. There are various options in dictionary.rfc2865, and a few from dictionary.iana files. If we can know for a fact it's ISDN (there are modules in asterisk that allows this) then we can use the ISDN NAS-Port-Type for example. So it's conceivable that we can map the tty name if it's available (find any trailing integer to the ttyname, default to Sync port type, unless radius config option says to use something else).

For L2TP and PPTP dial-ins my personal feeling says using probably Async with ppp unit (not network) id. Ie, the int that's intended to go into %d for the patch under discussion here.

On Wireless for example, I'm not sure what MT does, but in such a case I would hint that Port-Type should be Ethernet, with the Port being some interface ID, with the Calling-Station-ID eventually finalizing the unique triplet (ie, the combination of NAS-Port-Type, NAS-Port and Calling-Station-ID is unique per session). To be more precise, this is an example (obfuscated slightly, indicated with $), of an auth request we're receiving from a MT for a PPPoE connection:

        Packet-Type = Access-Request
        Service-Type = Framed-User
        Framed-Protocol = PPP
        NAS-Port = 15823683
        NAS-Port-Type = Ethernet
        User-Name = "$user@$realm"
        Calling-Station-Id = "11:22:33:44:55:66"
        Called-Station-Id = "service-name"
        NAS-Port-Id = "interface-name"
        MS-CHAP-Domain = "$realm"
        Acct-Session-Id = "$session_id"
        MS-CHAP-Challenge = $0xXX..XX
        MS-CHAP2-Response = $0xXX..XX
        NAS-Identifier = "$routers_system_name"
        NAS-IP-Address = $router_ip
        Mikrotik-Realm = "$realm"
        Timestamp = 1625101032

NAS-Port here seems to be the network identifier (we don't see recurring identifiers for successive access-requests).

Using the ppp unit number we can have repeats (eg, ppp0 disconnects, then some other ppp0 reconnects). So it would be great to be able to configure what gets sent here, along with having semi sane defaults here. This however starts moving into a patch for radius which IMHO should be separate, so either that should be a pre-requisite for the naming patch, or we should make minimal adjustments to radius.c now.

In radius.c the one approach currently tries to map the tty name, this shouldn't change, the other approach assumed ppp%d format here, so my suggestion is to rather somehow extract the ppp unit number in that case rather than trying to parse the interface name. If needed. This is already a bug since it's assuming ppp%d in get_client_port(), when it should probably rather use PPP_DRV_NAME (eg, sscanf(ifname, PPP_DRV_NAME "%d", &port) ... however, looking at the way "%s%d", PPP_DRV_NAME, ?? is used, it suggests PPP_DRV_NAME could legally contain % signs, so care should be taken to safeguard this (and I have no ideas how to do that compile time). There is also a potential bug in sys-solaris.c on this whilst I see this:

sys-solaris.c:    slprintf(ifr.ifr_name, sizeof(ifr.ifr_name), PPP_DRV_NAME "%d", ifunit);

All other code uses the *printf(..., "...%s...", ... PPP_DRV_NAME, ...) construct for this.

jkroonza · 2021-07-07T10:16:04Z

pppd/options.c

    { "ifname", o_string, req_ifname,
-      "Set PPP interface name",
+      "Set PPP interface name. If format is like ppp%%d / pppoe%%d / l2tp%%d, where %%d will be replaced by unit number. Linux Only.",


%%d will be replaced with the PPP unit number (Linux Only).

Just feels more concise for me.

jkroonza · 2021-07-07T10:16:38Z

pppd/plugins/radius/dict.c

@@ -150,7 +150,7 @@ int rc_read_dictionary (char *filename)
 			{
 				type = PW_TYPE_INTEGER;
 			}
-			else if (strcmp (typestr, "ipaddr") == 0)
+			else if (strcmp (typestr, "ipaddr") == 0 || strcmp (typestr, "ipv4addr") == 0)


I think it makes sense to have this change as it's own PR to be honest. This is good to go and makes zero sense to have this blocked whilst sorting out the rest of the PR.

jkroonza · 2021-07-07T10:18:44Z

pppd/pppd.8

+%d in the end of the string pppd will add ppp unit and will create a new
+interface named like pppoe0, l2tp1, ptpp3. This is valid only for LINUX.
+If is used without %d and the interface name is already in use, or if the 
+name cannot be used for any other reason, pppd will terminate.


The following format characters are supported:

%% - literal %.
%d - Linux only: Replace with the ppp unit number.
%u - replace with the authenticated username (will be trimmed automatically if the resulting interface name is too long).

jkroonza · 2021-07-07T10:20:27Z

pppd/sys-linux.c

@@ -667,6 +668,19 @@ static int make_ppp_unit(void)
 {
 	int x, flags;

+	// Variable to create regex
+	regex_t *regex;


This need not be a pointer, can be allocated on-stack (it's small enough in my opinion at 64 bytes).

jkroonza · 2021-07-07T10:21:22Z

pppd/sys-linux.c

+	// Variable to create regex
+	regex_t *regex;
+
+	regex = malloc(sizeof(regex_t));


Not seeing associated free().

jkroonza · 2021-07-07T10:29:52Z

pppd/sys-linux.c

+			error("Couldn't rename interface %s to %s: %m", old_ifname, new_ifname);
+		    else
+			info("Renamed interface %s to %s", old_ifname, new_ifname);
+		}


This looks good except I'm also of the opinion to rather use %fmt expansion,we can flag if/where %u gets used (not even sure if it's available yet at this point for shortening, start/stop char indexes), and I suggest only error'ing out if the resulting name is too long (eventually if we can shorten, only error after shortening has failed). Also, we may want to split %U (full username vs %u as username without realm kind of thing, I know in some cases there are potential issues with realm here in format \realm\username vs username@realm). So for now I suggest we ignore %u as an initial patch. There is technically no reason to disallow multiple instances of %d in the if name, although this would be weird.

pali · 2021-07-07T20:31:27Z

My idea with regex in raidus.c is very simple: doesn't metter what you add as interface name in config file, the regex will search the exact string until %d. Then will extract the ifunit and is using for the rest of the code.

So if your aim is to retrieve ppp unit number, why you are doing it in such complicated way (build regex and extract it from interface name)? If I'm looking at the code correctly, this ppp unit number is already stored in (global) variable ifunit. Or are there any issues that plugin cannot access this (global) variable? I always suggest to make implementation less complex and easier if is it possible. And "direct" access to variable where is value stored is "less complex" as trying to parse it from interface name via regex.

Using %fmt I guess is working the same. I don't know what is the performance penalities between these 2 aproches.

If everybody thinks is a good idea to implement %fmt solution instead regex, we can give a try. Regex is alread in libc, no need > to build a lot of code, again, beeing in my opinion the fastest end easieast solution to implement this.
Initially I done it with strcpy/strcmp. %fmt is going back to that solution.

I proposed %fmt parsing because it is easy extendable. Seems that this functionality to extend template/patterns for interface name is interesting for more people, e.g. @jkroonza had idea about username and I would like to see also other things (like name of ethernet interface). So in future it would be relatively easy to implement new %fmt for these things as opposite to regex implementation which is hard to extend for a new functionality (without reimplementing it again). And because lot of people have different taste how interface should be named it is possible that more other people would be interested by providing new patches for new %fmt formats...

Plus bonus for %fmt should be also working Solaris support as in this %fmt processing implementation is nothing platform specific. (I know that testing is harder, but there would be not need to hide this code under linux guards)

pali · 2021-07-07T20:41:33Z

In the case where we do have a serial interface (tty), eg, /dev/ttyS0 or the like, using NAS-Port == 0 along with some sensible NAS-Port-Type (Sync?) would be great. There are various options in dictionary.rfc2865, and a few from dictionary.iana files. If we can know for a fact it's ISDN (there are modules in asterisk that allows this) then we can use the ISDN NAS-Port-Type for example. So it's conceivable that we can map the tty name if it's available (find any trailing integer to the ttyname, default to Sync port type, unless radius config option says to use something else).

For L2TP and PPTP dial-ins my personal feeling says using probably Async with ppp unit (not network) id. Ie, the int that's intended to go into %d for the patch under discussion here.

@jkroonza: sync vs async is reference to type of tty/modem. In most cases people use async, e.g. RS232, UART. sync type is e.g. HDLC. Also if you want to use pppd in sync type you have to specify "sync" option to pppd. otherwise pppd is running in async mode. So choosing sync vs async NAS-Port-Type is simple, user specify to pppd which mode uses (I described it in comment #289 (comment)). I think that people in these days do not run pppd over serial interface or modem too often, so do not care about sync vs async mode.

jkroonza · 2021-07-08T09:45:38Z

In the case where we do have a serial interface (tty), eg, /dev/ttyS0 or the like, using NAS-Port == 0 along with some sensible NAS-Port-Type (Sync?) would be great. There are various options in dictionary.rfc2865, and a few from dictionary.iana files. If we can know for a fact it's ISDN (there are modules in asterisk that allows this) then we can use the ISDN NAS-Port-Type for example. So it's conceivable that we can map the tty name if it's available (find any trailing integer to the ttyname, default to Sync port type, unless radius config option says to use something else).
For L2TP and PPTP dial-ins my personal feeling says using probably Async with ppp unit (not network) id. Ie, the int that's intended to go into %d for the patch under discussion here.

@jkroonza: sync vs async is reference to type of tty/modem. In most cases people use async, e.g. RS232, UART. sync type is e.g. HDLC. Also if you want to use pppd in sync type you have to specify "sync" option to pppd. otherwise pppd is running in async mode. So choosing sync vs async NAS-Port-Type is simple, user specify to pppd which mode uses (I described it in comment #289 (comment)). I think that people in these days do not run pppd over serial interface or modem too often, so do not care about sync vs async mode.

Gotcha. Perhaps we need to split this PR/discussion into two (not sure how to do that on github) where the current deals specifically with the ifname change, and a separate discussion for improving the radius mechanism?

pali · 2021-07-08T09:47:44Z

Github does not support discussion split. The only option is to open a new "issue" and continue discussion there.

paulusmack · 2021-07-16T09:38:03Z

I think that for this patch, we should be changing get_client_port() in the radius code to just do 'return ifunit'. Then somebody should do another patch on top to make that more intelligent if necessary.

Also, since the code that does the %d substitution in the interface name seems to be needed in two places, it should be factored out into a function. That way if there is a bug in it, it will only need to be fixed in one place.

paulusmack · 2021-09-18T02:20:54Z

This is now a real mess, with merges from my repository with conflicts checked in, and then only later resolved in other commits. That will break bisection.

Also, none of the requested changes have been made; for example get_client_port() is still doing all the complicated regex gunk.

If you still want this in, please re-do it as a clean commit (or series of commits) without merges.

EasyNetDev · 2021-12-21T11:05:10Z

Hi @Neustradamus ,

I was able to merge the patch with latest version of pppd. Hope this time will be accepted.

…so plugin (#304) Backward compatibility symlink is there already for one ppp release. Remove it for next ppp release to not conflict with real rp-pppoe.so plugin. So both ppp's pppoe.so and rp's rp-pppoe.so plugins can be installed together. Now when conversion to automake was done, it is a good time to drop this problematic symlink from default installation. Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

Options that specify --with-logfile-dir, or --with-plugin-dir, or --with-runtime-dir needs to be specified using AC_ARG_WITH, not AC_ARG_ENABLE. If you try to specify --without-openssl, then the conditions should be tested against = "xyes". There is a case where the option is either blank or is set to "xno" and the former case wasn't properly handled. Signed-off-by: Eivind Næss <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

… option Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…ery2() pppoe-discovery.c needs to call only the first phase of discovery. Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…ry.c code Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…bose option is set Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…intf() Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…e plugin code Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

…cters This adds logic to pppoe_printpkt to print text fields as hex if the field contains any non-printable characters. This is so that a malicious, buggy or hacked access concentrator can't cause us to send non-printing characters to syslog. Signed-off-by: Paul Mackerras <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

….15. (#327) Signed-off-by: RICCIARDI-Adrien <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

Neustradamus · 2021-12-21T13:24:31Z

@EasyNetDev: Thanks for your comeback.

Have you looked all @pali and @jkroonza comments?

Hope that @paulusmack will merge the new PR soon.

Neustradamus · 2021-12-21T17:29:02Z

@EasyNetDev: Do not forget to sign in your new PR when you will publish.

Error in the current closed PR: https://github.com/ppp-project/ppp/pull/289/checks

Thanks in advance.

Neustradamus · 2021-12-24T00:02:53Z

@EasyNetDev: Have you a problem to create the new PR? :/

Neustradamus · 2022-01-13T07:59:52Z

@paulusmack: Can you look the @EasyNetDev commits?

https://github.com/EasyNetDev/ppp/commits?author=EasyNetDev

He had a problem with Git :/

It is good or not?

Neustradamus · 2022-07-08T01:37:05Z

@EasyNetDev: It is not possible to create a new PR?

I think it is the best solution...

EasyNetDev · 2022-07-08T11:17:48Z

Hi Neustradamus, I'm working to this patch to implemetn fmt format and skip regexp. Maybe these days I will be able to push the patch. Kind regards,

…

On 08/07/2022 03:37, Neustradamus wrote: @EasyNetDev <https://github.com/EasyNetDev>: It is not possible to create a new PR? I think it is the best solution... — Reply to this email directly, view it on GitHub <#289 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN5CKH7K2JMKHRMXSKZPX3VS6A43ANCNFSM47HLDFHA>. You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Adrian Ban Tel: +40788388190 Web: www.easynet.dev <https://www.easynet.dev>

Neustradamus · 2022-08-09T12:31:08Z

@EasyNetDev: Have you looked?

EasyNetDev · 2022-08-19T12:50:14Z

@EasyNetDev: Have you looked?

Hi Neustradamus. I'm finishing the part with %fmt. I still didn't understood very wheel how we should implement radius.c problem. I saw the comments, but the programming side regarding ioctl and so on are over my knowledge.

Neustradamus · 2022-08-21T01:42:39Z

@enaess and others: What do you think about last @EasyNetDev comment?

enaess · 2022-08-21T01:56:20Z

Which commit? I see 48 here. Maybe this PR needs to rebase?

Neustradamus · 2022-08-21T01:57:20Z

@enaess: sorry : comments!

jkroonza · 2022-08-23T08:33:04Z

This PR has been closed, with the promise of a new PR, perhaps this should be handled there? If an update is forthcoming?

jkroonza · 2022-08-24T20:44:43Z

@EasyNetDev would 0d12ca7 (Part of PR #367 suit your needs?

Neustradamus · 2022-08-24T23:44:37Z

The original commit from @EasyNetDev in this PR was:

330948b

The problem was when there was other merged PR...

Neustradamus · 2022-10-22T11:36:40Z

@EasyNetDev: What do you think?

We wait you for a new PPP version.

enaess · 2022-10-22T20:02:11Z

@Neustradamus I am not sure if this PR or fix for this problem should hold back a version 2.5 release.

jkroonza · 2022-10-24T06:48:07Z

@enaess @Neustradamus

I believe that renaming the interface externally (commit 0d12ca7) may be sufficient already, that said, an internal to pppd solution may well be very nice too, external ability to rename may well provide much more flexibility. I do agree that this should not hold back a 2.5 release.

EasyNetDev changed the title ~~PPP ifname echanced~~ PPP ifname enhanced Jun 24, 2021

jkroonza reviewed Jul 2, 2021

View reviewed changes

jkroonza reviewed Jul 7, 2021

View reviewed changes

Neustradamus requested a review from paulusmack September 16, 2021 23:03

pali and others added 2 commits December 21, 2021 13:29

pali and others added 9 commits December 21, 2021 13:33

pppoe: Rename variable printACNames to pppoe_verbose to match cmdline…

8a7e023

… option Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe: Split function discovery() into phases discovery1() and discov…

2bf4f78

…ery2() pppoe-discovery.c needs to call only the first phase of discovery. Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe: Do not use lcp_allowoptions[] and lcp_wantoptions[] in discove…

ca4d7ff

…ry.c code Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe: Show verbose information about all concentrator when pppoe-ver…

fc05868

…bose option is set Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe: Dump discovery packets when pppoe-verbose option is set to >= 2

34c72a6

Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe: Do not use %.*v and %.*B formats which are unsupported by C pr…

811c497

…intf() Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

pppoe-discovery: Define function and variables which are used by pppo…

cdb5d07

…e plugin code Signed-off-by: Pali Rohár <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

Updated Solaris CI action to fix build error on macOS greater than 10…

c529c17

….15. (#327) Signed-off-by: RICCIARDI-Adrien <[email protected]> Signed-off-by: Adrian Ban <[email protected]>

EasyNetDev closed this Dec 21, 2021

EasyNetDev deleted the ppp-ifname-echanced branch December 21, 2021 11:39

Neustradamus mentioned this pull request Dec 21, 2021

new ppp release ? #111

Closed

Neustradamus mentioned this pull request Jul 8, 2022

New version? #350

Closed

PPP ifname enhanced #289

PPP ifname enhanced #289

Conversation

EasyNetDev commented Jun 24, 2021 • edited Loading

Neustradamus commented Jun 30, 2021

EasyNetDev commented Jul 1, 2021

paulusmack commented Jul 2, 2021

jkroonza left a comment

Choose a reason for hiding this comment

EasyNetDev commented Jul 2, 2021

EasyNetDev commented Jul 2, 2021

EasyNetDev commented Jul 2, 2021 • edited Loading

pali commented Jul 2, 2021

EasyNetDev commented Jul 5, 2021 • edited Loading

jkroonza commented Jul 5, 2021

pali commented Jul 5, 2021

EasyNetDev commented Jul 5, 2021

pali commented Jul 5, 2021

pali commented Jul 5, 2021

pali commented Jul 5, 2021 • edited Loading

pali commented Jul 5, 2021 • edited Loading

pali commented Jul 6, 2021

EasyNetDev commented Jul 7, 2021

jkroonza left a comment

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

jkroonza Jul 7, 2021

Choose a reason for hiding this comment

pali commented Jul 7, 2021

pali commented Jul 7, 2021

jkroonza commented Jul 8, 2021

pali commented Jul 8, 2021

paulusmack commented Jul 16, 2021

paulusmack commented Sep 18, 2021

EasyNetDev commented Dec 21, 2021

Neustradamus commented Dec 21, 2021

Neustradamus commented Dec 21, 2021

Neustradamus commented Dec 24, 2021

Neustradamus commented Jan 13, 2022

Neustradamus commented Jul 8, 2022

EasyNetDev commented Jul 8, 2022 via email

Neustradamus commented Aug 9, 2022

EasyNetDev commented Aug 19, 2022

Neustradamus commented Aug 21, 2022 • edited Loading

enaess commented Aug 21, 2022

Neustradamus commented Aug 21, 2022

jkroonza commented Aug 23, 2022

jkroonza commented Aug 24, 2022

Neustradamus commented Aug 24, 2022

Neustradamus commented Oct 22, 2022

enaess commented Oct 22, 2022

jkroonza commented Oct 24, 2022

EasyNetDev commented Jun 24, 2021 •

edited

Loading

EasyNetDev commented Jul 2, 2021 •

edited

Loading

EasyNetDev commented Jul 5, 2021 •

edited

Loading

pali commented Jul 5, 2021 •

edited

Loading

pali commented Jul 5, 2021 •

edited

Loading

Neustradamus commented Aug 21, 2022 •

edited

Loading