[jira] [Comment Edited] (SSHD-860) org.apache.sshd.client.auth.pubkey.UserAuthPublicKeyIterator pre-loads client identities (private key files)

["Thomas Wolf (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/SSHD-860?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16680201#comment-16680201 ] 

Thomas Wolf edited comment on SSHD-860 at 11/8/18 7:10 PM:
-----------------------------------------------------------

{quote}I am not sure I understand how this happens
{quote}
It means that somewhere along the line, something still uses a stream operation to obtain an iterator. An iterator on a stream will be such a spliterator, and those buffer.
{quote}Can you indicate the code that uses these "newfangled spliterators" ?
{quote}
As far as I see, the culprit is this call chain:
 * UserAuthPublicKeyIterator.initializeSessionIdentities()
 * ClientSession.providerOf(session)
 * KeyIdentityProvider.resolveKeyIdentityProvider(session.getRegisteredIdentities(), session.getKeyPairProvider())
 * KeyIdentityProvider.multiProvider(identities, keys)
 * KeyIdentityProvider.multiProvider(GenericUtils.asList(providers))
 * KeyIdentityProvider.wrapKeyPairs(iterableOf(providers))
 ** iterableOf(providers) calls
 *** GenericUtils.multiIterableSuppliers(GenericUtils.wrapIterable(providers, p -> p::loadKeys))
 *** GenericUtils.wrapIterable(iter, mapper) returns {{() -> GenericUtils.wrapIterator(iter, mapper)}}
 **** GenericUtils.wrapIterator(iter, mapper) does {{return stream(iter).map(mapper).iterator()}}
 *** GenericUtils.multiIterableSuppliers(...) does {{return () -> stream(providers).flatMap(s -> stream(s.get())).map(Function.identity()).iterator()}}

I'm not sure which of the last two is the real problem here; tracing this through debugging is highly confusing once it enters hasNext(). Looks to me we have in the end a spliterator over a stream of spliterators (a spliterator of spliterators). Which also explains why in my JGit code it doesn't occur; I made a point of not using any stream operations in these key iterators.


was (Author: wolft):
{quote}I am not sure I understand how this happens
{quote}
It means that somewhere along the line, something still uses a stream operation to obtain an iterator. An iterator on a stream will be such a spliterator, and those buffer.
{quote}Can you indicate the code that uses these "newfangled spliterators" ?
{quote}
As far as I see, the culprit is this call chain:
 * UserAuthPublicKeyIterator.initializeSessionIdentities()
 * ClientSession.providerOf(session)
 * KeyIdentityProvider.resolveKeyIdentityProvider(session.getRegisteredIdentities(), session.getKeyPairProvider())
 * KeyIdentityProvider.multiProvider(identities, keys)
 * KeyIdentityProvider.multiProvider(GenericUtils.asList(providers))
 * KeyIdentityProvider.wrapKeyPairs(iterableOf(providers))
 ** iterableOf(providers) calls
 *** GenericUtils.multiIterableSuppliers(GenericUtils.wrapIterable(providers, p -> p::loadKeys)), which returns {{() -> GenericUtils.wrapIterator(iter, mapper)}}
 **** GenericUtils.wrapIterator(iter, mapper) does {{return stream(iter).map(mapper).iterator()}}
 *** GenericUtils.multiIterableSuppliers(...) does {{return () -> stream(providers).flatMap(s -> stream(s.get())).map(Function.identity()).iterator()}}

I'm not sure which of the last two is the real problem here; tracing this through debugging is highly confusing once it enters hasNext(). Looks to me we have in the end a spliterator over a stream of spliterators (a spliterator of spliterators). Which also explains why in my JGit code it doesn't occur; I made a point of not using any stream operations in these key iterators.

> 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
>            Assignee: Goldstein Lyor
>            Priority: Major
>             Fix For: 2.1.1
>
>
> {{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)