Add support for specifying the syslog facility, as requested in trac #188.

Users want this because it allows them to have syslog put all OpenVPN messages in specific file(s) by using syslog.conf.

Adds --syslog-facility name option, which must precede --daemon and --syslog.

Adds ability to specify facility as [name] in --daemon and --syslog's progname argument, which makes it possible to specify the syslog facility name without modifying certain system init scripts. --syslog-facility takes precedence.

For example, Debian automatically generates a --daemon vpn-{configname} directive. If you name the config foo[local1].conf, it's as though you were able to specify --syslog-facility local1 --daemon vpn-foo --config foo[local1].conf. Absent this feature, This isn't possible without modifying the distribution-provided init script.

--syslog-facility list (or any other undefined name) will list the facilities that the platform supports.

The old method of -DLOG_OPENVPN still provides the facility used by default. Thus, the priority is:
--syslog-facility
--daemon [facility] or --syslog [facility]
-DLOG_OPENVPN

This is forward and backward compatible with existing scripts & initfiles.

---
 doc/openvpn.8         |   15 ++++++
 src/openvpn/error.c   |  129 
+++++++++++++++++++++++++++++++++++++++++++++++--
 src/openvpn/error.h   |    1 +
 src/openvpn/options.c |   19 ++++++++
 4 files changed, 161 insertions(+), 3 deletions(-)

index 34894e5..2c63795 100644
@@ -2169,6 +2169,12 @@ When unspecified,
 .B progname
 defaults to "openvpn".
 
+If --syslog-facility has not been specified and the name
+includes [facility], the specified syslog facility name will be used with
+the system logger.  (This method of specifying the facility name may be
+convenient when initialization scripts generate --daemon progname from
+the config file name.  This avoids the need to edit the script.)
+
 When OpenVPN is run with the
 .B \-\-daemon
 option, it will try to delay daemonization until the majority of 

initialization
@@ -2188,6 +2194,15 @@ directive above for description of
.B progname
parameter.
.TP
+.B \-\-syslog-faciity name
+When logging to syslog, use name (e.g. local1 or daemon) for the facility. +If name is absent or not supported, a platform-dependent list of valid +facility names is provided.
+
+Must be specified before --daemon and --syslog.
+If this is not convenient, the facility name may be specified as [name] in +the name parameter of either directive.
+.TP
.B \-\-errors-to-stderr
Output errors to stderr instead of stdout unless log output is
redirected by one of the
.B \-\-log

index fd9f19d..fb0abbd 100644
@@ -89,9 +89,10 @@ static bool machine_readable_output;   /* GLOBAL */
 /* Should timestamps be included on messages to stdout/stderr? */
 static bool suppress_timestamps; /* GLOBAL */
 
-/* The program name passed to syslog */
+/* The program name and facility passed to syslog */
 #if SYSLOG_CAPABILITY
 static char *pgmname_syslog;  /* GLOBAL */
+static int facility_syslog = -1; /* Unspec (RFC5424 - can not be negative) */
 #endif
 
 /* If non-null, messages should be written here (used for debugging 

only) */
@@ -439,6 +440,93 @@ out_of_memory (void)
exit (1);
}

