eclipse-jgit / jgit

JGit, the Java implementation of git
https://www.eclipse.org/jgit/
Other
92 stars 31 forks source link

Degrade: when ssh/config IdentityFile is secret key, raise exception java.io.StreamCorruptedException: Failed (IllegalArgumentException) to parse key entry=-----BEGIN OPENSSH PRIVATE KEY-----: Bad format (no key data delimiter): KEY----- #53

Closed miurahr closed 2 months ago

miurahr commented 2 months ago

Version

6.9.0.202403050737-r

Operating System

Linux/Unix

Bug description

Issue #25 changes to try a file path specified in IdentityFile then try with an file extension ".pub". When user use traditional configuration that IdentityFile path is secret key on file system, JGit and apache MINA sshd library raise an exception and show a stack trace. A problem is only raised when user also configure IdentiesOnly = yes.

Here is gerrit entry of the change https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/1177073/6/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java

In previous code,

Path p = Paths.get(s + ".pub"); //$NON-NLS-1$
if (Files.isRegularFile(p, LinkOption.NOFOLLOW_LINKS)) {
    return AuthorizedKeyEntry.readAuthorizedKeys(p).get(0).resolvePublicKey(null,   PublicKeyEntryResolver.IGNORING);
}

It just adding ".pub" and check it, then no exception was observed.

Actual behavior

Record stack trace in log file.

Expected behavior

Run without exception or don't show a stack trace.

I think we can check existence of public key before try a file path of IdentityFile specified.

Approach 1. Check existence of file path Path p = Paths.get(s + ".pub"); //$NON-NLS-1$ and if exists, try it first.

Approach 2. Check specified path endsWith ".pub"

There is not necessary to put a file name rule in a new approach that file system hold only a public key and secret key is in HSM, approach 2 is not stable for future. Old style always has "foo.pub" and "foo" key pair, so approach 1 is better.

Relevant log output

Cannot read public key from file /home/miurahr/.ssh/id_ed25519 
java.io.StreamCorruptedException: Failed (IllegalArgumentException) to parse key entry=-----BEGIN OPENSSH PRIVATE KEY-----: Bad format (no key data delimiter): KEY----- 
    at org.apache.sshd.common.config.keys.AuthorizedKeyEntry.readAuthorizedKeys(AuthorizedKeyEntry.java:247) 
    at org.apache.sshd.common.config.keys.AuthorizedKeyEntry.readAuthorizedKeys(AuthorizedKeyEntry.java:226) 
    at org.apache.sshd.common.config.keys.AuthorizedKeyEntry.readAuthorizedKeys(AuthorizedKeyEntry.java:211) 
    at org.apache.sshd.common.config.keys.AuthorizedKeyEntry.readAuthorizedKeys(AuthorizedKeyEntry.java:195) 
    at org.apache.sshd.common.config.keys.KeyUtils.loadPublicKey(KeyUtils.java:342) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.readPublicKey(JGitPublicKeyAuthentication.java:353) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.lambda$0(JGitPublicKeyAuthentication.java:330) 
    at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) 
    at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) 
    at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) 
    at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) 
    at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921) 
    at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) 
    at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:682) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.getExplicitKeys(JGitPublicKeyAuthentication.java:337) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.identitiesOnly(JGitPublicKeyAuthentication.java:434) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.initializeAgentIdentities(JGitPublicKeyAuthentication.java:383) 
    at org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator.<init>(UserAuthPublicKeyIterator.java:59) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication$KeyIterator.<init>(JGitPublicKeyAuthentication.java:320) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication.createPublicKeyIterator(JGitPublicKeyAuthentication.java:137) 
    at org.apache.sshd.client.auth.pubkey.UserAuthPublicKey.init(UserAuthPublicKey.java:109) 
    at org.eclipse.jgit.internal.transport.sshd.JGitPublicKeyAuthentication.init(JGitPublicKeyAuthentication.java:123) 
    at org.apache.sshd.client.session.ClientUserAuthService.tryNext(ClientUserAuthService.java:410) 
    at org.apache.sshd.client.session.ClientUserAuthService.processUserAuth(ClientUserAuthService.java:331) 
    at org.apache.sshd.client.session.ClientUserAuthService.process(ClientUserAuthService.java:267) 
    at org.apache.sshd.common.session.helpers.CurrentService.process(CurrentService.java:109) 
    at org.apache.sshd.common.session.helpers.AbstractSession.doHandleMessage(AbstractSession.java:624) 
    at org.apache.sshd.common.session.helpers.AbstractSession.lambda$handleMessage$0(AbstractSession.java:545) 
    at org.apache.sshd.common.util.threads.ThreadUtils.runAsInternal(ThreadUtils.java:68) 
    at org.apache.sshd.common.session.helpers.AbstractSession.handleMessage(AbstractSession.java:544) 
    at org.apache.sshd.common.session.helpers.AbstractSession.decode(AbstractSession.java:1688) 
    at org.apache.sshd.common.session.helpers.AbstractSession.messageReceived(AbstractSession.java:505) 
    at org.eclipse.jgit.internal.transport.sshd.JGitClientSession.messageReceived(JGitClientSession.java:208) 
    at org.apache.sshd.common.session.helpers.AbstractSessionIoHandler.messageReceived(AbstractSessionIoHandler.java:64) 
    at org.apache.sshd.common.io.nio2.Nio2Session.handleReadCycleCompletion(Nio2Session.java:409) 
    at org.apache.sshd.common.io.nio2.Nio2Session$1.onCompleted(Nio2Session.java:382) 
    at org.apache.sshd.common.io.nio2.Nio2Session$1.onCompleted(Nio2Session.java:377) 
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.lambda$completed$0(Nio2CompletionHandler.java:38) 
    at java.base/java.security.AccessController.doPrivileged(AccessController.java:318) 
    at org.apache.sshd.common.io.nio2.Nio2CompletionHandler.completed(Nio2CompletionHandler.java:37) 
    at java.base/sun.nio.ch.Invoker.invokeUnchecked(Invoker.java:129) 
    at java.base/sun.nio.ch.Invoker$2.run(Invoker.java:221) 
    at java.base/sun.nio.ch.AsynchronousChannelGroupImpl$1.run(AsynchronousChannelGroupImpl.java:113) 
    at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) 
    at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) 
    at java.base/java.lang.Thread.run(Thread.java:840)

Other information

No response

miurahr commented 2 months ago

In the change in #25, it try to catch exception when trying reading a private key, but it did not catch because apache sshd raises StreamCorruptedException but not GeneralSecurityException.


            } catch (GeneralSecurityException e) {
                // ignore in case this is not a derived key path, as in most
                // cases this specifies a private key
                if (isDerived) {
                    log.warn("{}", //$NON-NLS-1$
                            format(SshdText.get().cannotReadPublicKey, keyFile),
                            e);
                }
tomaswolf commented 2 months ago

Gerrit change 1194667 will fix that.