Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.
Bug 316411 - SSH connection should ask for password if non was entered
Summary: SSH connection should ask for password if non was entered
Status: RESOLVED FIXED
Alias: None
Product: PTP
Classification: Tools
Component: Remote Tools (show other bugs)
Version: 4.0   Edit
Hardware: PC Windows 7
: P3 enhancement (vote)
Target Milestone: 4.0.2   Edit
Assignee: Greg Watson CLA
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-06-09 23:49 EDT by Roland Schulz CLA
Modified: 2010-09-10 18:18 EDT (History)
3 users (show)

See Also:


Attachments
New internal.ssh project (moved from core) (131.89 KB, application/x-zip-compressed)
2010-08-06 10:39 EDT, Roland Schulz CLA
no flags Details
Patch to move internal.ssh (225.17 KB, patch)
2010-08-06 10:47 EDT, Roland Schulz CLA
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Roland Schulz CLA 2010-06-09 23:49:43 EDT
org.eclipse.ptp.remotetools.environment.control.SSHTargetControl.connect(IProgressMonitor) should ask for the password if the password provided by the configuration is empty. 

This will allow the user to not enter in the configuration and instead enter it every time a connection is started. This is helpful if
- the user does not want the password be saved for security, or
- the password is a one-time password

Also if the password was originally entered incorrect, the user should be asked to enter a new password (which is then saved) or should be able to edit the connection. Currently she has to create a new configuration.

Because SSHTargetControl is not in the UI package, I'm not sure how to get the shell to be able to display a dialog. Also it is probably not good to directly show a dialog from a non-UI package. How should SSHTargetControl the UI notify that the UI should ask for the password?
Comment 1 Greg Watson CLA 2010-06-11 13:42:13 EDT
Connections can be edited through the "Remote Environments" view. Bug 252075 addresses the need to be able to do this from other places.
Comment 2 Greg Watson CLA 2010-06-14 11:31:08 EDT
(In reply to comment #0)
> 
> Because SSHTargetControl is not in the UI package, I'm not sure how to get the
> shell to be able to display a dialog. Also it is probably not good to directly
> show a dialog from a non-UI package. How should SSHTargetControl the UI notify
> that the UI should ask for the password?

The place to do this is the SSHUserInfo class in org.eclipse.ptp.remotetools.internal.ssh.Connection, but this is not a UI plugin either. I would suggest that the package org.eclipse.ptp.remotetools.internal.ssh be moved to its own plugin of the same name. Connection#SSHUserInfo could then be modified along the lines of org.eclipse.rse.internal.connectorservice.ssh.SshConnectorService#MyUserInfo to prompt the user if the password is empty or incorrect.
Comment 3 Roland Schulz CLA 2010-07-22 18:32:49 EDT
Assigned to Mayur.
Comment 4 Roland Schulz CLA 2010-08-06 10:39:12 EDT
Created attachment 176041 [details]
New internal.ssh project (moved from core)
Comment 5 Roland Schulz CLA 2010-08-06 10:47:01 EDT
Created attachment 176042 [details]
Patch to move internal.ssh

The patch + the zip for the new project together move the internal.ssh package into its own project. 

It is a little bit more tricky than just moving the package. There a quite a few interdependencies. I solved this by using Interfaces everywhere possible which are defined in core. The implementation is in internal.ssh. This makes internal.ssh depend on core but core not depend on internal.ssh.

@Mayur: You had dependencies in both directions in your earlier version and thus had a cyclic dependencies. This is why Eclipse wasn't able to build it.

For the interfaces to work everywhere I had to add an additional interface IKillableExecution and had to add methods to IRemoteExecution and IRemoteExecutionManager.

@Greg: Are you OK with having those methods in the interfaces are do you have an alternative idea which doesn't require adding those methods to these 3 interfaces?

Because I would like first Greg's opinion the patch/zip isn't cleaned up yet (e.g. no javadoc).
Comment 6 Greg Watson CLA 2010-08-17 17:29:29 EDT
Sorry about this, but I now think my suggestion was a dumb idea that is unnecessarily complex.

I think a better approach would be to:

1. Create a new interface, say IRemoteUserInfo in org.eclipse.ptp.remotetools.core. This interface would provide the same methods as UserInfo and UIKeyboardInteractive, but not extend them. 
2. Add this interface as the first argument to ITargetControl#create, PTPTargetControl#create, SSHTargetControl#create, SSHTargetControl#connect, and Connection#connect. 
3. Move PTPTargetControl to the remote.remotetools.ui package and provide an implementation of IRemoteUserInfo so that it displays a dialog. 
4. Copy the Connection#SSHUserInfo class and provide an implementation of IRemoteUserInfo in SSHTargetControl to use as the default. This doesn't use any UI so it can stay in core.
5. Connection#connect should wrap the IRemoteUserInfo in a class that extends UserInfo and UIKeyboardInteractive (the existing SSHUserInfo class would be suitable) and delegate the methods to the IRemoteUserInfo interfaces.

Hopefully this should create minimal disruption.

Thanks!
Comment 7 Greg Watson CLA 2010-09-09 14:48:29 EDT
Working on a fix for this. Should have a patch soon...
Comment 8 Greg Watson CLA 2010-09-10 10:59:28 EDT
Committed to HEAD. It is difficult to test all possible scenarios, so help would be appreciated.

Some things that should be tested:

1. Specifying different authentication mechanisms and ordering in the sshd_config.

2. Different combinations of empty/incorrect passwords and invalid usernames.

3. Using passphrases.

Also there are two problems I'm still working on:

1. Saving password when it is entered in the dialog. See bug 3249789.

2. Shutting down the resource manager if the remote tools connection is closed (this never worked). See bug 324979.