Fix to support windows by leifcr · Pull Request #164 · slack-ruby/slack-ruby-client

leifcr · 2017-09-12T09:52:06Z

The syntax for grep/awk/sed is problematic under cmdline on windows

This removes the dependency of awk/grep/sed., openssldir is parsed in ruby instead.

This can close #91, since everything else works as it should on Windows.

dangerpr-bot · 2017-09-12T09:59:06Z

	1 Warning
⚠️	There’re library changes, but not tests. That’s OK as long as you’re refactoring existing code.

Generated by 🚫 danger

mikz · 2017-09-12T10:12:38Z

        self.user_agent = "Slack Ruby Client/#{Slack::VERSION}"
-        self.ca_path = `openssl version -a | grep OPENSSLDIR | awk '{print $2}'|sed -e 's/\"//g'`
-        self.ca_file = "#{ca_path}/ca-certificates.crt"
+        self.ca_path = openssl_ca_path if ca_path.nil?


Can't this be just using OpenSSL::X509::DEFAULT_CERT_DIR and OpenSSL::X509::DEFAULT_CERT_FILE ?

You are right. OpenSSL::X509::DEFAULT_CERT_DIR points to the correct ca-folder.
However, OpenSSL::X509::DEFAULT_CERT_FILE points to cert.pem and not to ca-certificates.crt Not sure if .pem or .crt is the correct to use here. ?

Looking closer at this `openssl version -a | grep OPENSSLDIR | awk '{print $2}'|sed -e 's/\"//g'` returns /usr/lib/ssl on *nix, which is results in ca_file to be /usr/lib/ssl/ca-certificates.crt, which both are wrong paths/files.

This (openssl version -a) does not work on MacOS Sierra (10.12) also.

My OPENSSLDIR is /System/Library/OpenSSL.

But /System/Library/OpenSSL/ca-certificates.crt does not exist.

When using system ruby then OpenSSL::X509::DEFAULT_CERT_FILE points to /System/Library/OpenSSL/cert.pem which also does not exist.

But at least it works with own compiled ruby.

