Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PQ] Add experimental support for HPKE #398

Open
wants to merge 26 commits into
base: experimental-pq-hybrid
Choose a base branch
from

Conversation

sgmenda-aws
Copy link

Description of changes:

Add experimental support for using AWS-LC HPKE as a Cipher using AWS-LC's hpke.h:

  • update to building with an experimental branch of aws-lc
  • add new AlgorithmParameterSpec HpkeParameterSpec
  • add new EvpKey EvpHpkeKey
  • add new PublicKey EvpHpkePublicKey and new PrivateKey EvpHpkePrivateKey
  • add new KeyPairGenerator HpkeGen which generates a HPKE key, backed by the native hpke_gen.cpp
  • add new Cipher``HpkeCipher, which performs single-shot HPKE encryption and decryption, backed by the nativehpke_cipher.cpp.

Call-outs:

  • lines 40-209 in HpkeCipher.java contain the core java implementation
  • native files hpke_gen.cpp and hpke_cipher.cpp contain the main cpp wrappers
  • general code organization into classes and methods
  • there are some outstanding TODO that will be addressed in a later revision

Testing:

Added HpkeGenTest and HpkeCipherTest to test basic correctness.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@sgmenda-aws sgmenda-aws requested a review from a team as a code owner August 19, 2024 12:54
Copy link
Contributor

@geedo0 geedo0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still have to cover cipher implementation and testing.

.gitmodules Outdated
Comment on lines 3 to 4
url = https://github.com/sgmenda-aws/aws-lc
branch = experimental-pq-hybrid-with-hpke
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the story here? Is this temporary? Will this branch exist in upstream AWS-LC? We can't be referencing submodules in personal forks.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is temporary till aws/aws-lc#1777 is merged.

csrc/buffer.h Outdated Show resolved Hide resolved
build.gradle Outdated
workingDir awslcSrcPath
commandLine "git", "checkout", awsLcGitVersionId
}
// FIXME: Commented because it was breaking Github CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's going on here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github does not support git checkout <branch> in submodules. This is temporary till aws-lc merges the HPKE PR, once that's done, I'll add this back and replace the VersionId with a commit hash.

Sorry for not explicitly marking these temporary changes.

src/com/amazon/corretto/crypto/provider/HpkeGen.java Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeGen.java Outdated Show resolved Hide resolved
csrc/hpke_gen.cpp Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
src/com/amazon/corretto/crypto/provider/HpkeCipher.java Outdated Show resolved Hide resolved
geedo0
geedo0 previously approved these changes Aug 23, 2024
Copy link
Contributor

@geedo0 geedo0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking pretty good. Only pending items I see are the placeholder AWS-LC references and failing CI. If we can get the CI passing, I'll happily approve.

* for DECRYPT and UNWRAP
* @param params the algorithm parameters, must be an instance of HpkeParameterSpec, and may not
* be null.
* @param random a source of randomness
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...which we discard

@geedo0 geedo0 self-requested a review August 23, 2024 12:55
@initsecret
Copy link

Turns out -Xcheck:jni does not like splitting one java array into multiple buffers, like splitting output into enc and ct. And, it was silently preventing writes to ct, making encrypt return an all-zero buffer. This was causing the failures.

Since -Xcheck:jni is not enabled on MacOS, I didn't notice it initially. And since the truncated logs don't make it explicit that this test was failing with -Xcheck:jni (and passing without.) I was extremely confused.

The following commit fixes this issue, and the test now passes with -Xcheck:jni: initsecret@32eacb9

@geedo0
Copy link
Contributor

geedo0 commented Aug 28, 2024

Alright, just so I fully understand what's going on here. You understandably no longer have access to the original Github account so you pushed this fix to a separate fork/account?

Since you're no longer with us and this work is largely complete. The sensible next step is to cherry-pick this last commit to the branch and see how it runs against the tests. If we like the results enough, I'm inclined to merge the work into the experimental branch as-is since it's already largely complete. We can come back in later to do the necessary work to integrate it into the main trunk.

@initsecret
Copy link

@geedo0 Precisely. I figured out the bug after so pushed a fix to my personal account. Yes, if this passes the tests, I was hoping you'd merge into the experimental branch. Alternatively, if you prefer, I could open a new PR, but it seems easier to apply this fix on top with git am like

wget -O fix.patch https://github.com/initsecret/amazon-corretto-crypto-provider/commit/32eacb911d797e5352b3f8eb08271b543831f68d.patch
git am fix.patch
git push

@geedo0
Copy link
Contributor

geedo0 commented Aug 30, 2024

I've brought this to the team's attention and we are working on getting this PR integrated while being respectful of your personal time. The fact that we've locked you out of this account is problematic though. The idea we landed on is to have Amir fork your work and submit a new PR that includes your last change. We'll then test/review/iterate from there, but I've recommended that we simply bias towards merging it into the experimental branch as any further cleanup can be pulled into the greater effort of re-integrating this when the time comes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants