ssl_verify: define label only when required

The "cleanup" label in ssl_verify.c:verify_user_pass_plugin() is used only when PLUGIN_DEF_AUTH is defined, therefore make the label definition dependent on the same define.

Fixes the following warning when PLUGIN_DEF_AUTH is not defined:

ssl_verify.c: In function 'verify_user_pass_plugin':
ssl_verify.c:1223:1: warning: label 'cleanup' defined but not used [-Wunused-label]
cleanup:
^~~~~~~

---
 src/openvpn/ssl_verify.c | 2 ++
 1 file changed, 2 insertions(+)

index 61872251..5b46ea72 100644
@@ -1220,7 +1220,9 @@ verify_user_pass_plugin(struct tls_session *session, const struct user_pass *up,
         msg(D_TLS_ERRORS, "TLS Auth Error (verify_user_pass_plugin): peer provided a blank username");
     }
 
+#ifdef PLUGIN_DEF_AUTH
 cleanup:
+#endif
     return retval;
 }
 

2.18.0

Am 15.08.2018 um 08:12 schrieb Antonio Quartulli:

Hi,

Thanks for taking the time to look at this!

Actually we are in the process of getting rid of the various ifdef around the code or at least we are trying to bring that to a more reasonable state (it takes time....and right now we are more focussed on the networking and platform side of things).

I totally agree with you that more ifdefs are just ugly and make the code worse. However this "new" ifdef is just part of some logic that is already there (a few lines above).

This said, in the past months we have tried moving towards a 0-warnings-tolerance, so that we could catch as many bugs as possible hidden by "just annoying warnings".
For this reason using "-Wno-unused-label" is not an option (especially in CI/automated build tests where this was spotted).

Imho we should just get this patch through and then try to get rid of the whole #ifdef PLUGIN_DEF_AUTH logic at once.

Cheers,

Antonio Quartulli

Hi,

It is a new #ifdef, two more lines of code, breaking the flow of reading. So I tend to agree with Mathias that this might not be the best approach here.

I see "unused label" as a minor nuisance (unlike other warnings there is nothing like "it could be an integer overflow" or "watch out for the signs of your variables" here), so -Wno-unused-label would work for me...

Warnings are generally good, but adding extra lines just to silence the compiler in a specific combination of flags for a warning that is otherwise harmless is not always a good way forward.

If the idea is to get rid of PLUGIN_DEF_AUTH, then let's do so, not add extra #ifdef :-)

Anyway. Trying to understand the logic here - PLUGIN_DEF_AUTH is always defined if ENABLE_DEF_AUTH is set, which seems to be the default setting.

So, questions

- is there actually some benefit in --disable-def-auth? code size, performance, platform compatibility?

- why do we have it in the first place?

- can we get rid of it?

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...

Hi,

Good! Let's focus on that then :)

to me this feels like the classic pattern of "let's introduce this new feature, but let's add a compile time option in case people have troubles with it or don't want it for some reason".
I have seen this often also in newer code (like ovpn3).

Does any distro package openvpn with --disable-def-auth selected? If not, I guess we can get rid of it.

But maybe David knows something we don't :-)

Cheers,

Antonio Quartulli

On 16/08/18 13:37, Gert Doering wrote:
[...snip...]

I do see the arguments on both sides here. And in particular where these warnings halts the compilation (which I think I read some where that newer GCC compilers would do by default). But ... Gert asks a few reasonable questions

I tend to say no. ENABLE_DEF_AUTH is about deferred authentication. This has been enabled by default for so long, that I hardly think it would be an issue killing the possibility to turn off this feature. All it essentially does is to allow plug-ins to return OPENVPN_PLUGIN_FUNC_DEFERRED instead of OPENVPN_PLUGIN_FUNC_ERROR or OPENVPN_PLUGIN_FUNC_SUCCESS.

This feature will also improve the authentication performance by not locking up other connected clients if the authentication backend is slow *and* implements support for deferred authentication. So there is a performance gain for authentication plugins making use of this feature. Otherwise, I do not expect much performance difference.

I am definitely leaning towards "Yes!". This does grow the resulting binary by some bytes (I don't know how much; haven't checked), as it adds code which prepares a file the plug-in will use to write the final verdict of the authentication; plus the code paths related to polling this file the core OpenVPN process. If the If the code grows enough to validate the need for this macro, I am not so sure of.

The only users I would expect might do builds with --disable-def-auth are the builds for embedded devices. But I don't know how much of a concern this is any more; this feature shouldn't double or half the size of the resulting code.

There is also a related ./configure option: --enable-async-push. This adds inotify support, which reworks the polling model to be a notification based push model instead (iirc). But this I think makes sense to have as it is today for the time being. We might want to consider flipping it to be enabled by default, though.

kind regards,

David Sommerseth
OpenVPN Inc

Hi,

thanks for your thoughts so far, so it seems we just want to get rid of these conditionals for 2.5 :-)

One thing, though:

This carries along some platform dependencies, as "inotify" is not unversally available (I was about to write "Linux only", but I bet one of the BSDs has it, too, by now :-) - but definitely not all supported platforms).

Google points to "fswatch", which is

"A cross-platform file change monitor with multiple backends: Apple OS X
File System Events, *BSD kqueue, Solaris/Illumos File Events Notification, Linux inotify, ..."

so yeah - this has to stay an #ifdef for a while (and more esoteric platforms like *cough* Windows or AIX might not have anything reasonably compatible at all)

gert

"If was one thing all people took for granted, was conviction that if you 
 feed honest figures into a computer, honest figures come out. Never doubted 
 it myself till I met a computer with a sense of humor."
                             Robert A. Heinlein, The Moon is a Harsh Mistress

Gert Doering - Munich, Germany                             [email protected]...