bc-java icon indicating copy to clipboard operation
bc-java copied to clipboard

High-Level OpenPGP API

Open vanitasvitae opened this issue 1 year ago • 15 comments

This PR contains a high-level API for OpenPGP, which simplifies the following tasks:

  • Message Creation
    • OpenPGPMessageGenerator sets up an OpenPGPMessageOutputStream based on user configuration
  • Message Consumption
    • OpenPGPMessageProcessor processes encrypted / signed / compressed OpenPGP messages and emits the plaintext along with metadata
  • Certificate / Key Evaluation
    • OpenPGPCertificate acts as high-level API pendant to PGPPublicKeyRing, evaluating expiration / revocation / key signatures and allows the user to get verified information about the cert
    • OpenPGPKey provides similar high-level API pendant to PGPSecretKeyRing

I decided to use a common OpenPGP* naming scheme for the new HighLevel API classes and also decided to make use of general terminology from the book "OpenPGP for application developers" (https://openpgp.dev).

This PR is still pretty much very early work in progress (as is #1857, which will eventually integrate with this API), but I want you to be able to comment on it during the development phase already :)

vanitasvitae avatar Nov 15 '24 23:11 vanitasvitae

This PR is now based on #1926

vanitasvitae avatar Dec 03 '24 11:12 vanitasvitae

This PR contains #1931 and may need to be rebased if the other patch is merged.

vanitasvitae avatar Dec 03 '24 13:12 vanitasvitae

@dghgit I noticed, that in many places you replaced

for (item : collection)
{

with

for (Iterator it = collection.iterator(); it.hasNext();)
{
    Item item = iterator.next();

I was under the impression that BC was now targeting Java 1.8 primarily, which already supports the former method of collection iteration. Am I missing something? Why are the Iterators not using generics?

Edit: I see you recently made some changes for Java 4 and 5 compatibility.

vanitasvitae avatar Jan 02 '25 11:01 vanitasvitae

Based on my experience, this project strives to maintain compatibility across Java versions, from Java 1.4 to Java 21. Consequently, newer features such as lambdas and others will be replaced to ensure broader compatibility.

Additionally, each PR will be divided into multiple branches, with each branch containing an indivisible set of changes for the final review. However, you don’t need to worry about this aspect—I will handle the code refactoring to maintain efficiency and minimize duplication, as well as manage the separation of changes into distinct branches.

ligefeiBouncycastle avatar Jan 07 '25 01:01 ligefeiBouncycastle

Additionally, each PR will be divided into multiple branches, with each branch containing an indivisible set of changes for the final review. However, you don’t need to worry about this aspect—I will handle the code refactoring to maintain efficiency and minimize duplication, as well as manage the separation of changes into distinct branches.

One more thing: Could you postpone the merging process until the branch is marked as ready for review? I think in the past you once merged a WiP branch, which was a bit unfortunate :sweat_smile:

vanitasvitae avatar Jan 08 '25 12:01 vanitasvitae

Additionally, each PR will be divided into multiple branches, with each branch containing an indivisible set of changes for the final review. However, you don’t need to worry about this aspect—I will handle the code refactoring to maintain efficiency and minimize duplication, as well as manage the separation of changes into distinct branches.

One more thing: Could you postpone the merging process until the branch is marked as ready for review? I think in the past you once merged a WiP branch, which was a bit unfortunate 😅

Thanks for the reminder! No worries, I can wait until you’ve finished the work and marked the branch as ready for review. Let me know when it’s good to go!

ligefeiBouncycastle avatar Jan 08 '25 12:01 ligefeiBouncycastle

The PR is ready for review. Let me know if you like me to squash it for you :)

vanitasvitae avatar Jan 27 '25 12:01 vanitasvitae

As another cherry on top, I present to you bc-sop, which implements the stateless OpenPGP protocol as a native-image using Bouncy Castle :)

vanitasvitae avatar Jan 27 '25 13:01 vanitasvitae

The changes in https://github.com/bcgit/bc-java/pull/1911/commits/8a4b61eb5c2d598c227909a73aeeddbb1b0cfa16 were introduced, as I detected a major performance decrease while porting PGPainless over to the new API. The test run runtime increased from ~1 minute to over 7 minutes.

Turns out, PGPainless used relaxed parameters for secret key protection (which I had adapted from BC earlier). Going with an iteration count of 0xff is perhaps a bit over the top (and notably also the maximum allowed iteration count), so I changed the default to the former used 0x60 and made the factory method customizable to allow for other parameters.

vanitasvitae avatar Mar 13 '25 14:03 vanitasvitae

@vanitasvitae Thank you for the update. I've reviewed the code and have merged the majority of it into the main branch — about 20 files remain to be merged. Two test classes, DoubleBufferedInputStreamTest and PGPKeyPairGeneratorTest, will be merged into master at a later stage. Please take a look at the latest changes on the main branch.

ligefeiBouncycastle avatar May 02 '25 00:05 ligefeiBouncycastle

Looking good! Thank you for your work! :) One thing: If its not too much of a hassle, I'd prefer if you keep the commit authorship intact :)

vanitasvitae avatar May 03 '25 08:05 vanitasvitae

@vanitasvitae Apologies — the commit authorship information was lost during the branch merges (I had split the work across about 7 branches). I’ll do my best to recover and restore the original authorship.

ligefeiBouncycastle avatar May 05 '25 01:05 ligefeiBouncycastle

@vanitasvitae Thank you for your greater contribution to this PR. Now all tests related to this PR have been merged with related code. The remaining classes include: core/src/main/java/org/bouncycastle/util/Objects.java pg/src/main/java/org/bouncycastle/openpgp/api/exception/PolicyException.java pg/src/main/java/org/bouncycastle/openpgp/api/util/OpenPGPKeyPrinter.java pg/src/main/java/org/bouncycastle/openpgp/api/EncryptedDataPacketType.java pg/src/main/java/org/bouncycastle/openpgp/api/MessageEncryptionMechanism.java pg/src/main/java/org/bouncycastle/openpgp/api/OpenPGPSignature.java pg/src/main/java/org/bouncycastle/openpgp/PGPEncryptedDataList.java pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRing.java pg/src/main/java/org/bouncycastle/openpgp/PGPKeyRingGenerator.java pg/src/main/java/org/bouncycastle/openpgp/PGPPublicKey.java pg/src/main/java/org/bouncycastle/openpgp/PGPSecretKey.java pg/src/main/java/org/bouncycastle/openpgp/PGPSignatureSubpacketVector.java

Some of these classes will be merged later.

ligefeiBouncycastle avatar Jun 02 '25 01:06 ligefeiBouncycastle

Nice! Would it serve you if I rebased the remaining unmerged changes into smaller PRs?

vanitasvitae avatar Jun 02 '25 10:06 vanitasvitae

@vanitasvitae Thank you! If you're planning to build new functionality that depends on the unmerged changes, feel free to break them into smaller PRs—this will help with quicker reviews and integration. Otherwise, I’ll take care of merging the remaining changes into the main branch later.

ligefeiBouncycastle avatar Jun 03 '25 00:06 ligefeiBouncycastle