{"thread":{"id":"15231","subject":"[JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed","startedAt":"2008-08-27T23:02:05Z","lastAt":"2008-08-28T00:24:06Z","messageCount":5,"participants":["Shawn O. Pearce","Marek Zawirski"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"88808","messageId":"1219878126-18622-1-git-send-email-spearce@spearce.org","threadId":"15231","inReplyTo":null,"subject":"[JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-27T23:02:05Z","receivedAt":"2008-08-27T23:02:05Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"During SSH startup we read all keys in the user's ~/.ssh, even\nif we may not need them for this particular transport session.\n\nIf a file is not really a key, or it contains a key that JSch\ndoesn't recognize we shouldn't crash the transport.  Instead\nwe should skip the file and move on.  Later on we just don't\nhave that identity available to us, or we'll crash if we try\nto add that identity file explicitly from ~/.ssh/config.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../jgit/transport/DefaultSshSessionFactory.java   |   13 +++++++++++--\n 1 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\nindex a2437c2..aa72357 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n@@ -165,14 +165,23 @@ private void identities() throws JSchException {\n \t\t\tfinal File k = new File(sshdir, n.substring(0, n.length() - 4));\n \t\t\tif (!k.isFile())\n \t\t\t\tcontinue;\n-\t\t\taddIdentity(k);\n+\n+\t\t\ttry {\n+\t\t\t\taddIdentity(k);\n+\t\t\t} catch (JSchException e) {\n+\t\t\t\tif (e.getMessage().startsWith(\"invalid privatekey: \"))\n+\t\t\t\t\tcontinue;\n+\t\t\t\tthrow e;\n+\t\t\t}\n \t\t}\n \t}\n \n \tprivate void addIdentity(final File identityFile) throws JSchException {\n \t\tfinal String path = identityFile.getAbsolutePath();\n-\t\tif (loadedIdentities.add(path))\n+\t\tif (!loadedIdentities.contains(path)) {\n \t\t\tuserJSch.addIdentity(path);\n+\t\t\tloadedIdentities.add(path);\n+\t\t}\n \t}\n \n \tprivate static class AWT_UserInfo implements UserInfo,\n-- \n1.6.0.174.gd789c\n"},{"id":"88807","messageId":"1219878126-18622-2-git-send-email-spearce@spearce.org","threadId":"15231","inReplyTo":"1219878126-18622-1-git-send-email-spearce@spearce.org","subject":"[JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-27T23:02:06Z","receivedAt":"2008-08-27T23:02:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"If we don't close the transport when we are done with it\nthe SSH session stored within the Transport will still be\nopened to the remote side.  JSch may have created one or\nmore background user threads to handle that connection,\nwhich means the JVM won't terminate cleanly when we are\ndone with our work.\n\nWe must close each and every transport we opened.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n .../src/org/spearce/jgit/pgm/Push.java             |   24 ++++++++++++-------\n 1 files changed, 15 insertions(+), 9 deletions(-)\n\ndiff --git a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java\nindex f53f2fe..53ad080 100644\n--- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java\n+++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java\n@@ -50,6 +50,7 @@\n import org.spearce.jgit.transport.RefSpec;\n import org.spearce.jgit.transport.RemoteRefUpdate;\n import org.spearce.jgit.transport.Transport;\n+import org.spearce.jgit.transport.URIish;\n import org.spearce.jgit.transport.RemoteRefUpdate.Status;\n \n @Command(common = true, usage = \"Update remote repository from local refs\")\n@@ -111,13 +112,18 @@ protected void run() throws Exception {\n \t\t\tfinal Collection<RemoteRefUpdate> toPush = transport\n \t\t\t\t\t.findRemoteRefUpdatesFor(refSpecs);\n \n-\t\t\tfinal PushResult result = transport.push(new TextProgressMonitor(),\n-\t\t\t\t\ttoPush);\n-\t\t\tprintPushResult(transport, result);\n+\t\t\tfinal URIish uri = transport.getURI();\n+\t\t\tfinal PushResult result;\n+\t\t\ttry {\n+\t\t\t\tresult = transport.push(new TextProgressMonitor(), toPush);\n+\t\t\t} finally {\n+\t\t\t\ttransport.close();\n+\t\t\t}\n+\t\t\tprintPushResult(uri, result);\n \t\t}\n \t}\n \n-\tprivate void printPushResult(final Transport transport,\n+\tprivate void printPushResult(final URIish uri,\n \t\t\tfinal PushResult result) {\n \t\tshownURI = false;\n \t\tboolean everythingUpToDate = true;\n@@ -126,7 +132,7 @@ private void printPushResult(final Transport transport,\n \t\tfor (final RemoteRefUpdate rru : result.getRemoteUpdates()) {\n \t\t\tif (rru.getStatus() == Status.UP_TO_DATE) {\n \t\t\t\tif (verbose)\n-\t\t\t\t\tprintRefUpdateResult(transport, result, rru);\n+\t\t\t\t\tprintRefUpdateResult(uri, result, rru);\n \t\t\t} else\n \t\t\t\teverythingUpToDate = false;\n \t\t}\n@@ -134,25 +140,25 @@ private void printPushResult(final Transport transport,\n \t\tfor (final RemoteRefUpdate rru : result.getRemoteUpdates()) {\n \t\t\t// ...then successful updates...\n \t\t\tif (rru.getStatus() == Status.OK)\n-\t\t\t\tprintRefUpdateResult(transport, result, rru);\n+\t\t\t\tprintRefUpdateResult(uri, result, rru);\n \t\t}\n \n \t\tfor (final RemoteRefUpdate rru : result.getRemoteUpdates()) {\n \t\t\t// ...finally, others (problematic)\n \t\t\tif (rru.getStatus() != Status.OK\n \t\t\t\t\t&& rru.getStatus() != Status.UP_TO_DATE)\n-\t\t\t\tprintRefUpdateResult(transport, result, rru);\n+\t\t\t\tprintRefUpdateResult(uri, result, rru);\n \t\t}\n \n \t\tif (everythingUpToDate)\n \t\t\tout.println(\"Everything up-to-date\");\n \t}\n \n-\tprivate void printRefUpdateResult(final Transport transport,\n+\tprivate void printRefUpdateResult(final URIish uri,\n \t\t\tfinal PushResult result, final RemoteRefUpdate rru) {\n \t\tif (!shownURI) {\n \t\t\tshownURI = true;\n-\t\t\tout.format(\"To %s\\n\", transport.getURI());\n+\t\t\tout.format(\"To %s\\n\", uri);\n \t\t}\n \n \t\tfinal String remoteName = rru.getRemoteName();\n-- \n1.6.0.174.gd789c\n"},{"id":"88814","messageId":"48B5E2A1.3030007@gmail.com","threadId":"15231","inReplyTo":"1219878126-18622-1-git-send-email-spearce@spearce.org","subject":"Re: [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities","fromName":"Marek Zawirski","fromEmail":"marek.zawirski@gmail.com","sentAt":"2008-08-27T23:26:25Z","receivedAt":"2008-08-27T23:26:25Z","isPatch":true,"sender":{"key":"marek.zawirski@gmail.com","avatar":null},"body":"Shawn O. Pearce wrote:\n> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n(...)\n> +\t\t\ttry {\n> +\t\t\t\taddIdentity(k);\n> +\t\t\t} catch (JSchException e) {\n> +\t\t\t\tif (e.getMessage().startsWith(\"invalid privatekey: \"))\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\tthrow e;\n> +\t\t\t}\n\nThat's extreme error handling with JSch;) Do you really think it's \nbetter to rely on internal error message instead of continuing in any \ncase? Which other exceptions we would like to pass level up?\n\n-- \nMarek Zawirski [zawir]\nmarek.zawirski@gmail.com\n"},{"id":"88816","messageId":"20080827232946.GS26523@spearce.org","threadId":"15231","inReplyTo":"48B5E2A1.3030007@gmail.com","subject":"Re: [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-27T23:29:46Z","receivedAt":"2008-08-27T23:29:46Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Marek Zawirski <marek.zawirski@gmail.com> wrote:\n> Shawn O. Pearce wrote:\n>> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n> (...)\n>> +\t\t\ttry {\n>> +\t\t\t\taddIdentity(k);\n>> +\t\t\t} catch (JSchException e) {\n>> +\t\t\t\tif (e.getMessage().startsWith(\"invalid privatekey: \"))\n>> +\t\t\t\t\tcontinue;\n>> +\t\t\t\tthrow e;\n>> +\t\t\t}\n>\n> That's extreme error handling with JSch;) Do you really think it's  \n> better to rely on internal error message instead of continuing in any  \n> case? Which other exceptions we would like to pass level up?\n\nOh, that's a good question.  In this particular code we're just\ntrying to prime the list of known keys so there's a chance we could\nlater prompt you for a passphrase during the handshaking.  So we\nprobably could get away with just ignoring all JSchExceptions at\nthis stage and treat the key as though it wasn't present...\n\nI can't imagine what else we'd get back.  A FileNotFoundException\njust means the user deleted the key before we could actually read\nit (no big deal); an IOException because the key isn't readable\nisn't an issue either.\n\nI guess I can just change this to ignore everything.\n\n-- \nShawn.\n"},{"id":"88832","messageId":"20080828002406.GU26523@spearce.org","threadId":"15231","inReplyTo":"20080827232946.GS26523@spearce.org","subject":"[JGIT PATCH 1/2 v2] Ignore unreadable SSH private keys when autoloading identities","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2008-08-28T00:24:06Z","receivedAt":"2008-08-28T00:24:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"During SSH startup we read all keys in the user's ~/.ssh, even\nif we may not need them for this particular transport session.\n\nIf a file is not really a key, or it contains a key that JSch\ndoesn't recognize we shouldn't crash the transport.  Instead\nwe should skip the file and move on.  Later on we just don't\nhave that identity available to us, or we'll crash if we try\nto add that identity file explicitly from ~/.ssh/config.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n\n  \"Shawn O. Pearce\" <spearce@spearce.org> wrote:\n  > Marek Zawirski <marek.zawirski@gmail.com> wrote:\n  > > Shawn O. Pearce wrote:\n  > >> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n  > > (...)\n  > >> +\t\t\ttry {\n  > >> +\t\t\t\taddIdentity(k);\n  > >> +\t\t\t} catch (JSchException e) {\n  > >> +\t\t\t\tif (e.getMessage().startsWith(\"invalid privatekey: \"))\n  > >> +\t\t\t\t\tcontinue;\n  > >> +\t\t\t\tthrow e;\n  > >> +\t\t\t}\n  > >\n  > > That's extreme error handling with JSch;) Do you really think it's  \n  > > better to rely on internal error message instead of continuing in any  \n  > > case? Which other exceptions we would like to pass level up?\n  > \n  > I guess I can just change this to ignore everything.\n\n .../jgit/transport/DefaultSshSessionFactory.java   |   11 +++++++++--\n 1 files changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\nindex a2437c2..74fca66 100644\n--- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n+++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java\n@@ -165,14 +165,21 @@ private void identities() throws JSchException {\n \t\t\tfinal File k = new File(sshdir, n.substring(0, n.length() - 4));\n \t\t\tif (!k.isFile())\n \t\t\t\tcontinue;\n-\t\t\taddIdentity(k);\n+\n+\t\t\ttry {\n+\t\t\t\taddIdentity(k);\n+\t\t\t} catch (JSchException e) {\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t}\n \t}\n \n \tprivate void addIdentity(final File identityFile) throws JSchException {\n \t\tfinal String path = identityFile.getAbsolutePath();\n-\t\tif (loadedIdentities.add(path))\n+\t\tif (!loadedIdentities.contains(path)) {\n \t\t\tuserJSch.addIdentity(path);\n+\t\t\tloadedIdentities.add(path);\n+\t\t}\n \t}\n \n \tprivate static class AWT_UserInfo implements UserInfo,\n-- \n1.6.0.174.gd789c\n"}]}