The beauty of open source is, well, its open source. So after also seeing this in the event log and seeing your thread here, I decided to peek into the source.
Turns out the culprit is the GetRegString() function in common.c which logs any non-success return code immediately to the event log. So this is simply noise in the event log that can be disregarded. There is nothing wrong with your setup if you're seeing this.
Code: Select all
if (status != ERROR_SUCCESS)
{
SetLastError(status);
return MsgToEventLog(M_SYSERR, TEXT("Error querying registry value: HKLM\\%s\\%s"), REG_KEY, value);
}
Looking at the location its called from, I'd say it'd make sense to introduce a parameter to the GetRegString() function which governs if non-success return codes get logged to the event log or not. Perhaps by default they should be, but having a non-fatal error culminate in an error event being logged seems at least slightly odd.
Code: Select all
/* read if present, else use default */
error = GetRegString(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, sizeof(s->ovpn_admin_group));
if (error != ERROR_SUCCESS)
{
openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
error = 0; /* this error is not fatal */
}
Anyway, if I were an OpenVPN contributor, I'd probably rename GetRegString() into GetRegStringEx(), then create a very shallow wrapper around that by the old name GetRegString():
Code: Select all
static DWORD
GetRegStringEx(HKEY key, LPCTSTR value, LPTSTR data, DWORD size, BOOL bLogToEventLog)
{
DWORD type;
LONG status = RegQueryValueEx(key, value, NULL, &type, (LPBYTE) data, &size);
if (status == ERROR_SUCCESS && type != REG_SZ)
{
status = ERROR_DATATYPE_MISMATCH;
}
if (status != ERROR_SUCCESS && bLogToEventLog)
{
SetLastError(status);
return MsgToEventLog(M_SYSERR, TEXT("Error querying registry value: HKLM\\%s\\%s"), REG_KEY, value);
}
return ERROR_SUCCESS;
}
static DWORD
GetRegString(HKEY key, LPCTSTR value, LPTSTR data, DWORD size)
{
return GetRegStringEx(key, value, data, size, TRUE);
}
This way only the single instance (and possibly other non-fatal instances of calling GetRegString() to override a default) could be replaced by a call to GetRegStringEx(), passing FALSE as the last parameter:
Code: Select all
/* read if present, else use default */
error = GetRegStringEx(key, TEXT("ovpn_admin_group"), s->ovpn_admin_group, sizeof(s->ovpn_admin_group), FALSE);
if (error != ERROR_SUCCESS)
{
openvpn_sntprintf(s->ovpn_admin_group, _countof(s->ovpn_admin_group), OVPN_ADMIN_GROUP);
error = 0; /* this error is not fatal */
}
Alternatively it could be logged as a warning instead of an error in a similar fashion.
Btw: in case someone wants to pick this up, feel free to. I hereby put it into the public domain where applicable and under CC0 in jurisdictions that don't know public domain. Feel free to use, I don't care about attribution
TL;DR: the non-fatal error in accessing this value should be logged as a warning or not logged altogether. From reading the code it's clear that there is nothing wrong with the configuration if you get to see this "error" as of version 2.4.1.