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

Fix65 #349

Merged
merged 132 commits into from Mar 7, 2020
Merged

Fix65 #349

merged 132 commits into from Mar 7, 2020

Conversation

hanspr
Copy link
Contributor

@hanspr hanspr commented Jan 7, 2020

Support for KeePassXC #65 , #343

The functionality was complete rewritten to address several requests related in the same 2 issues:

  • Password no longer required in configuration file
    • If password is not provided, will ask for password on load and retain in memory until asbru exits.
  • User names and passwords are retrieved from database on the fly at the moment of connection
    • You can change your passwords in keepassxc and in the next connection the new one will be used
    • No passwords are retained in memory or files any more
    • User name and passwords are references now
    • url can be used as a reference to get the host to connect
  • Previous implementations would extract absolutely all passwords even when they were not related to any connection, pulling out any other information that a user could have stored like : bank accounts, nips, etc.
    • The full password list was located in memory in a log submenu. Difficult to follow.
    • Some entries could return more than one match and the user would be required to choose the correct one.
  • Button in Connection edit, allows to search entries and double-click to select desired entry. The correct format will be inserted in the corresponding fields for user,password or user key and passphrase. Depending on the selected authentication option
  • Right click on the hostname allows to select url from KeePassXC to use the reference in url as the hostname and retrieve at the time of connection.
  • KeepassXC entries can be added to Exec, Expect and Pre and Post Exec. With right mouse.

imagen

Search

  • You must type 3 characters or more to start a search

imagen

  • During the configuration the application will detect : Version available and capabilities detected on that current version.

imagen

Limitations found during integration:

  • The original proposal was to use field:uuid, keepassxc does not allow to retrieve the uuid, and last version of keepassxc-cli, no longer allow to search by uuid, so I had to drop it because is not reliable.
  • Make sure all titles in keepassxc are different when located in the same Path, to be able to associate to a unique key
  • Interactive command was removed in the most recent version of the CLI, so it can no longer be considered as an option

@hanspr
Copy link
Contributor Author

hanspr commented Jan 7, 2020

Sorry my bad, wrong command executed

@hanspr
Copy link
Contributor Author

hanspr commented Mar 1, 2020

@skipperTux

I never used to old KeePass integration

me either 😕 , never convinced me.

But thanks very much for the time you have put into this testing and your comments, It has helped a lot.

@skipperTux
Copy link

@skipperTux

Feature: When KeePassXC Search is confirmed and User/Password is set, also set Password Show to true. The field does not contain a secret anymore, but I can see it is populated by KeePassXC.

I do not understand this one, if you could help me with a set of images to follow the logic and understand the problem or feature request?

image

vs.

image

When the user selects KeePassXC value, and the Password field is cleared and populated with <password|/...>, set Show to true (once). I can see the field is populated with KeePassXC value, and if I want I can switch back to Show=False.

Well, thank you @hanspr @gfrenoy for your continuing work on asbru-cm!

@hanspr
Copy link
Contributor Author

hanspr commented Mar 1, 2020

So, if I understand you would like that Ásbru shows the extracted password value at the time of clicking into "Show", and not to show the mask.

Correct?

But the same concept could apply to username and host.

My premise was to show the mask used so you know you selected the correct one, and never extract the real values only at the time to connect.

This is also true for people that likes to use the tooltip option, it will not extract the user name or password. Mainly because it will be very slow to go one by one extracting the real values, and second, if some one is really that serious about hide information, will protect an unattended machine.

So, I understand the concept, and what is trying to achieve. Maybe I could do some mix of both, show the selected mask, and if show is selected, then show the user name or password in a tooltip when the user hovers over the text field or an icon. Will give it a thought to see how to achieve it.

@skipperTux
Copy link

So, if I understand you would like that Ásbru shows the extracted password value at the time of clicking into "Show", and not to show the mask.

Correct?

No, misunderstanding, do not show the extracted password. My feature idea is only about the Show checkbox.

Auto-activate (once) the Show checkbox when KeePassXC mask is choosen from KeePassXC Search.

IMHO it does not make sense to hide the mask <password|/...>, it is helpful to see this field is populated by KeePassXC by showing the mask, not asterisks.

@hanspr
Copy link
Contributor Author

hanspr commented Mar 1, 2020

Ok, got it, thanks

Well let me see, again it should be visible only if a keepassxc mask entry exists, because people could have mixed configurations.

@skipperTux
Copy link

skipperTux commented Mar 1, 2020

I would set Show to true once when the user searches the KeePassXC database, chooses an entry and the Password field is populated with a KeePassXC mask. Set Show to true when the KeePassXC module sets the value of the Password field with a mask. Only once this event occurs, not every time the dialog is shown and the mask entry exists. After that the user can either switch Show on or off just like in regular asbru-cm.

Just for convenience, maybe not worth the effort.

Edit:

  • Try to clarify with more details.

@hanspr
Copy link
Contributor Author

hanspr commented Mar 6, 2020

@gfrenoy

I have tested this option for several weeks, and it works, does not produce any conflicts if you do not use KeePas.

Unless there is an impediment yet or we plan to drop it, could we merge it?

@gfrenoy
Copy link
Contributor

gfrenoy commented Mar 7, 2020

Yes, let's merge this into loki so that it's easier to merge etc.

There are a few more suggestions in this thread we may want to implement ; if not yet done, I'll create dedicated issues to have them all under the 6.2 milestone.

In any case, thanks a lot for this major improvement !

@gfrenoy gfrenoy merged commit 2a5b0d0 into asbru-cm:loki Mar 7, 2020
@hanspr
Copy link
Contributor Author

hanspr commented Mar 7, 2020

All the issues in the comments were fixed, the suggestion of showing the password can be implemented if but not that I really think is necessary to do. And the only one related to keeping the database unlocked was already discussed. So all has been implemented.

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