Dissociate SSH key import from form submission #1895

ogayot · 2024-01-25T17:24:52Z

I started this work a year ago and never got a chance to finish it.
I spent some time today to clean it up and fix the tets. It looks correct now.

It brings several benefits over the existing implementation:

the user has the ability to import multiple keys
if running a partially automated installation, submitting the SSH form does not discard existing SSH keys that were imported using ssh: authorized-keys: directives.
the user has the ability to review the list of keys and remove any from the list

ogayot · 2024-01-25T17:58:24Z

Pushed a change to fix the CI issue on focal:
https://paste.ubuntu.com/p/mvgBKkHs2g/

EDIT: dropped this change after rebasing on main since we removed focal test enforcement.

mwhudson

I was sure I reviewed this last week but apparently not! I'm super happy to see this, and pleasantly surprised that it can all be done client side.

I have a few quibbles about widget enabling/disabling and a few pre-existing oddities I spotted when reviewing this. The former should probably be fixed (sorry, it'll be annoying) but feel free to ignore the latter for now.

mwhudson · 2024-01-28T19:55:18Z

subiquity/client/controllers/ssh.py

+                else:
+                    if isinstance(confirm_overlay, ConfirmSSHKeys):
+                        break
+                await asyncio.sleep(0.1)


Heh this is pretty gross. But well, all honest answers code is a bit gross.

It's a bit bikesheddy, but do we need the .1 portion of the sleep or are we just trying to allow other tasks to run? could asyncio.sleep(0) if it's just about the other tasks.

Agreed.. I'll move this to a function in view_helpers which will wait for an overlay to show up. I think that should make the answers code easier to write/read.

Well, asyncio.sleep(0) will cause high CPU load while fetching a key over the network. Maybe 0.01 if we want to speed things up ? @dbungert wdyt?

subiquity/client/controllers/ssh.py

mwhudson · 2024-01-28T19:59:24Z

subiquity/client/controllers/ssh.py

-                    id_.to_authorized_key() for id_ in identities
-                ]
-                self.done(ssh_data)
+            if isinstance(self.ui.body, SSHView):


Again, it's not your fault but we do we only do this check in the success case?

Super nice to get answers-handling code out of the "production" workflow though.

Again, it's not your fault but we do we only do this check in the success case?

Actually, no, we have the same check for IMPORT_ERROR and FINGERPRINT_ERROR:

subiquity/subiquity/client/controllers/ssh.py

Lines 77 to 81 in 3964014

if isinstance(self.ui.body, SSHView):

self.ui.body.fetching_ssh_keys_failed(

_("Importing keys failed:"), response.error

)

return

subiquity/subiquity/client/controllers/ssh.py

Lines 83 to 88 in 3964014

if isinstance(self.ui.body, SSHView):

self.ui.body.fetching_ssh_keys_failed(

_("ssh-keygen failed to show fingerprint of downloaded keys:"),

response.error,

)

return

subiquity/ui/views/ssh.py

Previously, on the SSH screen, the ability to enable/disable the SSH server and the ability to import a SSH identity were both covered by a single form. Therefore, there was no way to import multiple identities. This change adds a button "Import SSH key" which opens a new form to import an identity. The button can be pressed multiple times and the resulting identities are all submitted when the user clicks on Done. Furthermore, navigating back to the SSH screen does not "forget" already imported identities. Signed-off-by: Olivier Gayot <[email protected]>

When running answers-based automation, the SSH controller looks into more than one section to find ssh-import-id directives. If the "SSH" section exists, then it is where the ssh-import-id directives must be placed. However, if the section does not exist, the controller will also look for ssh-import-id directives in the "Identity" section. The answers.yaml file used this special mechanism. This is fine. However, if one adds a SSH section to customize other settings (e.g., install_server, pwauth), then the ssh-import-id directives in the Identity section suddently get ignored ; which is confusing and looks as if there is a bug. Let's move ssh-import-id directives to the SSH section. Signed-off-by: Olivier Gayot <[email protected]>

A new table shows all the SSH identities/keys that are currently imported. The user can select one and delete it from the list if he wants to. Signed-off-by: Olivier Gayot <[email protected]>

When selecting a SSH identity, the user can open a dialog showing the contents of the key. Signed-off-by: Olivier Gayot <[email protected]>

Signed-off-by: Olivier Gayot <[email protected]>

ogayot requested a review from mwhudson January 25, 2024 17:25

ogayot force-pushed the pr/ssh-dissociate-form-submission branch from 2dd26a7 to 1f47914 Compare January 26, 2024 08:15

mwhudson requested changes Jan 28, 2024

View reviewed changes

ogayot force-pushed the pr/ssh-dissociate-form-submission branch from 1f47914 to f60679e Compare February 8, 2024 14:49

ogayot requested a review from mwhudson February 8, 2024 14:51

ogayot force-pushed the pr/ssh-dissociate-form-submission branch from f60679e to 2465373 Compare February 8, 2024 16:54

mwhudson approved these changes Feb 8, 2024

View reviewed changes

ogayot force-pushed the pr/ssh-dissociate-form-submission branch from 2465373 to af5fb7f Compare February 12, 2024 08:10

ogayot added 7 commits February 12, 2024 13:59

ssh: add identity table with ability to remove key

feaf060

A new table shows all the SSH identities/keys that are currently imported. The user can select one and delete it from the list if he wants to. Signed-off-by: Olivier Gayot <[email protected]>

ssh: have a button to show a key

f5ecece

When selecting a SSH identity, the user can open a dialog showing the contents of the key. Signed-off-by: Olivier Gayot <[email protected]>

ssh: drop redundant use of context when importing key

0f3c53c

Signed-off-by: Olivier Gayot <[email protected]>

view-helpers: add function waiting for an overlay to be displayed

5861968

Signed-off-by: Olivier Gayot <[email protected]>

ssh: use view-helpers for detecting the confirmation key overlay

3ac1810

Signed-off-by: Olivier Gayot <[email protected]>

ssh: call GitHub / Launchpad service, not ssh_import_id

3f3bb29

Signed-off-by: Olivier Gayot <[email protected]>

ogayot force-pushed the pr/ssh-dissociate-form-submission branch from af5fb7f to 3f3bb29 Compare February 12, 2024 13:09

ogayot merged commit ce938e6 into canonical:main Feb 13, 2024
12 checks passed

ogayot deleted the pr/ssh-dissociate-form-submission branch February 13, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dissociate SSH key import from form submission #1895

Dissociate SSH key import from form submission #1895

ogayot commented Jan 25, 2024 •

edited

Loading

ogayot commented Jan 25, 2024 •

edited

Loading

mwhudson left a comment

mwhudson Jan 28, 2024

dbungert Feb 8, 2024

ogayot Feb 8, 2024

ogayot Feb 8, 2024

dbungert Feb 8, 2024

mwhudson Jan 28, 2024

mwhudson Jan 28, 2024

ogayot Feb 8, 2024

	if isinstance(self.ui.body, SSHView):
	self.ui.body.fetching_ssh_keys_failed(
	_("Importing keys failed:"), response.error
	)
	return

	if isinstance(self.ui.body, SSHView):
	self.ui.body.fetching_ssh_keys_failed(
	_("ssh-keygen failed to show fingerprint of downloaded keys:"),
	response.error,
	)
	return

Dissociate SSH key import from form submission #1895

Dissociate SSH key import from form submission #1895

Conversation

ogayot commented Jan 25, 2024 • edited Loading

ogayot commented Jan 25, 2024 • edited Loading

mwhudson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogayot commented Jan 25, 2024 •

edited

Loading

ogayot commented Jan 25, 2024 •

edited

Loading