Bug 236944
| Summary: | security/strongswan: startup script improvements | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | Ports & Packages | Reporter: | Jose Luis Duran <jlduran> | ||||||||||||||||||
| Component: | Individual Port(s) | Assignee: | Mateusz Piotrowski <0mp> | ||||||||||||||||||
| Status: | Closed Overcome By Events | ||||||||||||||||||||
| Severity: | Affects Only Me | CC: | 0mp, driesm, sc.gear, strongswan | ||||||||||||||||||
| Priority: | --- | Keywords: | needs-qa | ||||||||||||||||||
| Version: | Latest | Flags: | strongswan:
maintainer-feedback+
koobs: merge-quarterly? |
||||||||||||||||||
| Hardware: | Any | ||||||||||||||||||||
| OS: | Any | ||||||||||||||||||||
| See Also: | https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=234648 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Jose Luis Duran
2019-04-01 17:56:00 UTC
(In reply to Jose Luis Duran from comment #0) Thanks will have a look at the patch. Just have a deadline, so will try and get to it asap. But it looks good with the quick glance I gave it. Created attachment 203395 [details]
Patch strongswan.in
This patch includes the PORTREVISION bump and a change to https in the package description.
Created attachment 203489 [details]
Improve strongswan.in
Make the message more consistent with rc.subr.
Created attachment 203887 [details]
Improve strongswan.in
- Use https
- Fix a few typos
Hi Jose, thanks for making the code better. Small observations and suggestions, if you will: 1) check_charon() - Consider rc.subr err() to throw messages. Why exit code 7 ? Since "swanctl --stats" sets errno 61 on VICI socket failure. 2) wait_charon() - Consider hard-code the wait interval (e.g. "sleep 5"). Else while loop runs command "sleep 1" five times when $charon_pidfile exists. (In reply to Sam Chen from comment #5) Thank you for taking the time to review the patch: 1) I tried to keep it similar to ipsec.in, currently the failing exit code for reload is 7: https://github.com/strongswan/strongswan/blob/5c38a5ea832accec3a8d3942d878ac5be5bb2a4b/src/ipsec/_ipsec.in#L171 2) The idea is not to use a hard-coded sleep, else I would prefer to use 2 seconds, which is what ipsec.in is currently using (see comment #0). The idea is that if $charon_pidfile still exists after stopping the service, wait another second. What I would like to do is to rely more on rc.subr to clean up the code a bit more. By using $pidfile and perhaps $procname. (In reply to Jose Luis Duran from comment #6) Thanks, using code 7 for missing charon.pid now makes perfect sense to me. My mistake in #2, I misread the code and was about to ask you disregard that comment. Another observation: $strongswan_interface declaration changed from ":=" to ":-", which IIRC, means the parameter disappears after use. Since strongswan.in is the first (only?) place $strongswan_interface is defined (vs others preset in /etc/defaults/rc.conf). I'm not familiar with FreeBSD init. How does ":-" expansion change the scope of the variable outside of strongswan.in? Any problem if $strongswan_interface is not set in /etc/rc.conf? Thanks! (In reply to Sam Chen from comment #7) Please disregard observation #3 => Just a different coding style that give the same result. My interest is in the updated restart/stop_postcmd section, and I can confirm that works fine. Thank you. Changes to error messages, I'm indifferent. Just two things I noticed while testing: 1. strongswan.in check_charon() echos to STDOUT. /usr/local/sbin/ipsec and rc.subr warn() err() dups to STDERR. 2. "ipsec reload" has more verbose message of "Reloading strongSwan IPsec failed: starter is not running". (In reply to Sam Chen from comment #9) 1. I can add the missing `>&2`. 2. Something like "Reloading strongSwan IPsec failed: charon is not running"? Thanks. I'm happy with the updates. Seems like a good improvement on the previous script. Created attachment 204210 [details]
Improve strongswan.in
Redirect to stderr.
Created attachment 204703 [details]
Improve strongswan.in
Rebase 5.8.0
Created attachment 208097 [details] Improve strongswan.in Rebasing 5.8.1 Improving the script to rely more on rc.subr. I have setup an ATF (Kyua-based) with some tests for the rc script (https://github.com/jlduran/strongswan-tests/blob/master/tests/freebsd/rc_script/rc_script), the strongswan.in currently distributed fails some of these tests. The submitted version has a few differences, for instance I am keeping "stroke" as the default strongswan_interface, while upstream (starting 5.8.0) and in my GitHub repo "swanctl" (or "vici") is the default. Comment on attachment 208097 [details]
Improve strongswan.in
Happy with the updates on the script that improves to be more robust.
(In reply to strongswan from comment #15) Thank you for reviewing! I will upgrade the script a little bit so that the sleep waits 5 seconds (as it is right now), just to make sure no one is affected by this new version. Hopefully before next week. I was reluctant to do it because I wanted to use something kqueue-based. I have not found anything in base that allows me to do so. In the meantime, I'll keep the sleep loop. Thank you all! Created attachment 208247 [details]
Improve strongswan.in
Put back the sleep-wait block, I could not find a kqueue(2)-based tool to use that detects when the charon_pidfile has been created. Use the current polling method.
Hi! Based on the current rc script, most of this functionality seems to be already included upstream. Could be closed with "overcome by events". |
