[
https://issues.apache.org/jira/browse/SSHD-860?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Thomas Wolf updated SSHD-860:
-----------------------------
Description:
{{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} loads and
caches in memory private keys prematurely. Keys are loaded even if they are not
used at all in the end. In other words, incremental loading of keys doesn't
work.
This is bad for two reasons:
# Private keys should be kept in memory only if and when needed. Loading
completely unused private keys must be avoided.
# With encrypted private keys, the user may end up being asked for passphrases
for keys that are not used at all in the end, which is highly disruptive.
{{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} does in its
constructor:
{code:java}
Iterator<? extends PublicKeyIdentity> current;
Collection<Stream<? extends PublicKeyIdentity>> identities = new LinkedList<>();
...
identities.add(Stream.of(ClientSession.providerOf(session))
.map(KeyIdentityProvider::loadKeys)
.flatMap(GenericUtils::stream)
.map(kp -> new KeyPairIdentity(signatureFactories, session, kp)));
current = identities.stream().flatMap(r -> r).iterator();
{code}
The final {{current}} iterator uses some internal buffer (size unknown; didn't
try to determine it) and will pre-fill this buffer completely. So with buffer
size _N_ it'll pre-load the first _N_ keys from the combined identity stream.
If the first key authenticates successfully, loading all the others must not be
done.
See my [test
case|https://github.com/tomaswolf/mina-sshd/blob/SSHD-860/sshd-core/src/test/java/org/apache/sshd/client/ClientKeyLoadTest.java]
showing this faulty behavior. It does exactly the same as the
{{UserAuthPublicKeyIterator}} constructor, using two iterators with two
identity files each, and then tests the resulting iterator. The first
{{hasNext()/next()}} call on the {{current}} iterator _loads all four keys!_
was:
{{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} loads and
caches in memory private keys prematurely. Keys are loaded even if they are not
used at all in the end. In other words, incremental loading of keys doesn't
work.
This is bad for two reasons:
# Private keys should be kept in memory only if and when needed. Loading
completely unused private keys must be avoided.
# With encrypted private keys, the user may end up being asked for passphrases
for keys that are not used at all in the end, which is highly disruptive.
{{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} does in its
constructor:
{code:java}
Iterator<? extends PublicKeyIdentity> current;
Collection<Stream<? extends PublicKeyIdentity>> identities = new LinkedList<>();
...
identities.add(Stream.of(ClientSession.providerOf(session))
.map(KeyIdentityProvider::loadKeys)
.flatMap(GenericUtils::stream)
.map(kp -> new KeyPairIdentity(signatureFactories, session, kp)));
current = identities.stream().flatMap(r -> r).iterator();
{code}
The final {{current}} iterator uses some internal buffer (size unknown; didn't
try to determine it) and will pre-fill this buffer completely. So with buffer
size _N_ it'll pre-load the first _N_ keys from the combined identity stream.
If the first key authenticates successfully, loading all the others must not be
done.
See my [test case|https://github.com/tomaswolf/mina-sshd/commit/a71c62e66]
showing this faulty behavior. It does exactly the same as the
{{UserAuthPublicKeyIterator}} constructor, using two iterators with two
identity files each, and then tests the resulting iterator. The first
{{hasNext()/next()}} call on the {{current}} iterator _loads all four keys!_
Link to test case updated.
I'm a bit surprised you consider this "minor"... note that this also means that
a serious sshd client cannot use {{UserAuthPublicKeyIterator}} but needs to
provide its own implementation, which is not possible without some minor
hacking. See for instance what we do in JGit to get around this problem:
*
[JGitPublicKeyIterator.java|https://git.eclipse.org/r/#/c/131884/6/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyIterator.java]
*
[JGitPublicKeyAuthentication.java|https://git.eclipse.org/r/#/c/131884/6/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthentication.java]
– not calling {{super.init()}} is the "minor hack" I referred to above
*
[JGitPublicKeyAuthFactory.java|https://git.eclipse.org/r/#/c/131884/6/org.eclipse.jgit.ssh.apache/src/org/eclipse/jgit/internal/transport/sshd/JGitPublicKeyAuthFactory.java]
> org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator pre-loads client
> identities (private key files)
> ------------------------------------------------------------------------------------------------------------
>
> Key: SSHD-860
> URL: https://issues.apache.org/jira/browse/SSHD-860
> Project: MINA SSHD
> Issue Type: Bug
> Affects Versions: 2.0.0, 2.1.0, 2.1.1
> Reporter: Thomas Wolf
> Priority: Minor
>
> {{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} loads and
> caches in memory private keys prematurely. Keys are loaded even if they are
> not used at all in the end. In other words, incremental loading of keys
> doesn't work.
> This is bad for two reasons:
> # Private keys should be kept in memory only if and when needed. Loading
> completely unused private keys must be avoided.
> # With encrypted private keys, the user may end up being asked for
> passphrases for keys that are not used at all in the end, which is highly
> disruptive.
> {{org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator}} does in its
> constructor:
> {code:java}
> Iterator<? extends PublicKeyIdentity> current;
> Collection<Stream<? extends PublicKeyIdentity>> identities = new
> LinkedList<>();
> ...
> identities.add(Stream.of(ClientSession.providerOf(session))
> .map(KeyIdentityProvider::loadKeys)
> .flatMap(GenericUtils::stream)
> .map(kp -> new KeyPairIdentity(signatureFactories, session, kp)));
> current = identities.stream().flatMap(r -> r).iterator();
> {code}
> The final {{current}} iterator uses some internal buffer (size unknown;
> didn't try to determine it) and will pre-fill this buffer completely. So with
> buffer size _N_ it'll pre-load the first _N_ keys from the combined identity
> stream. If the first key authenticates successfully, loading all the others
> must not be done.
> See my [test
> case|https://github.com/tomaswolf/mina-sshd/blob/SSHD-860/sshd-core/src/test/java/org/apache/sshd/client/ClientKeyLoadTest.java]
> showing this faulty behavior. It does exactly the same as the
> {{UserAuthPublicKeyIterator}} constructor, using two iterators with two
> identity files each, and then tests the resulting iterator. The first
> {{hasNext()/next()}} call on the {{current}} iterator _loads all four keys!_
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
