Bug 541703 - ssh: support reading encrypted new-style OpenSSH private keys, especially encrypted ed25519 keys
Summary: ssh: support reading encrypted new-style OpenSSH private keys, especially enc...
Status: RESOLVED FIXED
Alias: None
Product: JGit
Classification: Technology
Component: JGit (show other bugs)
Version: 5.2   Edit
Hardware: All All
: P3 enhancement (vote)
Target Milestone: 5.4   Edit
Assignee: Project Inbox CLA
QA Contact:
URL:
Whiteboard:
Keywords: noteworthy
Depends on: 541272 541425
Blocks: 518745
  Show dependency tree
 
Reported: 2018-11-29 10:01 EST by Thomas Wolf CLA
Modified: 2019-05-06 19:14 EDT (History)
1 user (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Thomas Wolf CLA 2018-11-29 10:01:38 EST
Format specification: 

* https://github.com/openssh/openssh-portable/blob/master/PROTOCOL.key

Apache MINA sshd can read this format, but accepts only the "none" cipher and the "none" KDF. To read encrypted keys (and in particular encrypted ed25519 keys, which OpenSSH always writes in this format), one would need to accept other ciphers (such as "aes256-cbc"; no problem there) and the "bcrypt" KDF.

Encryption:

* https://github.com/openssh/openssh-portable/blob/master/openbsd-compat/bcrypt_pbkdf.c
* https://github.com/openssh/openssh-portable/blob/master/openbsd-compat/blowfish.c

Uses an EksBlowfish algorithm with magic string "OxychromaticBlowfishSwatDynamite".

* Bouncy Castle's org.bouncycastle.crypto.generators.BCrypt has a different EKS (expensive key setup) and uses the older magic string "OrpheanBeholderScryDoubt"
* Bouncy Castle's org.bouncycastle.crypto.engines.BlowfishEngine has no EKS step.

Both are final classes; crucial data is in private fields: it's not possible to subclass or otherwise modify them to do what the bcrypt used in OpenSSH needs.

* Damien Miller's jBCrypt https://github.com/djmdjm/jBCrypt/blob/master/src/org/mindrot/jbcrypt/BCrypt.java also does not fit the bill; it also uses the old magic string. Again crucial data is in private fields; subclassing again is not going to work.

* sshj contains a modified copy of jBCrypt https://github.com/hierynomus/sshj/blob/master/src/main/java/org/mindrot/jbcrypt/BCrypt.java; the modifications implement the bcrypt_pbkdf of OpenSSH. This is the only Java implementation of this algorithm.

So it looks that one has to copy an existing Blowfish/Bcrypt to get the magic string and the EKS right, and one has to re-implement bcrypt_pdkdf.

I have _no_ idea if doing this would be compatible in any way with the Eclipse legal framework. The sources mentioned all have licences that doing would allow it, but it would mean JGit would contain non-EDL code.

If upstream (Apache MINA sshd) does this, then we have the same uncertainty when we want to include such a future sshd version when we'd do the CQs for that.

In short: I think this needs guidance from the legal team. What steps would need to be taken if we did such copy-and-adapt-and-reimplement in JGit, and what would we need to pay attention to if we did the same upstream?
Comment 1 Thomas Wolf CLA 2018-11-29 16:57:03 EST
The sshj version apparently comes from a fork of Damien Miller's jBCrypt at https://github.com/kruton/jbcrypt/commit/37a5a774565b7eaf9e58f0fadba1291a3a06649f . The changes were never merged back into Damien's version; see https://github.com/kruton/jbcrypt/issues/1 .

Kruton (Kenny Root) has published this as maven artifact org.connectbot.jbcrypt:jbcrypt:1.0.0. See https://mvnrepository.com/artifact/org.connectbot.jbcrypt/jbcrypt/1.0.0 .

The license has been left unchanged (still ISC).

So one way forward here might be to use org.connectbot.jbcrypt. It contains the required implementation of bcrypt_pbkdf.

Now before I create a pull request for upstream Apache MINA sshd implementing this based on this org.connectbot.jbcrypt artifact it would be good if we already knew if we could have that in Orbit.

So we'd need a CQ for org.connectbot.jbcrypt:jbcrypt:1.0.0. My analysis of the source repo at https://github.com/kruton/jbcrypt makes me think that this is indeed just the ISC-licensed original from Damien Miller from https://github.com/djmdjm/jBCrypt, with original input done by Kenny Root at https://github.com/kruton/jbcrypt/commit/37a5a774565b7eaf9e58f0fadba1291a3a06649f and still ISC licensed.

I have verified that using this library we can build an OpenSSH key file parser that can successfully read and decrypt encoded OpenSSH ed25519 keys. But doing this in either JGit or in upstream Apache MINA sshd is moot unless we get this into Orbit.
Comment 2 Matthias Sohn CLA 2018-11-29 17:49:13 EST
I think your proposal to contribute a PR for mina SSHD using org.connectbot.jbcrypt:jbcrypt:1.0.0 sounds good, I'd aim at including the source code into mina SSHD if they agree to avoid adding another library containing only a single implementation class. Before we do this ask the legal team if we then can consume this implementation from mina SSHD regarding its license and provenance.

Can you create a CQ to get advise from the legal team on this topic?
Comment 3 Thomas Wolf CLA 2018-11-29 18:01:32 EST
(In reply to Matthias Sohn from comment #2)
> I think your proposal to contribute a PR for mina SSHD using
> org.connectbot.jbcrypt:jbcrypt:1.0.0 sounds good, I'd aim at including the
> source code into mina SSHD if they agree to avoid adding another library
> containing only a single implementation class. Before we do this ask the
> legal team if we then can consume this implementation from mina SSHD
> regarding its license and provenance.
> 
> Can you create a CQ to get advise from the legal team on this topic?

I can try. As for copying the source of this BCrypt directly into Apache MINA sshd: don't know. We would have to check with the Apache people first what they would prefer. I'll ask Lyor in SSHD-708.[1]

[1] https://issues.apache.org/jira/browse/SSHD-708
Comment 4 Matthias Sohn CLA 2018-11-29 18:41:59 EST
(In reply to Thomas Wolf from comment #3)
> (In reply to Matthias Sohn from comment #2)
> > I think your proposal to contribute a PR for mina SSHD using
> > org.connectbot.jbcrypt:jbcrypt:1.0.0 sounds good, I'd aim at including the
> > source code into mina SSHD if they agree to avoid adding another library
> > containing only a single implementation class. Before we do this ask the
> > legal team if we then can consume this implementation from mina SSHD
> > regarding its license and provenance.
> > 
> > Can you create a CQ to get advise from the legal team on this topic?
> 
> I can try. As for copying the source of this BCrypt directly into Apache
> MINA sshd: don't know. We would have to check with the Apache people first
> what they would prefer. I'll ask Lyor in SSHD-708.[1]
> 
> [1] https://issues.apache.org/jira/browse/SSHD-708

if they don't like this we will live with another tiny dependency
Comment 5 Thomas Wolf CLA 2018-12-10 12:44:20 EST
I followed the instructions on the Legal Process poster[1] which says to contact the legal team by e-mail, and explained the issue. Sharon looked at it and doesn't see any problem with including the Bcrypt source directly in upstream Apache MINA sshd.

So I created the pull request at [2].

We'll have to remember when we create a CQ for using an Apache MINA sshd version that does include this PR or that otherwise includes this Bcrypt source that we (Sharon and I) already discussed this by e-mail.

I guess that Apache MINA sshd version will be 2.2.0. I'll check with Lyor when that is planned to be released; hopefully in time for Egit 5.3.0.

[1] https://www.eclipse.org/legal/EclipseLegalProcessPoster.pdf
[2] https://github.com/apache/mina-sshd/pull/81
Comment 6 Thomas Wolf CLA 2018-12-14 17:24:08 EST
(In reply to Thomas Wolf from comment #5)
> So I created the pull request at https://github.com/apache/mina-sshd/pull/81

This is in upstream Apache MINA sshd now. It got somewhat transmogrified in the process, but the functionality is there, and BCrypt.java is included. It's commits 5bbd2840, 9d00a1bc, and 969f3cab in Apache MINA sshd.[1]

[1] https://github.com/apache/mina-sshd/commits/master
Comment 7 Eclipse Genie CLA 2019-04-29 14:42:06 EDT
New Gerrit change created: https://git.eclipse.org/r/141356
Comment 8 Eclipse Genie CLA 2019-05-06 19:13:22 EDT
Gerrit change https://git.eclipse.org/r/141356 was merged to [master].
Commit: http://git.eclipse.org/c/jgit/jgit.git/commit/?id=c33d2bfb9f5d0f407bb736fafe2fa8ff93309e93