openvpn_plugin_func_v3 version documentation disagrees with code

[gojolu]

The documentation for openvpn_plugin_func_v3() (as specified in include/openvpn-plugin.h.in) states that:

version: ... The plug-in should validate that this value is matching the OPENVPN_PLUGIN_VERSION value.

However, plugin_call_item() which calls openvpn_plugin_func_v3() (as specified in src/openvpn/plugin.c) actually passes OPENVPN_PLUGINv3_STRUCTVER instead of the expected OPENVPN_PLUGIN_VERSION on line 561 as show below.

status = (*p->func3)(OPENVPN_PLUGINv3_STRUCTVER, &args, &retargs);

This discrepancy causes all plug-ins that use openvpn_plugin_func_v3() and follow the recommendations of the documentation to fail for any OpenVPN versions where OPENVPN_PLUGIN_VERSION does not equal OPENVPN_PLUGINv3_STRUCTVER. A cursory examination reveals this to be all 2.3 versions and any 2.4 versions >= 2.4.2.

[Gert Döring]

@dazo, @selvanair throwing the ball your way, without actually investigating.

[Gert Döring]

The purpose of this version arg is to indicate the version of the argument structs passed around in the openvpn_plugin_open_v3 and openvpn_plugin_func_v3 calls, so I think the code in plugin.c is correct. But the documentation in the header file is not. So the caller should check this version argument against OPENVPN_PLUGINv3_STRUCTVER.

It would be better to rename this argument in both .._open_v3() and ..func_v3() signatures from version to structver and document it as the version of the argument struct of plugin v3 structures, not that of the plugin API. In case of openvpn_plugin_open_v3, the doc string already states it should be matched against OPENVPN_PLUGINv3_STRUCTVER. The same wording should apply to openvpn_plugin_func_v3 as well.

That said, instead of requiring an exact match, checking that its >= certain value should be enough and that would ease the burden of keeping plugins up to date. That's how we use it in the auth-pam plugin (though only openv_v3 used there, not func_v3). If we can guarantee that structs will only be extended (offset of old members preserved), we could document it like that.

David is the authority on this, so the rest I defer to him.

[Selva Nair]

The purpose of this version arg is to indicate the version of the argument structs passed around in the openvpn_plugin_open_v3 and openvpn_plugin_func_v3 calls, so I think the code in plugin.c is correct. But the documentation in the header file is not. So the caller should check this version argument against OPENVPN_PLUGINv3_STRUCTVER.

It would be better to rename this argument in both .._open_v3() and ..func_v3() signatures from version to structver and document it as the version of the argument struct of plugin v3 structures, not that of the plugin API. In case of openvpn_plugin_open_v3, the doc string already states it should be matched against OPENVPN_PLUGINv3_STRUCTVER. The same wording should apply to openvpn_plugin_func_v3 as well.

That said, instead of requiring an exact match, checking that its >= certain value should be enough and that would ease the burden of keeping plugins up to date. That's how we use it in the auth-pam plugin (though only openv_v3 used there, not func_v3). If we can guarantee that structs will only be extended (offset of old members preserved), we could document it like that.

David is the authority on this, so the rest I defer to him.

[David Sommerseth]

Yes, @selvanair is spot-on.

The OPENVPN_PLUGIN_VERSION macro in openvpn-plugin.h defines which plug-in functions are available. This header file must be from the same source tree version as the openvpn binary.

The OPENVPN_PLUGINv3_STRUCTVER macro defines the version of the structs being passed to the functions being exposed via the functions available when OPENVPN_PLUGIN_VERSION is 3 or newer. That said, when the OPENVPN_PLUGIN_VERSION 3 was defined, we changed the approach so we could avoid extending the API without adding more functions to get more advanced features.

The OPENVPN_PLUGINv3_STRUCTVER version covers the following declarations:

• struct openvpn_plugin_callbacks
• struct openvpn_plugin_args_open_in
• struct openvpn_plugin_args_open_return
• struct openvpn_plugin_args_func_in
• struct openvpn_plugin_args_func_return

If any of these structs changes, the OPENVPN_PLUGINv3_STRUCTVER must be increased. And it is only allowed to add members to these structs. Existing members cannot be modified in any way. This is to ensure older plug-ins can work with newer API versions. And plug-ins supporting more versions can adopt at runtime to the the API level the OpenVPN binary exposes.

Whether a strict version match is required or a >= match will suffice, depends on the plug-in.

[Gert Döring]

So - do we have to do something here (improve the documentation by means of a patch) or do we claim "all is fine" and close the ticket?