[
https://issues.apache.org/jira/browse/SSHD-860?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16680150#comment-16680150
]
Goldstein Lyor commented on SSHD-860:
-------------------------------------
Hi [~wolft]
{quote}
The first call to current.hasNext() in UserAuthPublicKeyIterator now still
loads the two keys of the first KeyPair iterator. Basically
LazyIterablesConcatenator.lazyConcatenateIterables(one, two) isn't lazy yet.
There's pre-loading going on for "one", and once "one" is exhausted and the
first hasNext() on "two" is called, there's pre-loading happening on "two".
{quote}
I am not sure I understand how this happens - after all, calling {{hasNext()}}
actually indicates that someone is going to call {{next()}} - so I don't see
how 2 instances are pre-loaded. I will review the code some more and try to
figure out if/how this happens. The only way I can see it happening is if
{{hasNext()}} is called twice (or more) without calling {{next()}}. But even in
this case, there is no "pre-loading" just a code error of having skipped a
key-pair.
{quote}
It is legal to call Iterator.next() without having called Iterator.hasNext()
before. So all those cases where the new code does throw new
IllegalStateException("'next()' called without a preceding 'hasNext()' query");
are wrong.
{quote}
I am not so sure - but I'll review the javadoc and see. If it is as you
describe I'll try and see if I can find the time to fix this.
{quote}
even with the new code, ends up using these newfangled spliterators,
{quote}
Can you indicate the code that uses these "newfangled spliterators" ? I'd like
to take a look at it and see if I can "un-fangle" it... :)
Thanks for the quick test and feedback. If you come up with some ides for a fix
please publish a PR - I'll be more than happy to review it and eventually merge
it into the code.
> 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)
