Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove some registry keys/options and use file association for open/edit #62

Merged
merged 3 commits into from Aug 23, 2016

Conversation

selvanair
Copy link
Collaborator

Note on commit 2: Use file associations to open config and log
Works fine on my win7 test machine, but more independent testing would help. Note that notepad is the default editor, so associate with something else to test.

Full commit messages:

  1. Simplify some parameters and registry keys
  • Replace allow_password by a runtime check that enables password
    change menu only when the user has write-access to the key file.
  • Read exe_path and priority from HKLM and do not duplicate in HKCU.
  • Always allow the user to view the config: edit will succeed if user
    has write access.
  • Always include the proxy settings tab which is the default.
  • Remove the unused power event handling and disconnect_on_suspend key.
  • Remove password_attempts -- user can stop the password dilaog
    by clicking cancel.
  • Remove allow_service: implicitly enabled if service_only is used.
  • Deprecate removed options in cmd-line parser
  • Update README.rst
  1. Use file associations to open config and log
  • Use ShellExecute to open config and log files so that
    file associations can be used. If that fails fall-back to
    the default editor (notepad.exe).
  • Remove the keys editor and log_viewer from registry.
    The user can change the editor/viewer through fie association.

@mattock
Copy link
Member

mattock commented Jul 14, 2016

As discussed in #61 I tested openvpn-gui.exe that has these changes, and it worked as expected. The changes are reasonable imho, so a feature-ACK from me. Having a code-ACK would be good to have, so that we don't again have to resort to lazy-ACK.

@chipitsine
Copy link
Contributor

it also makes sense to change behaviour of "Replace allow_password by a runtime check that enables password change menu only when the user has write-access to the key file"

(I never had a time for that, actually)

if someone running login/password (Active Directory, for instance) auth, there's still password change menu which ends with

  if ((!found_key) && (!found_pkcs12))
    {
      /* must have key or pkcs12 option */
      ShowLocalizedMsg(IDS_ERR_HAVE_KEY_OR_PKCS12);
      return(0);
    }

it makes sense to hide "password change menu" in such case, i.e. check if ((!found_key) && (!found_pkcs12)) before building menu list

@chipitsine
Copy link
Contributor

also, UI in "change password" is not very good.
people do not make sense what is it about.

@selvanair
Copy link
Collaborator Author

@chipitsine : If key or pkcs12 file is not found the menu will not be inserted and no error message will be shown. The code snippet you quoted is from the current master, not my patch. The original code is not capable of changing password in inline key/pkcs12 files and that limitation still holds.

Could you please test the patch and see whether it works? There is a binary at https://github.com/selvanair/openvpn-gui/releases/tag/v11.0-registry-test (this includes the proposed option editing dialogs as well).

You are right, the UI is confusing as it does not clarify this passphrase refers to that of the private key, not auth-user-pass. We could change the menu text.

@chipitsine
Copy link
Contributor

sure, I will test patch.

I vote for "protect private key with password" instead of "change password"

- Replace allow_password by a runtime check that enables password
  change menu only when the user has write-access to the key file.
- Read exe_path and priority from HKLM and do not duplicate in HKCU.
- Always allow the user to view the config: edit will succeed if user
  has write access.
- Always include the proxy settings tab which is the default.
- Remove the unused power event handling and disconnect_on_suspend key.
- Remove password_attempts -- user can stop the password dilaog
  by clicking cancel.
- Remove allow_service: implicitly enabled if service_only is used.
- Deprecate removed options in cmd-line parser
- Update README.rst
- Close config file before exit in GetKeyFileName
- Close thread and dialog handles in passphrase.c

Signed-off-by: Selva Nair <selva.nair@gmail.com>
- Use ShellExecute to open config and log files so that
  file associations can be used. If that fails fall-back to
  the default editor (notepad.exe).
- Remove the keys editor and log_viewer from registry.
  The user can change the editor/viewer through fie association.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
Option ediitng dialogs are in two tabs: General and Advanced.
Proxy related options are left in the proxy tab. Options
config_dir, config_ext, log_dir, script timeouts and
service-only flag are in the Advanced tab. All other more commonly
used flags and options are in the General tab.

- As options are editable, save values in registry only when they differ
  from the default values. This leaves the registry clean and makes changing
  options and their defaults during updates easier.

