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.

Sign up for GitHub

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

Support keypair calculation for PQDSA PKEY #2145

Merged
merged 8 commits into from
Jan 29, 2025
Merged

Support keypair calculation for PQDSA PKEY #2145

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3d1bff4
added keypair calculation to pqdsa pkey
jakemas Jan 28, 2025
09abb11
spacing nit
jakemas Jan 28, 2025
6756bfe
spelling mistake
jakemas Jan 28, 2025
a8c9562
Merge branch 'main' into pqdsa-keypair-consistency
jakemas Jan 28, 2025
cd8da04
CR fixes
jakemas Jan 28, 2025
aa87eeb
Merge branch 'pqdsa-keypair-consistency' of github.com:jakemas/aws-lc…
jakemas Jan 28, 2025
db900c0
free allocated memory
jakemas Jan 28, 2025
54224d2
CR fix
jakemas Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion crypto/evp_extra/p_pqdsa_asn1.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,40 @@ static int pqdsa_priv_decode(EVP_PKEY *out, CBS *params, CBS *key, CBS *pubkey)
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
return 0;
}
return PQDSA_KEY_set_raw_private_key(out->pkey.pqdsa_key,CBS_data(key));

// Set the private key
if (!PQDSA_KEY_set_raw_private_key(out->pkey.pqdsa_key, CBS_data(key))) {
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
return 0;
}

// Calculate public key from private key if method exists
Copy link
Contributor

Choose a reason for hiding this comment

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

How expensive is this calculation? What customer workflow needs the public key given the private key? Could this be lazily initialized?

Copy link
Contributor

Choose a reason for hiding this comment

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

Other types of EVP_PKEY (which have public/private components) ensure that the public key is populated whenever a private key is deserialized. For example, see EC and ED25519.

Other EVP_PKEY_xxx methods (such as EVP_PKEY_cmp) assume that the public key is populated for any EVP_PKEY that has functions available for public keys. (In fact EVP_PKEY_cmp can currently SEGFAULT for EVP_PKEY_PQDSA due to this.)

There might be other places for this computation to take place, but it needs to be done when deserializing a private key.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahhhh good point, looking at ED25519 a little more should we add raw support?

  1. priv_decode -> ed25519_priv_decode decodes the private key and calls ed25519_set_priv_raw
  2. set_priv_raw -> ed25519_set_priv_raw calculates the public key from the private key, and compares it to the supplied public key if provided

Here in ML-DSA:

  1. priv_decode -> pqdsa_priv_decode decodes the private key and calculates the public key and sets it
  2. set_prive_raw -> not implemented for pqdsa

Copy link
Contributor Author

@jakemas jakemas Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

We do implement set priv raw for pqdsa, just not as part of the EVP methods...

Some more background: set_priv_raw is not implemented for ML-DSA or ML-KEM. In ED25519 things are a little different, as there are no "parameter sets" for ED25519, it's just one algorithm. In ML-DSA/ML-KEM we have multiple NIDs to define algorithm versions.

As such, we implement EVP_PKEY_pqdsa_new_raw_private_key at https://github.com/aws/aws-lc/blob/main/include/openssl/evp.h#L960-L970 just like for KEMs. I suppose your suggestion is then to remove EVP_PKEY_pqdsa_new_raw_private_key / EVP_PKEY_pqdsa_new_raw_public_key and support via EVP_PKEY_new_raw_private_key/ EVP_PKEY_new_raw_public_key? The problem is, EVP_PKEY_new_raw_private_key expects type not nid and type = EVP_PKEY_PQDSA is not specific enough to know which NID it is... there's no OID, and we can't use the length to determine uniqueness as another scheme could have the same length.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the aws-lc-rs, I'm just using the length of the "raw" key to differentiate between the NIDs under the EVP_PKEY_PQDSA type: justsmth/aws-lc-rs@088e116

Would this be an acceptable approach for more cohesive integration into EVP_PKEY_new_raw_private_key and EVP_PKEY_new_raw_public_key?

Copy link
Contributor Author

@jakemas jakemas Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah I considered that too, it works, but it does present an issue should another algorithm be added that has the same length of raw keys -- which may sound unlikely, but would present issues with HashML-DSA that has a different OID/NID but same lengths. I do have some ideas -- but likely out of scope for this PR.

