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

Fix ssl_calls for OpenSSL 1.1.0 #459

Merged
merged 2 commits into from
Nov 1, 2016
Merged

Fix ssl_calls for OpenSSL 1.1.0 #459

merged 2 commits into from
Nov 1, 2016

Conversation

Natureshadow
Copy link
Contributor

No description provided.

@itamarjp
Copy link
Member

some people reported that this fixed the bug #424

@itamarjp
Copy link
Member

looks good for me.

@proski
Copy link
Contributor

proski commented Oct 28, 2016

I like that you code for the new API and then backport. The new API is always a better target.

But I would prefer if you used inline functions for the backport instead of adding ifdefs throughout the code. So you can create a function HMAC_CTX_new for older OpenSSL and put the backport code there. The backport functions could go to a separate header.

I worked on a project that needed to support multiple versions of dependencies, and this approach proved to be most maintainable in the long run.

@Natureshadow
Copy link
Contributor Author

But I would prefer if you used inline functions for the backport
instead of adding ifdefs throughout the code. So you can create a
function HMAC_CTX_new for older OpenSSL and put the backport code
there.

I tried that, but it turned out that the real implementations needed more and more inline functions (like EVP_MD_CTX_*) and it turned out that this approach is better in that case.

I expect to drop support for OpenSSL < 1.1.0 some time soon anyway.

@Natureshadow
Copy link
Contributor Author

Also mind that this way, users of old OpenSSL get the same code as before.

@proski
Copy link
Contributor

proski commented Oct 30, 2016

I've made a change to show what I mean. I actually tested that with OpenSSL 1.0.2j and 1.1.0b. Please see commit 8df61b9

Functions are stricter than macros in argument checking, and even macros are better than the conditional code spread all around the place. The functions can only use their arguments, not arbitrary variables from the caller. The arguments have types and descriptive names.

The main code uses the same logic regardless of the OpenSSL version, which makes development safer. When the backport code is all over the place, it's easy to copy some code and break support for a specific version of the dependency.

I don't know what was the "real" implementation you attempted. The backport functions should have the old code, but with the new API.

I actually expect OpenSSL 1.0 to stick around for a while. Sure, it would be patched for vulnerabilities, but its API would not change.

@proski
Copy link
Contributor

proski commented Oct 30, 2016

Actually, let's apply this PR, and I'll send my patch on top of it.

@metalefty metalefty merged commit cde3403 into neutrinolabs:devel Nov 1, 2016
metalefty added a commit to metalefty/xrdp that referenced this pull request May 26, 2017
metalefty added a commit that referenced this pull request May 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants