Hi, Thanks for the feedback.
I should have made it clear that this was a request for comment about the concept. Whilst the functionality works for the setup I've tested with, I've not reviewed cases of admin or auth-user-pass-verify scripts so there are potential gaps I'll need to go back and fill. For background, the options are based on Linux PAM: https://linux.die.net/man/5/pam.conf. I don't use all the PAM configuration in any of my set-ups, but I know of people who cover use-cases through the likes of Optional, so didn't want to exclude it unless there's a strong consensus that it wouldn't provide value in the OpenVPN case. > This seems to only if you are using a different MFA approach than the > current/recommended one, e.g. demonstrated by the demo script totp.py in > the OpenVPN repository. > This is primarily aimed at out-of-band Push OTP rather than a user entering a TOTP into their VPN client. > To better understand what this patch is actually addressing, it would be > nice to describe the scenario that you are addressing with this patch as > that me do better understand what real-world scenario is actually > addressed by this patch as the this is a bit abstract. > > The use case I'm working with is LDAP authentication followed by a Duo Push, with the requirement being not to perform a Duo Push unless LDAP succeeds. Both of these are initiated from server-side plugins, which I don't think the TOTP script handles, but I'm happy to be told there's an easy way of doing this! > plugin foo foo > > could be both as > > plugin module-name "arguments" > > and as > > plugin [scope] module-name > > > according to the man page. I do not like the approach of using a > whitelist of argument to determine which of the two is being used as a > typo for example or using an old version can then change the parsing in > unexpected way. I would rather see something that has not this ambiguity > like introducing a new keyword like plugin-something. > > > There would only be an incompatibility between old and new versions if you had a binary named `optional`, `required`, `requisite`, or `suffiicient` on your path, and at that point your OpenVPN is likely to fail on the Plugin Init phase as the binary won't have the expected init methods in it, otherwise you'd get an error about not finding the binary for plugin `required`. Similarly having a typo in the configuration would result in OpenVPN failing to start with a message about being unable to find the target binary for the plugin `reequired`. I did consider adding a new `scoped-plugin` item, but wanted to avoid the ambiguity of someone mixing 'auth' plugins in `plugin` and `scoped-plugin`, or using plugins that don't support scope (i.e. non-auth plugins) in `scoped-plugin`. Do you have any suggestions about how you'd differentiate existing plugins and the new plugin type to prevent ambiguity from mix-and-matching old-and-new configuration, e.g. adding two auth-pam entries, one under an existing `plugin` line and one under the newly named `scoped-plugin` line? > What happens with other auth methods like management and auth scripts? > That should be made clear without having to read the source code. > I've not looked at these yet. I'll come back to you on the actual implementation of these. > > What happens if all plugins are optinal. And what is the point of a > plugin being called but ignored? Some idea what to use this for would be > good. > > Good point. Just now OpenVPN terminates if verify-client-cert is `none` and then doesn't have any auth-user-pass-verify scripts or plugins, but this change doesn't prevent someone setting only `sufficient` plugins that all fail, or only setting `optional` plugins. I can harden this by performing a configuration check to ensure a non-optional plugin is set after loading all plugins and to ensure there's no fall-through for all 'sufficient' plugins failing. I don't use Optional in any of the PAM configurations I have, but know the common use-case is for ensuring keyrings/keychains are unlocked but not failing if they're unavailable. > > + > > + If :code:`scope` is not specified for an auth plugin then the plugin > will > > + be treated as :code:`required`. > > do we define what an auth plugin vs a non-plugin is anywhere? > No. This is a terminology I've gone with rather than an OpenVPN nomenclature. It's any plugin that returns the OPENVPN_PLUGIN_AUTH_USER_PASS_VERIFY mask but didn't have a meaningful name for it that didn't refer to a constant in the code. I'm happy to take any suggestions on how to better define that category of plugins. > > > + If :code:`scope` is specified for a non-auth plugin then the > configuration > > + will be treated as invalid. > > + > > And then what will happen? > OpenVPN will exit with an error about the configuration being invalid. Are you wanting this to be explicitly called-out? > > +static char * > > +plugin_scope_name(enum plugin_scope scope) > > +{ > > + switch (scope) > > + { > > + case PLUGIN_SCOPE_REQUIRED: > > + return "required"; > > + > > + case PLUGIN_SCOPE_REQUISITE: > > + return "requisite"; > > + > > + case PLUGIN_SCOPE_SUFFICIENT: > > + return "sufficient"; > > + > > + case PLUGIN_SCOPE_OPTIONAL: > > + return "optional"; > > + > > + default: > > + return ""; > > + } > The PLUGIN_SCOPE_UNDEFINED is missing from this function and default > should be something other than an empty string I think > > This was intentional - the only way you can have UNDEFINED is not to have defined a scope so when printing it back out anywhere I wanted to show the same value as what the user had originally entered. > > > > > + bool non_scope_sufficient_auth_plugin_seen = false; > > I have trouble parsing this variable name and meaning. A plugin without > scope is required from the earlier manpage changes. The combination of > no scope but a sufficent scope makes no sense to me. > Agreed the naming isn't great here. This is enforcing the section from the manpage about ":code:`sufficient` plugins must be listed before any other plugins.", so checking if we've seen something that's not a `sufficient` plugin as we iterate over plugins. However, re-reading the PAM config man page the 'sufficient' should be able to be placed anywhere but have the result ignore if a previous 'required' or 'requisite' plugin has failed. I'll do some refactoring to make this work as-per what PAM is doing which will ultimately remove the block of code in this area. > > + msg(M_FATAL, > > + "PLUGIN_INIT: auth plugin %s is defined as > 'sufficient' but has been specified after other auth plugins not specified > as 'sufficient'", > > + pc->plugins[i].so_pathname); > > + } > > Is this restriction documented in the man page? Also why has this > decision been made. I don't see any obvious reason for it. > > Yes: " :code:`sufficient` plugins must be listed before any other plugins.". This was to prevent ambiguity with a requisite or required plugin failing and having a sufficient plugin listed afterwards. As mentioned above I've misunderstood what PAM does in this area, so will update my implementation to be less restrictive. Thanks, Michael
_______________________________________________ Openvpn-devel mailing list Openvpn-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/openvpn-devel
