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

add temp key bindings #2076

Merged
merged 5 commits into from Nov 10, 2023
Merged

add temp key bindings #2076

merged 5 commits into from Nov 10, 2023

Conversation

jmayclin
Copy link
Contributor

@jmayclin jmayclin commented Oct 30, 2023

This PR adds bindings for a selection of the "temp key" apis.

https://www.openssl.org/docs/manmaster/man3/SSL_get_peer_tmp_key.html

These functions are implemented using the ctrl macro. Constants are taken from

https://github.com/openssl/openssl/blob/497a7810bcee48781aa12d4db870f6a565bd0592/include/openssl/ssl.h.in#L1314

https://github.com/openssl/openssl/blob/497a7810bcee48781aa12d4db870f6a565bd0592/include/openssl/ssl.h.in#L1334

API Shape

The api's return a Result<PKey> rather than a Result<Option<PKey>>, because it is expected that whenever the function returns a non-error value, the key is valid. This can be seen from the implementation of the function in openssl source:
https://github.com/openssl/openssl/blob/186b3f6a016de8fcf8573be111e3d174ca20f1bc/ssl/s3_lib.c#L3734-L3741

Macro Return Value

The Openssl ctrl macros return c_long, but the functions implemented the macros normally downcast this to a c_int. The tmp_key apis do not downcast this value, and instead return a c_long. I added an additional cvt_long error function to handle this.

History

The man pages don't mention anything about the history of the function and I couldn't find anything in the changelog so I dug through the openssl git repository to find the commit that introduced the tmp_key apis.

commit a51c9f637cdef7926d8a8991365e4b58975346db
Author: Viktor Dukhovni <openssl-users@dukhovni.org>
Date:   Sat Nov 10 01:53:56 2018 -0500

    Added missing signature algorithm reflection functions
    
        SSL_get_signature_nid()      -- local signature algorithm
        SSL_get_signature_type_nid() -- local signature algorithm key type
        SSL_get_peer_tmp_key()       -- Peer key-exchange public key
        SSL_get_tmp_key              -- local key exchange public key
    
    Aliased pre-existing SSL_get_server_tmp_key(), which was formerly
    just for clients, to SSL_get_peer_tmp_key().  Changed internal
    calls to use the new name.
    
    Reviewed-by: Matt Caswell <matt@openssl.org>

I then pulled in git branches until I found the ones that contained the feature commit.

~/workspace/openssl$ git branch
  OpenSSL_1_0_2-stable
  OpenSSL_1_1_0-stable
  OpenSSL_1_1_1-stable
  master
* openssl-3.0
~/workspace/openssl$ git branch --contains a51c9f637cdef7926d8a8991365e4b58975346db
  master
* openssl-3.0

From this, I concluded that the APIs were first available in Openssl 3.0.0

@jmayclin
Copy link
Contributor Author

The CI is failing because my cfg-gated definitions are causing "unused import" and "unused function" errors.

 error: unused imports: `PKey`, `Public`
  --> openssl/src/ssl/mod.rs:70:31
   |
70 | use crate::pkey::{HasPrivate, PKey, PKeyRef, Params, Private, Public};
   |                               ^^^^                            ^^^^^^
   |
   = note: `-D unused-imports` implied by `-D warnings`

error: unused import: `cvt_long`
  --> openssl/src/ssl/mod.rs:81:18
   |
81 | use crate::{cvt, cvt_long, cvt_n, cvt_p, init};
   |                  ^^^^^^^^

error: function `cvt_long` is never used
   --> openssl/src/lib.rs:216:4
    |
