From e7838b9c080011817ddb59c53298237a9c24a7a6 Mon Sep 17 00:00:00 2001 From: Thomas Wolf Date: Wed, 27 Oct 2021 14:03:10 +0200 Subject: [PATCH] [sshd agent] Introduce ConnectorDescriptor Once a factory supports different SSH agents on the same platform, which is planned for Windows once we use Apache MINA sshd 2.8.0, client code may need to have a way to specify which SSH agent shall be used when the SSH config doesn't define anything. Add a mechanism by which a ConnectorFactory can tell what Connectors it may provide. Client code can use this to set the identityAgent parameter of ConnectorFactory.create() to the wanted default if it would be null otherwise. A ConnectorDescriptor is a pair of strings: an internal name, and a display name. The latter is included because client code might want to communicate agent names to the user, be it in error messages or in some chooser dialog where a user could define which of several alternative SSH agents should be used as default. The internal name is intended to be used in the IdentityAgent directive in ~/.ssh/config. Also make the ConnectorFactory discovered via the ServiceLoader accessible and overrideable. Provide static get/setDefault() methods, similar to the SshSessionFactory itself. Change-Id: Ie3d077395d32dfddc72bc8627e92b23636938182 Signed-off-by: Thomas Wolf --- .../sshd/agent/connector/Texts.properties | 3 +- .../sshd/agent/connector/Factory.java | 24 ++++ .../agent/connector/PageantConnector.java | 17 +++ .../transport/sshd/agent/connector/Texts.java | 2 + .../connector/UnixDomainSocketConnector.java | 17 +++ .../sshd/agent/ConnectorFactoryProvider.java | 26 ++-- .../transport/sshd/SshdSessionFactory.java | 6 +- .../sshd/agent/ConnectorFactory.java | 116 +++++++++++++++++- .../jgit/transport/SshSessionFactory.java | 2 +- 9 files changed, 199 insertions(+), 14 deletions(-) diff --git a/org.eclipse.jgit.ssh.apache.agent/resources/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.properties b/org.eclipse.jgit.ssh.apache.agent/resources/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.properties index f884adc08..6fce08366 100644 --- a/org.eclipse.jgit.ssh.apache.agent/resources/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.properties +++ b/org.eclipse.jgit.ssh.apache.agent/resources/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.properties @@ -13,4 +13,5 @@ msgSendFailed=Sending {0} bytes to SSH agent failed; {0} bytes not written msgSendFailed2=Sending {0} bytes to SSH agent failed: {1} - {2} msgSharedMemoryFailed=Could not set up shared memory for communicating with Pageant msgShortRead=Short read from SSH agent, expected {0} bytes, got {1} bytes; last read() returned {2} - +pageant=Pageant +unixDefaultAgent=ssh-agent diff --git a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Factory.java b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Factory.java index 1cbf0e41b..d7409b0c3 100644 --- a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Factory.java +++ b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Factory.java @@ -11,6 +11,8 @@ import java.io.File; import java.io.IOException; +import java.util.Collection; +import java.util.Collections; import org.eclipse.jgit.transport.sshd.agent.Connector; import org.eclipse.jgit.transport.sshd.agent.ConnectorFactory; @@ -41,4 +43,26 @@ public boolean isSupported() { public String getName() { return NAME; } + + /** + * {@inheritDoc} + *

+ * This factory returns on Windows a + * {@link org.eclipse.jgit.transport.sshd.agent.ConnectorFactory.ConnectorDescriptor + * ConnectorDescriptor} for the internal name "pageant"; on Unix one for + * "SSH_AUTH_SOCK". + *

