Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Header file reorganization and cleaning up the public API for pppd version 2.5.0 #379

Merged
merged 35 commits into from
Jan 21, 2023

Conversation

enaess
Copy link
Contributor

@enaess enaess commented Dec 24, 2022

Summary:
Much of pppd.h used to pile on a bunch of extern declarations for internal use in other files of pppd. This was never meant as a "public API" to be exported and used by others. This change tries to limit that extensively by hiding the extern declarations into pppd-private.h (by virtue of git mv pppd.h to pppd-private.h to retain source control history, and creating a new pppd.h) and migrating only needed functions back into pppd.h for plugins to compile.

I've compiled a few different projects with the new pppd headers. It's is a breaking change, but plugin maintainers will have to do some maintenance here anyways...

This change reorganizes the header files:

  • Unexports the following headers:
    • chap-md5.h
    • crypto-priv.h
    • eap-tls.h
    • pathnames.h
    • peap.h
    • pppd-private.h
    • spinlock.h
    • tdb.h
  • Moves chap-new.h into chap.h
  • Moves ppp-crypto.* into crypto.*
  • Moves pppcrypt.* into crypto_ms.*
  • Uses unsigned char instead of u_char
  • Uses uint32_t instead of u_int32_t
  • Uses bool from <stdbool.h>
  • Rename pppd.h to pppd-private.h, creates another pppd.h with exported API
  • Creates a new options.h and moving relevant declarations from pppd.h
  • Creates a new multilink.h and moving relevant declarations from pppd.h
  • Documents the API in crypto.h
  • Removed extern variable declarations for MPPE keys in favor of using the new MPPE API
  • Converting to functions and/or renaming existing functions to better align with an API
    • Don't use "extern int status", it's way too common to use this in code, use ppp_set_status() or ppp_status(). Exit codes moved into an enum for compiler safety
    • Don't use "extern int debug", it's way too commong to use this in code. This is now known as debug_on()
    • Timer functions was renamed and prefixed with "ppp_"
    • Fixed up link statistics, call bool ppp_get_link_stats(&stats) to get the stats
    • Changed add_notifier to ppp_add_notify() and unexported the notifier structures in favor of a enumerated type.
    • Replace use of extern variables in favor of a function call
      • netif_(set|get)mru -> ppp(set|get)_mru
      • ppp_available -> ppp_check_kernel_support (just better nameing)
      • generic_(dis)establish_ppp -> ppp_generic_(dis)establish
      • script_(un)setenv -> ppp_script_(un)setenv
      • got_sigterm -> ppp_signaled(int signo)
      • sys_close -> ppp_sys_close
      • safe_fork -> ppp_safe_fork
      • get_time -> ppp_get_time
      • ...
    • While pppd.h declares the new functions, pppd still relies in most cases on the existing variables as declared by pppd-private.h (not public).
    • Some hooks where moved into their respective header files

This change was the continuation of a conversation starting out in PR#349 (#349).

@enaess
Copy link
Contributor Author

enaess commented Dec 24, 2022

@paulusmack @Neustradamus - Hope you have a Merry Christmas!

@Neustradamus
Copy link
Member

@enaess: Good job!

@paulusmack: When is the date for 2.5.0 release build?

@enaess
Copy link
Contributor Author

enaess commented Dec 27, 2022

@jkroonza I did cherry pick your radius changes into this branch. Would you be able to help with some QA here by building and trying this branch?

I've been connected over sstpc for days. Though that is only one aspect of this. Also, does your environment do VPN, PPPoE or similar? Looks like your change indicates you are using the radius plugin, does that also include radattr, radrealms too?

@enaess
Copy link
Contributor Author

enaess commented Dec 28, 2022

@pali you able to review and maybe test this change on your setup?

@jkroonza
Copy link
Contributor

@jkroonza I did cherry pick your radius changes into this branch. Would you be able to help with some QA here by building and trying this branch?

I've been connected over sstpc for days. Though that is only one aspect of this. Also, does your environment do VPN, PPPoE or similar? Looks like your change indicates you are using the radius plugin, does that also include radattr, radrealms too?

I can perform some basic tests albeit not in my production environment, so will have to deploy elsewhere but will include:

PPP/L2TP(/IPSec)
PPPoE (via rp-pppoe, current master from dfskoll).
PPTP

All of these in both client and server, but will not be able to perform massive "long" tests, nor high throughput, but given that ppp is mostly handled by kernel nowadays I believe that the above should be adequate.

@enaess
Copy link
Contributor Author

enaess commented Dec 29, 2022

That sounds perfect. I may need to create a PR against RP-PPPOE first. Maybe during my lunch break today (1pm PST). Does any of your test bed situation also include radius the radius modules or byte counting?

@jkroonza
Copy link
Contributor

Most of our auth happens against radius, so by implication yes, will have to transfer some large files to ensure that we get the >32-bit counters tested too.

@enaess
Copy link
Contributor Author

enaess commented Dec 29, 2022

@jkroonza
I don't have the permission to open a merge request, though I forked it and committed my changes here:

I wasn't able to run autoconf so you may need to manually toggle some of the #ifdef to #ifndef for the headers I included. Ideally, those should be a bart of the change too...

@Neustradamus
Copy link
Member

@enaess: You can see here:

@enaess
Copy link
Contributor Author

enaess commented Dec 30, 2022

@Neustradamus Yes, I forked that repository and made my changes. Eventually, I hope to create a PR onto that branch you sent the link to

@paulusmack
Copy link
Collaborator

This looks like great work. The only comment I have so far is that I want to have my copyright on options.h and multilink.h, not CMU's copyright, since I am the original author of the code in those files.

@enaess
Copy link
Contributor Author

enaess commented Dec 30, 2022

@paulusmack consider it done

@enaess
Copy link
Contributor Author

enaess commented Dec 30, 2022

@paulusmack Did the commit, though options.c didn't change copyright and still holds that of CMU. Should that file be "dual-copyright" or just leave it alone?

@enaess
Copy link
Contributor Author

enaess commented Dec 30, 2022

@paulusmack - just a few notes here. @jkroonza is helping out with testing some of the L2TP/PPTP and PPPoE changes. I am about to open a PR to the rp-pppoe project. And I've added needed changes for network-manager-sstp and sstp-client projects. There still may be a stray variable or so that has found its way into other projects, but I can try to create a PR for some of the major ones, e.g. network-manager, and xl2tpd. That has still to come, but the changes is steadily becoming more stable.

@paulusmack
Copy link
Collaborator

@paulusmack Did the commit, though options.c didn't change copyright and still holds that of CMU. Should that file be "dual-copyright" or just leave it alone?

I will check where that code comes from. I would have written most of it, so at least it should have my copyright as well as CMU, but the question is whether there is any code from CMU in there.

@enaess
Copy link
Contributor Author

enaess commented Dec 30, 2022

I just had network-manager project and xl2tpd compile against pppd and this patch. No further changes needed in that regards. Let me know if there are other projects you'd like to test against.

@enaess
Copy link
Contributor Author

enaess commented Jan 2, 2023

@jkroonza Did you have a chance to smoke test this change on your rig?

For your reference, I forked rp-pppoe with the changes here: https://salsa.debian.org/eivnaes/rp-pppoe

@Neustradamus
Copy link
Member

@enaess: Better to do on GitHub here like @jkroonza:

@enaess
Copy link
Contributor Author

enaess commented Jan 2, 2023

Sure, I'll rebase patch to this repo when ready. What I did on salsa was mostly on a whim so @jkroonza could test it

@jkroonza
Copy link
Contributor

jkroonza commented Jan 3, 2023

@jkroonza Did you have a chance to smoke test this change on your rig?

For your reference, I forked rp-pppoe with the changes here: https://salsa.debian.org/eivnaes/rp-pppoe

I have not ... been trying to spend time with the family and this year kicked off with a big bang ... going to fire up the test host later this afternoon.

