Finishing Touches? - Githubissues

michaeljs1990 commented 6 years ago

Hey sorry to open a ticket for this. I have always felt github doesn't have a great way to communicate with maintains when it's not about a bug. The README.md for this project says that this is still a work in progress. I have hit maybe one minor issue using this before but it turned out to be a bad configuration/error message rather than any issue with the software. What would you like to see done or have plans to do before this is no longer in a "work in progress" stage?

Thanks,

raoulh commented 6 years ago

Actually as I didn't have so much feedback from anyone about the ssh agent, I was thinking that I was the only one to really use it!

If it's ok for @limpkin, we can enable the SSH tab for good in moolticute and remove the "work in progress" text from the readme.

limpkin commented 6 years ago

Let me do some bug testing first to see if I still get the same bug as before. We will also need to update our user manual before releasing.

On Mon, May 7, 2018, 09:00 raoulh notifications@github.com wrote:

Actually as I didn't have so much feedback from anyone about the ssh agent, I was thinking that I was the only one to really use it!

If it's ok for @limpkin https://github.com/limpkin, we can enable the SSH tab for good in moolticute and remove the "work in progress" text from the readme.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/raoulh/mc-agent/issues/11#issuecomment-386975914, or mute the thread https://github.com/notifications/unsubscribe-auth/AF4msxxU8YftyB-A5ZxWR8ueOiRF89L2ks5tv_EDgaJpZM4T0RsL .

limpkin commented 6 years ago

my very first observation:

when mc-agent is not running and the ssh tab is enabled, clicking the unlock button leads to a "wait for device confirmation" menu.

I'm not sure if it is due to the mc-agent not running or the fact that I don't have the right cred in my device... but we need to inform the user (aka me) and make sure mc-agent is running.

raoulh commented 6 years ago

no. it does not work like that. The mc-agent should not be running when using the gui. it can, but it's not required. The agent is used for other apps like ssh client (putty on windows) can connect and use the keys.

raoulh commented 6 years ago

But it is indeed not good to have a "wait for device confirm" that does nothing else... I will investigate this

raoulh commented 6 years ago

@limpkin fixed in https://github.com/mooltipass/moolticute/commit/56a60ddab111ec346095a91c85c723076dca587b

limpkin commented 6 years ago

Nice! Next points:

why is it needed to "unlock"? i don't understand the point
after clicking on unlock, clicking on "import a key" and selecting a random file, i have an endless progress bar

michaeljs1990 commented 6 years ago

Thanks for the work on this! Out of curiosity you said that mc-agent should not be running when the GUI is. Will the gui now handle starting that the mc-agent by itself or how exactly will that work (sorry for the ignorance). Currently I am just running the mc-agent daemon along side the GUI.

limpkin commented 6 years ago

@michaeljs1990 i'd like to know this as well ;), there are no bad questions!

raoulh commented 6 years ago

It works like that:

mc-agent should run in the background to accept other ssh tools requests for signing. mc-agent can be started by hand yourself, or automatically executed by the GUI (there is a checkbox for that in the option tab). This background mode is only for handling of ssh signing requests.
When unlocking the SSH in the gui it starts a standalone mc-agent to do the work on keys: listing/adding/removing/exporting. This mode is started automatically by the GUI and can be running alongside the normal mc-agent background mode.

The need for unlocking is because mc-agent has to load the key listing from the device (and prompt the user). Of course if there is no keys, you would not have any prompt on the device, because the service does not exists (yet).

limpkin commented 6 years ago

let's therefore rename "unlock" to "manage SSH keys" then?
I'm guessing I have the endless loading bar because it's not a valid SSH key? is there any validation done in the backend?

raoulh commented 6 years ago

renaming: ok
The endless bar is a bug. That should not happen, even if the key is bad it should pop up a message box with the error. The backend is not moolticute, but mc-agent. Let me try to some testing...

raoulh commented 6 years ago

I did some test here, and cleared my device first. It works as it should...

limpkin commented 6 years ago

actually, what's the point of the unlock button in the first place? not keep the ssh keys in memory for too long?
works as it should when you import a non-ssh key file?