if (out->pkey.pqdsa_key->pqdsa->method->pqdsa_pack_pk_from_sk != NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we always return failure (0) if we're unable to initialize the public key? I assume the pqdsa_pack_pk_from_sk function ptr should always be non-NULL here (line 360) so this condition would never fail?

Upon seeing a successful return code, a caller assumes the EVP_PKEY is initialized and might subsequently SEGFAULT on a call to (e.g.) EVP_PKEY_cmp.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see. I was trying to prevent against a failure if the ptr is non-null, as you say. Removed this check in cd8da04 and cleaned up the surrounding logic to only return SUCESS when both priv and pub are populated.

size_t pk_len = out->pkey.pqdsa_key->pqdsa->public_key_len;
uint8_t *public_key = OPENSSL_malloc(pk_len);

if (public_key == NULL) {
OPENSSL_PUT_ERROR(EVP, ERR_R_MALLOC_FAILURE);
return 0;
}
// attempt to construct the public key from priv
if (!out->pkey.pqdsa_key->pqdsa->method->pqdsa_pack_pk_from_sk(
public_key, CBS_data(key))) {
OPENSSL_free(public_key);
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
return 0;
}
// set the public key
int ret = PQDSA_KEY_set_raw_public_key(out->pkey.pqdsa_key, public_key);
OPENSSL_free(public_key);
Copy link
Contributor

Choose a reason for hiding this comment

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

Since public_key was allocated internally can the PKEY just take ownership of it without having to copy it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see this as being possible, as the size of the public_key is dynamic, and defined by out->pkey.pqdsa_key->pqdsa->public_key_len. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, you allocate the key on L162, looking at PQDSA_KEY_set_raw_public_key it just calls OPENSSL_memdup which allocates a new buffer and memcpy the data into that new buffer, then this function frees public_key. Why not store public_key in out->pkey.pqdsa_key->public_key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

got it! 54224d2


if (!ret) {
OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
return 0;
}
}

return 1;
}

static int pqdsa_priv_encode(CBB *out, const EVP_PKEY *pkey) {
Expand Down
11 changes: 11 additions & 0 deletions crypto/evp_extra/p_pqdsa_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,6 +1397,17 @@ TEST_P(PQDSAParameterTest, MarshalParse) {
ASSERT_TRUE(priv_pkey_from_der);
EXPECT_EQ(Bytes(priv_pkey_from_der->pkey.pqdsa_key->private_key, GetParam().private_key_len),
Bytes(pkey->pkey.pqdsa_key->private_key, GetParam().private_key_len));

// When importing a PQDSA private key, the public key will be calculated and
// used to populate the public key. To test the calculated key is correct,
// we first check that the public key has been populated, then test for equality
// with the expected public key:
ASSERT_NE(priv_pkey_from_der, nullptr);
EXPECT_NE(priv_pkey_from_der->pkey.pqdsa_key->public_key, nullptr);
EXPECT_NE(priv_pkey_from_der->pkey.pqdsa_key->private_key, nullptr);

EXPECT_EQ(Bytes(priv_pkey_from_der->pkey.pqdsa_key->public_key, GetParam().public_key_len),
Bytes(pkey->pkey.pqdsa_key->public_key, GetParam().public_key_len));
}

TEST_P(PQDSAParameterTest, SIGOperations) {
Expand Down
3 changes: 3 additions & 0 deletions crypto/pqdsa/internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ typedef struct {
size_t sig_len,
const uint8_t *digest,
size_t digest_len);

int (*pqdsa_pack_pk_from_sk)(uint8_t *public_key,
const uint8_t *private_key);
} PQDSA_METHOD;

// PQDSA structure and helper functions.
Expand Down
9 changes: 6 additions & 3 deletions crypto/pqdsa/pqdsa.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,23 +89,26 @@ static const PQDSA_METHOD sig_ml_dsa_44_method = {
ml_dsa_44_sign,
ml_dsa_extmu_44_sign,
ml_dsa_44_verify,
ml_dsa_extmu_44_verify
ml_dsa_extmu_44_verify,
ml_dsa_44_pack_pk_from_sk
};

static const PQDSA_METHOD sig_ml_dsa_65_method = {
ml_dsa_65_keypair,
ml_dsa_65_sign,
ml_dsa_extmu_65_sign,
ml_dsa_65_verify,
ml_dsa_extmu_65_verify
ml_dsa_extmu_65_verify,
ml_dsa_65_pack_pk_from_sk
};

static const PQDSA_METHOD sig_ml_dsa_87_method = {
ml_dsa_87_keypair,
ml_dsa_87_sign,
ml_dsa_extmu_87_sign,
ml_dsa_87_verify,
ml_dsa_extmu_87_verify
ml_dsa_extmu_87_verify,
ml_dsa_87_pack_pk_from_sk
};

static const PQDSA sig_ml_dsa_44 = {
Expand Down
Loading