Move wagon-ssh to the maintained JSch fork and stop a stray key shadowing the agent - #903
Move wagon-ssh to the maintained JSch fork and stop a stray key shadowing the agent#903slachiewicz wants to merge 3 commits into
Conversation
2b479f0 to
0d1a96f
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. Same change as #902. 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. |
slachiewicz
added
dependencies
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 (retaining the com.jcraft.jsch API) and adjusts SSH authentication behavior to prefer explicitly configured keys first, then SSH agents, and only then auto-discovered keys in ~/.ssh. It also updates default private-key discovery to prefer modern key types.
Changes:
- Swaps
wagon-sshfromcom.jcraft:jsch+jsch.agentproxytocom.github.mwiede:jsch:2.28.6and replaces agentproxy integration with the fork’s built-in agent connectors. - Changes authentication precedence so an available agent is consulted before a “found” key file in
~/.ssh(while preserving “configured key wins” behavior). - Updates private-key discovery to check
id_ed25519,id_ecdsa, thenid_rsa(droppingid_dsa).
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 | Replaces agentproxy usage with fork-native agent connectors and changes auth method precedence; normalizes empty passphrase handling when adding identities. |
| wagon-providers/wagon-ssh/pom.xml | Updates dependency coordinates to the maintained JSch fork and removes agentproxy dependencies. |
| wagon-providers/wagon-ssh-common/src/main/java/org/apache/maven/wagon/providers/ssh/ScpHelper.java | Updates default key discovery order to prefer modern OpenSSH key types. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Ordering note, now that #904 has merged to Once #915 is in I will rebase this and re-run |
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.
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.
0d1a96f to
35ed71e
Compare
|
Rebased onto master and brought over the two fixes from #902, now that #915 has landed and The agent guard. The constructor references defeated it. A constructor reference resolves The key preference. master targets Java 8 as well, and the JDK only grew EdDSA in Java 15; Verified against the suite this time, which is the whole point of taking #915 first: One note for review: I applied the |
Same change as #902, on
master. The cherry-pick was clean.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.