Bug 171347 - Review Request: l2tpd - Layer 2 Tunneling Protocol daemon
Review Request: l2tpd - Layer 2 Tunneling Protocol daemon
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Dmitry Butskoy
David Lawrence
http://www.jacco2.dds.nl/networking/f...
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-10-20 18:15 EDT by Paul Wouters
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2005-12-23 10:56:15 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Required changes for the spec file (2.40 KB, patch)
2005-11-28 09:09 EST, Dmitry Butskoy
no flags Details | Diff
Required changes fot the init script (412 bytes, patch)
2005-11-28 09:12 EST, Dmitry Butskoy
no flags Details | Diff
suggested changes for the spec file (v2) (1.76 KB, patch)
2005-12-15 09:03 EST, Dmitry Butskoy
no flags Details | Diff

  None (edit)
Description Paul Wouters 2005-10-20 18:15:42 EDT
Spec Name or Url: ftp://ftp.xelerance.com/openswan/binaries/fedora/4/SRPMS/l2tpd-0.69-13.src.rpm
SRPM Name or Url: ftp://ftp.xelerance.com/openswan/binaries/fedora/4/SRPMS/l2tpd.spec
Description: l2tpd is an Layer 2 Tunneling Protocol daemon for use with Openswan to support "L2TP VPNs".
Comment 1 Dmitry Butskoy 2005-11-09 11:35:34 EST
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.

Comment 2 Paul Wouters 2005-11-09 18:15:32 EST
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)
Comment 3 Dmitry Butskoy 2005-11-10 07:06:23 EST
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. :(

Comment 4 Dmitry Butskoy 2005-11-11 10:46:09 EST
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...
Comment 5 Dmitry Butskoy 2005-11-11 11:04:45 EST
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).
Comment 6 Paul Wouters 2005-11-11 12:07:12 EST
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.
Comment 7 Dmitry Butskoy 2005-11-11 13:01:09 EST
> 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. :)
Comment 8 Dmitry Butskoy 2005-11-14 09:17:55 EST
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... :)
Comment 9 Dmitry Butskoy 2005-11-25 10:29:24 EST
Paul,

Any chance that you update something in the nearest future? Else I will start
with the present rpm...
Comment 10 Paul Wouters 2005-11-25 12:58:19 EST
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.
Comment 11 Paul Wouters 2005-11-27 20:51:19 EST
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@xelerance.com> 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)

Comment 12 Dmitry Butskoy 2005-11-28 09:08:15 EST
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...
Comment 13 Dmitry Butskoy 2005-11-28 09:09:56 EST
Created attachment 121546 [details]
Required changes for the spec file
Comment 14 Dmitry Butskoy 2005-11-28 09:12:25 EST
Created attachment 121547 [details]
Required changes fot the init script

Note, it is just "white-space" changes...
Comment 15 Dmitry Butskoy 2005-12-05 09:45:29 EST
> - 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 .
Comment 16 Paul Wouters 2005-12-05 13:16:23 EST
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.
Comment 17 Dmitry Butskoy 2005-12-09 12:51:18 EST
/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 .
Comment 18 Paul Wouters 2005-12-09 17:25:20 EST
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)
Comment 19 Dmitry Butskoy 2005-12-11 06:39:24 EST
> 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
Comment 20 Dmitry Butskoy 2005-12-11 06:47:25 EST
> 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 .
Comment 21 Dmitry Butskoy 2005-12-13 11:38:02 EST
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).
Comment 22 Paul Wouters 2005-12-13 22:10:52 EST
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.
Comment 23 Dmitry Butskoy 2005-12-14 04:58:27 EST
> 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...

Comment 24 Dmitry Butskoy 2005-12-14 12:36:35 EST
> > 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?

Comment 25 Paul Wouters 2005-12-14 14:08:06 EST
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).
Comment 26 Dmitry Butskoy 2005-12-15 09:00:13 EST
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.





Comment 27 Dmitry Butskoy 2005-12-15 09:03:26 EST
Created attachment 122280 [details]
suggested changes for the spec file (v2)

IMO after applying this we shall close to approval.
Comment 28 Paul Wouters 2005-12-15 10:51:21 EST
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
Comment 29 Dmitry Butskoy 2005-12-15 11:23:04 EST
You hurry up.  :(
"-g" appears again, wrong version-release come back, etc.

Please, apply changes against the spec file specified in the comment #25

Comment 30 Dmitry Butskoy 2005-12-15 11:26:55 EST
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).
Comment 31 Dmitry Butskoy 2005-12-15 12:21:22 EST
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.




Comment 32 Dmitry Butskoy 2005-12-16 07:54:00 EST
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.
Comment 33 Paul Wouters 2005-12-16 09:29:24 EST
Fixed. Thank you!
Comment 34 Dmitry Butskoy 2005-12-22 12:58:39 EST
As all is built OK, perhaps you should "close/nextrelease" this bugzilla
ticket... ;)

Note You need to log in before you can comment on or make changes to this bug.