Skip to content

Cloning from new host via ssh causes spurious error rather than prompting for confirmation and succeeding #1408

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions jupyterlab_git/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
from jupyter_server.services.contents.manager import ContentsManager
from jupyter_server.utils import ensure_async, url2path, url_path_join
from packaging.version import parse
from jupyter_server.auth.decorator import authorized

try:
import hybridcontents
Expand All @@ -24,10 +25,26 @@
from .git import DEFAULT_REMOTE_NAME, Git, RebaseAction
from .log import get_logger

from .ssh import SSH

# Git configuration options exposed through the REST API
ALLOWED_OPTIONS = ["user.name", "user.email"]
# REST API namespace
NAMESPACE = "/git"
# SSH Auth Resource to be authorized
SSH_AUTH_RESOURCE = "ssh"


class SSHHandler(APIHandler):
"""
Top-level parent class for SSH actions
"""

auth_resource = SSH_AUTH_RESOURCE

@property
def ssh(self) -> SSH:
return SSH()


class GitHandler(APIHandler):
Expand Down Expand Up @@ -1096,6 +1113,30 @@ async def get(self, path: str = ""):
self.finish(json.dumps(result))


class SshHostHandler(SSHHandler):
"""
Handler for checking if a host is known by SSH
"""

@authorized
@tornado.web.authenticated
async def get(self):
"""
GET request handler, check if the host is known by SSH
"""
hostname = self.get_query_argument("hostname")
is_known_host = self.ssh.is_known_host(hostname)
self.set_status(200)
self.finish(json.dumps(is_known_host))

@authorized
@tornado.web.authenticated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe worth adding authorized decorators to both methods just in case if operators wanted to block one of them?

from jupyter_server.auth.decorator import authorized

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much for your review. I added the decorator. I will be glad for any other review

async def post(self):
data = self.get_json_body()
hostname = data["hostname"]
self.ssh.add_host(hostname)


def setup_handlers(web_app):
"""
Setups all of the git command handlers.
Expand Down Expand Up @@ -1146,6 +1187,7 @@ def setup_handlers(web_app):
handlers = [
("/diffnotebook", GitDiffNotebookHandler),
("/settings", GitSettingsHandler),
("/known_hosts", SshHostHandler),
]

# add the baseurl to our paths
Expand Down
49 changes: 49 additions & 0 deletions jupyterlab_git/ssh.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
"""
Module for executing SSH commands
"""

import re
import subprocess
import shutil
from .log import get_logger
from pathlib import Path

GIT_SSH_HOST = re.compile(r"git@(.+):.+")


class SSH:
"""
A class to perform ssh actions
"""

def is_known_host(self, hostname):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be implemented in a "ask for forgiveness, not for permission" manner? That is, run clone, and if it fails on host not known, prompt user to add host?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be done as well, the issue for me doing this at first is to know if the error message remains consistent in all systems so I can check it on client then prompt user, hence I did added the check before attempting to clone. Should I go ahead and do the approach you suggested anyway?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know. It makes sense to me, don't know if it would make sense to the maintainers here.

Is it possible to somehow hook into the git/ssh/credential-helper stuff more tightly so that you aren't reduced to parsing user-facing messages on stdout to determine what's happening? If you'd have to do that, then current solution is way better.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reason for disliking the original implementation is that

The host is unknown, would you like to add it to the list of known hosts?

does not say what I'm actually adding to my known hosts by clicking the button. Also it does not reveal the file location being modified, which IMO there should be, somewhere, possibly in a smaller grayer font, like a postscript, something like that.

Also, I dislike that the fingerprint is examined twice, first during the initial check and then during actual connection attempt. I do not see how this could be turned into something like time-of-check/time-of-use vulnerability, maybe it cannot, but if there is another better way found, I would prefer that one.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does not say what I'm actually adding to my known hosts by clicking the button. Also it does not reveal the file location being modified, which IMO there should be, somewhere, possibly in a smaller grayer font, like a postscript, something like that.

Thanks for the suggestion, changing the dialog body would require us to create a form for the dialog. Let me know if that way is a feasible one.Or I could also emit a notification after the host is added:

image

Also, I dislike that the fingerprint is examined twice, first during the initial check and then during actual connection attempt. I do not see how this could be turned into something like time-of-check/time-of-use vulnerability, maybe it cannot, but if there is another better way found, I would prefer that one.

I could simply check the known_hosts file and see if a fingerprint is present there for a given host, this was my first try, but I want to avoid referent to this file due possible Windows incompatible, but I ended having to refer to this file directly anyway :/ Let me know if you that's a better approach and we can switch to it. Thanks!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The host %1 is not known

I love that, thanks!

"""
Check if the provided hostname is a known one
"""
cmd = ["ssh-keygen", "-F", hostname.strip()]
try:
subprocess.check_call(
cmd, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL
)
except Exception as e:
get_logger().debug("Error verifying host using ssh-keygen command")
return False
else:
return True

def add_host(self, hostname):
"""
Add the host to the known_hosts file
"""
get_logger().debug(f"adding host to the known hosts file {hostname}")
try:
result = subprocess.run(
["ssh-keyscan", hostname], capture_output=True, text=True, check=True
)
known_hosts_file = f"{Path.home()}/.ssh/known_hosts"
with open(known_hosts_file, "a") as f:
f.write(result.stdout)
get_logger().debug(f"Added {hostname} to known hosts.")
except Exception as e:
get_logger().error(f"Failed to add host: {e}.")
raise e
24 changes: 24 additions & 0 deletions src/cloneCommand.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,30 @@ export const gitCloneCommandPlugin: JupyterFrontEndPlugin<void> = {
const id = Notification.emit(trans.__('Cloning…'), 'in-progress', {
autoClose: false
});
const url = decodeURIComponent(result.value.url);
const hostnameMatch = url.match(/git@(.+):.+/);

if (hostnameMatch && hostnameMatch.length > 1) {
const hostname = hostnameMatch[1];
const isKnownHost = await gitModel.checkKnownHost(hostname);
if (!isKnownHost) {
const result = await showDialog({
title: trans.__('Unknown Host'),
body: trans.__(
'The host %1 is not known. Would you like to add it to the known_hosts file?',
hostname
),
buttons: [
Dialog.cancelButton({ label: trans.__('Cancel') }),
Dialog.okButton({ label: trans.__('OK') })
]
});
if (result.button.accept) {
await gitModel.addHostToKnownList(hostname);
}
}
}

try {
const details = await showGitOperationDialog<IGitCloneArgs>(
gitModel as GitExtension,
Expand Down
44 changes: 44 additions & 0 deletions src/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1042,7 +1042,7 @@
follow_path: this.selectedHistoryFile?.to
}
);
} catch (error) {

Check warning on line 1045 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.13

'error' is defined but never used

Check warning on line 1045 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.10

'error' is defined but never used

Check warning on line 1045 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.9

'error' is defined but never used
return { code: 1 };
}
}
Expand Down Expand Up @@ -2007,11 +2007,55 @@

const newSubmodules = data.submodules;
this._submodules = newSubmodules;
} catch (error) {

Check warning on line 2010 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.13

'error' is defined but never used

Check warning on line 2010 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.10

'error' is defined but never used

Check warning on line 2010 in src/model.ts

View workflow job for this annotation

GitHub Actions / Test Python 3.9

'error' is defined but never used
console.error('Failed to retrieve submodules');
}
}

/**
* Checks if the hostname is a known host
*
* @param hostname - the host name to be checked
* @returns A boolean indicating that the host is a known one
*
* @throws {ServerConnection.NetworkError} If the request cannot be made
*/
async checkKnownHost(hostname: string): Promise<boolean> {
try {
return await this._taskHandler.execute<boolean>(
'git:checkHost',
async () => {
return await requestAPI<boolean>(
`known_hosts?hostname=${hostname}`,
'GET'
);
}
);
} catch (error) {
console.error('Failed to check host: ' + error);
// just ignore the host check
return true;
}
}

/**
* Adds a hostname to the list of known host files
* @param hostname - the hostname to be added
* @throws {ServerConnection.NetworkError} If the request cannot be made
*/
async addHostToKnownList(hostname: string): Promise<void> {
try {
await this._taskHandler.execute<boolean>('git:addHost', async () => {
return await requestAPI<boolean>('known_hosts', 'POST', {
hostname: hostname
});
});
} catch (error) {
console.error('Failed to add hostname to the list of known hosts');
console.debug(error);
}
}

/**
* Make request for a list of all git branches in the repository
* Retrieve a list of repository branches.
Expand Down
17 changes: 17 additions & 0 deletions src/tokens.ts
Original file line number Diff line number Diff line change
Expand Up @@ -628,6 +628,23 @@ export interface IGitExtension extends IDisposable {
*/
revertCommit(message: string, hash: string): Promise<void>;

/**
* Checks if the hostname is a known host
*
* @param hostname - the host name to be checked
* @returns A boolean indicating that the host is a known one
*
* @throws {ServerConnection.NetworkError} If the request cannot be made
*/
checkKnownHost(hostname: string): Promise<boolean>;

/**
* Adds a hostname to the list of known host files
* @param hostname - the hostname to be added
* @throws {ServerConnection.NetworkError} If the request cannot be made
*/
addHostToKnownList(hostname: string): Promise<void>;

/**
* Get the prefix path of a directory 'path',
* with respect to the root directory of repository
Expand Down
Loading