mina-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Thomas Wolf (JIRA)" <j...@apache.org>
Subject [jira] [Comment Edited] (SSHD-708) Add support for password encrypted OpenSSH private key files
Date Wed, 12 Dec 2018 08:49:00 GMT

    [ 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:48 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.
 * [This constructor|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L76]
will bypass the length check on the salt in [Line 81|https://github.com/apache/mina-sshd/blob/master/sshd-common/src/main/java/org/apache/sshd/common/config/keys/loader/openssh/kdf/BCryptKdfOptions.java#L81].
Since this constructor is never used, why add it at all?
 * 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 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.

> 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)

Mime
View raw message