-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Openssl updates #2180
Openssl updates #2180
Conversation
mirrors the Ruby stdlib a little more. used from: https://github.com/datanoise/openssl.cr
If I recall correctly we weren't sure about adding those classes, because there are lots of digest methods; they may not all be available in the compiled OpenSSL; why have some classes for some digests but not others; ... Having a single interface for all digests sounded better. |
@ysbaddaden I can get behind that. Changes made. |
I guess a question here, what min-version of OpenSSL are we looking to support? |
suggested by @ysbaddaden
from https://github.com/datanoise/openssl.cr involves a little reorg of the bio lib
This is ready to go with the exception of the build failure due to out of date OpenSSL on the "no arch" build. I'd appreciate feedback and some guidance on the |
This was tested locally against |
Also, unrelated (but maybe related?)
No other output from that failure. I can successfully run |
I believe TravisCI uses Ubuntu Precise. Only the upcoming Xenial supports OpenSSL 1.0.2f. Trusty only has 1.0.1f and I don't know for Precise. |
Errata: the build fails on OSX. I believe this is Maverick, I don't know which OpenSSL version is installed. |
From my install, it's definitely an old one. Not sure how old off hand though I can check this tomorrow. Are we wanting to support vanilla install or push for adoption of newer OpenSSL? |
After unlinking homebrew's verison, the default OSX install is |
I was about to send in a PR with less OpenSSL ported from @datanoise and added ssl methods on HTTP::Client. But now that I see this, it looks far more complete. Should I wait until we fix this or send in my smaller PR? (mine just adds 2 LibSSL functions and 2 SSL methods: |
@jeromegn I would send multiple small PRs (or: I would start with one, wait for it to be merged, send another one, etc.). Huge PRs like this are very hard to review and take a lot of time. If the PR is small and focused it has many more changes to be merged. Thank you! :-) |
#triage Should this PR remain open? Is there any value? |
This PR still does a lot more than we integrated with @jhass, most notably generate/sign/validate with DSA/RSA keys. Right now we mostly only have what was required for a (hopefully) secure TLS communication (sockets, contexts) and validation (SNI, hostname). It needs a rebase and clean commits. If someone is courageous enough to work on this. |
Headsup. I merged this to my branch and rebased it against master. See master...discord-tech:openssl-updates-from-2180 Needs a bit of work still, all the merged specs don't work anymore. I'll make a PR a bit later if I get things to work at least a bit. I don't have extensive openssl knowledge, so any PR I make needs to be scrutinized rather well. |
Will glad to see updated PR from you @vegai |
Closing because it's not mergeable. Please send small individual PRs so we can more easily review them. |
Opening this up to give some visibility.
@datanoise this is mostly code from you, I'm just turning it into a more up-to-date PR that matches current progress in the stdlib. A 👍 or 👎 would be awesome.
All others, feel free to help or give feedback. Slow in coming as I've hit a few oddities with OpenSSL (which oddly enough also exist in the Ruby OpenSSL stdlib). At least it's consistent 😬