this is my log:

`DEBUG: WSServerCon.cpp:49 - JSON API recv: { "data": { "service": "moolticute ssh keys" }, "msg": "data_node_exists" }

INFO: AsyncJobs.cpp:98 - "Check if data service exists: moolticute ssh keys reqid: " INFO: MPDevice.cpp:6324 - service_exists success DEBUG: WSClient.cpp:135 - New message: {"data":{"exists":false,"service":"moolticute ssh keys"},"msg":"data_node_exists"} INFO: SSHManagement.cpp:278 - Running "C:/temp/build-Moolticute-Desktop_Qt_5_9_2_MinGW_32bit-Debug/debug/mc-agent" ("--output_progress", "cli", "-c", "add", "--key", "C:/temp/build-Moolticute-Desktop_Qt_5_9_2_MinGW_32bit-Debug/.qmake.stash") DEBUG: WSServer.cpp:81 - Connection closed WSServerCon(0x20e70ec0) DEBUG: WSServerCon.cpp:926 - Send card db metadata DEBUG: WSServerCon.cpp:950 - Sended card db metadata DEBUG: WSServer.cpp:73 - New connection DEBUG: WSServerCon.cpp:49 - JSON API recv: {"msg":"get_application_id"}`

raoulh commented 6 years ago

Running from qtcreator is not working, the mc-agent binary is not in your path.... Use a release.
unlock: loading the keys from the device so that they can be displayed on the UI

limpkin commented 6 years ago

I see!

so the button should actually be named "load ssh keys" then
I'm getting a major slow down for at least 10s when clicking on the import file button to bring up the select file dialog (tried it three times):
file select dialog: should we be strict to restrict to .key files?
what kind of keys are allowed? rsa / dsa / ecdsa / ed.... I'm asking so I can test using some generated by puttygen

raoulh commented 6 years ago

Ok for renaming
Slow down: this is related to your windows installation/your PC, and there is nothing we can do about it.
.key: no, there is no standard extension for ssh keys. .key is mostly a windows thing. On linux and macos keys are named id_rsa and such
All keys types are supported: rsa, dsa (not recommended but works), ecdsa and ed25519.

limpkin commented 6 years ago

Then for some reason i wasn't able to import a priv key generated by puttygen:

Running "C:/Users/XXXX/AppData/Local/Programs/Moolticute/mc-agent" ("--output_progress", "cli", "-c", "add", "--key", "C:/Users/XXXX/temp/temppriv.ppk")

limpkin commented 6 years ago

@michaeljs1990 : could you check for the slow down I mentioned on windows using https://mooltipass-tests.com/mc_betas/v0.17.2-testing/ ?

raoulh commented 6 years ago

@limpkin You need openssh keys, not putty keys. Those are not supported.

As for the slowdown of the open dialog, it's windows based really. The file dialog is a standard windows file dialog and to populate it, windows blocks the UI for refreshing whatever it needs....

limpkin commented 6 years ago

@raoulh anyway to check for these in the daemon to prevent users from doing the same mistake as me? As for the windows dialog, i don't get the same behavior for all the other prompts we have

raoulh commented 6 years ago

Again: We can't do anything for the file dialog.

limpkin commented 6 years ago

never said you could, just want to check if this dialog-specific dialog slow down is the same for @michaeljs1990 .

raoulh commented 6 years ago

What you don't understand is that the file dialog is standard native windows stuff, we have no control on that. If you have a slow network share for example, the dialog will try to enumerate it somehow and it will slow down and even block the dialog. Also it's only on windows that stuff like that happens. If @michaeljs1990 is running an other OS he will not have those problems.

limpkin commented 6 years ago

I completely understand it, just wanted to double check with OP (I did mention windows) as it doesn't hurt. Anyway, is there any way to check for puttygen files to inform users that they should use openssh instead?

raoulh commented 6 years ago

You can read putty file and try to decode them. But I will not do it, too much work for just telling the user the format is unsupported. Not worth it.

limpkin commented 6 years ago

I can understand.

limpkin commented 6 years ago

Here are required actions on my side to allow the ssh keys tab to be displayed by default on MC, as I'll be one of the few directly dealing with customers and doing the support. In the SSH keys main tab:

change "SSH Keys management" to "SSH Keys Management"
change "ssh is locked." to "SSH Keys Not Loaded"
change the text below to: "Press the load keys button to load SSH keys from your device"
change "unlock" button to "Load Keys" Once the button is pressed:
layout: move all the buttons to the right except "quit ssh management"
change "Quit SSH Management" to "Leave SSH Keys Management", center the button under everything
below "SSH Keys Management", after the separator, add the following text: "Only passphrase-free OpenSSH keys are supported"
title all buttons inner texts (first letter of each word in upper case)
disable "Export selected" button when no key is selected (make them grey)
same for "Delete selected"
(I'm sorry but I'm not going to over estimate our customers) check for puttygen files when clicking on import
adding a new ssh key, when no "moolticute ssh keys" service is present, and approving both prompts: the key is not listed
leaving ssh management, re-entering: i get the key, with only "RSA - " written. This key was exported using puttygen... I'm guessing the name of the key is within the export file?
deleting a key: why do I need to approve 2 prompts? (one send data and one update data)
approving the 2 prompts for key deletion: the key is still listed. leaving management & reintering i can confirm the key isn't here anymore

These are first action points but will add more in the future as i'll play with MC & ssh keys.

michaeljs1990 commented 6 years ago

I can try and get a windows VM spun up but unfortunately I have no windows computers at my house or at work currently :|

netromdk commented 6 years ago

@limpkin I'm working on the changes you requested.

The "Leave SSH Keys Management" button cannot be centered as-is right now because there's a progress bar to the right of it. Leaving it like that for now.

limpkin commented 6 years ago

:(

netromdk commented 6 years ago

@limpkin the double approving is due to the fact that mc-agent will first get the keys, remove the one in question, and then upload the changed data. I'm not sure if or how it should be changed for the better. You need access to read the data - that makes sense. And for uploading data - makes sense too. If it should be done in one action then perhaps mc-agent should keep all private keys so it can change and push as needed? But I'm not sure that's a good idea either.

In any case, I've made changes given your bullet points and will create a PR for MC.

asashnov commented 5 years ago

@netromdk did a great job in https://github.com/mooltipass/moolticute/pull/304

Every point in https://github.com/raoulh/mc-agent/issues/11#issuecomment-387362273 seems to be fixed now.

"Leave SSH Keys Management" button is on the right now with the other buttons (previously it was at the bottom below SSH keys list) so it looks good.

@limpkin wrote:

leaving ssh management, re-entering: i get the key, with only "RSA - " written. This key was exported using puttygen... I'm guessing the name of the key is within the export file?

Need to instruct a user in "Putty Key Generator" to use "File -> Load private key", then "Conversions -> Export OpenSSH key (force new file format)". In this case RSA key comment will be preserved and displayed after a dash ('-') in Moolticute SSH Keys list. My suggestion to add this text in the messagebox we show when a user choose Putty key.

@limpkin wrote:

deleting a key: why do I need to approve 2 prompts? (one send data and one update data)

Moolticute SSH Keys tab is just a wrapper around subsequent independent from each other mc-agent runs. When "Load Keys" clicked Moolticute runs new programm "mc-agent cli -c list". It asks daemon to get "moolticute ssh keys" file, mc-agent parses it and outputs the list of keys on its stdout, than exits.

When RSA key added or deleted, Moolticute SSH Keys tab runs:

mc-agent cli -c add --key /home/alex/.ssh/id5nopass_rsa
or
mc-agent cli -c delete --num=0

This new instance of mc-agent have no assumption about the previous state, so it reads "moolticute ssh keys" file from the device first, then modify it in memory and sends back.

The only way to avoid re-asking of sending "moolticute ssh keys" again is to cache it for some time (let's say, 1 min) in moolticute daemon's memory so it can send it via the socket without asking a user again.

But I think it is not possible because of security concern - nothing should be read without a confirmation.

raoulh / mc-agent

Finishing Touches? #11