Fix for PKCS8 EC private key support

[tsaarni]

This pull request fixes loading of PKCS#8 EC private keys.

The code did not parse the unencrypted PKCS8 file correctly in case of EC private key. The proposed fix skips calling the old method that did more complicated parsing. Instead it takes a similar approach to the existing code for encrypted PKCS#8, which seems to work for both RSA and EC private keys.

The change also proposes renaming ECDSA algorithm name to EC. I'm uncertain if some of the instances should remain, but at least for the KeyManager the key type must be EC: when the Java TLS stack calls the KeyManager (defined by jruby-openssl) to select the key alias, it uses type EC not ECDSA. Because the code previously used ECDSA the TLS handshake still failed even after the loading of EC key was fixed, since no match was found for key type EC.

Possible TODOs to improve the change:

• There is some dead code after this change that could be removed.
• Unit testing

Fixes #266

Signed-off-by: Tero Saarni tero.saarni@est.tech

[kares]

Thanks Tero, not sure all these "EC" strings really need to be changed.
My guess would be not but let's see how unit test come back, also please if you can write a simple test case that would help a lot.

[tsaarni]

Sorry, I realize I broke ECDSA signing with the renaming!

I now committed a change that should fix it, by introducing new method getKeyType() that makes it possible to differentiate between the signature algorithm and key type. I would be interested to hear what you think about this approach?

The reasoning is following:

In case of sign() method PKey::getAlgorithm() is used to construct signature algorithm name here

jruby-openssl/src/main/java/org/jruby/ext/openssl/PKey.java

Line 212 in 1f95043

ByteList sign = sign(digAlg + "WITH" + getAlgorithm(), getPrivateKey(), data.convertToString().getByteList());

In this case, it really needs to return ECDSA and not EC. There is obviously no signature algorithm named SHA256withEC. On the other hand here in case of SSLContext it needs to be EC:

jruby-openssl/src/main/java/org/jruby/ext/openssl/SSLContext.java

Line 997 in 1f95043

this.keyAlgorithm = pKey.getAlgorithm();

It needs to be EC because there is no keyType named ECDSA according to JDK docs.

Docs mention that in the context of Java standard naming, ECDSA refers to the SHA1withECDSA signature algorithm. Grepping "ECDSA" with quotes from JDK source code enforces this - it is not used in that many places. EC would refer to the key type, ECDSA to the signature algorithm.

An example:

When looking at JDK KeyFactory standard names, it only list EC and not ECDSA. Regardless of the standard names, both EC and ECDSA are registered as BouncyCastle key factory algorithms: see this code which points to these classes - both seem to refer to the same provider configuration.

With this code:

import java.security.Security;
import org.bouncycastle.jce.provider.BouncyCastleProvider;

class KeyAlgorithms {
    public static void main(String[] args) {
        System.out.println("JDK KeyFactory algorithms: " + Security.getAlgorithms("KeyFactory"));

        Security.addProvider(new BouncyCastleProvider());
        System.out.println("BC KeyFactory algorithms: " + Security.getAlgorithms("KeyFactory"));
    }
}

I can observe following:

$ java -cp /home/tsaarni/.m2/repository/org/bouncycastle/bcprov-jdk18on/1.71/bcprov-jdk18on-1.71.jar keyalgorithms.java
JDK KeyFactory algorithms: [RSA, X25519, DSA, DIFFIEHELLMAN, RSASSA-PSS, X448, XDH, EC]
BC KeyFactory algorithms: [ECDH, DH, X.509, DIFFIEHELLMAN, ECGOST3410-2012, X448, ECDHC, ED25519, COMPOSITE, GOST3410, ELGAMAL, DSA, EXTERNAL, ED448, DSTU4145, XDH, EC, ECDSA, RSA, X25519, ECGOST3410, OID.1.3.6.1.4.1.22554.4.2, LMS, 1.3.6.1.4.1.22554.4.2, RSASSA-PSS, OID.1.3.6.1.4.1.18227.2.1, 1.3.6.1.4.1.18227.2.1, SPHINCSPLUS, ECMQV, EDDSA]

BC really seems to have registered both names while JDK does not. If grepping the BC source code, one can find that it uses ECDSA in this context. I wonder why is it using it as key factory algorithm name even if that would be signature algorithm name and JDK does not do that?

PS: I have still the task to add a test case for EC keys!

[tsaarni]

Small additional note: I found this old BouncyCastle issue where this topic was also touched. It was said this is handled by BC PKIX key manager. I can take this approach too, that is: keep the key type as ECDSA in the code but create matcher for key manager methods that will consider ECDSA as equal to EC.

[kares]

👍 this is looking great.
The keyType extra property makes sense, thanks for taking care of the rename down the stack as well.
I will take a look into figuring out a test case for the PKCS#8 formatted key as I'll circle back to the JOSSL code (might take me a few days).

[tsaarni]

Note that the approach proposed here does not try to derive the public key from private key, like org.jruby.ext.openssl.impl.PKey.readPrivateKey() attempted to do. Instead, it takes the same approach as reading encrypted PKCS#8 has done, here few lines below

jruby-openssl/src/main/java/org/jruby/ext/openssl/x509store/PEMInputOutput.java

Line 371 in 1f95043

return new KeyPair(null, privKey);

This approach worked for me in TLS tests.

[tsaarni]

I have added a test case test_read_pkcs8_with_ec and pem file as test data private_key_pkcs8.pem.

I did some further experiments as well (not included in PR):

I wanted to add similar test case for encrypted PKCS#8 which I did not find existing. Something like:

  def test_read_pkcs8_enc_with_ec
    key_file = File.join(File.dirname(__FILE__), 'private_key_pkcs8_enc.pem')

    key = OpenSSL::PKey::read(File.read(key_file), 'password')
    assert_equal '37273549501637553234010607973347901861080883009977847480473501706546896416762', key.private_key.to_s
    assert_empty key.public_key.to_s
  end

I converted the PKCS#8 file I included in this PR to encrypted PKCS#8

openssl pkcs8 -in private_key_pkcs8.pem -topk8 -out private_key_pkcs8_enc.pem

It seems I cannot get the existing code to read encrypted PKCS#8 file either. That code was not changed by this PR. Few observations:

(1) When trying to read encrypted PKCS#8 the code throws exception java.lang.IllegalArgumentException: initialisation vector must be the same length as block size from this line

jruby-openssl/src/main/java/org/jruby/ext/openssl/x509store/PEMInputOutput.java

Line 425 in 1f95043

cipher.init(false, param);

(2) I then tried to create encrypted PKCS#8 with old algorithm instead, to see if we can succeed with that

openssl pkcs8 -in private_key_pkcs8.pem -topk8 -out private_key_pkcs8_enc.pem -v1 PBE-SHA1-RC2-40

Now that made the code execute derivePrivateKeyPBES1() instead of derivePrivateKeyPBES2(). But it failed with different error: java.security.NoSuchAlgorithmException: pbeWithSHA1And40BitRC2 SecretKeyFactory not available. The secret factory name is coming from here

jruby-openssl/src/main/java/org/jruby/ext/openssl/x509store/PEMInputOutput.java

Lines 397 to 398 in 1f95043

	String algorithm = ASN1Registry.o2a(algId.getAlgorithm());
	algorithm = (algorithm.split("-"))[0];

I searched for SecretKeyFactory algorithm that is supported by JDK from ASN1Registry.java and ended up with this

openssl pkcs8 -in private_key_pkcs8.pem -topk8 -out private_key_pkcs8_enc.pem -v1 PBE-MD5-DES

and then the test passed!

(3) I also noticed that derivePrivateKeyPBES2() assumes RSA but that should be easy to fix.

jruby-openssl/src/main/java/org/jruby/ext/openssl/x509store/PEMInputOutput.java

Line 439 in 1f95043

KeyFactory fact = SecurityHelper.getKeyFactory("RSA"); // It seems to work for both RSA and DSA.

I do not need encrypted PKCS#8 support myself now, but initially I thought I could quickly add a test. It seems bit more difficult, unless adding test data with that legacy PBE-MD5-DES encryption...

[kares]

I do not need encrypted PKCS#8 support myself now, but initially I thought I could quickly add a test. It seems bit more difficult, unless adding test data with that legacy PBE-MD5-DES encryption...

Recall the issues you've mentioned, indeed PBES2 is not supported 100% by OpenJDK's provider.
If you have improvements/refactorings on that front (e.g. using BC APIs directly) feel free to fire up another PR.

[tsaarni]

Hi @kares, I wanted to ask if there has been plans of making a new release in near future? I see there hasn't been many changes, but a release would make it easier to use this feature 😄

[kares]

Hey Tero, planning to but I still haven't found time to look into another (EC) incompatibility: #241
Which I was hoping to include in the next release...

Feel free to ping me again if I do not do anything in a week, so I can than just cut a release afterwards whatever is on master.

[tsaarni]

Hi @kares 👋 Sorry to bother again, but just wondering how it is going with regards the release? And if there something I could try to help with :)

[kares]

Hey, give 0.14.1 a try and let us know if there's still issues on your end.

[tsaarni]

@kares Thank you!