Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 171347
Summary: | Review Request: l2tpd - Layer 2 Tunneling Protocol daemon | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | [Fedora] Fedora | Reporter: | Paul Wouters <paul> | ||||||||
Component: | Package Review | Assignee: | Dmitry Butskoy <dmitry> | ||||||||
Status: | CLOSED NEXTRELEASE | QA Contact: | David Lawrence <dkl> | ||||||||
Severity: | medium | Docs Contact: | |||||||||
Priority: | medium | ||||||||||
Version: | rawhide | CC: | fedora-extras-list | ||||||||
Target Milestone: | --- | ||||||||||
Target Release: | --- | ||||||||||
Hardware: | All | ||||||||||
OS: | Linux | ||||||||||
URL: | http://www.jacco2.dds.nl/networking/freeswan-l2tp.html | ||||||||||
Whiteboard: | |||||||||||
Fixed In Version: | Doc Type: | Bug Fix | |||||||||
Doc Text: | Story Points: | --- | |||||||||
Clone Of: | Environment: | ||||||||||
Last Closed: | 2005-12-23 15:56:15 UTC | Type: | --- | ||||||||
Regression: | --- | Mount Type: | --- | ||||||||
Documentation: | --- | CRM: | |||||||||
Verified Versions: | Category: | --- | |||||||||
oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |||||||||
Cloudforms Team: | --- | Target Upstream Version: | |||||||||
Embargoed: | |||||||||||
Bug Depends On: | |||||||||||
Bug Blocks: | 163779 | ||||||||||
Attachments: |
|
Description
Paul Wouters
2005-10-20 22:15:42 UTC
It seems that l2tp is patented, see http://www.ietf.org/ietf/IPR/CISCO-L2TP Please, check out the possible issues, or ask in the mailing list. Correct. But as you can see from the link you provided, Cisco allows anyone to use their patent as long as it is IETF RFC compliant. This is Cisco's standard policy on patents for their IETF work. In fact, their statement was made before being awarded the patent. They complied fully with the IPR disclosure policy of the IETF. The IETF does not release RFC's that are limited or in any way discriminatory in their use. The patent holder (in this case Ciso) agree to a royalty free, unrevocable use of their patent as needed for implementing the IETF standards. If there were any limitations on the implementation and use of L2TP, the L2TP working group would not exist any more, and no new protocol additions or changes would be accepted as RFC standard. The L2TP became an IETF standard, see http://www.ietf.org/rfc/rfc2661.txt Notice the RFC was issued after the disclosure for IPR by Cisco, so the IETF fully knew about the patent and confirmed that there were no restrictions before it issued the RFC. In other words, there is no issue with this software patent. RedHat and users are free to implement, use and deploy the L2TP protocol, without limitation of royalties. If RedHat refuses any software that has any software patent in it, then this request should be denied (but then it should also strip NAT Traversal from the Linux kernel's NETKEY module) OK According to http://fedoraproject.org/wiki/PackagingGuidelines#head-2d596c519e2f2be4e19d1e5bfe5f473d24aad777 , in such a case some text file should be added to %doc . It would be better if the appropriate SourceXX will have full url (if it is possible). It looks like a paranoia, but in the current legal atmosphere such things are actually needed. :( According to http://l2tpd.sourceforge.net/, > The l2tpd project is now officially inactive and they give a link to rp-l2tp project. In such a situation, it would be better to compare these two similar projects, and probably choose alive rp-l2tp instead of dead l2tpd... BTW, there is an existing rp-l2tp rpm in some RedHat-related distro: ftp://ftp.asplinux.ru/pub/contribs/7.3/SRPMS/rp-l2tp-0.3-3asp.src.rpm (but with old upstream version). Information from the previous two entries is not as up to date as you think. For a more up to date reference on various l2tp daemons, see: http://www.jacco2.dds.nl/networking/freeswan-l2tp.html#L2TPoverview And in an update on that. rp-l2tp development has stalled. l2tp development in cvs has started again, and some of the long standing patches that are in Jacco's rpm have been integrated. I am waiting for a real release before updating my rpm. My rpm is based on Jacco's, but adapted to remove all the suse / mandrake / slackware specifics. This l2tp daemon is by far the most commonly deployed l2tp in combination with openswan, since it has a buil-in IP address pool option. All other l2tp daemons need IP Adress Pooling via a Radius server and/or the very latest pppd server options, which no one who has deployed l2tp servers on linux really has any experience with AFAIK. In the next few days I will be updating the l2tp rpm to include the patent documentation. I do hope we will not be stalling based on a long discussion on which l2tp daemon to include. There is no reason to later on not include openltp or rp-l2tp. I can make sure that there is a proper provide for 'l2tp server' so that these packages can co-exist together. ExecSum: l2tpd is great for small l2tp/ipsec deployments. People who have a radius server running with IP address pools might want openl2tp or l2tpns. If there is an interest, i could also adapt rp-l2tpd, but this package has also not seen much development lately, and has not been deployed as widely as l2tpd. > rp-l2tp development has stalled. Well, foo on it :) > l2tp development in cvs has started again, and some of the long standing > patches that are in Jacco's rpm have been integrated. I am waiting for a real > release before updating my rpm. May be it is better to use CVS tarball instead of the old version with a lot of patches? Especially if a real release will be coming soon. > In the next few days I will be updating the l2tp rpm to include the patent > documentation. I could not find the license text in Internet (only patent itself is present without any license info). But now I am sure that there are no any license issues, therefore just write some own text from the scratch and include it as another README file. Some remark before I'll review it: is "Requires: openswan" actually needed? Is it possible to use l2tpd without IPSec? In the nearest future (next week) I am going to make a VPN gate for my local network, and just this package is required to me. Therefore I can check up all in work, which is good for any review. :) Some first remarks & nitpicks: - IMO, URL should be http://sourceforge.net/projects/l2tpd - Group tag should be "System Environment/Daemons" (as have ppp, openswan, wvdial and other similar stuff). - AFAIK l2tp can be useful without IPSec too. If it is true, remove "Requires: openswan" - As unix98pty is a default for FC2+, and spec file should be more Fedora-oriented rather than generic, get rid of unneeded patches and macros here. - Source3 (RPM.README) seems to be unuseful (to be included into %doc for "end" users) - BUGS file in %doc seems to be unuseful too (it is mostly for l2tpd developers). - As Patch0 is not used at all, get rid of it. - According to http://fedoraproject.org/wiki/ScriptletSnippets#head-55b46ef483e6a08c24a8fc3b0b7e2ef7bfb84efd , don't run "chkconfig ... on" on install. Also remove "comment + exit 0;" at the ends of the scripts, it looks a little bit ugly. - Change "try-restart" to "condrestart" in spec and l2tpd.init file. - I would prefer to not activate l2tpd service by default (i.e., change "2345 80 30" to "- 80 30" in l2tpd.init file). Don't use extra spaces in the appropriate line, rpmlint worries about it. - remove the last paragraph from %description (all rpm history should be in the %changelog section). Consider an opportunity of replacement of the current source tarball + a lot of patches to some current CVS snapshot (it seems that several patches are already commited upstream). Of course, if it is stable enough... :) Paul, Any chance that you update something in the nearest future? Else I will start with the present rpm... Hi Dmitry, I will create the updated l2tpd rpm today. It has been stalled since we were patching it to use ptmx instead of Unix98 pty's. We also found a race condition that is triggered within our UML's, and the code in CVS failed to properly work for us. I'll finish up the work today and present an updated rpm. If I cannot get the cvs tree to reliably work, I'll go back to the current patch set. I have created a new l2tpd rpm. All suggestions from the comments has been integrated, except for the removal of the BUGS file. Since there is no l2tpd-devel package, I cannot move it there, so I opted to leave it in the current rpm instead of leaving it out completely. ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd.spec ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd-0.69.20051030-14.src.rpm From the spec file's changelog: * Sun Nov 27 2005 Paul Wouters <paul> 0.69.20051030 - Pulled up sourceforget.net CVS fixes. - various debugging added, but debugging should not be on by default. - async/sync conversion routines must be ready for possibility that the read will block due to routing loops. - refactor control socket handling. - move all logic about pty usage to pty.c. Try ptmx first, if it fails try legacy ptys - rename log() to l2tp_log(), as "log" is a math function. - if we aren't deamonized, then log to stderr. - added install: and DESTDIR support. I have confirmed this works with MacOSX Tiger as well as Windows XP, both with all updates installed. (provided one uses this with Openswan) Some more remarks. First of all, don't try to ship Fedora package with a general-use spec file. Fedora has a lot of specific requirements, therefore it is better to strongly follow these requirements. OTOH for a general use, some general spec file can be distributed inside the upstream's tarball. - according to http://fedoraproject.org/wiki/PackageNamingGuidelines#head-975237cdcb9aa7775601adeaaccbc70290f69812 version-release must be 0.69-0.14.20051030 As release field is "significantly" changed :), you may begin from the "1" again, i.e. 0.69-0.1.20051030 ... - while Source0 is a cvs snapshot, add a comment (before the Source0) how to obtain this snapshot (i.e. an appropriate cvs commands etc.). - get rid of "-g" in the DFLAGS=..., $RPM_OPT_FLAGS already has it (and $RPM_OPT_FLAGS only decides whether to use "-g" or not). - there was some openswan-related conf.samples in the previous release. Don't get rid of them completely, it can be useful to add these examples into the %doc ... (as ./samples-ipsec.d/*) - add "rm -rf %{buildroot}" for %clean and %install sections - move %clean section immediately after %install - according to http://fedoraproject.org/wiki/ScriptletSnippets#head-24ef9d59bda6032df14cf3cb433ce4ef09348f69 %post etc. sections should be fixed properly (see in the patch below). - there is a patch which removes -DDEBUG_... from the default DFLAGS, but these DEBUGs then specified again for make in the spec file. Are they actually needed for compiling for Fedora? - l2tpd-patents.patch actually adds a whole file - it is better to ship this as a separate Source... - rpmlint reports for the .src.rpm: W: l2tpd strange-permission l2tpd-chapsecrets.sample 0600 As this file will be installed with "-m600", add usual 644 permissions for the source, just to make rpmlint happy by the source rpm. - an extra space (or tab) causes rpmlint to worry about the "chkconfig" line in the init script (see another patch below). - rpmlint reports for the binary rpm: W: l2tpd wrong-file-end-of-line-encoding /usr/share/doc/l2tpd-0.69/CREDITS i.e. there are "\r\n" instead of "\n". IMHO, this is a hint for upstream :), it can be leaved as is for a while... Created attachment 121546 [details]
Required changes for the spec file
Created attachment 121547 [details]
Required changes fot the init script
Note, it is just "white-space" changes...
> - there was some openswan-related conf.samples in the previous release. Don't
> get rid of them completely, it can be useful to add these examples into the
> %doc ... (as ./samples-ipsec.d/*)
No more needed, as new openswan-2.4.4 already include it in its
/etc/ipsec.d/examples .
Correct. After talking to Jacco de Leeuw, we decided to move them from the l2tpd rpm to the openswan rpm, since they are configuration files for openswan when using l2tpd. /etc/ppp/chap-secrets.example is not good filename. PPP potentially can have such a file itself. Either don't use it at all (as PPP should have its own docs about it), or use "l2tp" in the filename (i.e. chap-secrets.example-l2tp ) IMHO, for security reasons, it is better to ship /etc/l2tpd/l2tpd.conf with "local-address" set to 127.0.0.1 . I've changed chap-secrets.example to chap-secrets.example-l2tp. I do not know what you mean with "local-address". There are two options that resemble your word: listen-addr: the bind address, default to INADDRANY local-ip: the ip address for the P-t-P link on the l2tp server end (needs to be seperately configured by admin) if local-ip is not configured properly, after authentication packets will get dropped cause the l2tpd cannot sent/receive them. no risk, it just won't work. listen-addr could be defined to listen on the example local-ip, but then people need to configure port forwarding between interfaces. Most will not do this, even though it is more secure. (It's really tricky, especially with IPsec and the NETKEY-specific weirdness that comes from it). It is far easier to just mark ESP and UDP 500/4500 packets and only allow marked packets to reach port 1701 (l2tp port). This ensures no one but the IPsec authenticated users can start to try and authenticate with l2tp. Setting any of these to 127.0.0.1 will confuse users a lot. New src.rpm and spec file pushed: ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd.spec ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd-0.69.20051030-15.src.rpm (I am still using high releases because otherwise the rpms in this correspondence would not be incremental in their release) > I do not know what you mean with "local-address". Surely, "listen-address" :) The problem is we should not allow world-wide access to this port by default. > It is far easier to just mark ESP and UDP 500/4500 packets and only allow > marked packets to reach port 1701 (l2tp port). Not easy for "end-users". Also IMHO in general it is possible to use l2tp even without IPSec ... If after the default installation and "service l2tpd start" there are no any secure holes, all is OK. If not, the default config must be changed properly. > I am still using high releases because otherwise the rpms in this > correspondence would not be incremental in their release I can guess incrementation by the comment's number :) Paul, This package cannot be added to FE with the current version-release scheme, because of the appropriate Fedora versioning policy. I don't understand why you want to save the wrong (for the Fedora) versioning, even temporary... :( Also, please, either apply or answer something for comment #12 - comment #13 > according to
>http://fedoraproject.org/wiki/PackageNamingGuidelines#head-975237cdcb9aa7775601adeaaccbc70290f69812
> version-release must be
BTW if it is a cvs post-release (not pre-release), the version-release must be
0.69-15.20051030 .
Well, I've successfully tested l2tpd (witn WinXP client). One problem (perhaps for upstream): When I detach at WinXP side, pppd daemon on the Fedora box still exist (and seem to be frozen). The IP address are deleted from ppp0 (!), pppd don't react on signals (only "kill -9" is useful), "strace -p" shows that pppd does some futex(2) syscall... Also one little issue: After "ikelifetime" (default 1 hour) is expired, the connection is dropped (it seems the "rekey" mechanism does not work with WinXp box). Possible solution is to increase "ikelifetime" up to 8h (as "keylife" default value). These should probably be taken to the openswan mailinglist, and not further discussed here. I have never seen hanging pppd's. rekey should be done by the client and works. openswan should be configured with rekey=no, because it cannot rekey a right=%any connection. > hanging pppd's. Well, I'll try to figure out it myself. Paul, I'm a little confused with your silence about comment #13 remarks etc. The review process is iterative, therefore it is better to do changes step by step... > > hanging pppd's. > Well, I'll try to figure out it myself. Unfortunately, this bug has not repeatability. Today it happened only once (in more than 20 various attempts). I hope this bug will disappear after system upgrade to FC5 (from the current FC3 :)). Anyway, it is not a blocker for the review. > /etc/ppp/chap-secrets.example I've closely looked at contents of this file and have not found anything specifically related to l2tpd. It seems just an example for the ordinary PPP's chap-secrets file. As /etc/ppp/chap-secrets is "owned" by the "ppp" package, only the "ppp" package "decides" whether to include some examples for this file into /etc/ppp or not. Currently, "ppp" has decided to not include it. Let it be such. > listen-address As we cannot set this address to localhost, some another solution should be found. With the default /etc/l2tpd/l2tpd.conf, at each connection attempt the child "pppd" process will be invoked (and then probably exited due to auth failure). It is not a good "secure" default configuration. Perhaps specifying "challenge = yes" with the empty l2tp=secrets can solve this problem? Sorry, I missed those comments. I've incorporated all changes proposed in l2tpd.init (white space issues) I've incorporated most of the changes proposed in l2tpd.spec. I did not add the chkconfig -add call, since I was told one shouldn't start a service upon install. Where should I move the chap-secrets.sample to? /etc/l2tpd/ or /usr/share/doc/l2tpd ? I don't want to start using l2tp challange or l2tp-secrets files, because when used with ipsec (100% of l2tp usage AFAIK) these files are no used. I guess we can do 127.0.0.1 but I will need to add a comment to it that this is made clear to be the internal ip of the gateway. I have put up new rpms and the spec file, incorporating all but this last change. ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd.spec ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd-0.69-0.1.20051030.src.rpm As for your l2tpd/ppp problems, please give me a log of the l2tpd -D output, and if possible an 'ipsec barf' output. (though I think this should move out of this bugzilla item and perhaps move to the openswan-users mailinglist). Well. - rm -f %{buildroot} must be at the beginning of %install section too. > I did not add the chkconfig -add call, since I was told one shouldn't start a > service upon install. Here is some misunderstanding. When we say "shouldn't start a service upon install", it means we should not do "chkconfig l2tpd on" and "service l2tpd start" . But we must "chkconfig --add l2tpd"! See http://fedoraproject.org/wiki/ScriptletSnippets#head-55b46ef483e6a08c24a8fc3b0b7e2ef7bfb84efd , for this. As l2tpd.init has "chkconfig: - 80 30", the "-" means that this service will be "off" after install (try "chkconfig l2tpd --list" to make sure of it). So, don't worry about it. For this package, all the recommendations according to http://fedoraproject.org/wiki/ScriptletSnippets#head-24ef9d59bda6032df14cf3cb433ce4ef09348f69 are acceptable. Also, don't check .pid file at %preun etc., IMO it is not needed (l2tpd.init works fine without this workaround), and it is not recommended way. Nitpicks: - defattr should be "%defattr(-,root,root,-)" (with the last ",-"). - l2tpd-chapsecrets.sample has 0600 permissions, which cause rpmlint to be confused a little on the source rpm. As you explicitly "install -m600" this file, too restrictive permissions of the source are not needed. I still suggest to get rid of chap-secrets.example at all. If you consider it is needed, then just move it to %doc - I would prefer to use "make" instead of "%{__make}", IMHO the last variant is less clean and is not actually useful in Fedora. - to make rpmlint more happy, remove DOS'ish '\r' from CREDITS file. All suggested changes are in the following patch. > I guess we can do 127.0.0.1 but I will need to add a comment to it that this > is made clear to be the internal ip of the gateway. Well, as user should set "ip-range" and "local-ip" to his/her specific values anyway, it would be not much hard to set or comment-out the listen-address too. At least, the user will be noticed about possible security risks (reading a comment you will write), and will remember this notation (as he/she has been compelled not only to read the comment, but also to change the listen-address manually :)). > As for your l2tpd/ppp problems, please give me a log of the l2tpd -D output, > and if possible an 'ipsec barf' output. This bug is more rare rather than repeated. I'll try to catch something later. Created attachment 122280 [details]
suggested changes for the spec file (v2)
IMO after applying this we shall close to approval.
Thanks Dmitry. I've manually applied your changes (patch did not apply cleanly). I did add the chap-secrets.sample to %doc, and made some minor changes in the text for l2tpd.conf. (and thanks, I've incorporated these last corrections to nsd as well) ftp://ftp.xelerance.com/l2tpd/binaries/fedora/4/SRPMS/l2tpd.spec ftp://ftp.xelerance.com/l2tpd/binaries/fedora/4/SRPMS/l2tpd-0.69.20051030-16.src.rpm Thanks again for your time to look at the package You hurry up. :( "-g" appears again, wrong version-release come back, etc. Please, apply changes against the spec file specified in the comment #25 Just apply my last patch (it should be successful with comment #25 spec file), then add your chap-secrets file back (as you want it). OK BTW, always incement release after each change (even trrivial). For the current: ftp://ftp.openswan.org/l2tpd/binaries/fedora/4/SRPMS/l2tpd-0.69-0.1.20051030.src.rpm MUST/SHOULD items OK Works good. APPROVED! Before commit to CVS, please, add some comment about how to obtain the source tarball from the upstream's cvs. One thing I just forgot: Use %{?dist} in the release field. It helps to differ packages (and appropriate CVS tags) between FC3/FC4/devel . When "release" field contains cvs/svn dates, the incrementation must be: 0.1.20051030%{?dist} --> 0.2.20051030%{?dist} --> 0.3.20051030%{?dist} etc. After the upstream's 0.69 is finished, just use 1%{?dist} --> 2%{?dist} etc. Fixed. Thank you! As all is built OK, perhaps you should "close/nextrelease" this bugzilla ticket... ;) |