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

Attaching debdiff

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?

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

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.

Thanks for the suggestions.

How does it looks like now?

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?

Unsubscribing main sponsors for now.

Attaching new patch.

@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...

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?

@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).

@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.

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.

@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

Attaching screenshot.

@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 :)

@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. :)

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 :)

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.

@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 :)

Ok. Following your recommendations. Attaching final diff.

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

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