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

test_ssl.rb: Test respecting system default min. #851

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

junaruga
Copy link
Member

@junaruga junaruga commented Feb 3, 2025

This PR is adding a test for the #710 to ensure system wide setting MinProtocol is respected.

The test logic is similar with the #710 (comment). On the MinProtocol = TLSv1.3, it is testing that the client should fail to connect to TLS 1.2 server, and should succeed to connect to TLS 1.3 server.

What do you think?

test/openssl/test_ssl.rb Outdated Show resolved Hide resolved
@rhenium
Copy link
Member

rhenium commented Feb 4, 2025

In #849 I didn't bother adding a separate test for MinProtocol, but this does add improve coverage.

Not blocking this, but 100 lines total for testing it feels a bit excessive. Perhaps we should expose SSLContext#{min,max}_version? Normally we'd provide a matching getter for a setter method, and the reason we didn't was that SSL_CTX_get_{min,max}_proto_version() didn't exist at that time (added in OpenSSL 1.1.0g). Since we now require OpenSSL 1.1.1 or later, that's no longer an issue.

@junaruga
Copy link
Member Author

junaruga commented Feb 5, 2025

In #849 I didn't bother adding a separate test for MinProtocol, but this does add improve coverage.

Not blocking this, but 100 lines total for testing it feels a bit excessive. Perhaps we should expose SSLContext#{min,max}_version? Normally we'd provide a matching getter for a setter method, and the reason we didn't was that SSL_CTX_get_{min,max}_proto_version() didn't exist at that time (added in OpenSSL 1.1.0g). Since we now require OpenSSL 1.1.1 or later, that's no longer an issue.

I wondered why there were no getter methods of the SSLContext#{min,max}_version. Yes, I think these getter methods are very useful.

@junaruga junaruga force-pushed the wip/test-ssl-respect-system-default branch from d6ce16d to 7de5ff5 Compare February 5, 2025 16:24
@rhenium rhenium merged commit f09f920 into ruby:master Feb 6, 2025
53 checks passed
@rhenium
Copy link
Member

rhenium commented Feb 6, 2025

Looks good to me, thank you!

@junaruga junaruga deleted the wip/test-ssl-respect-system-default branch February 10, 2025 17:05
@junaruga
Copy link
Member Author

junaruga commented Feb 10, 2025

Thank you for your review!

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.

2 participants