- Entries for config_dir and log_dir must be absolute paths.
  Environemental variables such as %PROFILEDIR% may be used
  to construct these.

- Empty config_dir, config_ext and log_dir entries are silently
  ignored (i.e., the current values are left unchanged).

- Store all numeric and boolean parameters in registry as DWORD instead of
  strings.

- On startup, the default parameters are loaded, then the registry is read
  and finally command-line parameters parsedi.

- Out of range script timeout values in registry truncated with a
  warning instead of fatal error. This allows the user to access the
  settings dialog and make corrections.

- Save proxy and language settings under the same
  HKCU\Software\OpenVPN-GUI key as other options instead of under Nilings.

- Save the current version of the GUI in regsitry so that updates
  can be detected and any needed registry cleanup done.

- If no version info is present in the registry any values in OpenVPN-GUI
  key in HKCU are deleted for a clean start as this is the first version
  to save registry values in HKCU. Language and proxy data if present
  under Nilings is migrated.

Note: new controls in the General tab and newly added Advanced tab dialog
are copied to all language files from the English version. These need to
be translated.

Signed-off-by: Selva Nair <selva.nair@gmail.com>
@selvanair
Copy link
Collaborator Author

Rebased to master and added a commit implementing dialogs for editing options saved in registry. Now only non-default values saved in the registry. (as discussed here [https://github.com//issues/61]).

@mattock
Copy link
Member

mattock commented Aug 16, 2016

@selvanair: is there an openvpn-gui.exe that includes your latest changes?

@selvanair
Copy link
Collaborator Author

@mattock: Yes, see here

@mattock
Copy link
Member

mattock commented Aug 16, 2016

@selvanair: great, I will test this tomorrow. I have some other Windows testing in the queue anyways.

@mattock
Copy link
Member

mattock commented Aug 17, 2016

It seems I had tested this earlier and had reported my findings in issue #61, so I only did light testing of openvpn-gui-pr64.exe. Based on that I can verify that the registry values are written only if they're modified by the user. Once a key is in the registry, it remains there, even if it's value is later reverted to the default by user. I think this behavior is reasonable, although have a "Reset to defaults" button might be useful to have, now that people actually can change the configuration easily.

As for "change password" - I did not see that menu item, but depending on the context chipsine's suggestion "protect private key with password" could work. Another option would be "change private key password".

@mattock
Copy link
Member

mattock commented Aug 17, 2016

Note to self: close https://community.openvpn.net/openvpn/ticket/494 once this PR has been merged.

@selvanair
Copy link
Collaborator Author

selvanair commented Aug 17, 2016

@mattock : thanks for testing.

  • For the "reverting to default" case I chose to keep the value in registry in such cases. The assumption is that any edits mean the user has a preference and even if future defaults changes it should be respected. But I'm open to change this to remove if reverted to default.
    A reset to defaults (except language and proxy?) button will indeed be useful. Will add it to my TODO.
  • For change password, its now shown only if password change is likely to succeed. Inline key files will not activate it as the current code is not capable of changing the password in such cases. It should show up if you use an external key file and the file is readable.
  • I vote for "Change Private Key Password". This could be handled in a separate patch along with translations..

Edit: s/readable/writeable/

@cron2
Copy link
Contributor

cron2 commented Aug 17, 2016

Hi,

On Wed, Aug 17, 2016 at 07:46:14AM -0700, Selva Nair wrote:

  • I vote for "Change Private Key Password". This could be handled in a separate patch along with translations..

+1 :)

gert

USENET is not the non-clickable part of WWW!
//www.muc.de/~gert/
Gert Doering - Munich, Germany gert@greenie.muc.de
fax: +49-89-35655025 gert@net.informatik.tu-muenchen.de

@mattock
Copy link
Member

mattock commented Aug 23, 2016

👍

@selvanair
Copy link
Collaborator Author

@mattock : Shall we "lazy-ack and merge" this? The proposed improvements such as menu for reverting options to default could be tackled in another PR.

@mattock
Copy link
Member

mattock commented Aug 23, 2016

Yes, makes sense. I'll merge it now.

@mattock mattock merged commit 2398e32 into OpenVPN:master Aug 23, 2016
@selvanair selvanair deleted the registry branch January 10, 2017 00:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants