-
Notifications
You must be signed in to change notification settings - Fork 795
Add bindings for SSL_CIPHER_get_protocol_id #2462
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get a test?
Something simple like this is enough? |
| pub fn protocol_id(&self) -> [u8; 2] { | ||
| unsafe { | ||
| let id = ffi::SSL_CIPHER_get_protocol_id(self.as_ptr()); | ||
| id.to_be_bytes() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is big endian always right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It definitely should be. Those are two byte constants (they are not defined as u16), moreover TLS is big-endian in general, even if they weren't: https://www.rfc-editor.org/rfc/rfc8446#section-3.1.
OpenSSL could have a bug with this though, I think it's unlikely, but I don't know. Two bytes are received, then they are turned into an integer, which is native-endian - in other words, on little-endian architectures a byte swap is necessary. It seems like on all architectures you test it did properly byte swap as needed, since tests passed, but I don't see a test on PowerPC so who knows. I don't have a big-endian machine to test myself.
Simply said - on big-endian architectures, there should be no byte swap, and on little-endian there should be two byte swaps, which cancel each other out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Building the branch on a sparc64 machine with OpenSSL 3.5 gives:
test ssl::test::cipher_id ... ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great, thanks
SSL_CIPHER_get_protocol_idThis includes both bindings for
openssl-sysand high-level bindings foropenssl. I wasn't sure whether to use[u8; 2]oru16for the high-level return type, decided on[u8; 2]for now as it feels more natural, I can change this ifu16(the same as in C) is better.