The PR for rp-pppoe will need to remain such that rp-pppoe will compile against both older and newer versions of ppp ...

@enaess
Copy link
Contributor Author

enaess commented Jan 3, 2023

The PR for rp-pppoe will need to remain such that rp-pppoe will compile against both older and newer versions of ppp ...

Yes I know, I did this change on a whim so you had something you could build with, and to check if any additional functions are needed to be exported.

@enaess
Copy link
Contributor Author

enaess commented Jan 4, 2023

@jkroonza did you have time to look into smoke testing this. Time is of essence now

@enaess
Copy link
Contributor Author

enaess commented Jan 4, 2023

@paulusmack I did some more smoke testing of the pppoe (and rp-pppoe) against accel-pppd. These modules still work, no problem.

This change is ready to merge. Let's get 2.5.0 out the door.

@jkroonza
Copy link
Contributor

jkroonza commented Jan 4, 2023

News so far is not good, but I'm still trying to confirm exactly what's going on, my incoming test is failing to auth, but what bugs me is I'm receiving a SIGTERM from xl2tpd for some reason, and thereafter pppd itself still segfaults (sorry, I have to obfuscate quite a bit):

Jan  4 21:10:36 arthur xl2tpd[16569]: Call established with 1.2.3.4, PID: 5723, Local: 53857, Remote: 9543, Serial: 5551018
Jan  4 21:10:36 arthur pppd[5723]: Plugin pppol2tp.so loaded.
Jan  4 21:10:36 arthur pppd[5723]: Plugin radius.so loaded.
Jan  4 21:10:36 arthur pppd[5723]: RADIUS plugin initialized.
Jan  4 21:10:36 arthur pppd[5723]: Plugin radattr.so loaded.
Jan  4 21:10:36 arthur pppd[5723]: RADATTR plugin initialized.
Jan  4 21:10:36 arthur pppd[5723]: pppd options in effect:
Jan  4 21:10:36 arthur pppd[5723]: debug        # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: nodetach     # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: dump     # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: plugin pppol2tp.so       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: plugin radius.so     # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: plugin radattr.so        # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]:      # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: name iewc-pppoe      # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: remotenumber B310053424      # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: radius-config-file /etc/ppp/radius-fno.conf      # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: avpair xxx # [don't know how to print value]     # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: map-to-ifname        # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp 7       # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_lns_mode        # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_tunnel_id 32280     # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_session_id 53857        # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp 7       # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_lns_mode        # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_tunnel_id 32280     # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: pppol2tp_session_id 53857        # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: asyncmap 0       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: mru 1452     # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: mtu 1452     # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: passive      # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: lcp-echo-failure 3       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: lcp-echo-interval 5      # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: lcp-echo-adaptive        # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: noendpoint       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: ms-dns 154.73.32.2       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: ms-dns 154.73.32.1       # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: 100.66.0.0:100.66.0.4        # (from command line)
Jan  4 21:10:36 arthur pppd[5723]: -ipv6        # (from /etc/ppp/options.fno)
Jan  4 21:10:36 arthur pppd[5723]: pppd 2.5.0 started by root, uid 0
Jan  4 21:10:36 arthur pppd[5723]: using channel 11 
Jan  4 21:10:36 arthur pppd[5723]: Using interface ppp0
Jan  4 21:10:36 arthur pppd[5723]: Connect: ppp0 <--> 
Jan  4 21:10:36 arthur pppd[5723]: Overriding mtu 1500 to 1452
Jan  4 21:10:36 arthur pppd[5723]: PPPoL2TP options: lnsmode tid 32280 sid 53857 debugmask 0
Jan  4 21:10:36 arthur pppd[5723]: Overriding mru 1500 to mtu value 1452
Jan  4 21:10:36 arthur pppd[5723]: sent [LCP ConfReq id=0x1 <mru 1452> <asyncmap 0x0> <auth chap MD5> <magic 0x7d3e74a>]
Jan  4 21:10:36 arthur pppd[5723]: rcvd [LCP ConfReq id=0x6a <mru 1480> <magic 0xe7a50f5e>]
Jan  4 21:10:36 arthur pppd[5723]: sent [LCP ConfAck id=0x6a <mru 1480> <magic 0xe7a50f5e>]
Jan  4 21:10:36 arthur pppd[5723]: rcvd [LCP ConfRej id=0x1 <asyncmap 0x0>]
Jan  4 21:10:36 arthur pppd[5723]: sent [LCP ConfReq id=0x2 <mru 1452> <auth chap MD5> <magic 0x7d3e74a>]
Jan  4 21:10:36 arthur pppd[5723]: rcvd [LCP ConfAck id=0x2 <mru 1452> <auth chap MD5> <magic 0x7d3e74a>]
Jan  4 21:10:36 arthur pppd[5723]: Overriding mtu 1480 to 1452
Jan  4 21:10:36 arthur pppd[5723]: PPPoL2TP options: lnsmode tid 32280 sid 53857 debugmask 0
Jan  4 21:10:36 arthur pppd[5723]: sent [LCP EchoReq id=0x0 magic=0x7d3e74a]
Jan  4 21:10:36 arthur pppd[5723]: rcvd [LCP EchoRep id=0x0 magic=0xe7a50f5e]
Jan  4 21:10:41 arthur pppd[5723]: sent [LCP EchoReq id=0x1 magic=0x7d3e74a]
Jan  4 21:10:41 arthur pppd[5723]: rcvd [LCP EchoRep id=0x1 magic=0xe7a50f5e]
Jan  4 21:10:46 arthur pppd[5723]: rcvd [LCP TermReq id=0x6b "failed to authenticate"]
Jan  4 21:10:46 arthur pppd[5723]: LCP terminated by peer (failed to authenticate)
Jan  4 21:10:46 arthur pppd[5723]: Overriding mtu 1500 to 1452
Jan  4 21:10:46 arthur pppd[5723]: PPPoL2TP options: lnsmode tid 32280 sid 53857 debugmask 0
Jan  4 21:10:46 arthur pppd[5723]: Overriding mru 1500 to mtu value 1452
Jan  4 21:10:46 arthur pppd[5723]: sent [LCP TermAck id=0x6b]
Jan  4 21:10:46 arthur xl2tpd[16569]: Terminating pppd: sending TERM signal to pid 5723
Jan  4 21:10:46 arthur pppd[5723]: Terminating on signal 15 
Jan  4 21:10:49 arthur pppd[5723]: Link terminated.
Jan  4 21:10:49 arthur pppd[5723]: Modem hangup
Jan  4 21:10:49 arthur pppd[5723]: Fatal signal 11 