+#if SYSLOG_CAPABILITY
+static int syslog_fac_code (const char *name, int default_code) +{
+ static struct {
+ int code;
+ const char *const name;
+ } facnames[] = {
+ { LOG_AUTH, "auth" },
+#ifdef LOG_AUTHPRIV /* Prefered (private), but non-POSIX */
+ { LOG_AUTHPRIV, "authpriv" },
+#else
+ { LOG_AUTH, "authpriv" }, /* Map to non-secure code */
+#endif
+ { LOG_CRON, "cron" },
+ { LOG_DAEMON, "daemon" },
+#ifdef LOG_FTP
+ { LOG_FTP, "ftp" },
+#endif

+    { LOG_LPR, "lpr" },
+    { LOG_MAIL, "mail" },
+    { LOG_NEWS, "news" },

+#ifdef LOG_SYSLOG
+ { LOG_SYSLOG, "syslog" },
+#endif
+ { LOG_USER, "user" },
+#ifdef LOG_UUCP
+ { LOG_UUCP, "uucp" },
+#endif

+    { LOG_LOCAL0, "local0" },
+    { LOG_LOCAL1, "local1" },
+    { LOG_LOCAL2, "local2" },
+    { LOG_LOCAL3, "local3" },
+    { LOG_LOCAL4, "local4" },
+    { LOG_LOCAL5, "local5" },
+    { LOG_LOCAL6, "local6" },
+    { LOG_LOCAL7, "local7" },

+
+ { 0, NULL },
+ }, *fac;
+
+ if (use_syslog)
+ {
+ msg (M_ERR, "syslog facility can not be changed after logging has started");
+ return;
+ }
+
+ /* Lookup facility code by name */
+
+ for (fac = facnames; fac->name; fac++)
+ {
+ if (!strcmp (fac->name, name))
+ {
+ return fac->code;
+ }
+ }
+
+ /* Not found, look for name of facility that will be used */ +
+ if (default_code == -1)
+ default_code = LOG_OPENVPN;
+
+ for (fac = facnames; fac->name; fac++)
+ {
+ if (fac->code == default_code)
+ {
+ break;
+ }
+ }
+
+ /* Warn, and list valid names (they are platform-dependent) */ +
+ msg (M_WARN, "syslog: %s is not a valid facility name, using %s", name,
+ (fac->name? fac->name : "default"));
+ for (fac = facnames; fac->name; fac++)
+ {
+ msg (M_INFO, "syslog: %s facility is valid on this platform", fac->name);
+ }
+ return default_code;
+}
+
+void set_syslog_facility (const char *name)
+{
+ facility_syslog = syslog_fac_code (name, -1);
+}
+
+#endif
+
void
open_syslog (const char *pgmname, bool stdio_to_null)
{
@@ -447,8 +535,43 @@ open_syslog (const char *pgmname, bool stdio_to_null)

     {
       if (!use_syslog)
        {
-         pgmname_syslog = string_alloc (pgmname ? pgmname : PACKAGE, NULL);
-         openlog (pgmname_syslog, LOG_PID, LOG_OPENVPN);
+         char *pname = pgmname_syslog = string_alloc (pgmname ?

+ pgmname : PACKAGE, NULL);
+ int facility = facility_syslog;
+ if (facility == -1)
+ {
+ /* Unspecified, default */
+
+ facility = LOG_OPENVPN;
+
+ /* Attempt to extract from program name (simplifies init files) */
+ char *facb, *face;
+
+ /* Decode optional [facilityname] prefix */
+
+ if ((facb = strchr (pgmname_syslog, '[')) != NULL
+ && (face = strchr (facb+1, ']')) != NULL)
+ {
+ char *facname = malloc (face - facb); /* [name] => name\0 */
+
+ /* Extract facility name and remove from option string */
+

+                 *face++ = '\0';
+                 strcpy (facname, facb+1);
+                 memmove (facb, face, strlen(face)+1);

+
+ /* Default program name */
+
+ if (!*pname)
+ pname = PACKAGE_NAME;
+
+ /* Convert extracted facility name to code */
+
+ facility = syslog_fac_code (facname, facility);
+ free (facname);
+ }
+ }

+         openlog (pname, LOG_PID, facility);
          use_syslog = true;

          /* Better idea: somehow pipe stdout/stderr output to msg() */

index 1e1f2ac..cbd1980 100644
@@ -234,6 +234,7 @@ void msg_forked (void);
 
 /* syslog output */
 
+void set_syslog_facility (const char *name);
 void open_syslog (const char *pgmname, bool stdio_to_null);
 void close_syslog ();
 

index 40210e6..e6cc1cc 100644
@@ -333,9 +333,21 @@ static const char usage_message[] =
   "--setcon context: Apply this SELinux context after initialization.\n"
 #endif
   "--cd dir        : Change to this directory before initialization.\n"
+#if SYSLOG_CAPABILITY
+  "--syslog-facility name : When logging to syslog, use name (e.g. \n"
+  "                  local1) for the facility.  If name is absent or not\n"
+  "                  supported, a platform-dependent list of valid\n"
+  "                  facility names is provided.  Must be specified\n"
+  "                  before --daemon and --syslog.\n"
+#endif
   "--daemon [name] : Become a daemon after initialization.\n"
   "                  The optional 'name' parameter will be passed\n"
   "                  as the program name to the system logger.\n"
+#if SYSLOG_CAPABILITY
+  "                  If --syslog-facility has not been specified and \n"
+  "                  the name includes [facility], the specified\n"
+  "                  syslog facility name will be used.\n"
+#endif
   "--syslog [name] : Output to syslog, but do not become a daemon.\n"
   "                  See --daemon above for a description of the 

'name' parm.\n"
"--inetd [name] ['wait'|'nowait'] : Run as an inetd or xinetd server.\n"
@@ -4661,6 +4673,13 @@ add_option (struct options *options,

       VERIFY_PERMISSION (OPT_P_GENERAL);
       options->up_restart = true;
     }

+#if SYSLOG_CAPABILITY
+ else if (streq (p[0], "syslog-facility"))
+ {
+ VERIFY_PERMISSION (OPT_P_GENERAL);
+ set_syslog_facility (p[1]);
+ }
+#endif
else if (streq (p[0], "syslog"))

     {
       VERIFY_PERMISSION (OPT_P_GENERAL);

--
1.7.10.4

Timothe Litt
ACM Distinguished Engineer
--------------------------
This communication may not represent the ACM or my employer's views,
if any, on the matters discussed.

Hi,

(why is this being discussed on -users? Taking it over to -devel)

Uh. Feature-NAK - especially the last one really makes me want to throw up.

Using [] as part of an option string is a bad idea, because it is commonly used for optional arguments (as you saw in David's question), and intermixing option argument "A" and "B" in the same string is not going to help anyone.

If we take this at all - and I have reservations, because the amount of code it brings in is quite significant - it should not touch the --daemon command line. It brings it's own config statement (--syslog-facility), so there really is no reason to mess up the syntax for --daemon.

Also, --syslog-facility should be more clean, like

--sysloc-facility facility [progname]

with "progname" actually being *optional*, not "marked by using rectangular brackets".

[..]

Init scripts can be adjusted. Obscure code inside OpenVPN stays forever
(guess why the developers have so little time? right, maintaining stuff that someone else considered "a clever hack" 5 years ago).

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

Er, I did post it to -devel, and I don't see any discussion of it on -users...I just double-checked the archives, and it's all in -devel...

--daemon and --syslog already control syslog's program name. Since we would have to support the existing usages, your suggestion to add that to --syslog-facility would add complexity, not simplify things. You really don't want that.

Sorry that you don't like my accommodation for init scripts. It helps OpenVPN and its USERS.

The fact is that I'm not going to get all the distributions to change their init scripts, and neither are you. It's a major effort to do that and get it coordinated across all the distributions. No one -- not me, not you, and not the distro maintainers -- has time for that.

Asking our users to do it piecemeal - and maintain it or lobby their distributions is not good engineering. It's a problem that we can solve on this end.

Even if one could get distro cycles to change their init scripts - why would we waste them on something like this? We'll want them when we want something that can't be handled without their help...

Yes, it's ugly. And my stomach rumbles too.

If OpenVPN were engineered from scratch, --daemon would just daemonize, --syslog would control the syslog program name and facility, and the two could be used together. And there wouldn't be silly ordering constraints among the options. But that's not the way the existing code -- and customers' config and distros' init files -- work.

This is the real world, and sometimes we have to live with ugly. Even if it makes us a little queasy.

As for the choice of square brackets - the delimiter had to be something easy to parse that is uncommon in program and file names.

The init file usage does not make it easy. Facility can not be a prefix, because the Debian usage I cite generates the --daemon argument from the config file name -- adding a prefix. It can't be a suffix because of the way systemd-based distros auto-generate multiple instances.

So it has to be something that can be embedded anywhere in the string, And that means delimiters on both ends.

I was aware that [ ] is used for 'optional' in syntax descriptions when I picked them. But I provided examples to clarify the point. And the other choices were worse, as they have other meanings to shells and filesystems. At least [ ] is visually distinctive and easy to remember.

It could be something else. Do you have a better idea that doesn't add complexity?

Actually, the first pass didn't - the facility was ONLY in the progname argument. See the patch in Trac, which was the minimal change required to solve the problems.

However, the additional code is minimal, and for cases where it is possible to avoid the ugly overloading of progname, it is a cleaner way to specify facility. I thought it worth the effort so as not to force ugliness on users when it isn't required.

None of this was the result of random choices or "cleverness". I've been in the software business long enough to have dealt with more of that than most. Try adding a major feature to a 30-year old OS that's written in assembler...or to microcode.

I thought carefully about the issues, the requirement and how to meet it in a way that minimizes the internal and external impacts.

I'm busy too - but I took the time to understand the problem and do a proper job. I checked multiple distributions, consulted standards, localized the changes and did careful testing in the environments that I have. And tried to give back to the community.

I could have just compiled with -DLOG_OPENVPN=LOG_LOCAL5 and gone on with my life... Or thrown an 'elegant' solution over the wall and left customers to deal with the init file issues.

I didn't ignore it for 2 years, and I didn't let aesthetics get in the way of the best solution that I could come up with.

The scope is hardly large. If you look at the actual code - the size of the patch is dominated by documentation and the lookup table. There are a couple of lines to parse the option, a subroutine to lookup the names, and local block of code in open to handle the syntax.

Although the init-script compatibility is ugly and adds a few lines of code, it meets the requirements with the least total impact to all concerned.

If you don't want the feature, why did you write in Trac #188:

?

You clearly understood what was requested and the scope of the change, aside from the init file issue that you didn't anticipate. (And neither did I, until I dug in to the problem.)

As soon as I tried to configure OpenVPN, I saw that it used syslog, and the first thing I looked for was how to send output to a facility of my choice. I saw the request in Trac - and several times in the mailing lists/forums. I'm hardly the only person requesting the feature.

Given that the standard specifies the facility names, the local scope of the code and the fact that to trigger any of the new code paths one has to take positive action, I think this is reasonably safe for 2.3.

As I noted in the Trac ticket and in the previous notes, what I did was carefully portable and standard-compliant. It's not a one-line change; it does need testing, but it's not as bad as you thought. Standards help.

I don't expect issues - but if there are any, they will happen the first time it's compiled on some (probably obscure) platform that doesn't have a required facility code. It's unlikely, but possible. It won't be something subtle in the field -- like TLS1.2 :-(

But if you want to hold it for 2.4, it's your project.

And if you have a solution that is a lot prettier AND addresses the init-file issues, I'm open to considering it.

At the moment, I'm still convinced that what I posted is the best solution available.

Timothe Litt
ACM Distinguished Engineer
--------------------------
This communication may not represent the ACM or my employer's views, if any, on the matters discussed.

Hi,

The mail I replied to went to -users, and only there...

Then let's not do that. Fine with me :-)

"Helping init scripts" is actually something that only helps distribution builders, and they are few, and usually listen quite well if we tell them what needs to be adjusted.

"Having unmaintainable code inside OpenVPN" is something that does not help anyone - people will forget why it is the way it is, break it when changing something else, and that will cause endless pain. We've been through that right now when rewriting socket.c to properly support dual-stack systems, breaking *some* bits of SOCKS proxy support in the process, breaking --inetd in the process, and so on - so we know all about non-intuitive options and hard-to-maintain code.

[..]

So do not write such code, and do not expect us to merge it.

Then let's fix *that* problem (the ordering constraints), instead of adding more convolutions. That would indeed help "OpenVPN and the users".

No :-) - it's open source world, and since everyone can see what we do, we're not going to add more ugliness if we can avoid it.

Closed source works differently.

[..]

"Anywhere in the string" is a hard "no".

[..]

Yeah, I understand that the amount of free time we can give into OpenVPN is annoying you. It's annoying me as well that I have spend time in paid-for projects not related to OpenVPN to help feed my family, or that David had to tune down his involvement into OpenVPN due to health reasons.

The amount of cleanup we've done in the last two years is quite enormous :-)
- even if it's not really visible outside of the git log.

[..]

I'm fine with the addition of a way to set the syslog-facility, but not the way it is presented. Not intermixing stuff in other stuff in arguments.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

That's my fault. Well, it was the "Correct Identity" add-on in Thunderbird which suddenly began to pick the wrong identity (-users ML) which has a different reply-to setting. Sorry about that!

kind regards,

David Sommerseth

Agreed!

Agreed as well. This is primarily what I'd expect this to do too.

On distros which don't have package management, this is probably a fine thing.
But on most Linux distros with package management, init.d scripts will be
replaced on package updates. As we don't provide init.d script for all distros, it will not really be possible to change this. Further, it's not that uncommon that software adds a facility option, so I find this core feature a good enhancement.

But! I do not like the [] syntax. The core principle in OpenVPN's option parsing is that the last argument wins. So if you have f.ex. --ping-exit 3 times in a command line and two times in a config file, it's the last one which really sets the option. --syslog-facility should be no different.

IMO, distro init.d scripts should add the config file as the last argument when they kick off openvpn. If they add stuff which overrides the config file, then it's a bug in the init.d script.

Anyway NAK to the v2 patch. Because it really didn't give answers to many of my questions.

- We do not need SYSLOG_CAPABILITY, is there another #ifdef which restricts SYSLOG somehow? If yes, use that, if not, don't do this.

- Why not re-use facilitynames[] from sys/syslog.h, instead of adding something which is basically a duplicate of exactly that lookup table?

- Further, why the #ifdef's on LOG_FTP, LOG_SYSLOG, LOG_UUCP?

- Don't fiddle around with malloc(). Use gc_arena and the string buffer functions defined in buffer.c/h

Please, make the next round a far more simpler version, reuse what is already implemented and well tested other places rather than re-inventing the wheel.

Using --syslog-facility is a bare minimum acceptable but not preferred. Rather consider --daemon [progname] [facility] and --syslog [progname] [facility]. If facility is not set, default to whatever is the default now (IIRC, it is syslog).

kind regards,

David Sommerseth

Hi,

Oh, wow. I didn't know that this exists - learned something new today.

We might have to figure out how this is portably used - I see it exists on Linux and FreeBSD, at least, and both have it inside #ifdef SYSLOG_NAMES, so our code might have to do

#define SYSLOG_NAMES
#include <sys/syslog.h>

or such. Plus possibly an autoconf test to see whether it's there, and if not, fall back to something in compat/ - that would make the change to options.c much smaller.

"Because not all systems have them", I'd say :-) - syslog facilities are far from universal, alas.

[..]

I think it would be good to loosen the restrictions that --syslog has to be called before --daemon, and move that to "latest option wins" (as usual). Haven't checked the code to see how complicated that is.

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...

That might be good for distros, but Tunnelblick purposefully starts OpenVPN with "--management" *after* "--config"; other GUIs may do this, too. Tunnelblick does this to make sure that the management interface is used only by Tunnelblick. (Tunnelblick separately warns the user if the configuration file contains "--management" or other problematic options.)

For some options, it is the *first* argument that wins: "--log", "--log-append", and I think "--daemon" (and possibly some other options like "--syslog"). Tunnelblick puts --log and --daemon first to make sure it controls logging. (Tunnelblick sends the log to a file, which it monitors, instead of logging through the management interface, so Tunnelblick (the GUI) does not need to be running when the OpenVPN instance is running -- for example, when nobody is logged in.)

Hi,

JFTR, this does *not* exist on Solaris (which we support) or AIX (which we do not support yet, but I'm toying with it since it recently grew TAP support...).

It does exist on Linux, FreeBSD, NetBSD, MacOS X, OpenBSD.

MinGW/Windows does not seem to have a syslog.h at all....

gert

USENET is *not* the non-clickable part of WWW!
                                                           //www.muc.de/~gert/
Gert Doering - Munich, Germany                             [email protected]...
fax: +49-89-35655025                        [email protected]...