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

Jump to bottom

OpenSSL: use proper factories over direct allocation #4669

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

OpenSSL: use proper factories over direct allocation #4669

wants to merge 1 commit into from

Conversation

ysbaddaden
Copy link
Contributor

While investigating #4650 I noticed that OpenSSL::SSL::Context insecure initializer relied on #allocate. This patch avoids this by using .new factories an a single #initialize method taking a LibSSL::SSLContext pointer, and a boolean argument to set secure defaults, or not (using OpenSSL defaults).

@ysbaddaden
Use proper factories over direct allocation.
4568b16 
This avoids calling `#allocate` and uses proper `self.new` factories
to initialize the SSL context.
@akzhan
Copy link
Contributor

akzhan commented Jul 11, 2017

It's OK, but using of new should be documented for newcomers, for example.

Here or anywhere else.

@straight-shoota
Copy link
Member

I'm wondering if Context::Server and Context::Client really need to be two separate types. The behaviour is exactly the same, except from the constructors but they could still just be different ones.
Client context has protected method set_cert_verify_callback but it wouldn't hurt if it was accessible on server as well, it's not public anyway. And it's only needed for OpenSSL < 1.0.2 which is out of support.

@jhass
Copy link
Member

jhass commented Apr 18, 2018

The original idea between separate types was to actually have type safety for not using a client context in a server socket and vice versa.

@straight-shoota straight-shoota added this to In progress in Linking OpenSSL via automation Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@straight-shoota straight-shoota straight-shoota left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
kind:refactor topic:stdlib:networking
Projects
Linking OpenSSL
In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ysbaddaden @akzhan @straight-shoota @jhass @HertzDevil