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

#9969 Expose SSL_session_reused. #9978

Merged
merged 4 commits into from
Dec 9, 2023
Merged

Conversation

adiroiban
Copy link
Contributor

Fixes #9969

This (re)adds the SSL_session_reused API.

I think that this API is criticial.

First, this complements the already supported int SSL_set_session(SSL *, SSL_SESSION *); to allow you to verify that after the handshake the session was reused.

Second, when implementing an FTPS server, this API is the only way to make sure you don't end up with hijacked FTP data connection.

I only added a simple tests to check that the API is there.

Let me know if more tests are required.

Thanks

@adiroiban
Copy link
Contributor Author

adiroiban commented Dec 9, 2023

This is read for review. Thanks!

Maybe it needs a release note?
I think there was no explicit relase note when this API was removed.
So no sure if this is important.

@adiroiban
Copy link
Contributor Author

I now see #8675

There are 2 things here: session cache vs session reuse.

The server/client can have session cache disabled and still reuse the session.

For example a session is initiated between server and client via connection A.

The client can initiate a new connection B to the server and setup the connection B via session_a = SSL_get1_session(connection_a) -> SSL_set_session(connection_b, session_a)

@@ -112,3 +112,16 @@ def test_rust_internal_error(self):
assert error.reason == b.lib.EVP_R_DATA_NOT_MULTIPLE_OF_BLOCK_LENGTH
if not b.lib.CRYPTOGRAPHY_IS_BORINGSSL:
assert b"data not multiple of block length" in error.reason_text

def test_ssl_session_reused_new(self):
Copy link
Member

Choose a reason for hiding this comment

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

No need for a test, we don't add them for all bindings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this a request to remove the test?

I think that a test can help with any accedental removal of the API.

It can also serve as an example on how to use the API.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please remove the test.

src/_cffi_src/openssl/ssl.py Outdated Show resolved Hide resolved
adiroiban and others added 2 commits December 9, 2023 19:37
Co-authored-by: Alex Gaynor <alex.gaynor@gmail.com>
@adiroiban
Copy link
Contributor Author

adiroiban commented Dec 9, 2023

Thanks Alex for the review.

This should be ready for another review.

I hope all is ok.

I have removed the test. I was not expecting a request to submit a PR without tests.

I don't understand why is bad to have that test.

The test was kind a reminder that the binding in important and it should not be removed at the next "great purge".

No hard feelings. I am happy to have the new API available with or without tests.
I have automated tests for this API in my project, so that's fine be me.

Thanks for maintaining cryptography. Much appreciated.

Copy link
Member

@alex alex left a comment

Choose a reason for hiding this comment

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

The reason we don't add tests for bindings is that they're not actually testing anything about this project, except that we bound the function correctly -- but that's handled in other ways.

We'll know not to remove this because a) it'll be used in pyOpenSSL (which we test in CI), b) we check the commit history before removing something.

@alex alex merged commit 14c16eb into pyca:main Dec 9, 2023
57 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

SSL_session_reused bindings not available
2 participants