Skip to content
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

Incompatibility with legacy jsch as used by jgit #85

Closed
ST-DDT opened this issue Oct 12, 2021 · 19 comments
Closed

Incompatibility with legacy jsch as used by jgit #85

ST-DDT opened this issue Oct 12, 2021 · 19 comments

Comments

@ST-DDT
Copy link
Contributor

ST-DDT commented Oct 12, 2021

I tried to replace the original jsch with this updated variant, but that caused an NPE.

CloneCommand cloneCommand = Git.cloneRepository()
                .setURI("git@github.com:mwiede/jsch.git")
                .setDirectory(new File("target/junit/clone-test));
assertDoesNotThrow(cloneCommand::call).close(); // <-- Error

This is caused by the following line: Source

JSch.setConfig("ssh-rsa", JSch.getConfig("signature.rsa"));
JSch.setConfig("ssh-dss", JSch.getConfig("signature.dss"));

In the original jsch version this was set to:

config.put("signature.rsa", "com.jcraft.jsch.jce.SignatureRSA");

This version no longer sets these properties.

My dependencies:

        <!-- Git -->
        <dependency>
            <groupId>org.eclipse.jgit</groupId>
            <artifactId>org.eclipse.jgit</artifactId>
            <version>5.13.0.202109080827-r</version>
        </dependency>
        <dependency>
            <groupId>org.eclipse.jgit</groupId>
            <artifactId>org.eclipse.jgit.ssh.jsch</artifactId>
            <version>5.13.0.202109080827-r</version>
            <exclusions>
                <exclusion>
                    <groupId>com.jcraft</groupId>
                    <artifactId>jsch</artifactId>
                </exclusion>
            </exclusions>
        </dependency>
        <dependency>
            <groupId>com.github.mwiede</groupId>
            <artifactId>jsch</artifactId>
            <version>0.1.68</version>
        </dependency>
        <dependency>
            <groupId>com.jcraft</groupId>
            <artifactId>jsch.agentproxy.connector-factory</artifactId>
            <version>0.0.9</version>
        </dependency>
        <dependency>
            <groupId>com.jcraft</groupId>
            <artifactId>jsch.agentproxy.jsch</artifactId>
            <version>0.0.9</version>
            <exclusions>
                <exclusion>
                    <groupId>com.jcraft</groupId>
                    <artifactId>jsch</artifactId>
                </exclusion>
            </exclusions>
        </dependency>

Stacktrace

Caused by: org.eclipse.jgit.api.errors.TransportException: git@github.com:mwiede/jsch.git: remote hung up unexpectedly
	at org.eclipse.jgit.api.FetchCommand.call(FetchCommand.java:224)
	at org.eclipse.jgit.api.CloneCommand.fetch(CloneCommand.java:303)
	at org.eclipse.jgit.api.CloneCommand.call(CloneCommand.java:178)
	at org.junit.jupiter.api.AssertDoesNotThrow.assertDoesNotThrow(AssertDoesNotThrow.java:72)
	... 71 more
Caused by: org.eclipse.jgit.errors.TransportException: git@github.com:mwiede/jsch.git: remote hung up unexpectedly
	at org.eclipse.jgit.transport.TransportGitSsh$SshFetchConnection.<init>(TransportGitSsh.java:313)
	at org.eclipse.jgit.transport.TransportGitSsh.openFetch(TransportGitSsh.java:153)
	at org.eclipse.jgit.transport.FetchProcess.executeImp(FetchProcess.java:142)
	at org.eclipse.jgit.transport.FetchProcess.execute(FetchProcess.java:94)
	at org.eclipse.jgit.transport.Transport.fetch(Transport.java:1309)
	at org.eclipse.jgit.api.FetchCommand.call(FetchCommand.java:213)
	... 74 more
Caused by: java.lang.NullPointerException
	at java.util.Hashtable.put(Hashtable.java:460)
	at com.jcraft.jsch.JSch.setConfig(JSch.java:666)
	at org.eclipse.jgit.transport.JschConfigSessionFactory.createDefaultJSch(JschConfigSessionFactory.java:401)
	at org.eclipse.jgit.transport.JschConfigSessionFactory.getJSch(JschConfigSessionFactory.java:361)
	at org.eclipse.jgit.transport.JschConfigSessionFactory.createSession(JschConfigSessionFactory.java:317)
	at org.eclipse.jgit.transport.JschConfigSessionFactory.createSession(JschConfigSessionFactory.java:184)
	at org.eclipse.jgit.transport.JschConfigSessionFactory.getSession(JschConfigSessionFactory.java:108)
	at org.eclipse.jgit.transport.SshTransport.getSession(SshTransport.java:107)
	at org.eclipse.jgit.transport.TransportGitSsh$SshFetchConnection.<init>(TransportGitSsh.java:281)
	... 79 more
@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

Please see #43 for a potential workaround.

You may also want to open a bug with the Eclipse team, as their decision to unconditionally perform JSch.setConfig("ssh-rsa", JSch.getConfig("signature.rsa")); appears dubious. I would open one, but the JGit project is not maintained on Github and it appears you have to jump through an enormous number of hurdles in order to report issues and contribute patches in their privately maintained Bugzilla + Gerrit.

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 12, 2021

Sure, I can open one, but can you give me a little bit of context what these values refer to and whats the difference between them?
Could you please also tell me the reason, why the the value/mapping was removed in the first place?

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

  1. The original signature.rsa config item was removed as part of adding support for rsa-sha2-256 & rsa-sha2-512.
  2. This JSch fork already contains a config item for ssh-rsa: overriding it is not necessary, and could possibly lead to issues.

The quickest way for them to fix it would probably be to simply check whether the config items they attempt to add are already present or not:

if (JSch.getConfig("ssh-rsa") == null) {
  JSch.setConfig("ssh-rsa", JSch.getConfig("signature.rsa"));
}
if (JSch.getConfig("ssh-dss") == null) {
  JSch.setConfig("ssh-dss", JSch.getConfig("signature.dss"));
}

Also, I'm not sure if they would be interested in changing their JSch dependency long-term to this particular fork, since the original JSch from JCraft is unmaintained these days? I'd be happy (and I'm sure @mwiede would be as well) to collaborate with them on any changes we could make in this fork that could improve their integration with JSch: I suspect this code was added by them to workaround a deficiency in the original JSch.

Thanks,
Jeremy

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

On an unrelated note, I also noticed that you had the following dependencies:

        <dependency>
            <groupId>com.jcraft</groupId>
            <artifactId>jsch.agentproxy.connector-factory</artifactId>
            <version>0.0.9</version>
        </dependency>
        <dependency>
            <groupId>com.jcraft</groupId>
            <artifactId>jsch.agentproxy.jsch</artifactId>
            <version>0.0.9</version>
            <exclusions>
                <exclusion>
                    <groupId>com.jcraft</groupId>
                    <artifactId>jsch</artifactId>
                </exclusion>
            </exclusions>
        </dependency>

Are you aware of how we directly integrated support for ssh-agent based upon that original code as part of #65?

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 12, 2021

Are you aware of how we directly integrated support for ssh-agent based upon that original code as part of #65?

No, I wasn't. I have searched for agentproxy packages in the jar, but since I couldn't find any, I kept my original ssh-agent-glue code.
Thank you very much for that hint! I will try your variant asap.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 12, 2021

I tested with the libraries as used in the commit, but I was unable to make it work with windows 10 and java 11 64bit.
Is it possible that the used libraries only support it for unix systems || Java 16+?
I updated my glue code a bit to account for that one missing method and now it also works on windows (with jsch.agentporxy.jsch).

The problem might be the use of Paths.get(...) as that converts the unix paths to windows paths, which might cause the agent/socket to fail to connect.

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

I've only tested on Linux & macOS, so I'm not sure if there could be issues on Windows.

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

For future reference, the windows implementation uses the pageant connector (for me). com.jcraft.jsch.agentproxy.connector.PageantConnector
I will check whether I can add a PR that adds support for windows (soonish).

EDIT: I found this to be present in this library as well, but it isn't as easy to setup as before. Maybe I can improve that a bit instead.
EDIT2: It does not work...

Caused by: java.lang.IllegalAccessException: Class com.sun.jna.Structure can not access a member of class com.jcraft.jsch.PageantConnector$COPYDATASTRUCT64 with modifiers "public"
	at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
	at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
	at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
	at java.lang.reflect.Field.get(Field.java:390)
	at com.sun.jna.Structure.getFieldValue(Structure.java:650)
	... 101 more

EDIT3: The COPYDATASTRUCT64 class must be protected or public.

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

Here is my jgit + jsch + agent bridge code so far:

/**
 * Tries to setup the ssh agent connection.
 *
 * @return True, if the agent was configured successfully. False, if the setup failed or was not
 *         attempted.
 */
public static boolean trySetupSSHAgent() {

    try {
        final AgentConnector connector = tryGetAgentConnector();
        if (connector == null) {
            return false; // Not attempted
        }

        final IdentityRepository identityRepository = new AgentIdentityRepository(connector);

        final JschConfigSessionFactory factory = new JschConfigSessionFactory() {

            @Override
            protected JSch createDefaultJSch(final FS fs) throws JSchException {
                final JSch jsch = super.createDefaultJSch(fs);

                jsch.setIdentityRepository(identityRepository);

                return jsch;
            }

        };

        SshSessionFactory.setInstance(factory);
        return true; // Success

    } catch (final Exception e) {
        if (LOGGER.isDebugEnabled()) {
            LOGGER.info("Failed to setup ssh agent connector", e);
        } else {
            LOGGER.info("Failed to setup ssh agent connector");
        }
        return false; // Failed
    }

}

/**
 * Tries to get an available agent connector.
 *
 * @return An available agent connector or null.
 */
private static AgentConnector tryGetAgentConnector() {
    AgentConnector connector = callOrNull(SSHAgentConnector::new, "SSHAgentConnector");
    if (connector != null && connector.isAvailable()) {
        return connector;
    }
    connector = callOrNull(PageantConnector::new, "PageantConnector");
    if (connector != null && connector.isAvailable()) {
        return connector;
    }
    return null;
}

/**
 * Calls the given callable. Returning the result on success or {@code null} on error.
 *
 * @param <T> The type of the result.
 * @param callable The callable to call.
 * @param hint The hint for the log message.
 * @return The result of the callable or null.
 */
private static <T> T callOrNull(final Callable<T> callable, final String hint) {
    try {
        return callable.call();
    } catch (final Exception e) {
        LOGGER.debug("Failed to call {} (Can usually be ignored)", hint, e);
        return null;
    }
}

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

Am I correct in my understanding that after the changes you submitted with #86 everything worked for you on Windows?

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

Hi @ST-DDT,

Am I correct in my understanding that after the changes you submitted with #86 everything worked for you on Windows?

Thanks, Jeremy

Yes, it does. After the PR is merged I will build the project locally and test again. Then I will probably propose a convenience method to AgentIdentityRepository that automatically tries to select the correct connector. (Along with some javadocs that explain which dependencies to include)

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,
Am I correct in my understanding that after the changes you submitted with #86 everything worked for you on Windows?
Thanks, Jeremy

Yes, it does. After the PR is merged I will build the project locally and test again. Then I will probably propose a convenience method to AgentIdentityRepository that automatically tries to select the correct connector. (Along with some javadocs that explain which dependencies to include)

Hi @ST-DDT ,

That is excellent to hear!
Am I correct in understanding that the convenience method you are referring to would be some sort of auto-selection of the connector based upon the OS?
If so, I had considered that but elected not to because I thought both connectors ought to be usable on Windows.
It's my understanding that recent versions of Windows support unix domain sockets, so I believe it should be possible for Windows users to utilize either the PageantConnector or SSHAgentConnector, therefore I didn't want to lock users into one or the other connector implementations.
I don't know if you would be in a position to confirm if this is true or not?

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

Am I correct in understanding that the convenience method you are referring to would be some sort of auto-selection of the connector based upon the OS? If so, I had considered that but elected not to because I thought both connectors ought to be usable on Windows. It's my understanding that recent versions of Windows support unix domain sockets, so I believe it should be possible for Windows users to utilize either the PageantConnector or SSHAgentConnector, therefore I didn't want to lock users into one or the other connector implementations.

Yes, that was exactly my intention. Using it is optional, so it won't limit the user to one or the other, but help them to get started and show an example "implementation", that they then can adapt to their needs. My idea is to implement it similar to my above example: Trying them one by one (Starting with the unix dmonain sockets). IIRC the jsch.agentproxy.jsch did exactly that.

I don't know if you would be in a position to confirm if this is true or not?

I can try, if the result is positive, then I'm sure that it works. But if it is negative, I'm not sure whether it is related to my untested ssh-agent setup, or the path used to access it, or the the java code.
I hope I can find a way to reliably test this in order to add some (automated) unit tests for that.

@norrisjeremy
Copy link
Contributor

Am I correct in understanding that the convenience method you are referring to would be some sort of auto-selection of the connector based upon the OS? If so, I had considered that but elected not to because I thought both connectors ought to be usable on Windows. It's my understanding that recent versions of Windows support unix domain sockets, so I believe it should be possible for Windows users to utilize either the PageantConnector or SSHAgentConnector, therefore I didn't want to lock users into one or the other connector implementations.

Yes, that was exactly my intention. Using it is optional, so it won't limit the user to one or the other, but help them to get started and show an example "implementation", that they then can adapt to their needs. My idea is to implement it similar to my above example: Trying them one by one (Starting with the unix dmonain sockets). IIRC the jsch.agentproxy.jsch did exactly that.

Ok, that sounds like it would be workable.

I don't know if you would be in a position to confirm if this is true or not?

I can try, if the result is positive, then I'm sure that it works. But if it is negative, I'm not sure whether it is related to my untested ssh-agent setup, or the path used to access it, or the the java code. I hope I can find a way to reliably test this in order to add some (automated) unit tests for that.

Yes, any test results you could provide from a Windows environment would be great!

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 13, 2021

I just tested the pageant fix in v0.1.69 and it works as expected.
You are really fast, thanks a lot!

I'll now contact jgit for a fix for: #85 (comment)

@norrisjeremy
Copy link
Contributor

Hi @ST-DDT,

That is great to hear, thanks!
As for the jgit part, please feel free to let them know that they can contact me via Github here if they would like to discuss this further!

Thanks,
Jeremy

@ST-DDT
Copy link
Contributor Author

ST-DDT commented Oct 20, 2021

Update from JGit:
The first one will be fixed soon.
The later one probably not (JSch is considered for deprecation/removal).

@mwiede
Copy link
Owner

mwiede commented Jul 21, 2022

closing this as it is solved. Somebody asked on Stackoverflow compare at https://stackoverflow.com/questions/73046450/spring-cloud-config-server-algorithm-negotiation-fail-problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants