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

Improve vibe.stream.openssl support for later OpenSSL versions (1.1.1h, 3.0.x) #2652

Closed
wants to merge 5 commits into from

Conversation

Geod24
Copy link
Contributor

@Geod24 Geod24 commented May 12, 2022

I recently installed Ubuntu 22.04 as my main work machine and Vibe.d didn't build.
Looking more into the module, it's obvious why: The 1.1 support that was added was made in terms of the 1.0 API.
This causes various problems and make it hard to see what piece of code is actually useful.
This is by no mean a full fix, but a good first step (and hopefully inspiration) towards how I believe we can best get there.
Also, as I am on OpenSSL3, it allows me to test that things still work with 1.1 :)

As explained in the comment, a breaking change happened while fixing a bug.
OpenSSL issues are not rare, and are an ongoing source of pain.
Auto-detection has massively improved since this module was originally
written, hence the soft fallback should almost never been hit.
If it is, it's better to inform the user.
As explained in the comment, the current approach is to define a 1.0.x API in terms of a 1.1.x API.
This now breaks again because the same 1.0.x API would need to be also defined in terms of the 3.0.x API.
The diff in this commit starts turning things around, by using a piece of the 3.0.x (and 1.1.x) API
in the OpenSSL code and just aliasing the bindings for 1.0.x
As explained in the comments, the init code is no longer necessary.
@Geod24 Geod24 requested a review from s-ludwig May 12, 2022 10:30
@s-ludwig
Copy link
Member

I've made a small correction yesterday - #2650 - and it looks like this is still needed for the changes here(?)

@Geod24
Copy link
Contributor Author

Geod24 commented May 12, 2022

@s-ludwig : Not really. We're hitting a dub limitation. The tls subpackage dependency on openssl is ~>1.0, but there hasn't been a release to v1 since 2017.
So I thought I'd be clever and update the dependency for openssl config (leaving openssl-1.1 and openssl-1.0 to do their own thing). But obviously that doesn't work, because:

Unresolvable dependencies to package openssl:
  vibe-d:tls 0.9.0-rc.1+commit.298.ge53b865d depends on openssl >=1.0.0+1.0.0e
  vibe-d:tls 0.9.0-rc.1+commit.298.ge53b865d depends on openssl ~>2
  vibe-d:tls 0.9.0-rc.1+commit.298.ge53b865d depends on openssl ~>1
  vibe-d:tls 0.9.0-rc.1+commit.298.ge53b865d depends on openssl ~>1

Which is another instance of dlang/dub#1706

EDIT: The CI failure you are seeing is because we're using Deimos package v1. The issue described above is what happens when I try fixing that issue.

@s-ludwig
Copy link
Member

Okay, I just saw that the OPENSSL_VERSION.startsWith("1.1") condition was still there at line 67, but missed the other one further down. But if we want to go down the route of versioning the Deimos packages like that, we'll have to rethink the approach that vibe.d takes. Usually the way this would work would be to depend on >=1.0.0 <3.0.0-0 and then use static if to select the right code path based on the actual version.

The problem is of course the OpenSSL version auto-detection, which currently works transparently. I don't see how that would work without some serious hacks when the dependency version needs to be adjusted accordingly.

But for now I'd say lets get #2650 merged and then decouple the code refactoring from the Deimos dependency issue to keep things going.

@s-ludwig
Copy link
Member

BTW, solving the DUB dependency resolution one way or another (where I think that the current approach to resolve versions first and configurations second is really the right one) won't solve the issue of the version auto-detection, we may have to think about a new mechanic for version resolution there.

@Geod24
Copy link
Contributor Author

Geod24 commented May 13, 2022

For the approach I was taking to work, we would need the Deimos package to have bindings that are compatible with any version we support. That might work for 1.1 / 3.0, but I think that we would have ended up forcing 1.0 users to explicitly select their openssl version. Given the age (and known bugs) in 1.0, I didn't think that'd be a big deal.

@Geod24
Copy link
Contributor Author

Geod24 commented May 23, 2022

Since this is a cleanup, I'll continue once #2658 is in.

@Geod24 Geod24 marked this pull request as draft May 23, 2022 12:59
@Geod24
Copy link
Contributor Author

Geod24 commented May 31, 2022

So most of my fixes are now directed directly at Deimos, and this can go. I'll submit the relevant bits individually.

@Geod24 Geod24 closed this May 31, 2022
@Geod24 Geod24 deleted the i-hate-openssl branch May 31, 2022 11:51
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