[wagon-3.x] Move wagon-ssh to the maintained JSch fork and stop a stray key shadowing the agent - #902
Conversation
cbd644e to
35a4e29
Compare
|
Updated, and the headline is that this is now verified rather than asserted. The harness came back. #904 revives the embedded SSH tests, which had not run in over a decade. Running this branch's two commits on top of it: 42 tests, 0 failures against a real embedded MINA sshd server. That replaces the "unverified by the project's own tests" caveat this PR opened with. #904 should go in first. It immediately caught a fatal bug in this branch, now fixed. My agent lookup listed the three connector kinds in a private static final AgentConnectorFactory[] AGENT_CONNECTORS = {
SSHAgentConnector::new, WindowsSSHAgentConnector::new, PageantConnector::new
};
Each connector is now both constructed and used inside its own guard that catches That is a good argument for #904 on its own merits — this defect was invisible to everything else in the build. |
There was a problem hiding this comment.
Pull request overview
This PR updates the wagon-ssh provider to use the maintained com.github.mwiede:jsch fork (keeping the com.jcraft.jsch API) and adjusts SSH authentication behavior so the SSH agent is consulted before a “stray” discovered key in ~/.ssh, addressing long-standing key/agent interoperability issues.
Changes:
- Swap
wagon-sshfromcom.jcraft:jsch+jsch.agentproxy.*tocom.github.mwiede:jsch(agent support now comes from the fork). - Change auth selection order in
AbstractJschWagonto prefer: explicitly configured key → SSH agent → discovered~/.sshkey. - Update discovered private-key filename preference order in
ScpHelper(now considers modern key names).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wagon-providers/wagon-ssh/src/main/java/org/apache/maven/wagon/providers/ssh/jsch/AbstractJschWagon.java | Switch agent integration to mwiede JSch’s built-in agent connectors and change key/agent selection order. |
| wagon-providers/wagon-ssh/pom.xml | Replace unmaintained JSch + agentproxy dependencies with com.github.mwiede:jsch:2.28.6. |
| wagon-providers/wagon-ssh-common/src/main/java/org/apache/maven/wagon/providers/ssh/ScpHelper.java | Update private-key discovery to prefer modern key filenames. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
com.jcraft:jsch was last released in 2018 and never gained what current servers and current keys need. It has no rsa-sha2-256 or rsa-sha2-512, so RSA public key authentication fails against OpenSSH 8.8 and later, which stopped accepting SHA-1 signatures by default in 2021. It also cannot read the OpenSSH-v1 private key format, which ssh-keygen has produced by default since OpenSSH 7.8. A key generated today is rejected outright, encrypted or not. com.github.mwiede:jsch is a maintained fork of the same code under the same com.jcraft.jsch package, so the provider keeps its API. The fork carries the agent itself, so the two jsch.agentproxy artifacts go. Their ConnectorFactory picked between agent kinds for us; the fork has no equivalent, so the choice is now explicit, and all three kinds the old stack could reach are still tried: the OpenSSH agent named by SSH_AUTH_SOCK, the Win32 OpenSSH agent, and Pageant. One difference is worth knowing: reaching the SSH_AUTH_SOCK agent needs a Unix domain socket, which the JDK provides only from Java 16, and the fork declares no dependencies, so on Java 8 to 15 that agent is out of reach unless a helper library is on the class path. The other two do not need one. An agent holding no identities is now also skipped rather than being installed as an empty repository. Passing a passphrase needs care with the fork. getPrivateKey() substitutes an empty passphrase when the settings name none, which the old JSch quietly ignored, leaving an encrypted key to be unlocked later. The fork rejects it instead, which would have failed every encrypted key. Empty is therefore translated back to none before the key is added.
Authentication preferred a key file over the agent whenever one existed: the agent was consulted only in the else branch, and getPrivateKey() falls back to a default key file whenever no password is configured. On any machine with a key in ~/.ssh the agent was therefore never reached, which is most machines, and pointing wagon.privateKeyDirectory at an empty directory was the only way to reach it. Order the methods by how deliberate they are instead: a key named in settings.xml first, then the agent, and a key file that merely happens to be in ~/.ssh only after that. Naming a key keeps the behaviour it has today. This does mean a reachable agent holding identities now takes precedence over a discovered key file. Someone whose agent holds unrelated keys while the key the server wants sits unloaded in ~/.ssh will need to load it, or to name it in settings.xml. Combining the two is not open to us: adding a file identity on top of an agent repository makes JSch push that key into the user's agent, which is not ours to do. While here, look for the key types ssh-keygen actually produces -- id_ed25519, id_ecdsa, id_rsa -- rather than id_dsa followed by id_rsa. Trying id_dsa first reached for the one type OpenSSH has refused by default for years, and the fork disables ssh-dss too. This part is in ScpHelper, so it also changes which key wagon-ssh-external passes to ssh with -i. This fixes WAGON-446, on runtimes where the agent can be reached.
Two problems, both real. The agent connectors were handed to the guard as constructor references. A constructor reference resolves its class when the reference is evaluated, which happens at the call site -- outside the guard it was meant to be protected by -- so a missing connector class threw NoClassDefFoundError past the catch instead of skipping that agent. Verified with a cut-down case: the constructor-reference form escapes the guard where a lambda body is caught by it. Written as lambda bodies the class is not mentioned until the body runs, which is inside the try. Preferring id_ed25519 could also fail a connection outright. The JDK grew EdDSA in Java 15 and this branch still targets Java 8, where JSch can only do Ed25519 through a provider such as Bouncy Castle, which is not a dependency here -- the base SignatureEdDSA in the JSch jar says as much: "SignatureEdDSA requires Java15+". So on Java 8 a stray id_ed25519 would be chosen and fail even with a usable id_rsa next to it. Ed25519 is now demoted rather than dropped when the runtime cannot use it, so a key that works wins, while an id_ed25519 that is the only key present is still found -- which is what wagon-ssh-external needs, since the host's own scp does the cryptography there and the JVM's capabilities say nothing about it.
|
Both review comments were right; fixed in the last commit, and the branch is rebased onto The agent guard. The constructor references did defeat it. A constructor reference Written as The key preference. Also right, and it matters here because this branch targets Java 8. I demoted it rather than skipping it. Skipping outright would break the case where Full reactor green, and |
35a4e29 to
49fdce8
Compare
The same two fixes made on the 3.x side in apache#902. The agent connectors were handed to the guard as constructor references. A constructor reference resolves its class when the reference is evaluated, which happens at the call site -- outside the guard it was meant to be protected by -- so a missing connector class threw NoClassDefFoundError past the catch instead of skipping that agent. Written as lambda bodies the class is not mentioned until the body runs, which is inside the try. Preferring id_ed25519 could also fail a connection outright. The JDK grew EdDSA in Java 15 and this project still targets Java 8, where JSch can only do Ed25519 through a provider such as Bouncy Castle, which is not a dependency here. So a stray id_ed25519 would be chosen and fail even with a usable id_rsa next to it. Ed25519 is now demoted rather than dropped when the runtime cannot use it, so a key that works wins, while an id_ed25519 that is the only key present is still found -- which is what wagon-ssh-external needs, since the host's own scp does the cryptography there.
Two commits, separable: the first swaps the SSH library, the second fixes an authentication bug that is independent of it.
Fixes #901. Fixes #503. Carries forward #680.
1. Move to the maintained JSch fork
com.jcraft:jsch:0.1.55was last released in 2018. The provider is not merely unmaintained, it is already broken against current keys — here is 0.1.55 against two keysssh-keygenproduced with defaults on this machine:It cannot read the OpenSSH-v1 format
ssh-keygenhas emitted by default since OpenSSH 7.8, encrypted or not, and it has norsa-sha2-*, so RSA authentication fails against OpenSSH 8.8 and later anyway.com.github.mwiede:jsch:2.28.6keeps thecom.jcraft.jschpackage, so the API is unchanged. Its base bytecode is Java 8.Two things needed care rather than a straight coordinate swap:
The agent. The
jsch.agentproxyartifacts have no counterpart, and theirConnectorFactoryused to pick between agent kinds. The choice is now explicit and still covers all three the old stack could reach —SSH_AUTH_SOCK, Win32 OpenSSH, Pageant. One capability does narrow: reaching theSSH_AUTH_SOCKagent needs a Unix domain socket, which the JDK provides only from Java 16, and the fork declares no dependencies, so on Java 8–15 that particular agent is out of reach without a helper library on the class path. The other two need nothing. An agent holding no identities is now skipped rather than installed as an empty repository.Passphrases.
getPrivateKey()substitutes an empty passphrase when the settings name none. The old JSch discarded the result ofsetPassphrase, so an encrypted key stayed encrypted and was unlocked later; the fork rejects it instead. Left alone, that would have failed every encrypted key. Empty is now translated back to none before the key is added:2. Stop a stray key file shadowing the agent (#503)
The agent was consulted only when no key file was found, and
getPrivateKey()finds one whenever no password is configured. On any machine with a key in~/.sshthe agent was therefore unreachable — which is why the reported workaround was to pointwagon.privateKeyDirectoryat an empty directory.The order is now: a key named in
settings.xml, then the agent, then a key file that merely happens to be in~/.ssh. Naming a key keeps today's behaviour.This is a behaviour change to note in the release notes. A reachable agent holding identities now outranks a discovered key file, so someone whose agent holds unrelated keys while the needed key sits unloaded in
~/.sshmust load it or name it. Combining both is not available to us: adding a file identity on top of an agent repository makes JSch push that key into the user's agent.Also in this commit, key discovery looks for
id_ed25519,id_ecdsa,id_rsainstead ofid_dsathenid_rsa. That code is inScpHelper, so it also changes which keywagon-ssh-externalpasses tossh -i.Verification, and its limits
Modules build,
spotless:checkclean, default test run green.The change is not verified by the project's own SSH tests, because those do not run. The
-Dssh-testsprofile is excluded by default, and on the unmodified branch point it gives 112 tests / 94 errors —ComponentLookupException: Unable to lookup component 'org.apache.maven.wagon.Wagon', roleHint: scp, i.e. plexus test wiring rather than anything SSH. My branch gives exactly the same 112 / 94, so this change neither helps nor hurts it, but it means the embedded-MINA-server suite that would genuinely exercise a library swap is unavailable. That harness deserves fixing before this is relied on, and it is worth doing regardless of this PR.What was verified directly is the key-handling matrix above, run against both libraries.
Not yet done, and worth someone's hands before release: a real
site-deployover scp and sftp against a current OpenSSH, on both Java 11 and Java 17, and one Windows check of the agent path.