[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718592#comment-16718592
]
Thomas Wolf edited comment on SSHD-708 at 12/12/18 8:26 AM:
------------------------------------------------------------
This would have needed a review.
* Thanks for the attribution, but it wouldn't have been necessary. Most of the
code I had written is not in that commit anyway but in the parent commit.
* It would have helped if you had created your own PR for this. Now it's
rather difficult for me to comment on this. :(
* It's *broken*.
[MAX_ROUNDS|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L58]
is wrong. OpenSSH does *not* store an exponent for the rounds but the number
of rounds. I even had a comment in my code pointing this out. The code as is
will fail to decode a key generated with {{ssh-keygen -t ed25519 -a 33}}.
Besides, I don't understand the comment about "doubling". What doubles?
* I very much like the addition of tests for the password-provider re-try loop.
* Setting local variables to {{null}} at the end of methods doesn't improve
"getting rid of sensitive data", does it? For arrays, yes, but if a password
comes in as a String, there's not much one can do.
*
[bcryptKdf|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L192]
doesn't throw any checked exceptions. The [catch block you added
above|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L167]
should actually be inside {{bcryptKdf}}, and it should not handle
{{IOException}} (never thrown there), only {{GeneralSecurityException}}. This
should simply be as in my PR.
* Otherwise it's very hard to review this because the code has changed very
much, has become IMO needlessly general and complicated, and the individual
commits include unrelated formatting changes.
was (Author: wolft):
This would have need a review.
* Thanks for the attribution, but it wouldn't have been necessary. Most of the
code I had written is not in that commit anyway but in the parent commit.
* It would have helped if you had created your own PR for this. Now it's
rather difficult for me to comment on this. :(
* It's *broken*.
[MAX_ROUNDS|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L58]
is wrong. OpenSSH does *not* store an exponent for the rounds but the number
of rounds. I even had a comment in my code pointing this out. The code as is
will fail to decode a key generated with {{ssh-keygen -t ed25519 -a 33}}.
Besides, I don't understand the comment about "doubling". What doubles?
* I very much like the addition of tests for the password-provider re-try loop.
* Setting local variables to {{null}} at the end of methods doesn't improve
"getting rid of sensitive data", does it? For arrays, yes, but if a password
comes in as a String, there's not much one can do.
*
[bcryptKdf|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L192]
doesn't throw any checked exceptions. The [catch block you added
above|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L167]
should actually be inside {{bcryptKdf}}, and it should not handle
{{IOException}} (never thrown there), only {{GeneralSecurityException}}. This
should simply be as in my PR.
* Otherwise it's very hard to review this because the code has changed very
much, has become IMO needlessly general and complicated, and the individual
commits include unrelated formatting changes.
> Add support for password encrypted OpenSSH private key files
> ------------------------------------------------------------
>
> Key: SSHD-708
> URL: https://issues.apache.org/jira/browse/SSHD-708
> Project: MINA SSHD
> Issue Type: Improvement
> Affects Versions: 1.4.0
> Reporter: Goldstein Lyor
> Assignee: Goldstein Lyor
> Priority: Minor
> Fix For: 2.1.1
>
>
> The current code supports only reading un-encrypted private key files
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
