Todo #9511
closedOpenVPN server/client/override advanced settings privilege separation
100%
Description
This issue needs some additional thought and debate.
Due to advanced directives in OpenVPN it is possible for users to gain elevated privileges since they can run commands or scripts (#9510). Many users rely on these commands to make OpenVPN behave as they want, so we can't just filter them out.
We could create a new privilege to (dis)allow editing the advanced settings on clients, servers, and overrides. Though splitting the advanced editing privilege might be OK, it poses its own set of tricky questions:
- If a user without privileges to edit the box edits an entry with advanced settings, are they preserved or cleared? (preserved seems best)
- If a user with access to delete OpenVPN instances doesn't also have access to edit the advanced settings, should they be allowed to delete an instance with an advanced configuration set? If so, they could remove advanced settings by deleting the instances and creating a new one without advanced settings. While not technically a path to elevated privileges, it could lead to unintended network access or behavior if they were able to remove custom routing commands.
- Do we add that new privilege to all users with access to edit those pages on upgrade, or do leave it up to the user to discover that they can no longer edit the advanced section? Either way is bound to cause lots of support questions. Using a negative form such as "deny advanced edit access" is bound to be even more trouble, given the issues users have understanding "Deny config write".
A new privilege would look like this, in src/etc/inc/priv/user.priv.inc
$priv_list['page-openvpn-server-advanced'] = array(); $priv_list['page-openvpn-server-advanced']['name'] = gettext("WebCfg - OpenVPN: Edit Clients Advanced"); $priv_list['page-openvpn-server-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Clients' Advanced settings field."); $priv_list['page-openvpn-server-advanced']['warn'] = "standard-warning-root"; $priv_list['page-openvpn-csc-advanced'] = array(); $priv_list['page-openvpn-csc-advanced']['name'] = gettext("WebCfg - OpenVPN: Client Specific Override Advanced"); $priv_list['page-openvpn-csc-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Client Specific Override' advanced settings field."); $priv_list['page-openvpn-csc-advanced']['warn'] = "standard-warning-root"; $priv_list['page-openvpn-client-advanced'] = array(); $priv_list['page-openvpn-client-advanced']['name'] = gettext("WebCfg - OpenVPN: Edit Servers Advanced"); $priv_list['page-openvpn-client-advanced']['descr'] = gettext("Allow edit access to the 'OpenVPN: Servers' Advanced settings field."); $priv_list['page-openvpn-client-advanced']['warn'] = "standard-warning-root";
And then on vpn_openvpn_client.php, vpn_openvpn_csc.php, and vpn_openvpn_server.php the code would need to test for this privilege before allowing changes to Advanced settings.
I'd like to see this in 2.5.0 but if we can't reach a viable consensus then it can be nudged forward to the next release.