-
Notifications
You must be signed in to change notification settings - Fork 121
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
Add EVP KEM APIs to integrate-pq branch #485
Add EVP KEM APIs to integrate-pq branch #485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS-LC-specific style comments.
typedef struct { | ||
uint8_t pub[KYBER512_PUBLICKEY_BYTES]; | ||
uint8_t priv[KYBER512_SECRETKEY_BYTES]; | ||
uint8_t pub[800]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
magic number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I normally prefer not to have it like this either, but I deferred to the convention of this file (see the other key structures above this).
#include <openssl/evp.h> | ||
#include <openssl/mem.h> | ||
|
||
#define SUCCESS (1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We just use 0
or 1
without macro's. Maybe sometime in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be a problem to leave the macros? I personally find it more readable to use them, especially since the openssl convention of 1 for success and 0 for failure is different from what I'm used to (no data to back this up, but anecdotally it seems most *nix programs use 0 for success).
Please revert - I'm not done reviewing. |
This reverts commit 32fc107.
This reverts commit 32fc107.
This reverts commit 32fc107.
* Remove PQ_KEM API and replace with EVP_KEM API. Update KAT tests to match new API. * Address memory issues found during testing. * Update comment, braces, and function parameter style * Update test names to match convention
* Remove PQ_KEM API and replace with EVP_KEM API. Update KAT tests to match new API. * Address memory issues found during testing. * Update comment, braces, and function parameter style * Update test names to match convention
Description of changes:
In AWS-LC, the reference code for the post-quantum cryptography (PQC) key encapsulation mechanism (KEM) PQCrystals-Kyber currently exists on a feature branch, integrate-pq. The API for using Kyber’s KEM operations is currently defined in pq_kem.h. This is a thin layer around the Kyber KEM operations, defined using an EVP_PQ_KEM structure. See this example of how it is done for Kyber512.
The problem with the current state of the integrate-pq branch is that it takes us down a path which diverges from the way KEMs are interfaced within the upstream OpenSSL codebase beginning with OpenSSL 3.0.
This change removes the custom PQ_KEM interface and replaces it with an EVP interface which is compatible with the calling signatures of the OpenSSL 3 EVP KEM APIs.
Call-outs:
The kem.c file was placed into the fipsmodule directory to align with where other EVP structures are. If this is not acceptable from a FIPS standpoint, I can move it elsewhere.
Testing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.