I suspect the remote side (it's either CISCO or Huawei, which exactly I can't reliably tell since we know the specific FNO uses a mix) is just faster after LCP TermReq to tear down and then close the l2tp tunnel, resulting in xl2tpd sending SIGTERM prior to pppd itself terminating. The Fatal signal 11 is SIGSEGV, I don't have a backtrace, will need to figure out how to get a core dump generated.

This PR is causing the SIGSEGV though, since when using the master branch this doesn't occur.

Busy trying to fix the underlying auth issue (which is caused by me uninstalling the system pppd and it took a bunch of critical config files with it).

@enaess
Copy link
Contributor Author

enaess commented Jan 4, 2023

@jkroonza

If you did a "sudo make install", then please check your /etc/ppp/chap-secrets, it may have been replaced. That should allow pppd to use mschapv2 if that is what you configured. At the point you receive a SIGSEGV (11), is after it is being signaled a SIGTERM (15) so there maybe some cleanup logic that isn't working 100%, not sure; but that is also something we can fix in a follow up release. But I'll take another look at the changes related to signaling.

enaess added 21 commits January 12, 2023 08:39
Signed-off-by: Eivind Næss <[email protected]>
Signed-off-by: Eivind Næss <[email protected]>
… should improve clarity in the crypto subsystem. Also adding comments to the crypto.h as it is a public API