+ */ + @Override + public Collection getSupportedConnectors() { + return Collections.singleton(getDefaultConnector()); + } + + @Override + public ConnectorDescriptor getDefaultConnector() { + if (SystemReader.getInstance().isWindows()) { + return PageantConnector.DESCRIPTOR; + } + return UnixDomainSocketConnector.DESCRIPTOR; + } } diff --git a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/PageantConnector.java b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/PageantConnector.java index 59fe2fc24..b0e3bce72 100644 --- a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/PageantConnector.java +++ b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/PageantConnector.java @@ -12,12 +12,29 @@ import java.io.IOException; import org.eclipse.jgit.transport.sshd.agent.AbstractConnector; +import org.eclipse.jgit.transport.sshd.agent.ConnectorFactory.ConnectorDescriptor; /** * A connector using Pageant's shared memory IPC mechanism. */ public class PageantConnector extends AbstractConnector { + /** + * {@link ConnectorDescriptor} for the {@link PageantConnector}. + */ + public static final ConnectorDescriptor DESCRIPTOR = new ConnectorDescriptor() { + + @Override + public String getIdentityAgent() { + return "pageant"; //$NON-NLS-1$ + } + + @Override + public String getDisplayName() { + return Texts.get().pageant; + } + }; + private final PageantLibrary lib; /** diff --git a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.java b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.java index 6b6626154..fb45b30dd 100644 --- a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.java +++ b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/Texts.java @@ -42,5 +42,7 @@ public static Texts get() { /***/ public String msgSendFailed2; /***/ public String msgSharedMemoryFailed; /***/ public String msgShortRead; + /***/ public String pageant; + /***/ public String unixDefaultAgent; } diff --git a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/UnixDomainSocketConnector.java b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/UnixDomainSocketConnector.java index 4c698d974..3b75f3a7d 100644 --- a/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/UnixDomainSocketConnector.java +++ b/org.eclipse.jgit.ssh.apache.agent/src/org/eclipse/jgit/internal/transport/sshd/agent/connector/UnixDomainSocketConnector.java @@ -24,6 +24,7 @@ import org.apache.sshd.common.SshException; import org.eclipse.jgit.transport.sshd.agent.AbstractConnector; +import org.eclipse.jgit.transport.sshd.agent.ConnectorFactory.ConnectorDescriptor; import org.eclipse.jgit.util.StringUtils; import org.eclipse.jgit.util.SystemReader; import org.slf4j.Logger; @@ -38,6 +39,22 @@ */ public class UnixDomainSocketConnector extends AbstractConnector { + /** + * {@link ConnectorDescriptor} for the {@link UnixDomainSocketConnector}. + */ + public static final ConnectorDescriptor DESCRIPTOR = new ConnectorDescriptor() { + + @Override + public String getIdentityAgent() { + return ENV_SSH_AUTH_SOCK; + } + + @Override + public String getDisplayName() { + return Texts.get().unixDefaultAgent; + } + }; + private static final Logger LOG = LoggerFactory .getLogger(UnixDomainSocketConnector.class); diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/ConnectorFactoryProvider.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/ConnectorFactoryProvider.java index 9984f9976..aba7a7645 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/ConnectorFactoryProvider.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/agent/ConnectorFactoryProvider.java @@ -19,7 +19,7 @@ */ public final class ConnectorFactoryProvider { - private static final ConnectorFactory FACTORY = loadDefaultFactory(); + private static volatile ConnectorFactory INSTANCE = loadDefaultFactory(); private static ConnectorFactory loadDefaultFactory() { ServiceLoader loader = ServiceLoader @@ -35,17 +35,27 @@ private static ConnectorFactory loadDefaultFactory() { } - private ConnectorFactoryProvider() { - // No instantiation - } - /** - * Retrieves the default {@link ConnectorFactory} obtained via the - * {@link ServiceLoader}. + * Retrieves the currently set default {@link ConnectorFactory}. * * @return the {@link ConnectorFactory}, or {@code null} if none. */ public static ConnectorFactory getDefaultFactory() { - return FACTORY; + return INSTANCE; + } + + /** + * Sets the default {@link ConnectorFactory}. + * + * @param factory + * {@link ConnectorFactory} to use, or {@code null} to use the + * factory discovered via the {@link ServiceLoader}. + */ + public static void setDefaultFactory(ConnectorFactory factory) { + INSTANCE = factory == null ? loadDefaultFactory() : factory; + } + + private ConnectorFactoryProvider() { + // No instantiation } } diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java index 336418009..58cf8e1dd 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/SshdSessionFactory.java @@ -56,7 +56,6 @@ import org.eclipse.jgit.internal.transport.sshd.OpenSshServerKeyDatabase; import org.eclipse.jgit.internal.transport.sshd.PasswordProviderWrapper; import org.eclipse.jgit.internal.transport.sshd.SshdText; -import org.eclipse.jgit.internal.transport.sshd.agent.ConnectorFactoryProvider; import org.eclipse.jgit.internal.transport.sshd.agent.JGitSshAgentFactory; import org.eclipse.jgit.transport.CredentialsProvider; import org.eclipse.jgit.transport.SshConfigStore; @@ -456,12 +455,15 @@ protected ServerKeyDatabase createServerKeyDatabase(@NonNull File homeDir, /** * Gets a {@link ConnectorFactory}. If this returns {@code null}, SSH agents * are not supported. + *

+ * The default implementation uses {@link ConnectorFactory#getDefault()} + *

* * @return the factory, or {@code null} if no SSH agent support is desired * @since 6.0 */ protected ConnectorFactory getConnectorFactory() { - return ConnectorFactoryProvider.getDefaultFactory(); + return ConnectorFactory.getDefault(); } /** diff --git a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/agent/ConnectorFactory.java b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/agent/ConnectorFactory.java index b351d89ef..da98ea7fe 100644 --- a/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/agent/ConnectorFactory.java +++ b/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/transport/sshd/agent/ConnectorFactory.java @@ -11,8 +11,10 @@ import java.io.File; import java.io.IOException; +import java.util.Collection; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.internal.transport.sshd.agent.ConnectorFactoryProvider; /** * A factory for creating {@link Connector}s. This is a service provider @@ -24,14 +26,45 @@ */ public interface ConnectorFactory { + /** + * Retrieves the currently set default {@link ConnectorFactory}. This is the + * factory that is used unless overridden by the + * {@link org.eclipse.jgit.transport.sshd.SshdSessionFactory}. + * + * @return the current default factory; may be {@code null} if none is set + * and the {@link java.util.ServiceLoader} cannot find any suitable + * implementation + */ + static ConnectorFactory getDefault() { + return ConnectorFactoryProvider.getDefaultFactory(); + } + + /** + * Sets a default {@link ConnectorFactory}. This is the factory that is used + * unless overridden by the + * {@link org.eclipse.jgit.transport.sshd.SshdSessionFactory}. + *

+ * If no default factory is set programmatically, an implementation is + * discovered via the {@link java.util.ServiceLoader}. + *

+ * + * @param factory + * {@link ConnectorFactory} to set, or {@code null} to revert to + * the default behavior of using the + * {@link java.util.ServiceLoader}. + */ + static void setDefault(ConnectorFactory factory) { + ConnectorFactoryProvider.setDefaultFactory(factory); + } + /** * Creates a new {@link Connector}. * * @param identityAgent * identifies the wanted agent connection; if {@code null}, the * factory is free to provide a {@link Connector} to a default - * agent. The value will typically come from the IdentityAgent - * setting in ~/.ssh/config. + * agent. The value will typically come from the + * {@code IdentityAgent} setting in {@code ~/.ssh/config}. * @param homeDir * the current local user's home directory as configured in the * {@link org.eclipse.jgit.transport.sshd.SshdSessionFactory} @@ -58,4 +91,83 @@ Connector create(String identityAgent, File homeDir) */ String getName(); + /** + * {@link ConnectorDescriptor}s describe available {@link Connector}s a + * {@link ConnectorFactory} may provide. + *

+ * A {@link ConnectorFactory} may support connecting to different SSH + * agents. Agents are identified by name; a user can choose a specific agent + * for instance via the {@code IdentityAgent} setting in + * {@code ~/.ssh/config}. + *

+ *

+ * OpenSSH knows two built-in names: "none" for not using any agent, and + * "SSH_AUTH_SOCK" for using an agent that communicates over a Unix domain + * socket given by the value of environment variable {@code SSH_AUTH_SOCK}. + * Other agents can be specified in OpenSSH by specifying the socket file + * directly. (The "standard" OpenBSD OpenSSH knows only this communication + * mechanism.) "SSH_AUTH_SOCK" is also the default in OpenBSD OpenSSH if + * nothing is configured. + *

+ *

+ * A particular {@link ConnectorFactory} may support more communication + * mechanisms or different agents. For instance, a factory on Windows might + * support Pageant, Win32-OpenSSH, or even git bash ssh-agent, and might + * accept internal names like "pageant", "openssh", "SSH_AUTH_SOCK" in + * {@link ConnectorFactory#create(String, File)} to choose among them. + *

+ * The {@link ConnectorDescriptor} interface and the + * {@link ConnectorFactory#getSupportedConnectors()} and + * {@link ConnectorFactory#getDefaultConnector()} methods provide a way for + * code using a {@link ConnectorFactory} to learn what the factory supports + * and thus implement some way by which a user can influence the default + * behavior if {@code IdentityAgent} is not set or + * {@link ConnectorFactory#create(String, File)} is called with + * {@code identityAgent == null}. + */ + interface ConnectorDescriptor { + + /** + * Retrieves the internal name of a supported {@link Connector}. The + * internal name is the one a user can specify for instance in the + * {@code IdentityAgent} setting in {@code ~/.ssh/config} to select the + * connector. + * + * @return the internal name; not empty + */ + @NonNull + String getIdentityAgent(); + + /** + * Retrieves a display name for a {@link Connector}, suitable for + * showing in a UI. + * + * @return the display name; properly localized and not empty + */ + @NonNull + String getDisplayName(); + } + + /** + * Tells which kinds of SSH agents this {@link ConnectorFactory} supports. + *

+ * An implementation of this method should document the possible values it + * returns. + *

+ * + * @return an immutable collection of {@link ConnectorDescriptor}s, + * including {@link #getDefaultConnector()} and not including a + * descriptor for internal name "none" + */ + @NonNull + Collection getSupportedConnectors(); + + /** + * Tells what kind of {@link Connector} this {@link ConnectorFactory} + * creates if {@link ConnectorFactory#create(String, File)} is called with + * {@code identityAgent == null}. + * + * @return a {@link ConnectorDescriptor} for the default connector + */ + ConnectorDescriptor getDefaultConnector(); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshSessionFactory.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshSessionFactory.java index e216a56ac..1e98a56f7 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshSessionFactory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/SshSessionFactory.java @@ -36,7 +36,7 @@ */ public abstract class SshSessionFactory { - private static SshSessionFactory INSTANCE = loadSshSessionFactory(); + private static volatile SshSessionFactory INSTANCE = loadSshSessionFactory(); private static SshSessionFactory loadSshSessionFactory() { ServiceLoader loader = ServiceLoader.load(SshSessionFactory.class);