Also it is reasonable to use SSL_CERT_DIR and SSL_CERT_FILE env variables before the openssl default. (like https://github.com/mikz/httpclient/blob/ba21d0d44c9f5f4602c7fc151f7c24f827480361/lib/httpclient/ssl_config.rb#L418-L419)

As those are used by OpenSSL to set custom cert dir/path.

I'll change the pr to use OpenSSL::X509::DEFAULT_CERT_DIR and OpenSSL::X509::DEFAULT_CERT_FILE, since that works on nix, win and osx.

mikz · 2017-09-12T12:01:21Z

@@ -1,5 +1,5 @@
 ### 0.9.2 (Next)
-
+* [#163](https://github.com/slack-ruby/slack-ruby-client/pull/164): Use OpenSSL::X509::DEFAULT_CERT_DIR and OpenSSL::X509::DEFAULT_CERT_FILE for default ca_cert and ca_file [@leifcr](https://github.com/leifcr)


would be good to highlight code constants as code with `CODE`

mikz · 2017-09-12T12:02:09Z

@@ -1,3 +1,4 @@
+require 'openssl'


This would make OpenSSL hard dependency.

Might be better to wrap it in a block and rescue LoadError.

mikz · 2017-09-12T12:03:10Z

        self.user_agent = "Slack Ruby Client/#{Slack::VERSION}"
-        self.ca_path = `openssl version -a | grep OPENSSLDIR | awk '{print $2}'|sed -e 's/\"//g'`
-        self.ca_file = "#{ca_path}/ca-certificates.crt"
+        self.ca_path = OpenSSL::X509::DEFAULT_CERT_DIR


If OpenSSL is not defined (guess we don't want hard dependency) then this would crash. So this can be done as

self.ca_path = OpenSSL::X509::DEFAULT_CERT_DIR if defined?(OpenSSL)

leifcr · 2017-09-12T14:07:32Z

@mikz I added check for OpenSSL, so that vars are set to nil if missing as suggested.

mikz · 2017-09-12T14:16:47Z

          options[:headers]['User-Agent'] = user_agent if user_agent
          options[:proxy] = proxy if proxy
-          options[:ssl] = { ca_path: ca_path, ca_file: ca_file }
+          options[:ssl] = { ca_path: ca_path, ca_file: ca_file } unless ca_path.nil? || ca_file.nil?


This could be tricky.

Do both options have to be specified? Can't just one do the trick? I think one certificate chain can work just fine as well as just the cert directory.

So this could be nicer as if ca_path || ca_file. Unless with || can be evil.

agree. if ca_path || ca_file is better. And only one needs to be present.

mikz · 2017-09-12T14:18:22Z

@leifcr great change! I commented one last time #164 (comment) to clarify the ca info assignment.

Then it would get mine 👍 Good job 🥇

dblock · 2017-09-12T15:24:44Z

@@ -1,5 +1,5 @@
 ### 0.9.2 (Next)
-


Put back the line, please.

dblock · 2017-09-12T15:25:05Z

@@ -1,5 +1,5 @@
 ### 0.9.2 (Next)
-
+* [#163](https://github.com/slack-ruby/slack-ruby-client/pull/164): Use `OpenSSL::X509::DEFAULT_CERT_DIR` and `OpenSSL::X509::DEFAULT_CERT_FILE` for default ca_cert and ca_file [@leifcr](https://github.com/leifcr)


Make this in the same format as everything else, needs a - before your name and a . at the end.

dblock · 2017-09-12T15:25:49Z

+begin
+  require 'openssl'
+rescue LoadError # rubocop:disable Lint/HandleExceptions
+end


I don't think this is a great place to put this, lib/slack-ruby-client.rb is better, it has all the require.

@dblock Agree. Will move it there.

dblock · 2017-09-12T15:27:13Z

Made some minor comments. Thanks for making this happen @leifcr. Do you think we can get a Windows CI somewhere?

mikz · 2017-09-12T15:53:03Z

@dblock seen https://www.appveyor.com a few times.

leifcr · 2017-09-12T19:48:22Z

@mikz and @dblock Thanks for feedback. Appveyor supports ruby: https://www.appveyor.com/docs/lang/ruby/

dblock · 2017-09-12T20:07:38Z

Run rubocop -a ; rubocop --auto-gen-config to fix CI.

dblock · 2017-09-12T20:08:45Z

@leifcr If you want to PR Appveyor support (appveyor.yml, etc.), I can help enable if something needs to be done here. I used it in https://github.com/Waffle/waffle and https://github.com/resourcelib/resourcelib :)

leifcr · 2017-09-13T07:13:23Z

@dblock I'll create a new branch for Appveyor and do a separate PR For that.

dblock · 2017-09-13T16:28:22Z

Merged, thanks.

leifcr · 2017-09-20T20:53:55Z

Thanks

Don't use awk/grep/sed when detecing openssldir

e532183

mikz reviewed Sep 12, 2017

View reviewed changes

leifcr added 2 commits September 12, 2017 13:52

Use OpenSSL::X509::DEFAULT_CERT_DIR and OpenSSL::X509::DEFAULT_CERT_FILE

a91d407

Update changelog

db1633e

mikz reviewed Sep 12, 2017

View reviewed changes

leifcr added 2 commits September 12, 2017 15:45

Use code for constants in changelog

e7a4a06

Set ca_path and ca_file to nil if OpenSSL is undefined

6ad9030

mikz reviewed Sep 12, 2017

View reviewed changes

dblock requested changes Sep 12, 2017

View reviewed changes

leifcr added 3 commits September 12, 2017 21:41

Update changelog format

abe698e

Use if instead of unless

a2c0890

require in slack-ruby-client instead of slack/web/config

f5f2f75

Rubocopping

1d099b0

dblock merged commit 385b30b into slack-ruby:master Sep 13, 2017

leifcr deleted the bugfix_openssl_windows_compatible_detection branch September 20, 2017 20:54

		@@ -1,5 +1,5 @@
		### 0.9.2 (Next)

		* [#163](https://github.com/slack-ruby/slack-ruby-client/pull/164): Use OpenSSL::X509::DEFAULT_CERT_DIR and OpenSSL::X509::DEFAULT_CERT_FILE for default ca_cert and ca_file [@leifcr](https://github.com/leifcr)

Uh oh!

Conversation

leifcr commented Sep 12, 2017

Uh oh!

dangerpr-bot commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leifcr commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikz commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dblock commented Sep 12, 2017

Uh oh!

mikz commented Sep 12, 2017

Uh oh!

leifcr commented Sep 12, 2017

Uh oh!

dblock commented Sep 12, 2017

Uh oh!

dblock commented Sep 12, 2017

Uh oh!

leifcr commented Sep 13, 2017

Uh oh!

dblock commented Sep 13, 2017

Uh oh!

leifcr commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dangerpr-bot commented Sep 12, 2017 •

edited

Loading

leifcr commented Sep 12, 2017 •

edited

Loading