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 bindings to SSL_bytes_to_cipher_list #1921

Merged
merged 1 commit into from
May 15, 2023

Conversation

RoastVeg
Copy link
Contributor

@RoastVeg RoastVeg commented May 9, 2023

I have a use case where I need to change the SSL context on the server side based on the supported cipher suite provided in the client hello callback. Without SSL_bytes_to_cipher_list support in rust-openssl, there really isn't a convenient way to handle the output from client_hello_ciphers, as the format is a real mess.

pub fn client_hello_ciphers_stack(&self) -> Option<(Stack<SslCipher>, Stack<SslCipher>)> {
unsafe {
let mut ptr = ptr::null();
let len = ffi::SSL_client_hello_get0_ciphers(self.as_ptr(), &mut ptr);
Copy link
Owner

Choose a reason for hiding this comment

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

I generally prefer to bind to each OpenSSL API as directly as possible, so I think we'd want separate methods for SSL_client_hello_get0_ciphers, SSL_bytes_to_cipher list, and SSL_client_hello_isv2 (though really the answer to that is just false as this point!).

Copy link
Contributor Author

@RoastVeg RoastVeg May 10, 2023

Choose a reason for hiding this comment

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

In that case as SSL_client_hello_get0_ciphers and SSL_client_hello_isv2 both already have bindings, would you accept a new method like the following?

// struct Ssl
#[corresponds(SSL_bytes_to_cipher_list)]
pub fn bytes_to_ciphers_stack(&self, bytes: &[u8], isv2format: bool) -> Option<(Stack<SslCipher>, Stack<SslCipher>)> { // ... }

Although I don't know where else this method is useful, it would have the added benefit of being usable outside of the client hello callback.

@RoastVeg RoastVeg force-pushed the client_hello_ciphers branch from 56d2e32 to 33e0df2 Compare May 11, 2023 08:43
@RoastVeg RoastVeg changed the title Add support for fetching SslCipher stack from ClientHelloResponse Add bindings to SSL_bytes_to_cipher_list May 11, 2023
@RoastVeg RoastVeg requested a review from sfackler May 11, 2023 12:38
&self,
bytes: &[u8],
isv2format: bool,
) -> Option<(Stack<SslCipher>, Stack<SslCipher>)> {
Copy link
Owner

Choose a reason for hiding this comment

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

Can we have this return a struct CipherLists { pub suites: Stack<SslCipher>, pub signaling_suites: Stack<SslCipher> } so it's a bit more descriptive?

Copy link
Owner

Choose a reason for hiding this comment

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

Does the function not push errors onto the stack on failure, or should this return a Result<CipherLists, ErrorStack> instead?

@RoastVeg RoastVeg force-pushed the client_hello_ciphers branch from 33e0df2 to 7e6d518 Compare May 12, 2023 09:37
@RoastVeg RoastVeg requested a review from sfackler May 12, 2023 10:03
@sfackler sfackler merged commit 5c7b570 into sfackler:master May 15, 2023
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.

2 participants