Add support for setting the nonce type and digest on a PKEY_CTX by facutuesca · Pull Request #2144 · rust-openssl/rust-openssl

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

+    /// 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

        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

    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

+    /// 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

+    /// 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

+    /// 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 😄

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

+    #[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

+    #[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

+    #[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();

chore(deps): update compatible [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [annotate-snippets](https://togithub.com/rust-lang/annotate-snippets-rs) | workspace.dependencies | patch | `0.10.1` -> `0.10.2` | | [anstream](https://togithub.com/rust-cli/anstyle) | workspace.dependencies | patch | `0.6.11` -> `0.6.13` | | [anyhow](https://togithub.com/dtolnay/anyhow) | workspace.dependencies | patch | `1.0.79` -> `1.0.80` | | [curl](https://togithub.com/alexcrichton/curl-rust) | workspace.dependencies | patch | `0.4.44` -> `0.4.46` | | [curl-sys](https://togithub.com/alexcrichton/curl-rust) | workspace.dependencies | patch | `0.4.71` -> `0.4.72+curl-8` | | [openssl](https://togithub.com/sfackler/rust-openssl) | workspace.dependencies | patch | `0.10.63` -> `0.10.64` | | [pkg-config](https://togithub.com/rust-lang/pkg-config-rs) | workspace.dependencies | patch | `0.3.29` -> `0.3.30` | | [semver](https://togithub.com/dtolnay/semver) | workspace.dependencies | patch | `1.0.21` -> `1.0.22` | | [serde](https://serde.rs) ([source](https://togithub.com/serde-rs/serde)) | workspace.dependencies | patch | `1.0.196` -> `1.0.197` | | [serde_json](https://togithub.com/serde-rs/json) | workspace.dependencies | patch | `1.0.113` -> `1.0.114` | | [snapbox](https://togithub.com/assert-rs/trycmd/tree/main/crates/snapbox) ([source](https://togithub.com/assert-rs/trycmd)) | workspace.dependencies | patch | `0.5.6` -> `0.5.7` | | [tempfile](https://stebalien.com/projects/tempfile-rs/) ([source](https://togithub.com/Stebalien/tempfile)) | workspace.dependencies | minor | `3.9.0` -> `3.10.1` | | [thiserror](https://togithub.com/dtolnay/thiserror) | workspace.dependencies | patch | `1.0.56` -> `1.0.57` | | [toml_edit](https://togithub.com/toml-rs/toml) | workspace.dependencies | patch | `0.22.4` -> `0.22.6` | --- ### Release Notes <details> <summary>rust-lang/annotate-snippets-rs (annotate-snippets)</summary> ### [`v0.10.2`](https://togithub.com/rust-lang/annotate-snippets-rs/blob/HEAD/CHANGELOG.md#0102---2024-02-29) [Compare Source](https://togithub.com/rust-lang/annotate-snippets-rs/compare/0.10.1...0.10.2) ##### Added - Added `testing-colors` feature to remove platform-specific colors when testing [#82](https://togithub.com/rust-lang/annotate-snippets-rs/pull/82) </details> <details> <summary>rust-cli/anstyle (anstream)</summary> ### [`v0.6.13`](https://togithub.com/rust-cli/anstyle/compare/anstream-v0.6.12...anstream-v0.6.13) [Compare Source](https://togithub.com/rust-cli/anstyle/compare/anstream-v0.6.12...anstream-v0.6.13) ### [`v0.6.12`](https://togithub.com/rust-cli/anstyle/compare/anstream-v0.6.11...anstream-v0.6.12) [Compare Source](https://togithub.com/rust-cli/anstyle/compare/anstream-v0.6.11...anstream-v0.6.12) </details> <details> <summary>dtolnay/anyhow (anyhow)</summary> ### [`v1.0.80`](https://togithub.com/dtolnay/anyhow/releases/tag/1.0.80) [Compare Source](https://togithub.com/dtolnay/anyhow/compare/1.0.79...1.0.80) - Fix unused_imports warnings when compiled by rustc 1.78 </details> <details> <summary>alexcrichton/curl-rust (curl)</summary> ### [`v0.4.46`](https://togithub.com/alexcrichton/curl-rust/compare/0.4.45...0.4.46) [Compare Source](https://togithub.com/alexcrichton/curl-rust/compare/0.4.45...0.4.46) ### [`v0.4.45`](https://togithub.com/alexcrichton/curl-rust/compare/0.4.44...0.4.45) [Compare Source](https://togithub.com/alexcrichton/curl-rust/compare/0.4.44...0.4.45) </details> <details> <summary>sfackler/rust-openssl (openssl)</summary> ### [`v0.10.64`](https://togithub.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.64) [Compare Source](https://togithub.com/sfackler/rust-openssl/compare/openssl-v0.10.63...openssl-v0.10.64) ##### What's Changed - Make \_STACK opaque for LibreSSL >= 3.9.0 by [`@botovq](https://togithub.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2153](https://togithub.com/sfackler/rust-openssl/pull/2153) - enable x509 verify and groups list for boringssl by [`@zh-jq](https://togithub.com/zh-jq)` in [https://github.com/sfackler/rust-openssl/pull/2155](https://togithub.com/sfackler/rust-openssl/pull/2155) - Cleanup some not-required Path::new invocations by [`@alex](https://togithub.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2158](https://togithub.com/sfackler/rust-openssl/pull/2158) - fixed a clippy (nightly) warning by [`@alex](https://togithub.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2161](https://togithub.com/sfackler/rust-openssl/pull/2161) - Bump actions versions by [`@alex](https://togithub.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2162](https://togithub.com/sfackler/rust-openssl/pull/2162) - Add support for setting the nonce type and digest on a PKEY_CTX by [`@facutuesca](https://togithub.com/facutuesca)` in [https://github.com/sfackler/rust-openssl/pull/2144](https://togithub.com/sfackler/rust-openssl/pull/2144) - rebuild openssl-sys if the underlying openssl has changed by [`@reaperhulk](https://togithub.com/reaperhulk)` in [https://github.com/sfackler/rust-openssl/pull/2157](https://togithub.com/sfackler/rust-openssl/pull/2157) - Added binding for EVP_default_properties_enable_fips by [`@alex](https://togithub.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2168](https://togithub.com/sfackler/rust-openssl/pull/2168) - LibreSSL 3.9: fix CRYPTO_malloc/free signatures by [`@botovq](https://togithub.com/botovq)` in [https://github.com/sfackler/rust-openssl/pull/2170](https://togithub.com/sfackler/rust-openssl/pull/2170) - Expose alias on X509 structs by [`@alex](https://togithub.com/alex)` in [https://github.com/sfackler/rust-openssl/pull/2167](https://togithub.com/sfackler/rust-openssl/pull/2167) - bump openssl and openssl-sys + changelogs by [`@reaperhulk](https://togithub.com/reaperhulk)` in [https://github.com/sfackler/rust-openssl/pull/2175](https://togithub.com/sfackler/rust-openssl/pull/2175) **Full Changelog**: rust-openssl/rust-openssl@openssl-v0.10.63...openssl-v0.10.64 </details> <details> <summary>rust-lang/pkg-config-rs (pkg-config)</summary> ### [`v0.3.30`](https://togithub.com/rust-lang/pkg-config-rs/blob/HEAD/CHANGELOG.md#0330---2024-02-14) [Compare Source](https://togithub.com/rust-lang/pkg-config-rs/compare/0.3.29...0.3.30) ##### Changed - Update documentation for cross-compilation ([#161](https://togithub.com/rust-lang/pkg-config-rs/issues/161)). - Update GitHub Action CI ([#160](https://togithub.com/rust-lang/pkg-config-rs/issues/160)). </details> <details> <summary>dtolnay/semver (semver)</summary> ### [`v1.0.22`](https://togithub.com/dtolnay/semver/releases/tag/1.0.22) [Compare Source](https://togithub.com/dtolnay/semver/compare/1.0.21...1.0.22) - Fix unused_imports warnings when compiled by rustc 1.78 </details> <details> <summary>serde-rs/serde (serde)</summary> ### [`v1.0.197`](https://togithub.com/serde-rs/serde/releases/tag/v1.0.197) [Compare Source](https://togithub.com/serde-rs/serde/compare/v1.0.196...v1.0.197) - Fix unused_imports warnings when compiled by rustc 1.78 - Optimize code size of some Display impls ([#2697](https://togithub.com/serde-rs/serde/issues/2697), thanks [`@nyurik](https://togithub.com/nyurik))` </details> <details> <summary>serde-rs/json (serde_json)</summary> ### [`v1.0.114`](https://togithub.com/serde-rs/json/releases/tag/v1.0.114) [Compare Source](https://togithub.com/serde-rs/json/compare/v1.0.113...v1.0.114) - Fix unused_imports warnings when compiled by rustc 1.78 </details> <details> <summary>assert-rs/trycmd (snapbox)</summary> ### [`v0.5.7`](https://togithub.com/assert-rs/trycmd/compare/snapbox-v0.5.6...snapbox-v0.5.7) [Compare Source](https://togithub.com/assert-rs/trycmd/compare/snapbox-v0.5.6...snapbox-v0.5.7) </details> <details> <summary>Stebalien/tempfile (tempfile)</summary> ### [`v3.10.1`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#3101) [Compare Source](https://togithub.com/Stebalien/tempfile/compare/v3.10.0...v3.10.1) - Handle potential integer overflows in 32-bit systems when seeking/truncating "spooled" temporary files past 4GiB (2³²). - Handle a theoretical 32-bit overflow when generating a temporary file name larger than 4GiB. Now it'll panic (on allocation failure) rather than silently succeeding due to wraparound. Thanks to [`@stoeckmann](https://togithub.com/stoeckmann)` for finding and fixing both of these issues. ### [`v3.10.0`](https://togithub.com/Stebalien/tempfile/blob/HEAD/CHANGELOG.md#3100) [Compare Source](https://togithub.com/Stebalien/tempfile/compare/v3.9.0...v3.10.0) - Drop `redox_syscall` dependency, we now use `rustix` for Redox. - Add `Builder::permissions` for setting the permissions on temporary files and directories (thanks to [`@Byron](https://togithub.com/Byron)).` - Update rustix to 0.38.31. - Update fastrand to 2.0.1. </details> <details> <summary>dtolnay/thiserror (thiserror)</summary> ### [`v1.0.57`](https://togithub.com/dtolnay/thiserror/releases/tag/1.0.57) [Compare Source](https://togithub.com/dtolnay/thiserror/compare/1.0.56...1.0.57) - Generate more efficient `Display` impl for error message which do not contain any interpolated value ([#286](https://togithub.com/dtolnay/thiserror/issues/286), thanks [`@nyurik](https://togithub.com/nyurik))` </details> <details> <summary>toml-rs/toml (toml_edit)</summary> ### [`v0.22.6`](https://togithub.com/toml-rs/toml/compare/v0.22.5...v0.22.6) [Compare Source](https://togithub.com/toml-rs/toml/compare/v0.22.5...v0.22.6) ### [`v0.22.5`](https://togithub.com/toml-rs/toml/compare/v0.22.4...v0.22.5) [Compare Source](https://togithub.com/toml-rs/toml/compare/v0.22.4...v0.22.5) </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 5am on the first day of the month" (UTC), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 👻 **Immortal**: This PR will be recreated if closed unmerged. Get [config help](https://togithub.com/renovatebot/renovate/discussions) if that's undesired. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/rust-lang/cargo).

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 rust-openssl:master Feb 8, 2024

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

Closed

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

	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();

Conversation

facutuesca commented Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

facutuesca Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reaperhulk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

facutuesca commented Jan 8, 2024 •

edited

Loading

facutuesca Feb 6, 2024 •

edited

Loading