Please merge openvpn 2.1~rc15-1 (main) from Debian unstable (main)

Bug #372358 reported by Andres Rodriguez on 2009-05-05
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
openvpn (Ubuntu)
Undecided
Mathias Gug

Bug Description

Binary package hint: openvpn

 openvpn (2.1~rc15-1) unstable; urgency=low
 .
   * New upstream version (Closes: #515575)
   * remote_env.patch: patched options.c to fix remote* enviroment vars.
   * openvpn-pkcs11warn.patch: warn on deprecated pkcs11 options.
     Thanks A LOT to Florian Kulzer for the README.Debian text & patch!
     (Closes: #475353)
   * Removed lladdr-is-not-ip.patch, since it was included upstream.
   * init.d script: Use start-stop-daemon to avoid failure on start when
     a PID file is not deleted. (Closes: #445061)
   * init.d script: Added 'status' action. Thanks Thierry Carrez for
     the patch. (Closes: #498493)
   * Updated debian/copyright: Point to GPL-2
   * Updated debian/control: Added ${misc:Depends}
   * Bumped Standards-Version to 3.8.1
   * Moved to debhelper compat 7.

Related branches

Andres Rodriguez (andreserl) wrote :

Attaching debdiff

summary: - Please merge oepnvpn 2.1~rc11-1 (main) from Debian unstable (main)
+ Please merge oepnvpn 2.1~rc15-1 (main) from Debian unstable (main)
Changed in openvpn (Ubuntu):
assignee: nobody → Andres E. Rodriguez Lazo (andreserl)
Changed in openvpn (Ubuntu):
status: New → Confirmed
summary: - Please merge oepnvpn 2.1~rc15-1 (main) from Debian unstable (main)
+ Please merge openvpn 2.1~rc15-1 (main) from Debian unstable (main)
Changed in openvpn (Ubuntu):
status: Confirmed → In Progress
Mathias Gug (mathiaz) wrote :

Thanks for taking the time to prepare a merge. Here are a few comments:

- the changelog entry for the merge needs to be expanded to cover the remaining changes. It will help for the next round of merges.

- have the changes been forwarded to Debian?

Andres Rodriguez (andreserl) wrote :

Heya.

 1. Attaching new debdiff with fixed changelog. How does it looks now?
 2. I've not forward those changes to Debian. I'm not sure how to do that.

Thanks for the suggestions

Cheers,

RoAkSoAx

Thierry Carrez (ttx) wrote :

Comments:
- "Fix unexpected operator on startup" is not a remaining change to Debian, as it was a fix in the "add --script-security 2 by default for backwards compatibility" part which was an Ubuntu delta
- "Do not use start-stop-daemon and use < /dev/null to avoid blocking boot" is a remaining change to Debian

About deltaforwarding to Debian:
- show per-VPN result messages: this patch is Ubuntu-specific as we have a different way of handling initscript messages than Debian
- "Do not use start-stop-daemon and use < /dev/null to avoid blocking boot": this is a reversion of a Debian change (we choose not to block the boot in case of needed user interaction, they chose to block it) so I'd suggest not forwarding that change (see debian bug 454371 and LP: #280428)
- add "--script-security 2" by default for backwards compatibility: Debian broke backward compat in previous releases so they don't need that one (which would break it again for them)
- Added lsb-base>=3.2-14 depend to allow status_of_proc(): this was part of my patch in Debian bug 498493... it probably got lost in translation, and could be resubmitted/reopened.

Andres Rodriguez (andreserl) wrote :

Thanks for the suggestions.

How does it looks like now?

Changed in openvpn (Ubuntu):
assignee: Andres E. Rodriguez Lazo (andreserl) → nobody
status: In Progress → Confirmed
Mathias Gug (mathiaz) wrote :

Thanks for the diff. It seems that there is one part of the merge in the init script that should properly handled:

Debian changed the way openvpn daemons are started:

- # Check to see if it's already started...
- if test -e /var/run/openvpn.$NAME.pid ; then
- log_failure_msg "Already running (PID file exists)"
- STATUS=0
- else
- $DAEMON $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
- $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
- --config $CONFIG_DIR/$NAME.conf || STATUS=1
- fi
+ start-stop-daemon --start --quiet --oknodo \
+ --pidfile /var/run/openvpn.$NAME.pid \
+ --exec $DAEMON -- $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
+ $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
+ --config $CONFIG_DIR/$NAME.conf || STATUS=1
 }

Ubuntu used to modify the way the daemon were started:

+ STATUS=0
     # Check to see if it's already started...
     if test -e /var/run/openvpn.$NAME.pid ; then
       log_failure_msg "Already running (PID file exists)"
- STATUS=0
     else
       $DAEMON $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
- $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
- --config $CONFIG_DIR/$NAME.conf || STATUS=1
+ $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
+ --config $CONFIG_DIR/$NAME.conf $script_security < /dev/null || STATUS=1
     fi
+ log_end_msg $STATUS

However in the proposed merge, the old Debian behavior is restored:

- start-stop-daemon --start --quiet --oknodo \
- --pidfile /var/run/openvpn.$NAME.pid \
- --exec $DAEMON -- $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
- $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
- --config $CONFIG_DIR/$NAME.conf || STATUS=1
+ STATUS=0
+ # Check to see if it's already started...
+ if test -e /var/run/openvpn.$NAME.pid ; then
+ log_failure_msg "Already running (PID file exists)"
+ else
+ $DAEMON $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
+ $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
+ --config $CONFIG_DIR/$NAME.conf $script_security < /dev/null || STATUS=1
+ fi
+ log_end_msg $STATUS

Could the Ubuntu changes be ported to the new way Debian uses to start daemon?

Mathias Gug (mathiaz) wrote :

Unsubscribing main sponsors for now.

Andres Rodriguez (andreserl) wrote :

Attaching new patch.

Thierry Carrez (ttx) wrote :

@Andres:
It's trickier than that. Doing it purely the Debian way will probably restore bug 280428, where we chose to differ from Debian. Maybe it's possible to use start-stop-daemon and still pass </dev/null... This would need to be checked with a password-protected autostarted VPN for example. Otherwise I would advise to keep the Ubuntu chunk...

Andres Rodriguez (andreserl) wrote :

Ohh I now understand. What about $script_security???

So, will doing something similar to the following solve this issue?

+ start-stop-daemon --start --quiet --oknodo \
+ --pidfile /var/run/openvpn.$NAME.pid \
+ --exec $DAEMON -- $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
+ $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
+ --config $CONFIG_DIR/$NAME.conf $script_security < /dev/null || STATUS=1

We'll have to test it setting up a password-protected VPN right?

Thierry Carrez (ttx) wrote :

@Andres:
About $script_security, yes, we also need it to be passed as an option.

And yes, something similar to what you propose just might take the best of both worlds : use start-stop-daemon to avoid failure on start when a PID file is not deleted, and still have our $script_security and boot-time password bypass.

The only issue is that I'm not sure (at all) start-stop-daemon would pass the </dev/null to the daemon and that it would work as intended. So yes, it needs to be thoroughly tested: setup a password-protected VPN, reproduce success to boot with current ubuntu, upgrade to the merged version and check if it would not restore the bug (boot process blocked by openvpn asking for a password).

Andres Rodriguez (andreserl) wrote :

@Thierry

What you mean by password-protected VPN?? Do you mean that I have to setup a VPN that asks clients for username/password?? Can you point me out a sample config?

Thanks.

Thierry Carrez (ttx) wrote :

iirc you should be able to test it by setting up a password on your server key. OpenVPN will ask you for the key password before being able to use it to set up the VPN. If you use easy-rsa, use pkitool --server "--pass" option.

Andres Rodriguez (andreserl) wrote :

@Thierry

I've tested the configuration according to this howto: https://help.ubuntu.com/community/OpenVPN. I have just made a change in: ./pkitool --initca to ./pkitool --initca --pass since ./pkitool --server didn't support the "--pass" option. Well, after configuring, I updated openvpn to this version, and no password was asked at boot, however OpenVPN didn't prompt an [OK] message on boot... is this the expected behavior??

I'm attaching an screenshot and debdiff

Andres Rodriguez (andreserl) wrote :

Attaching screenshot.

Thierry Carrez (ttx) wrote :

@Andres: Yes, I seem to remember it was the chosen behavior. Fail to start VPN needing user interaction rather than blocking the boot with a console prompt. Ideally, you would replicate the exact same configuration in a Jaunty environment and check the behavior there, and just doublecheck there is no regression/change in behavior by using Karmic and the new package.

Thanks again for handling this, it is very much appreciated :)

Andres Rodriguez (andreserl) wrote :

@Thierry

I've replicated the same exact configuration on Jaunty and compared it with the karmic setup, and it doesn't seem to be a regression or change in behavior. I'm attaching the new debdiff with fixed changelog. :)

Thierry Carrez (ttx) wrote :

Hey Andres, thank you for your work on this !

About testing:
Did you doublecheck that you can still start the password-protected VPNs manually (after boot) using "sudo service openvpn start <vpnname>" and entering the right password ?

About the changelog entries:
 * Do not use start-stop-daemon and use < /dev/null to avoid blocking boot
    In fact you're now using start-stop-daemon so it should just read "Use < /dev/null to avoid blocking boot"
 * Fix VPNs always reported started [ OK ]
    This was a fix to our "per-VPN" patch so mentioning "show per-VPN result messages" is sufficient

A few more nitpicking comments:
 * There is some (unneeded) spacing difference in the first three lines of:
- --pidfile /var/run/openvpn.$NAME.pid \
- --exec $DAEMON -- $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
- $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
- --config $CONFIG_DIR/$NAME.conf || STATUS=1
+ --pidfile /var/run/openvpn.$NAME.pid \
+ --exec $DAEMON -- $OPTARGS --writepid /var/run/openvpn.$NAME.pid \
+ $DAEMONARG $STATUSARG --cd $CONFIG_DIR \
+ --config $CONFIG_DIR/$NAME.conf $script_security < /dev/null || STATUS=1
It could probably be fixed so that the diff doesn't show the first three lines as different, something like:
- --config $CONFIG_DIR/$NAME.conf || STATUS=1
+ --config $CONFIG_DIR/$NAME.conf $script_security < /dev/null || STATUS=1

 * I would just drop the lsb-base (>= 3.2-14) dependency in debian/control to match what debian does (we have 3.2-20ubuntu4 in karmic anyway)

In all cases I'm not a core-dev yet, so someone else still needs to comment/sponsor this :)

Andres Rodriguez (andreserl) wrote :

Thierry:

 - About the testing: Yes I've tested everything you are mentioning and it works the same way comparing Jaunty openvpn package and the Karmic openvpn package I have on my ppa.

 - Changed those changelog entries.

 - Regarding the diff, it shows that difference because in Debian, they are using whitespaces before the commands, and I'm using tabs, so that's why it shows that they are different lines.

 - Now, before uploading the last debdiff, By dropping the lsb-base dependency, You mean I should drop the dependency as a whole, or just drop the version restriction, since Debian does not include that dependency. However, Isn't that dependency necessary for status_of_proc() ??

Cheers.

Thierry Carrez (ttx) wrote :

@Andres:

You're right, it's technically more correct to have that lsb-base dependency (with the version), even if in most cases lsb-base is always around. This delta could be forwarded to Debian.

The idea is to keep the Debian diff as small as possible, which will allow simpler merges in the future. So even if in some cases Debian is cosmetically wrong, it makes sense to keep the Debian syntax, which means in your case keeping Debian whitespaces. That's just a personal advice, it doesn't mean that your merge would be wrong if you kept your syntax :)

Andres Rodriguez (andreserl) wrote :

Ok. Following your recommendations. Attaching final diff.

Mathias Gug (mathiaz) wrote :

Looks good. I'll upload the merge once alpha2 has been released.

Changed in openvpn (Ubuntu):
assignee: nobody → Mathias Gug (mathiaz)
status: Confirmed → Fix Committed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package openvpn - 2.1~rc15-1ubuntu1

---------------
openvpn (2.1~rc15-1ubuntu1) karmic; urgency=low

  * Merge from debian unstable (LP: #372358), remaining changes:
    - debian/openvpn.init.d:
      - Do not use start-stop-daemon and use < /dev/null to avoid blocking boot
      - show per-VPN result messages
      - add "--script-security 2" by default for backwards compatibility
      - Added lsb-base>=3.2-14 depend to allow status_of_proc()

openvpn (2.1~rc15-1) unstable; urgency=low

  * New upstream version (Closes: #515575)
  * remote_env.patch: patched options.c to fix remote* enviroment vars.
  * openvpn-pkcs11warn.patch: warn on deprecated pkcs11 options.
    Thanks A LOT to Florian Kulzer for the README.Debian text & patch!
    (Closes: #475353)
  * Removed lladdr-is-not-ip.patch, since it was included upstream.
  * init.d script: Use start-stop-daemon to avoid failure on start when
    a PID file is not deleted. (Closes: #445061)
  * init.d script: Added 'status' action. Thanks Thierry Carrez for
    the patch. (Closes: #498493)
  * Updated debian/copyright: Point to GPL-2
  * Updated debian/control: Added ${misc:Depends}
  * Bumped Standards-Version to 3.8.1
  * Moved to debhelper compat 7.

 -- Andres Rodriguez <email address hidden> Tue, 05 May 2009 14:25:37 -0500

Changed in openvpn (Ubuntu):
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers