Add support for JCA security providers #39

smirandamedallia · 2021-10-02T01:25:04Z

Update the various classes to use jcajce rather than bc
Add support to set the security provider at a library level
Use BC and default provider

smirandamedallia · 2021-10-02T01:31:57Z

src/test/groovy/org/c02e/jpgpj/EncryptorSpec.groovy

@@ -763,6 +771,7 @@ hQEMAyne546XDHBhAQ...
        meta.verified
    }

+    @Ignore


I couldn't make this work when I run this with ./gradlew build.
It gives me the following error:

org.bouncycastle.openpgp.PGPException: exception constructing public key at org.c02e.jpgpj.Encryptor.encrypt(Encryptor.java:1130) at org.c02e.jpgpj.Encryptor.prepareCiphertextOutputStream(Encryptor.java:974) at org.c02e.jpgpj.Encryptor.encrypt(Encryptor.java:857) at org.c02e.jpgpj.Encryptor.encrypt(Encryptor.java:826) at org.c02e.jpgpj.EncryptorSpec.encrypt and sign with no usage flags(EncryptorSpec.groovy:781) Caused by: java.security.spec.InvalidParameterSpecException: Not a supported curve: java.security.spec.ECGenParameterSpec@5b0e7c05 at java.base/sun.security.util.ECParameters.engineInit(ECParameters.java:130) at java.base/java.security.AlgorithmParameters.init(AlgorithmParameters.java:294) ... 5 more

Could you please take a look

This is just an artifact of some peculiarities with groovy compilation and class loading (it fails here and not elsewhere because the JDK's default JCA implementation can handle the other test public keys, but not this test key using the P-256 curve). I believe right now it fails only when this class is recompiled and run in the same process (and not when compiled and run separately); and it will go away if a BouncyCastleProvider instance is set directly on the JcaContextHelper.securityProvider property (instead of indirectly via the JCA apparatus of Security.addProvider() / Security.getProvider()).

smirandamedallia · 2021-10-02T01:41:28Z

I have upgraded the version number since BC provider must be present in the list of providers or the Provider must be set before performing PGP operations

Security.addProvider(new BouncyCastleProvider())

justinludwig

Thank you for going ahead and doing this refactoring! It looks good to me -- I mainly have a few small suggestions (noted inline).

The one big suggestion I have is that instead of requiring JPGPJ users to register the BouncyCastleProvider with JCA (ie by calling Security.addProvider(new BouncyCastleProvider())), the JcaContextHelper class should itself try to initialize its securityProvider property with a BouncyCastleProvider instance directly. That way, existing users won't have to make any changes to their code, and can avoid messing around with JCA if they don't want to.

So that users can also use the alternative FIPS implementation if they do want to, we can do this initialization via reflection (see my suggestion for this inline) -- it'll be kind of ugly, but will make it easy for users who just use the regular Bouncy Castle implementation to do so without any JCA fiddling -- while still avoiding a big fireball that stymies users who do want to use the FIPS implementation instead.

justinludwig · 2021-10-04T19:55:01Z

src/main/java/org/c02e/jpgpj/Encryptor.java

@@ -1248,7 +1248,11 @@ protected PublicKeyKeyEncryptionMethodGenerator buildPublicKeyEncryptor(Key key,
        }

        PGPPublicKey publicKey = key.getEncryption().getPublicKey();
-        return new BcPublicKeyKeyEncryptionMethodGenerator(publicKey);
+        JcePublicKeyKeyEncryptionMethodGenerator generator = new JcePublicKeyKeyEncryptionMethodGenerator(publicKey);


Pull the creation of this generator (and the setting of the security provider on it) into a new factory method of the new JcaContextProvider class, like you did elsewhere for other JCA-aware objects.

justinludwig · 2021-10-04T19:55:40Z

src/main/java/org/c02e/jpgpj/Encryptor.java

-            new BcPGPDigestCalculatorProvider().get(kdAlgorithm.ordinal()),
-            workFactor);
+        PGPDigestCalculatorProvider digestCalculatorProvider = JCAContextHelper.getPGPDigestCalculatorProvider();
+        JcePBEKeyEncryptionMethodGenerator jcePBEKeyEncryptionMethodGenerator = new JcePBEKeyEncryptionMethodGenerator(


Pull this into a new factory method of the JcaContextProvider class, as well.

justinludwig · 2021-10-04T19:57:03Z

src/main/java/org/c02e/jpgpj/Encryptor.java

@@ -1313,7 +1322,11 @@ protected PGPSignatureGenerator buildSigner(Key key, FileMetadata meta)
     * Builds a PGPContentSignerBuilder for the specified algorithms.
     */
    protected PGPContentSignerBuilder buildSignerBuilder(int keyAlgorithm, int hashAlgorithm) {
-        return new BcPGPContentSignerBuilder(keyAlgorithm, hashAlgorithm);
+        JcaPGPContentSignerBuilder jcaPGPContentSignerBuilder = new JcaPGPContentSignerBuilder(keyAlgorithm, hashAlgorithm);


Pull this into a new factory method of the JcaContextProvider class, as well.

justinludwig · 2021-10-04T20:00:06Z

src/main/java/org/c02e/jpgpj/JCAContextHelper.java

+ * consisting of {@link java.security.Provider provider}
+ * Note: The following class is not thread safe, the security provider should not be changed during PGP operations
+ */
+public class JCAContextHelper {


Let's name this class JcaContextHelper instead of JCAContextHelper, to match the casing convention of other class names with initialisms in this project, like EncryptedAsciiArmorHeadersCallback etc.

justinludwig · 2021-10-04T20:04:21Z

src/main/java/org/c02e/jpgpj/JCAContextHelper.java

+ * Note: The following class is not thread safe, the security provider should not be changed during PGP operations
+ */
+public class JCAContextHelper {
+	private static Provider securityProvider = Security.getProvider("BC");


For the convenience of users who don't otherwise need to fiddle with JCA, let's try to automatically set the securityProvider property to a BouncyCastleProvider instance by default -- and fail as gracefully as possible if it's not available. Something like this:

private static Provider securityProvider; static { try { securityProvider = (Provider) Class.forName( "org.bouncycastle.jce.provider.BouncyCastleProvider" ).getDeclaredConstructor().newInstance(); } catch (Exception e) { LoggerFactory.getLogger(JCAContextHelper.class.getName()).warn( "not using BC security provider"); } }

justinludwig · 2021-10-04T20:05:53Z

src/main/java/org/c02e/jpgpj/Ring.java

+                try {
+                    return JCAContextHelper.getJcaKeyBoxBuilder().build(result.stream).getKeyBlobs().iterator();
+                } catch (NoSuchProviderException | NoSuchAlgorithmException e) {
+                    throw new RuntimeException(e);


Instead of wrapping this with a RuntimeException, wrap this with a PGPException (so users can catch a PGPException in their own code if that want to handle anything PGP-related going wrong when they use JPGPJ).

justinludwig · 2021-10-04T20:07:19Z

src/main/java/org/c02e/jpgpj/Subkey.java

+        try {
+            return JCAContextHelper.getJcePBESecretKeyDecryptorBuilder().build(chars);
+        } catch (PGPException e) {
+            throw new RuntimeException(e);


Instead of wrapping the PGPException with a RuntimeException, let's just change the signature of this buildDecryptor method to throws PGPException.

justinludwig · 2021-10-04T20:08:46Z

src/test/groovy/org/c02e/jpgpj/EncryptorSpec.groovy

 class EncryptorSpec extends Specification {
    def cipherOut = new ByteArrayOutputStream()
    def plainOut = new ByteArrayOutputStream()

+    def setupSpec() {
+        Security.addProvider(new BouncyCastleProvider())


We won't need this (here, and same for the other test specs) if we change the JcaProviderHelper to automatically use the BouncyCastleProvider when available.

justinludwig · 2021-10-04T20:24:02Z

src/test/groovy/org/c02e/jpgpj/EncryptorSpec.groovy

@@ -763,6 +771,7 @@ hQEMAyne546XDHBhAQ...
        meta.verified
    }

+    @Ignore


This is just an artifact of some peculiarities with groovy compilation and class loading (it fails here and not elsewhere because the JDK's default JCA implementation can handle the other test public keys, but not this test key using the P-256 curve). I believe right now it fails only when this class is recompiled and run in the same process (and not when compiled and run separately); and it will go away if a BouncyCastleProvider instance is set directly on the JcaContextHelper.securityProvider property (instead of indirectly via the JCA apparatus of Security.addProvider() / Security.getProvider()).

smirandamedallia · 2021-10-04T23:32:08Z

@justinludwig Thanks for the quick response. i have addressed the changes and have updated the version to 1.2 since it is non-breaking change

justinludwig · 2021-10-05T20:15:24Z

Thanks! I will push out a release for this to maven central in a day or two.

smirandamedallia · 2021-10-05T20:27:25Z

Thanks

justinludwig · 2021-10-07T01:56:11Z

I pushed JPGPJ version 1.2 with this to maven central.

smirandamedallia added 4 commits September 30, 2021 16:17

some test failing with CAST5/CFB/NoPadding

ab77aeb

working

2c8a52b

working test

cd733d0

clean up

ab56497

smirandamedallia commented Oct 2, 2021

View reviewed changes

justinludwig requested changes Oct 4, 2021

View reviewed changes

smirandamedallia added 5 commits October 4, 2021 14:34

using classloader to create instance of BouncyCastleProvider

a07fbf8

clean up

5e7a4f7

use version 1.2 since not breaking change

2f96db5

change tabs to spaces

d544704

use ordinal

c84d71d

smirandamedallia requested a review from justinludwig October 4, 2021 23:31

justinludwig merged commit ca31412 into justinludwig:master Oct 5, 2021

justinludwig mentioned this pull request Oct 7, 2021

Support for bc-fips #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JCA security providers #39

Add support for JCA security providers #39

smirandamedallia commented Oct 2, 2021

smirandamedallia Oct 2, 2021

justinludwig Oct 4, 2021

smirandamedallia commented Oct 2, 2021

justinludwig left a comment

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

justinludwig Oct 4, 2021

smirandamedallia commented Oct 4, 2021

justinludwig commented Oct 5, 2021

smirandamedallia commented Oct 5, 2021

justinludwig commented Oct 7, 2021

Add support for JCA security providers #39

Add support for JCA security providers #39

Conversation

smirandamedallia commented Oct 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smirandamedallia commented Oct 2, 2021

justinludwig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smirandamedallia commented Oct 4, 2021

justinludwig commented Oct 5, 2021

smirandamedallia commented Oct 5, 2021

justinludwig commented Oct 7, 2021