216 | fn cvt_long(r: c_long) -> Result<c_long, ErrorStack> {
    |    ^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`

I think the neatest way to solve this from my side is by gating the import statements/definitions behind cfg(ossl300) statements. It does seem a little odd to gate the definition of cvt_long on the openssl version, but I think that's the easiest way to work with the current CI setup. Let me know if you have different thoughts. 🙂

@sfackler
Copy link
Owner

Yeah cfg'ing the imports off is the way to go.

openssl/src/ssl/mod.rs Outdated Show resolved Hide resolved
* tmp_key return private information, use the appropriate type
* gate imports behind the appropriate flags
@jmayclin jmayclin marked this pull request as ready for review October 30, 2023 20:45
@jmayclin
Copy link
Contributor Author

jmayclin commented Nov 9, 2023

Hi @sfackler, I'm wondering if I could have another review on this? Thanks!

@sfackler sfackler merged commit f89c20b into sfackler:master Nov 10, 2023
59 checks passed
@sfackler
Copy link
Owner

Thanks!

bors added a commit to rust-lang/cargo that referenced this pull request Nov 28, 2023
…, r=weihanglo

chore(deps): update rust crate openssl to 0.10.60 [security]

[![Mend Renovate logo banner](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [openssl](https://togithub.com/sfackler/rust-openssl) | workspace.dependencies | patch | `0.10.57` -> `0.10.60` |

### GitHub Vulnerability Alerts

#### [GHSA-xphf-cx8h-7q9g](https://togithub.com/sfackler/rust-openssl/issues/2096)

This function returned a reference into an OpenSSL datastructure, but there was no way to ensure OpenSSL would not mutate the datastructure behind one's back.

Use of this function should be replaced with `X509StoreRef::all_certificates`.

---

### Release Notes

<details>
<summary>sfackler/rust-openssl (openssl)</summary>

### [`v0.10.60`](https://togithub.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.60)

[Compare Source](https://togithub.com/sfackler/rust-openssl/compare/openssl-v0.10.59...openssl-v0.10.60)

#### What's Changed

-   Correct off-by-one in minimum output buffer size computation by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2088
-   Expose a few more (bad) ciphers in cipher::Cipher by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2084
-   add temp key bindings by [`@&#8203;jmayclin](https://togithub.com/jmayclin)` in [sfackler/rust-openssl#2076
-   Expose ChaCha20 on LibreSSL by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2093
-   Revert "Correct off-by-one in minimum output buffer size computation" by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2090
-   Added `update_unchecked` to `symm::Crypter` by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2100
-   fixes [#&#8203;2096](https://togithub.com/sfackler/rust-openssl/issues/2096) -- deprecate `X509StoreRef::objects`, it is unsound by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2099
-   Don't leak when overwriting ex data by [`@&#8203;sfackler](https://togithub.com/sfackler)` in [sfackler/rust-openssl#2102
-   Release openssl v0.10.60 and openssl-sys v0.9.96 by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2104

**Full Changelog**: sfackler/rust-openssl@openssl-v0.10.59...openssl-v0.10.60

### [`v0.10.59`](https://togithub.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.59)

[Compare Source](https://togithub.com/sfackler/rust-openssl/compare/openssl-v0.10.58...openssl-v0.10.59)

#### What's Changed

-   Add binding to NID of Chacha20-Poly1305 cipher by [`@&#8203;Arnavion](https://togithub.com/Arnavion)` in [sfackler/rust-openssl#2081
-   Fixed cfg for RSA_PSS by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2079
-   fixes [#&#8203;2050](https://togithub.com/sfackler/rust-openssl/issues/2050) -- build and test on libressl 3.8.2 by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2082
-   Release openssl v0.10.59 and openssl-sys v0.9.95 by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2083

#### New Contributors

-   [`@&#8203;Arnavion](https://togithub.com/Arnavion)` made their first contribution in [sfackler/rust-openssl#2081

**Full Changelog**: sfackler/rust-openssl@openssl-v0.10.58...openssl-v0.10.59

### [`v0.10.58`](https://togithub.com/sfackler/rust-openssl/releases/tag/openssl-v0.10.58)

[Compare Source](https://togithub.com/sfackler/rust-openssl/compare/openssl-v0.10.57...openssl-v0.10.58)

#### What's Changed

-   LibreSSL 3.8.1 support by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2035
-   Update vendored version to openssl 3 by [`@&#8203;amousset](https://togithub.com/amousset)` in [sfackler/rust-openssl#1925
-   Test against 3.2.0-alpha1 by [`@&#8203;sfackler](https://togithub.com/sfackler)` in [sfackler/rust-openssl#2037
-   Removed reference to non-existent method by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2039
-   Bump CI to 1.1.1w by [`@&#8203;sfackler](https://togithub.com/sfackler)` in [sfackler/rust-openssl#2040
-   \[openssl-sys] Add X509\_check\_{host,email,ip,ip_asc} fns by [`@&#8203;jgallagher](https://togithub.com/jgallagher)` in [sfackler/rust-openssl#2042
-   Expose CBC mode for several more (bad) ciphers by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2045
-   Expose two additional Pkey IDs by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2046
-   Add support for CRL extensions and the Authority Information Access e… by [`@&#8203;AdmiralGT](https://togithub.com/AdmiralGT)` in [sfackler/rust-openssl#2003
-   Fix clippy warnings produced by newer Rust by [`@&#8203;wiktor-k](https://togithub.com/wiktor-k)` in [sfackler/rust-openssl#2052
-   Use osslconf on BoringSSL by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2056
-   Make X509\_ALGOR opaque for LibreSSL by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2060
-   Don't ignore ECDSA tests without GF2m support by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2061
-   Clarify 'possible LibreSSL bug' by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2062
-   Enable BN_mod_sqrt() for upcoming LibreSSL 3.8.2 by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2063
-   Enable SHA-3 for LibreSSL 3.8.0 by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2064
-   Remove DH_generate_parameters for LibreSSL 3.8.2 by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2065
-   Use EVP_MD_CTX\_{new,free}() in LibreSSL 3.8.2 by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2067
-   Enable HKDF support for LibreSSL >= 3.6.0 by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2066
-   Two build script fixes for LibreSSL by [`@&#8203;botovq](https://togithub.com/botovq)` in [sfackler/rust-openssl#2068
-   Respect OPENSSL_NO_OCB on AES functions by [`@&#8203;GuyLewin](https://togithub.com/GuyLewin)` in [sfackler/rust-openssl#2070
-   Support OPENSSL_NO_SCRYPT by [`@&#8203;GuyLewin](https://togithub.com/GuyLewin)` in [sfackler/rust-openssl#2071
-   Bump 3.2.0 beta by [`@&#8203;sfackler](https://togithub.com/sfackler)` in [sfackler/rust-openssl#2073
-   add security level bindings by [`@&#8203;jmayclin](https://togithub.com/jmayclin)` in [sfackler/rust-openssl#2074
-   Release openssl v0.10.58 and openssl-sys v0.9.94 by [`@&#8203;alex](https://togithub.com/alex)` in [sfackler/rust-openssl#2078

#### New Contributors

-   [`@&#8203;amousset](https://togithub.com/amousset)` made their first contribution in [sfackler/rust-openssl#1925
-   [`@&#8203;jgallagher](https://togithub.com/jgallagher)` made their first contribution in [sfackler/rust-openssl#2042
-   [`@&#8203;AdmiralGT](https://togithub.com/AdmiralGT)` made their first contribution in [sfackler/rust-openssl#2003
-   [`@&#8203;botovq](https://togithub.com/botovq)` made their first contribution in [sfackler/rust-openssl#2060
-   [`@&#8203;GuyLewin](https://togithub.com/GuyLewin)` made their first contribution in [sfackler/rust-openssl#2070
-   [`@&#8203;jmayclin](https://togithub.com/jmayclin)` made their first contribution in [sfackler/rust-openssl#2074

**Full Changelog**: sfackler/rust-openssl@openssl-v0.10.57...openssl-v0.10.58

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "" (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.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->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).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy41OS44IiwidXBkYXRlZEluVmVyIjoiMzcuNTkuOCIsInRhcmdldEJyYW5jaCI6Im1hc3RlciJ9-->
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.

None yet

2 participants