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 support for SSL_group_to_name and SSL_get_negotiated_group #2201

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

romen
Copy link

@romen romen commented Mar 7, 2024

This PR adds support for OpenSSL 3.0 functions to retrieve information about the effective group negotiated in the TLS handshake from an SSL* object.

Marked as draft as I need more guidance from maintainers:

  • cargo test fails with

    error: returning ‘const char * (*)(SSL *, int)’ {aka ‘const char * (*)(struct ssl_st *, int)’} from a function with incompatible 
           return type ‘const char * (*)(const SSL *, int)’ {aka ‘const char * (*)(const struct ssl_st *, int)’}
           [-Werror=incompatible-pointer-types]
    

    so I must be doing something wrong with the declaration, but I am struggling to see it

  • in openssl/src/ssl/mod.rs:3523 I currently have an .expect() which is probably undesirable, but I'd like guidance on the proper way to handle that error to be consistent with the rest of the crate.

There is a mismatch upstream in OpenSSL between documentation and header
file regarding the `const` for the SSL argument.

See sfackler#2201 (comment)
Clippy suggests removing superfluous lifetime annotations
@romen romen force-pushed the nt/SSL_get_negotiated_group branch from f92c896 to 0d838a5 Compare March 8, 2024 12:47
/// bitwise OR of TLSEXT_nid_unknown (0x1000000) and the id of the group.
#[corresponds(SSL_get_negotiated_group)]
#[cfg(ossl300)]
pub fn negotiated_group(&self) -> Result<c_int, ErrorStack> {
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we shouldn't be exposing this as a raw c_int. Since their doing the weird unknown thing, I think we'd need to make a NegotiatedGroup type that provides a pub fn nid(self) -> Option<Nid> and pub fn unknown_group_id(self) -> Option<i32>.

Copy link
Author

Choose a reason for hiding this comment

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

I can do this change.

Then again in SSL::group_to_name which calls SSL_group_to_name(), we need the raw unaltered c_int.

Should the NegotiatedGroup type implement From/To trait to convert back to the raw c_int?
Suggestions on how to implement that?

Copy link
Owner

Choose a reason for hiding this comment

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

Yep, these wrapper types have an as_raw method to get the c_int out: https://docs.rs/openssl/latest/openssl/nid/struct.Nid.html#method.as_raw

Copy link
Author

Choose a reason for hiding this comment

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

I tried addressing this with 893ce6a

@romen romen marked this pull request as ready for review March 15, 2024 14:07
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

3 participants