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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Openssl updates #2180

Closed
wants to merge 8 commits into from
Closed

Openssl updates #2180

wants to merge 8 commits into from

Conversation

plukevdh
Copy link
Contributor

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 馃槵

@ysbaddaden
Copy link
Contributor

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.

@plukevdh
Copy link
Contributor Author

@ysbaddaden I can get behind that. Changes made.

@plukevdh
Copy link
Contributor Author

I guess a question here, what min-version of OpenSSL are we looking to support?

@plukevdh plukevdh changed the title WIP: Openssl updates Openssl updates Feb 19, 2016
@plukevdh
Copy link
Contributor Author

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 require convention for these kinds of libraries. Lots of things requiring similar things. Not sure how to approach it in the sense of whether to push people to just require "openssl" or be able to piecemeal it.

@plukevdh
Copy link
Contributor Author

This was tested locally against OpenSSL 1.0.2f 28 Jan 2016. Not sure what CI is running.

@plukevdh
Copy link
Contributor Author

Also, unrelated (but maybe related?) make spec fails for me with:

make spec
./bin/crystal build  -o .build/all_spec spec/all_spec.cr
Using compiled compiler at .build/crystal
Error: execution of command failed with code: 127: `cc -o ".build/all_spec" "${@}"  -rdynamic  -lxml2 -lssl -lz -lcrypto -lreadline -lgmp -lyaml /Users/plukevdh/src/Articulate/tooling/vendor/crystal/src/llvm/ext/llvm_ext.o `(llvm-config-3.6 --libs --system-libs --ldflags 2> /dev/null) || (llvm-config-3.5 --libs --system-libs --ldflags 2> /dev/null) || (llvm-config --libs --system-libs --ldflags 2>/dev/null)` -lstdc++ /Users/plukevdh/src/Articulate/tooling/vendor/crystal/src/ext/libcrystal.a -levent -lpcre -liconv -lgc -lpthread -ldl`
make: *** [.build/all_spec] Error 127

No other output from that failure. I can successfully run ./bin/crystal spec though.

@ysbaddaden
Copy link
Contributor

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.

@ysbaddaden
Copy link
Contributor

Errata: the build fails on OSX. I believe this is Maverick, I don't know which OpenSSL version is installed.

@plukevdh
Copy link
Contributor Author

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?

@plukevdh
Copy link
Contributor Author

After unlinking homebrew's verison, the default OSX install is OpenSSL 0.9.8zg 14 July 2015. This seems unreasonably and certainly dangerously out of date. But I guess how do we weight that with a "works out of the box" goal? Can we make an OpenSSL version requirement for the homebrew install script for Crystal?

@jeromegn
Copy link
Contributor

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: TLSv1 and SSLv3, but it does add some stuff to HTTP::Client)

@asterite
Copy link
Member

@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! :-)

@miketheman
Copy link
Contributor

#triage
This appears to have stalled, CI failed, and is no longer mergable.
The efforts undertaken in previously linked PRs, and @ysbaddaden appears to have become the defacto leader on OpenSSL-in-Crystal.

Should this PR remain open? Is there any value?

@ysbaddaden
Copy link
Contributor

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.

@vegai
Copy link
Contributor

vegai commented Aug 23, 2017

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.

@akzhan
Copy link
Contributor

akzhan commented Sep 17, 2017

Will glad to see updated PR from you @vegai

@asterite
Copy link
Member

Closing because it's not mergeable. Please send small individual PRs so we can more easily review them.

@asterite asterite closed this Sep 29, 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.

None yet

8 participants