Add support for PGP Encryption/Decryption #555

dinukaamarasinghe817 · 2024-05-09T12:59:31Z

Purpose

To Fix : ballerina-platform/ballerina-library#6462

Examples

Checklist

daneshk · 2024-05-09T15:38:11Z

ballerina/pgp_utils.bal

+// specific language governing permissions and limitations
+// under the License.
+
+public type PgpEncryptionOptions record {|


Add API doc comments to all public types

Shall we just rename this to Options?

public isolated function encryptPgp(byte[] plainText, byte[] publicKey, *Options options) returns byte[]|Error = @java:Method {

IMO, the words pgp and encrypt are redundant as this API name itself provides an understanding that we are going to do a PGP encryption. So it is obvious that these options are related to PGP encryption.

WDYT?

Yes, we can reduce the redundancy by renaming it to Options. I have addressed it here 557f72b

ayeshLK · 2024-05-10T02:54:33Z

native/src/main/java/io/ballerina/stdlib/crypto/nativeimpl/Encrypt.java

@@ -34,6 +40,11 @@
 */
 public class Encrypt {

+    public static final String COMPRESSION_ALGORITHM = "compressionAlgorithm";


Since these constants are anyway converted to BString its better to use following approach. Same for other constants as well.

Suggested change

public static final String COMPRESSION_ALGORITHM = "compressionAlgorithm";

public static final BString COMPRESSION_ALGORITHM = StringUtils.fromString("compressionAlgorithm");

And if there constants are not use anywhere other than here, its better to mark them as private as well.

ayeshLK · 2024-05-10T02:56:28Z

native/src/main/java/io/ballerina/stdlib/crypto/nativeimpl/Encrypt.java

+    public static Object encryptPgp(BArray plainTextValue, BArray publicKeyValue, BMap options) {
+        byte[] plainText = plainTextValue.getBytes();
+        byte[] publicKey = publicKeyValue.getBytes();
+        InputStream publicKeyStream = new ByteArrayInputStream(publicKey);


When we create InputStream the best practice is to close them once the required work is done. And I think it is better to do that in the same place where we create the stream using a try-with-resources block.

I have addressed this in 78e3fb4

ayeshLK · 2024-05-10T02:57:36Z

native/src/main/java/io/ballerina/stdlib/crypto/nativeimpl/Decrypt.java

+        byte[] cipherText = cipherTextValue.getBytes();
+        byte[] privateKey = privateKeyValue.getBytes();
+        byte[] passphraseInBytes = passphrase.getBytes();
+        InputStream keyStream = new ByteArrayInputStream(privateKey);


Check whether this stream is properly closed. This comment [1] is valid here as well.

[1] - https://github.com/ballerina-platform/module-ballerina-crypto/pull/555/files#r1596194670

ayeshLK · 2024-05-10T03:00:40Z

native/src/main/java/io/ballerina/stdlib/crypto/PgpEncryptionGenerator.java

+        compressedDataGenerator.close();
+        cipherOutStream.close();
+        encryptOut.close();


Rather than manually doing this it is better to use try-with-resources block to close the created IO streams.

ayeshLK · 2024-05-10T03:02:20Z

native/src/main/java/io/ballerina/stdlib/crypto/PgpDecryptionGenerator.java

+        PGPPublicKeyEncryptedData publicKeyEncryptedData = null;
+
+        Iterator<PGPEncryptedData> encryptedDataItr = pgpEncryptedDataList.getEncryptedDataObjects();
+        while (pgpPrivateKey == null && encryptedDataItr.hasNext()) {


Suggested change

while (pgpPrivateKey == null && encryptedDataItr.hasNext()) {

while (Objects.isNull(pgpPrivateKey) && encryptedDataItr.hasNext()) {

ayeshLK · 2024-05-10T03:02:50Z

native/src/main/java/io/ballerina/stdlib/crypto/PgpDecryptionGenerator.java

+            throw new PGPException("Could not generate PGPPublicKeyEncryptedData object");
+        }
+
+        if (pgpPrivateKey == null) {


Suggested change

if (pgpPrivateKey == null) {

if (Objects.isNull(pgpPrivateKey)) {

ayeshLK · 2024-05-10T03:04:05Z

native/src/main/java/io/ballerina/stdlib/crypto/PgpDecryptionGenerator.java

+
+    private PGPPrivateKey findSecretKey(long keyID) throws PGPException {
+        PGPSecretKey pgpSecretKey = pgpSecretKeyRingCollection.getSecretKey(keyID);
+        return pgpSecretKey == null ? null : pgpSecretKey.extractPrivateKey(new JcePBESecretKeyDecryptorBuilder()


Rather than returning null here can't we use Optionals [1] ?

[1] - https://www.baeldung.com/java-optional

Yes we can, and I have addressed this in 78e3fb4

ayeshLK · 2024-05-10T03:06:48Z

native/src/main/java/io/ballerina/stdlib/crypto/nativeimpl/Decrypt.java

+            PgpDecryptionGenerator pgpDecryptionGenerator = new PgpDecryptionGenerator(
+                    keyStream,
+                    passphraseInBytes
+            );


Suggested change

PgpDecryptionGenerator pgpDecryptionGenerator = new PgpDecryptionGenerator(

keyStream,

passphraseInBytes

);

PgpDecryptionGenerator pgpDecryptionGenerator = new PgpDecryptionGenerator(

keyStream, passphraseInBytes);

Bhashinee · 2024-05-10T04:20:54Z

native/src/main/java/io/ballerina/stdlib/crypto/PgpEncryptionGenerator.java

+        }
+    }
+
+    private static Optional<PGPPublicKey> extractPGPKeyFromRing(PGPPublicKeyRing pgpPublicKeyRing) {


Shall we rename this function to extractPgpKeyFromRing

Bhashinee · 2024-05-10T04:32:42Z

ballerina/encrypt_decrypt.bal

+# + publicKey - Public key
+# + options - PGP encryption options
+# + return - Encrypted data or else a `crypto:Error` if the key is invalid
+public isolated function encryptPgp(byte[] plainText, byte[] publicKey, *PgpEncryptionOptions options)


Let's take in the publicKey and privateKey as file paths for the moment.

The same should applied for the params privateKey and passphrase in decryptPgp as well right? Should we keep the return type to byte [] as it is for those two methods encryptPgp and decryptPgp?

Let's have privateKey as a string file path in decryptPgp function as well. The passphrase should be a byte[] and what we return from the APIs also should be byte[] s.

I have addressed this in 78e3fb4

Bhashinee · 2024-05-10T04:33:19Z

ballerina/pgp_utils.bal

+// specific language governing permissions and limitations
+// under the License.
+
+# Represents the PGP encryption options


Suggested change

# Represents the PGP encryption options

# Represents the PGP encryption options.

Bhashinee · 2024-05-10T04:35:09Z

ballerina/pgp_utils.bal

+    boolean withIntegrityCheck = true;
+|};
+
+# Represents the compressions algorithms available in PGP


Suggested change

# Represents the compressions algorithms available in PGP

# Represents the compression algorithms available in PGP.

Bhashinee · 2024-05-10T04:35:56Z

ballerina/pgp_utils.bal

+    BZIP2= "3"
+}
+
+# Represent the symmetric key algorithms available in PGP


Suggested change

# Represent the symmetric key algorithms available in PGP

# Represent the symmetric key algorithms available in PGP.

native/src/main/java/io/ballerina/stdlib/crypto/PgpDecryptionGenerator.java

native/src/main/java/io/ballerina/stdlib/crypto/PgpEncryptionGenerator.java

Bhashinee · 2024-05-10T05:27:10Z

ballerina/tests/encrypt_decrypt_pgp_test.bal

+    byte[] cipherText = check encryptPgp(message, publicKey, symmetricKeyAlgorithm = AES_128, armor = false);
+    byte[] plainText = check decryptPgp(cipherText, privateKey, passphrase);
+    test:assertEquals(plainText.toBase16(), message.toBase16());
+}


Shall we also add a negative test case by creating another PGP key pair, like encrypting with a public key and trying to decrypt using a wrong private key?

I have added two more test cases for following in here 78e3fb4

Invalid Private key for decryption with it's passphrase

Valid Private key for decryption with invalid passphrase

Bhashinee · 2024-05-13T02:44:18Z

ballerina/tests/encrypt_decrypt_pgp_test.bal

+    if plainText is Error {
+        test:assertEquals(plainText.message(), "Error occurred while PGP decrypt: Could Not Extract private key");
+    } else {
+        test:assertTrue(false, "Should return a crypto Error");


Suggested change

test:assertTrue(false, "Should return a crypto Error");

test:assertFail("Should return a crypto Error");

I have addressed this in f24bd9a

Bhashinee · 2024-05-13T02:45:27Z

ballerina/tests/encrypt_decrypt_pgp_test.bal

+        test:assertEquals(plainText.message(),
+        "Error occurred while PGP decrypt: checksum mismatch at in checksum of 20 bytes");
+    } else {
+        test:assertTrue(false, "Should return a crypto Error");


This has been addressed in f24bd9a

sonarqubecloud · 2024-05-13T03:21:40Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

dinukaamarasinghe817 added 5 commits May 6, 2024 10:19

[Automated] Update the native jar versions

1897673

[Automated] Update the native jar versions

07115d4

[Automated] Update the native jar versions

fd56930

Add pgp encryption and decryption

9110389

Update pgp method params

04f59c4

dinukaamarasinghe817 requested review from shafreenAnfar, Bhashinee and MohamedSabthar as code owners May 9, 2024 12:59

dinukaamarasinghe817 added 2 commits May 9, 2024 20:16

Fix resource leak bug in PGP encryption

a3bfb6d

Merge branch 'master' of https://github.com/dinukaamarasinghe817/modu…

6fc145a

…le-ballerina-crypto into pgp

daneshk reviewed May 9, 2024

View reviewed changes

dinukaamarasinghe817 added 2 commits May 9, 2024 21:17

[Automated] Update the native jar versions

d89771b

Add API documentation for public records for PGP

dc1d31e

ayeshLK reviewed May 10, 2024

View reviewed changes

Bhashinee reviewed May 10, 2024

View reviewed changes

NipunaRanasinghe reviewed May 10, 2024

View reviewed changes

Bhashinee reviewed May 10, 2024

View reviewed changes

dinukaamarasinghe817 added 2 commits May 10, 2024 15:04

[Automated] Update the native jar versions

247f52a

Address suggestions on pgp APIs

78e3fb4

Change PgpEncryptionOptions to Options

557f72b

Bhashinee reviewed May 13, 2024

View reviewed changes

Address minor comments on PGP testing

f24bd9a

Bhashinee approved these changes May 13, 2024

View reviewed changes

Bhashinee merged commit 2e30356 into ballerina-platform:master May 13, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for PGP Encryption/Decryption #555

Add support for PGP Encryption/Decryption #555

dinukaamarasinghe817 commented May 9, 2024 •

edited

Loading

daneshk May 9, 2024

Bhashinee May 10, 2024

dinukaamarasinghe817 May 11, 2024

ayeshLK May 10, 2024

ayeshLK May 10, 2024

dinukaamarasinghe817 May 10, 2024

ayeshLK May 10, 2024

ayeshLK May 10, 2024

ayeshLK May 10, 2024

ayeshLK May 10, 2024

ayeshLK May 10, 2024

dinukaamarasinghe817 May 10, 2024

ayeshLK May 10, 2024

Bhashinee May 10, 2024

Bhashinee May 10, 2024

dinukaamarasinghe817 May 10, 2024

Bhashinee May 10, 2024

dinukaamarasinghe817 May 10, 2024

Bhashinee May 10, 2024

Bhashinee May 10, 2024

Bhashinee May 10, 2024

Bhashinee May 10, 2024

dinukaamarasinghe817 May 10, 2024

Bhashinee May 13, 2024

dinukaamarasinghe817 May 13, 2024

Bhashinee May 13, 2024

dinukaamarasinghe817 May 13, 2024

sonarqubecloud bot commented May 13, 2024

	public static final String COMPRESSION_ALGORITHM = "compressionAlgorithm";
	public static final BString COMPRESSION_ALGORITHM = StringUtils.fromString("compressionAlgorithm");

	while (pgpPrivateKey == null && encryptedDataItr.hasNext()) {
	while (Objects.isNull(pgpPrivateKey) && encryptedDataItr.hasNext()) {

	if (pgpPrivateKey == null) {
	if (Objects.isNull(pgpPrivateKey)) {

	# Represents the PGP encryption options
	# Represents the PGP encryption options.

	# Represents the compressions algorithms available in PGP
	# Represents the compression algorithms available in PGP.

	# Represent the symmetric key algorithms available in PGP
	# Represent the symmetric key algorithms available in PGP.

	test:assertTrue(false, "Should return a crypto Error");
	test:assertFail("Should return a crypto Error");

Add support for PGP Encryption/Decryption #555

Add support for PGP Encryption/Decryption #555

Conversation

dinukaamarasinghe817 commented May 9, 2024 • edited Loading

Purpose

Examples

Checklist

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

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

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

sonarqubecloud bot commented May 13, 2024

Quality Gate passed

dinukaamarasinghe817 commented May 9, 2024 •

edited

Loading