[jira] [Reopened] (SSHD-708) Add support for password encrypted OpenSSH private key files
[ https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Goldstein Lyor reopened SSHD-708: - https://github.com/apache/mina-sshd/pull/82 > 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)
[jira] [Work started] (SSHD-708) Add support for password encrypted OpenSSH private key files
[ https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Work on SSHD-708 started by Goldstein Lyor. --- > 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)
[jira] [Created] (SSHD-875) Propagate connection attributes repository to HostConfigEntryResolver
Goldstein Lyor created SSHD-875:
---
Summary: Propagate connection attributes repository to
HostConfigEntryResolver
Key: SSHD-875
URL: https://issues.apache.org/jira/browse/SSHD-875
Project: MINA SSHD
Issue Type: Task
Affects Versions: 2.1.1
Reporter: Goldstein Lyor
Assignee: Goldstein Lyor
Following SSHD-859 we now have the capability to propagate a user-defined
connection {{AttributeRepository}} context when network connection is
established. We should propagate this context to the
{{HostConfigEntryResolver}} as well.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[ https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718975#comment-16718975 ] Goldstein Lyor commented on SSHD-708: - [~wolft] - as promised made the changes and published the [pull request|https://github.com/apache/mina-sshd/pull/82] - I would appreciate your feedback (no hurry, within the next few days...). > 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)
[GitHub] mina-sshd pull request #82: [SSHD-708] Fixed support for OpenSSH encrypted p...
GitHub user lgoldstein opened a pull request: https://github.com/apache/mina-sshd/pull/82 [SSHD-708] Fixed support for OpenSSH encrypted private keys decoding when 'bcrypt' KDF is used * Increased the limit on `MAX_ROUNDS` * Made the limitation configurable via system property * Updated `README.md` file about the `bcrypt` settings and behavior regarding the `MAX_ROUNDS` * Removed dangerous constructor/initializer of the KDF options You can merge this pull request into a Git repository by running: $ git pull https://github.com/lgoldstein/mina-sshd SSHD-708 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/mina-sshd/pull/82.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #82 commit 687571f6f4c6c448d56deb98eb76d87ad622d7ff Author: Lyor Goldstein Date: 2018-12-12T13:23:04Z [SSHD-708] Fixed support for OpenSSH encrypted private keys decoding when 'bcrypt' KDF is used ---
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[ https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718969#comment-16718969 ] ASF GitHub Bot commented on SSHD-708: - GitHub user lgoldstein opened a pull request: https://github.com/apache/mina-sshd/pull/82 [SSHD-708] Fixed support for OpenSSH encrypted private keys decoding when 'bcrypt' KDF is used * Increased the limit on `MAX_ROUNDS` * Made the limitation configurable via system property * Updated `README.md` file about the `bcrypt` settings and behavior regarding the `MAX_ROUNDS` * Removed dangerous constructor/initializer of the KDF options You can merge this pull request into a Git repository by running: $ git pull https://github.com/lgoldstein/mina-sshd SSHD-708 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/mina-sshd/pull/82.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #82 commit 687571f6f4c6c448d56deb98eb76d87ad622d7ff Author: Lyor Goldstein Date: 2018-12-12T13:23:04Z [SSHD-708] Fixed support for OpenSSH encrypted private keys decoding when 'bcrypt' KDF is used > 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)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718934#comment-16718934
]
Goldstein Lyor commented on SSHD-708:
-
{quote}
OpenSSH doesn't limit this; any value in the range [1 .. INT_MAX] is allowed.
IMO we shouldn't worry about unreasonably large values here; this is reading a
private key of a user. If the user created that key with 2**30 rounds, so be
it. The code should just guard against rounds < 1.
{quote}
This is where our philosophies differ - I believe I will make it configurable
via a system property (just in case) with a default of 2^16. Thanks for the
feedback - I will make the necessary changes and publish a PR for more feedback
from you before merging so we can avoid a back and forth on the issue.
> 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)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718870#comment-16718870
]
Thomas Wolf commented on SSHD-708:
--
{quote}
The code is *generic* - i.e., if we ever replace the {{bcrypt}} with something
else, this is how the I would like the {{catch}} block to behave, so I don't
see the harm in writing the code in this manner now rather than later.
{quote}
I had already noticed that we have quite different philosophies about this. :-)
I'm nearer the opposite end of the spectrum: do what's needed and not more, and
generalize later when and if a need arises. Yes, with a general _library_ one
needs to plan and build in _some_ genericity and configurability from the
beginning, but even then I like to keep interfaces as narrow and simple as
possible and generalize further only when there's a real need for it. I don't
think OpenSSH will switch from bcrypt to something else soon...
Anyway, it's allright as it is.
> 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)
[jira] [Comment Edited] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718819#comment-16718819
]
Thomas Wolf edited comment on SSHD-708 at 12/12/18 11:26 AM:
-
{quote}
What I am trying to do is prevent some kind of "attack" by providing a
malicious (or corrupted) value that would cause the code to "hang" by executing
a very large number of round
{quote}
OpenSSH doesn't limit this; any value in the range [1 .. INT_MAX] is allowed.
IMO we shouldn't worry about unreasonably large values here; this is reading a
_private_ key of a user. If the user created that key with 2**30 rounds, so be
it. The code should just guard against rounds < 1.
Re attribution: of course it's a community effort. But with so many changes and
the code I provided spread even over two commits, one authored by you and a
second small one with my name on it, it isn't really worth the trouble. It's no
big deal; just that I would have done this differently. (Merge the PR, maybe
with just a little amend to remove the {{MessageFormat}}, then rebase my own
work on top of that merge and continue from there on.) But as I said, no big
deal.
was (Author: wolft):
{quote}What I am trying to do is prevent some kind of "attack" by providing a
malicious (or corrupted) value that would cause the code to "hang" by executing
a very large number of round\{quote}
OpenSSH doesn't limit this; any value in the range [1 .. INT_MAX] is allowed.
IMO we shouldn't worry about unreasonably large values here; this is reading a
_private_ key of a user. If the user created that key with 2**30 rounds, so be
it. The code should just guard against rounds < 1.
Re attribution: of course it's a community effort. But with so many changes and
the code I provided spread even over two commits, one authored by you and a
second small one with my name on it, it isn't really worth the trouble. It's no
big deal; just that I would have done this differently. (Merge the PR, maybe
with just a little amend to remove the {{MessageFormat}}, then rebase my own
work on top of that merge and continue from there on.) But as I said, no big
deal.
> 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)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718819#comment-16718819
]
Thomas Wolf commented on SSHD-708:
--
{quote}What I am trying to do is prevent some kind of "attack" by providing a
malicious (or corrupted) value that would cause the code to "hang" by executing
a very large number of round\{quote}
OpenSSH doesn't limit this; any value in the range [1 .. INT_MAX] is allowed.
IMO we shouldn't worry about unreasonably large values here; this is reading a
_private_ key of a user. If the user created that key with 2**30 rounds, so be
it. The code should just guard against rounds < 1.
Re attribution: of course it's a community effort. But with so many changes and
the code I provided spread even over two commits, one authored by you and a
second small one with my name on it, it isn't really worth the trouble. It's no
big deal; just that I would have done this differently. (Merge the PR, maybe
with just a little amend to remove the {{MessageFormat}}, then rebase my own
work on top of that merge and continue from there on.) But as I said, no big
deal.
> 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)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718803#comment-16718803
]
Goldstein Lyor commented on SSHD-708:
-
Hi [~wolft],
Sorry about the PR, but I thought everything is in order since all tests passed
- including the ones with the encryption.
{quote}
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.
{quote}
I usually like to acknowledge those who contributed to the code since it is a
*community* effort.
{quote}
It's broken. MAX_ROUNDS 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?
{quote}
As I was reading the {{BCrypt.java}} code and its comments I came across the
following:
{quote}
The amount of work increases exponentially (2**log_rounds), so each increment
is twice as much work. The default log_rounds is 10, and the valid range is 4
to 30.
{quote}
and also this
{code:java}
public byte[] crypt_raw(byte password[], byte salt[], int log_rounds,
int cdata[]) {
int rounds, i, j;
int clen = cdata.length;
byte ret[];
if (log_rounds < 4 || log_rounds > 30)
throw new IllegalArgumentException ("Bad number of rounds");
{code}
This is what (mis-)led me to set a {{MAX_ROUNDS = 31}}. I see now that I was
looking at the wrong location since {{pbkdf}} uses it in a simple loop:
{code:java}
for (int round = 1; round < rounds; round++) {
sha512.reset();
sha512.update(tmp);
sha512.digest(hsalt, 0, hsalt.length);
}
{code}
What I am trying to do is prevent some kind of "attack" by providing a
malicious (or corrupted) value that would cause the code to "hang" by executing
a very large number of round (e.g., what if someone uses 2^31 rounds). While
the value is defined as {{unit32}} I would like to impose a reasonable limit
value on it so when we decode the KDF options we will know that it is wrong.
There are 2 options that I would value your opinion:
# Define a large hardcoded value to accommodate all reasonable values - 1024 ?
2048 ? 4096 ? ... ?
# Do the above as *default value*, but add a system property and/or session
property to allow users to define a different value.
{quote}
Setting local variables to null at the end of methods doesn't improve "getting
rid of sensitive data", does it? For arrays, yes, clearing the values should be
done, but if a password comes in as a String, there's not much one can do.
{quote}
In essence you are right, however, the variable is holding a reference to the
object - by null-ifying it I am hinting to the GC that it can be disposed of.
Especially the password that is part of a loop. I agree that there are no
guarantees, but in the spirit of zero-ing key related data a.s.a.p. I don't see
the harm even if there is no GC.
{quote}
This constructor will bypass the length check on the salt in Line 81. Since
this constructor is never used, why add it at all?
{quote}
Agreed - I actually wrote it before incorporating your code and forgot to
remove it - will do so.
{quote}
bcryptKdf doesn't throw any checked exceptions. The catch block you added above
should actually be inside bcryptKdf, and it should not handle IOException
(never thrown there), only GeneralSecurityException. This should simply be as
in my PR.
{quote}
The code is *generic* - i.e., if we ever replace the {{bcrypt}} with something
else, this is how the I would like the {{catch}} block to behave, so I don't
see the harm in writing the code in this manner now rather than later.
{quote}
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.
{quote}
You are right in essence - sometimes I include formatting changes as part of
the commit since I don't want to use a separate commit only for them. I try to
resist this tendency, but sometimes I just can't help it ... :-).
I will publish a PR on the changes once done.
> 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 priva
[jira] [Comment Edited] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
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:50 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, clearing the values
should be done, 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.
* [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.
> Add support for
[jira] [Comment Edited] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
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 rea
[jira] [Comment Edited] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
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)
[jira] [Commented] (SSHD-708) Add support for password encrypted OpenSSH private key files
[
https://issues.apache.org/jira/browse/SSHD-708?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16718592#comment-16718592
]
Thomas Wolf commented on SSHD-708:
--
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)
