Add support for setting the nonce type and digest on a PKEY_CTX #2144

facutuesca · 2024-01-08T14:39:42Z

This PR adds getters and setters for the digest and the nonce type of a PKEY_CTX. In OpenSSL, this is done using the OSSL_PARAM API, but here we hide that detail and just expose PkeyCtxRef::{digest, set_digest} and PkeyCtxRef::{nonce_type, set_nonce_type} methods that internally take care of the OSSL_PARAM details.

Adding these is motivated by pyca/cryptography#9795, particularly adding support for RFC6979 (deterministic ECDSA) to cryptography.

Here's the OpenSSL documentation for nonce-type, the new parameter added in OpenSSL 3.2.0 for RFC6979

reaperhulk · 2024-02-05T15:28:41Z

openssl/src/pkey_ctx.rs

+    /// Requires OpenSSL 3.0.0 or newer.
+    #[cfg(ossl300)]
+    #[corresponds(EVP_PKEY_CTX_set_params)]
+    pub fn set_digest(&mut self, hash_algorithm: MessageDigest) -> Result<(), ErrorStack> {


I don't think we need this. Check out set_signature_md and the underlying EVP_PKEY_CTX_set_signature_md impl in crypto/evp/pmeth_lib.c

Yes, good catch! I removed set_digest, although I did leave digest() as a method to access the digest set by set_signature_md.

reaperhulk · 2024-02-05T15:31:14Z

openssl/src/pkey_ctx.rs

@@ -999,4 +1123,34 @@ mod test {
        // The digest is the end of the DigestInfo structure.
        assert_eq!(result_buf[length - digest.len()..length], digest);
    }
+
+    #[test]


We should add a test that confirms setting noncetype + digest works as expected. You can get a test vector from: https://github.com/openssl/openssl/blob/master/test/recipes/30-test_evp_data/evppkey_ecdsa_rfc6979.txt

Good idea, fixed!

reaperhulk · 2024-02-05T15:46:02Z

openssl-sys/src/handwritten/evp.rs

@@ -646,3 +652,16 @@ extern "C" {
    pub fn EVP_EncodeBlock(dst: *mut c_uchar, src: *const c_uchar, src_len: c_int) -> c_int;
    pub fn EVP_DecodeBlock(dst: *mut c_uchar, src: *const c_uchar, src_len: c_int) -> c_int;
 }
+
+extern "C" {


These should go in a new file params.rs as they're declared in openssl/params.h rather than evp.

reaperhulk · 2024-02-05T19:51:11Z

openssl/src/pkey_ctx.rs

+    /// Requires OpenSSL 3.0.0 or newer.
+    #[cfg(ossl300)]
+    #[corresponds(EVP_PKEY_CTX_get_params)]
+    pub fn digest(&mut self) -> Result<Option<MessageDigest>, ErrorStack> {


This doesn't really match the naming rust-openssl typically uses for getters. It'd be signature_md I think? But honestly I'm not sure we need this getter at all. I'd drop it until someone articulates a need.

reaperhulk · 2024-02-05T19:52:27Z

openssl/src/pkey_ctx.rs

+    /// This is only useful for DSA and ECDSA.
+    /// Requires OpenSSL 3.2.0 or newer.
+    #[cfg(ossl320)]
+    #[corresponds(EVP_PKEY_CTX_set_params)]


These corresponds probably should be dropped since this uses the function, but it is a generic func that can be used in many places. That said, I'd defer to @sfackler for his opinion.

I thought the same at first, but there's also a #corresponds(EVP_PKEY_CTX_ctrl)] (also a generic func) for set_keygen_cipher and set_keygen_mac_key, so I went with it anyway. I can remove it if it should not be there.

reaperhulk · 2024-02-05T19:52:47Z

openssl/src/pkey_ctx.rs

+    /// This is only useful for DSA and ECDSA.
+    /// Requires OpenSSL 3.2.0 or newer.
+    #[cfg(ossl320)]
+    #[corresponds(EVP_PKEY_CTX_get_params)]


Same corresponds statement.

(same comment as above)

reaperhulk

modulo the corresponds question this lgtm 😄

openssl/src/pkey_ctx.rs

alex · 2024-02-07T23:44:56Z

openssl/src/pkey_ctx.rs

+    #[cfg(ossl320)]
+    #[corresponds(EVP_PKEY_CTX_set_params)]
+    pub fn set_nonce_type(&mut self, nonce_type: NonceType) -> Result<(), ErrorStack> {
+        let nonce_field_name = CString::new("nonce-type").unwrap();


Can this be CStr::new("nonce-type\0") or something, to avoid the allocation?

Yes, changed to

CStr::from_bytes_with_nul("nonce-type\0".as_bytes())

alex · 2024-02-08T13:57:07Z

openssl/src/pkey_ctx.rs

+    #[cfg(ossl320)]
+    #[corresponds(EVP_PKEY_CTX_set_params)]
+    pub fn set_nonce_type(&mut self, nonce_type: NonceType) -> Result<(), ErrorStack> {
+        let nonce_field_name = CStr::from_bytes_with_nul("nonce-type\0".as_bytes()).unwrap();


Suggested change

let nonce_field_name = CStr::from_bytes_with_nul("nonce-type\0".as_bytes()).unwrap();

let nonce_field_name = CStr::from_bytes_with_nul(b"nonce-type\0").unwrap();

alex · 2024-02-08T13:57:17Z

openssl/src/pkey_ctx.rs

+    #[cfg(ossl320)]
+    #[corresponds(EVP_PKEY_CTX_get_params)]
+    pub fn nonce_type(&mut self) -> Result<NonceType, ErrorStack> {
+        let nonce_field_name = CStr::from_bytes_with_nul("nonce-type\0".as_bytes()).unwrap();


Suggested change

let nonce_field_name = CStr::from_bytes_with_nul("nonce-type\0".as_bytes()).unwrap();

let nonce_field_name = CStr::from_bytes_with_nul(b"nonce-type\0").unwrap();

facutuesca force-pushed the deterministic-nonce branch 4 times, most recently from a056b21 to 78b2e98 Compare January 8, 2024 15:12

facutuesca force-pushed the deterministic-nonce branch from 78b2e98 to 8f11103 Compare February 5, 2024 15:13

Add support for setting the nonce type and digest on a PKEY_CTX

335b96b

facutuesca force-pushed the deterministic-nonce branch from 8f11103 to 335b96b Compare February 5, 2024 15:14

reaperhulk reviewed Feb 5, 2024

View reviewed changes

facutuesca added 2 commits February 5, 2024 17:56

Remove unnecessary PkeyCtxRef::set_digest function

e77c613

Move OSSL_PARAM definitions to new file

76043a9

facutuesca force-pushed the deterministic-nonce branch from ee6cb8b to 76043a9 Compare February 5, 2024 17:05

Add test vector for ECDSA with deterministic signature

7a66dad

facutuesca requested a review from reaperhulk February 5, 2024 19:08

reaperhulk reviewed Feb 5, 2024

View reviewed changes

Remove PkeyCtxRef::digest() getter

ce5e9e4

facutuesca force-pushed the deterministic-nonce branch from 8ec061c to ce5e9e4 Compare February 6, 2024 09:56

facutuesca requested a review from reaperhulk February 6, 2024 10:11

reaperhulk approved these changes Feb 6, 2024

View reviewed changes

alex reviewed Feb 7, 2024

View reviewed changes

Use CStr to avoid heap allocation

870e290

alex reviewed Feb 8, 2024

View reviewed changes

Readability improvement

14c8247

alex approved these changes Feb 8, 2024

View reviewed changes

alex merged commit 84162bf into sfackler:master Feb 8, 2024
53 checks passed

facutuesca deleted the deterministic-nonce branch February 8, 2024 14:58

facutuesca mentioned this pull request Feb 8, 2024

Support for ECDSA deterministic signing (RFC 6979) pyca/cryptography#10369

Merged

adiroiban mentioned this pull request Mar 15, 2024

OpenSSL 3.2 features to expose pyca/cryptography#9795

Open

8 tasks

renovate bot mentioned this pull request Jul 8, 2024

chore(deps): update all non-major dependencies lyonkit/lyonkit-api#222

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for setting the nonce type and digest on a PKEY_CTX #2144

Add support for setting the nonce type and digest on a PKEY_CTX #2144

facutuesca commented Jan 8, 2024 •

edited

Loading

reaperhulk Feb 5, 2024

facutuesca Feb 5, 2024

reaperhulk Feb 5, 2024

facutuesca Feb 5, 2024

reaperhulk Feb 5, 2024

facutuesca Feb 5, 2024

reaperhulk Feb 5, 2024

facutuesca Feb 6, 2024

reaperhulk Feb 5, 2024

facutuesca Feb 6, 2024 •

edited

Loading

reaperhulk Feb 5, 2024

facutuesca Feb 6, 2024

reaperhulk left a comment

alex Feb 7, 2024

facutuesca Feb 8, 2024

alex Feb 8, 2024

facutuesca Feb 8, 2024

alex Feb 8, 2024

facutuesca Feb 8, 2024

	let nonce_field_name = CStr::from_bytes_with_nul("nonce-type\0".as_bytes()).unwrap();
	let nonce_field_name = CStr::from_bytes_with_nul(b"nonce-type\0").unwrap();

Add support for setting the nonce type and digest on a PKEY_CTX #2144

Add support for setting the nonce type and digest on a PKEY_CTX #2144

Conversation

facutuesca commented Jan 8, 2024 • edited Loading

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

facutuesca Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reaperhulk left a 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

facutuesca commented Jan 8, 2024 •

edited

Loading

facutuesca Feb 6, 2024 •

edited

Loading