Signed-off-by: Eivind Næss <[email protected]>
- chap-md5.h is an internal headerfile, don't install
- At this point, there is nothing new with chap-new, it's the chap implementation
- chap_ms.c contains the specific Microsoft specific defines

Signed-off-by: Eivind Næss <[email protected]>
Signed-off-by: Eivind Naess <[email protected]>
Signed-off-by: Eivind Næss <[email protected]>
…o check, i.e. in_phase(); then unexporting phase in pppd-private.h

Signed-off-by: Eivind Naess <[email protected]>
…n already deprecated and there is an API in place for it

Signed-off-by: Eivind Næss <[email protected]>
…() function. Moving variables and functions to tty.c

Signed-off-by: Eivind Naess <[email protected]>
…ad the equivalent of mp_on defined to 0, meaning \!0 would execute code below

Signed-off-by: Eivind Naess <[email protected]>
@enaess
Copy link
Contributor Author

enaess commented Jan 13, 2023

Last push was due to rebase and syncing up branch with ppp-project/ppp. Checks still passes. What do you think @paulusmack?

@paulusmack paulusmack merged commit ba7f7e0 into ppp-project:master Jan 21, 2023
@enaess enaess deleted the ppp-header branch January 21, 2023 21:31
@Neustradamus
Copy link
Member

Thanks @paulusmack for merging, @enaess for this big work, @jkroonza and @dfskoll for comments... :)

enometh pushed a commit to enometh/ppp that referenced this pull request Aug 17, 2024
* pppd/ipcp.c: (ipcp_down): fix comment
* pppd/main.c: (reset_link_stats): reset print_link_stats to 1, set
start_time even if get_ppp_stats fails.

This is an attempt to fix the problem noted in the linux-ppp mailing list on
mar-26-2024 and may-03-2024 under the subject "ppp-2.5.0 sometimes doesn't
print stats on terminating on signal 2"

The sent/recv log messages were being lost, especially with the persist option.
This seems to be an oversight during reorg in commit ba7f7e0 "Header file
reorganization and cleaning up the public API for pppd version 2.5.0 (ppp-project#379)"
around the repurposing of the link_stats_valid variable as link_stats_print.

It also fixes a stray reference to the old variable in a comment.  please
review. the reordering in reset_link_stats

Signed-off-by: S Madhu <[email protected]>
enometh added a commit to enometh/ppp that referenced this pull request Aug 20, 2024
* pppd/ipcp.c: (ipcp_down): fix comment
* pppd/main.c: (reset_link_stats): reset print_link_stats to 1, set
start_time even if get_ppp_stats fails.

This is an attempt to fix the problem noted in the linux-ppp mailing list on
mar-26-2024 and may-03-2024 under the subject "ppp-2.5.0 sometimes doesn't
print stats on terminating on signal 2"

The sent/recv log messages were being lost, especially with the persist option.
This seems to be an oversight during reorg in commit ba7f7e0 "Header file
reorganization and cleaning up the public API for pppd version 2.5.0 (ppp-project#379)"
around the repurposing of the link_stats_valid variable as link_stats_print.

It also fixes a stray reference to the old variable in a comment.  please
review the reordering in reset_link_stats

Signed-off-by: S Madhu <[email protected]>
paulusmack pushed a commit that referenced this pull request Sep 10, 2024
* pppd/ipcp.c: (ipcp_down): fix comment
* pppd/main.c: (reset_link_stats): reset print_link_stats to 1, set
start_time even if get_ppp_stats fails.

This is an attempt to fix the problem noted in the linux-ppp mailing list on
mar-26-2024 and may-03-2024 under the subject "ppp-2.5.0 sometimes doesn't
print stats on terminating on signal 2"

The sent/recv log messages were being lost, especially with the persist option.
This seems to be an oversight during reorg in commit ba7f7e0 "Header file
reorganization and cleaning up the public API for pppd version 2.5.0 (#379)"
around the repurposing of the link_stats_valid variable as link_stats_print.

It also fixes a stray reference to the old variable in a comment.

Signed-off-by: S Madhu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants