[ https://issues.apache.org/jira/browse/SSHD-860?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16679404#comment-16679404 ]
Thomas Wolf commented on SSHD-860: ---------------------------------- Thanks for the effort. But it doesn't work yet. # 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". # 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. And actually I should test (1) in the solution I implemented in JGit, too. I noticed that the JGit tests exercise this only with iterators containing one element each, so I wouldn't have noticed. I need to add a test in JGit with several identities per iterator. I suspect I'll then see that same behavior in JGit, too. :( > 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)