* [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities @ 2008-08-27 23:02 Shawn O. Pearce 2008-08-27 23:02 ` [JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed Shawn O. Pearce 2008-08-27 23:26 ` [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Marek Zawirski 0 siblings, 2 replies; 5+ messages in thread From: Shawn O. Pearce @ 2008-08-27 23:02 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git During SSH startup we read all keys in the user's ~/.ssh, even if we may not need them for this particular transport session. If a file is not really a key, or it contains a key that JSch doesn't recognize we shouldn't crash the transport. Instead we should skip the file and move on. Later on we just don't have that identity available to us, or we'll crash if we try to add that identity file explicitly from ~/.ssh/config. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../jgit/transport/DefaultSshSessionFactory.java | 13 +++++++++++-- 1 files changed, 11 insertions(+), 2 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java index a2437c2..aa72357 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java @@ -165,14 +165,23 @@ private void identities() throws JSchException { final File k = new File(sshdir, n.substring(0, n.length() - 4)); if (!k.isFile()) continue; - addIdentity(k); + + try { + addIdentity(k); + } catch (JSchException e) { + if (e.getMessage().startsWith("invalid privatekey: ")) + continue; + throw e; + } } } private void addIdentity(final File identityFile) throws JSchException { final String path = identityFile.getAbsolutePath(); - if (loadedIdentities.add(path)) + if (!loadedIdentities.contains(path)) { userJSch.addIdentity(path); + loadedIdentities.add(path); + } } private static class AWT_UserInfo implements UserInfo, -- 1.6.0.174.gd789c ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed 2008-08-27 23:02 [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Shawn O. Pearce @ 2008-08-27 23:02 ` Shawn O. Pearce 2008-08-27 23:26 ` [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Marek Zawirski 1 sibling, 0 replies; 5+ messages in thread From: Shawn O. Pearce @ 2008-08-27 23:02 UTC (permalink / raw) To: Robin Rosenberg; +Cc: git If we don't close the transport when we are done with it the SSH session stored within the Transport will still be opened to the remote side. JSch may have created one or more background user threads to handle that connection, which means the JVM won't terminate cleanly when we are done with our work. We must close each and every transport we opened. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- .../src/org/spearce/jgit/pgm/Push.java | 24 ++++++++++++------- 1 files changed, 15 insertions(+), 9 deletions(-) diff --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 index f53f2fe..53ad080 100644 --- a/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java +++ b/org.spearce.jgit.pgm/src/org/spearce/jgit/pgm/Push.java @@ -50,6 +50,7 @@ import org.spearce.jgit.transport.RefSpec; import org.spearce.jgit.transport.RemoteRefUpdate; import org.spearce.jgit.transport.Transport; +import org.spearce.jgit.transport.URIish; import org.spearce.jgit.transport.RemoteRefUpdate.Status; @Command(common = true, usage = "Update remote repository from local refs") @@ -111,13 +112,18 @@ protected void run() throws Exception { final Collection<RemoteRefUpdate> toPush = transport .findRemoteRefUpdatesFor(refSpecs); - final PushResult result = transport.push(new TextProgressMonitor(), - toPush); - printPushResult(transport, result); + final URIish uri = transport.getURI(); + final PushResult result; + try { + result = transport.push(new TextProgressMonitor(), toPush); + } finally { + transport.close(); + } + printPushResult(uri, result); } } - private void printPushResult(final Transport transport, + private void printPushResult(final URIish uri, final PushResult result) { shownURI = false; boolean everythingUpToDate = true; @@ -126,7 +132,7 @@ private void printPushResult(final Transport transport, for (final RemoteRefUpdate rru : result.getRemoteUpdates()) { if (rru.getStatus() == Status.UP_TO_DATE) { if (verbose) - printRefUpdateResult(transport, result, rru); + printRefUpdateResult(uri, result, rru); } else everythingUpToDate = false; } @@ -134,25 +140,25 @@ private void printPushResult(final Transport transport, for (final RemoteRefUpdate rru : result.getRemoteUpdates()) { // ...then successful updates... if (rru.getStatus() == Status.OK) - printRefUpdateResult(transport, result, rru); + printRefUpdateResult(uri, result, rru); } for (final RemoteRefUpdate rru : result.getRemoteUpdates()) { // ...finally, others (problematic) if (rru.getStatus() != Status.OK && rru.getStatus() != Status.UP_TO_DATE) - printRefUpdateResult(transport, result, rru); + printRefUpdateResult(uri, result, rru); } if (everythingUpToDate) out.println("Everything up-to-date"); } - private void printRefUpdateResult(final Transport transport, + private void printRefUpdateResult(final URIish uri, final PushResult result, final RemoteRefUpdate rru) { if (!shownURI) { shownURI = true; - out.format("To %s\n", transport.getURI()); + out.format("To %s\n", uri); } final String remoteName = rru.getRemoteName(); -- 1.6.0.174.gd789c ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities 2008-08-27 23:02 [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Shawn O. Pearce 2008-08-27 23:02 ` [JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed Shawn O. Pearce @ 2008-08-27 23:26 ` Marek Zawirski 2008-08-27 23:29 ` Shawn O. Pearce 1 sibling, 1 reply; 5+ messages in thread From: Marek Zawirski @ 2008-08-27 23:26 UTC (permalink / raw) To: Shawn O. Pearce; +Cc: Robin Rosenberg, git Shawn O. Pearce wrote: > diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java (...) > + try { > + addIdentity(k); > + } catch (JSchException e) { > + if (e.getMessage().startsWith("invalid privatekey: ")) > + continue; > + throw e; > + } That's extreme error handling with JSch;) Do you really think it's better to rely on internal error message instead of continuing in any case? Which other exceptions we would like to pass level up? -- Marek Zawirski [zawir] marek.zawirski@gmail.com ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities 2008-08-27 23:26 ` [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Marek Zawirski @ 2008-08-27 23:29 ` Shawn O. Pearce 2008-08-28 0:24 ` [JGIT PATCH 1/2 v2] " Shawn O. Pearce 0 siblings, 1 reply; 5+ messages in thread From: Shawn O. Pearce @ 2008-08-27 23:29 UTC (permalink / raw) To: Marek Zawirski; +Cc: Robin Rosenberg, git Marek Zawirski <marek.zawirski@gmail.com> wrote: > Shawn O. Pearce wrote: >> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java > (...) >> + try { >> + addIdentity(k); >> + } catch (JSchException e) { >> + if (e.getMessage().startsWith("invalid privatekey: ")) >> + continue; >> + throw e; >> + } > > That's extreme error handling with JSch;) Do you really think it's > better to rely on internal error message instead of continuing in any > case? Which other exceptions we would like to pass level up? Oh, that's a good question. In this particular code we're just trying to prime the list of known keys so there's a chance we could later prompt you for a passphrase during the handshaking. So we probably could get away with just ignoring all JSchExceptions at this stage and treat the key as though it wasn't present... I can't imagine what else we'd get back. A FileNotFoundException just means the user deleted the key before we could actually read it (no big deal); an IOException because the key isn't readable isn't an issue either. I guess I can just change this to ignore everything. -- Shawn. ^ permalink raw reply [flat|nested] 5+ messages in thread
* [JGIT PATCH 1/2 v2] Ignore unreadable SSH private keys when autoloading identities 2008-08-27 23:29 ` Shawn O. Pearce @ 2008-08-28 0:24 ` Shawn O. Pearce 0 siblings, 0 replies; 5+ messages in thread From: Shawn O. Pearce @ 2008-08-28 0:24 UTC (permalink / raw) To: Robin Rosenberg, Marek Zawirski; +Cc: git During SSH startup we read all keys in the user's ~/.ssh, even if we may not need them for this particular transport session. If a file is not really a key, or it contains a key that JSch doesn't recognize we shouldn't crash the transport. Instead we should skip the file and move on. Later on we just don't have that identity available to us, or we'll crash if we try to add that identity file explicitly from ~/.ssh/config. Signed-off-by: Shawn O. Pearce <spearce@spearce.org> --- "Shawn O. Pearce" <spearce@spearce.org> wrote: > Marek Zawirski <marek.zawirski@gmail.com> wrote: > > Shawn O. Pearce wrote: > >> diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java > > (...) > >> + try { > >> + addIdentity(k); > >> + } catch (JSchException e) { > >> + if (e.getMessage().startsWith("invalid privatekey: ")) > >> + continue; > >> + throw e; > >> + } > > > > That's extreme error handling with JSch;) Do you really think it's > > better to rely on internal error message instead of continuing in any > > case? Which other exceptions we would like to pass level up? > > I guess I can just change this to ignore everything. .../jgit/transport/DefaultSshSessionFactory.java | 11 +++++++++-- 1 files changed, 9 insertions(+), 2 deletions(-) diff --git a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java index a2437c2..74fca66 100644 --- a/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java +++ b/org.spearce.jgit/src/org/spearce/jgit/transport/DefaultSshSessionFactory.java @@ -165,14 +165,21 @@ private void identities() throws JSchException { final File k = new File(sshdir, n.substring(0, n.length() - 4)); if (!k.isFile()) continue; - addIdentity(k); + + try { + addIdentity(k); + } catch (JSchException e) { + continue; + } } } private void addIdentity(final File identityFile) throws JSchException { final String path = identityFile.getAbsolutePath(); - if (loadedIdentities.add(path)) + if (!loadedIdentities.contains(path)) { userJSch.addIdentity(path); + loadedIdentities.add(path); + } } private static class AWT_UserInfo implements UserInfo, -- 1.6.0.174.gd789c ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2008-08-28 0:25 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2008-08-27 23:02 [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Shawn O. Pearce 2008-08-27 23:02 ` [JGIT PATCH 2/2] pgm.push: Ensure SSH connections are closed Shawn O. Pearce 2008-08-27 23:26 ` [JGIT PATCH 1/2] Ignore unreadable SSH private keys when autoloading identities Marek Zawirski 2008-08-27 23:29 ` Shawn O. Pearce 2008-08-28 0:24 ` [JGIT PATCH 1/2 v2] " Shawn O. Pearce
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).