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

ssl: add ssl_cert option to support pkcs12 self-signed cert #165

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
### 0.13.0

* Added ssl_cert, ssl_cert_type, ssl_key_password to support self-signed SSL cert file

### 0.12.1

* Ensure HTTP2 response headers/status lines are correctly handled
Expand Down Expand Up @@ -28,10 +32,10 @@
### 0.9.1

* Added ssl_version options `TLSv1_1`, `TLSv1_2`, `TLSv1_3` for explicitly forcing the SSL version
* requires the appropriate versions of libCURL and OpenSSL installed to support these new options
* requires the appropriate versions of libCURL and OpenSSL installed to support these new options
* reference: https://curl.haxx.se/libcurl/c/CURLOPT_SSLVERSION.html
* Added a new `:http_version` option with `HTTPv1_1` and `HTTPv2_0` values to explicitly set the HTTP version of HTTP/1.1 or HTTP/2.0
* requires the appropriate versions of libCURL and OpenSSL installed to support these new options
* requires the appropriate versions of libCURL and OpenSSL installed to support these new options
* reference: https://curl.haxx.se/libcurl/c/CURLOPT_HTTP_VERSION.html
* Updates the gem release procedure for more convenience, using the updated Rubygems.org tasks
* Update a few minor dependencies and documentation to be Ruby 2.4.1-compatible, add 2.4.1. to Travis CI matrix
Expand Down
52 changes: 35 additions & 17 deletions ext/patron/session_ext.c
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ static int session_progress_handler(void* clientp, size_t dltotal, size_t dlnow,

// If a progress proc has been set, re-acquire the GIL and call it using
// `call_user_rb_progress_blk`. TODO: use the retval of that proc
// to permit premature abort
// to permit premature abort
if(RTEST(state->user_progress_blk)) {
// Even though it is not documented, rb_thread_call_with_gvl is available even when
// rb_thread_call_without_gvl is not. See https://bugs.ruby-lang.org/issues/5543#note-4
Expand All @@ -118,7 +118,7 @@ static int session_progress_handler(void* clientp, size_t dltotal, size_t dlnow,
if(state->download_byte_limit != 0 && (dltotal > state->download_byte_limit)) {
state->interrupt = INTERRUPT_DOWNLOAD_OVERFLOW;
}

// If the interrupt value is anything except 0, the perform() call
// will be aborted by libCURL.
// Note however that some older versions of libcurl have a bug which
Expand Down Expand Up @@ -239,7 +239,7 @@ static struct patron_curl_state* get_patron_curl_state(VALUE self) {

/*
* Returns the version of the embedded libcurl.
*
*
* @return [String] libcurl version string
*/
static VALUE libcurl_version(VALUE klass) {
Expand All @@ -250,7 +250,7 @@ static VALUE libcurl_version(VALUE klass) {

/*
* Returns the version of the embedded libcurl.
*
*
* @return [Array] an array of MAJOR, MINOR, PATCH integers
*/
static VALUE libcurl_version_exact(VALUE klass) {
Expand All @@ -269,7 +269,7 @@ static VALUE libcurl_version_exact(VALUE klass) {
* @return [String] the escaped string
*/
static VALUE session_escape(VALUE self, VALUE value) {

VALUE string = StringValue(value);
char* escaped = NULL;
VALUE retval = Qnil;
Expand Down Expand Up @@ -318,7 +318,7 @@ static int each_http_header(VALUE header_key, VALUE header_value, VALUE self) {
VALUE name = rb_obj_as_string(header_key);
VALUE value = rb_obj_as_string(header_value);
VALUE header_str = Qnil;

// TODO: see how to combine this with automatic_content_encoding
if (rb_str_cmp(name, rb_str_new2("Accept-Encoding")) == 0) {
if (rb_funcall(value, rb_intern("include?"), 1, rb_str_new2("gzip"))) {
Expand Down Expand Up @@ -388,7 +388,7 @@ static FILE* open_file(VALUE filename, const char* perms) {

static void set_request_body_file(struct patron_curl_state* state, VALUE r_path_str) {
CURL* curl = state->handle;

state->request_body_file = open_file(r_path_str, "rb");
curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
curl_easy_setopt(curl, CURLOPT_READDATA, state->request_body_file);
Expand Down Expand Up @@ -439,6 +439,9 @@ static void set_options_from_request(VALUE self, VALUE request) {
VALUE insecure = Qnil;
VALUE cacert = Qnil;
VALUE ssl_version = Qnil;
VALUE ssl_cert = Qnil;
VALUE ssl_cert_type = Qnil;
VALUE ssl_key_password = Qnil;
VALUE http_version = Qnil;
VALUE buffer_size = Qnil;
VALUE action_name = rb_funcall(request, rb_intern("action"), 0);
Expand Down Expand Up @@ -501,7 +504,7 @@ static void set_options_from_request(VALUE self, VALUE request) {
} else if (action == rb_intern("patch")) {
curl_easy_setopt(curl, CURLOPT_CUSTOMREQUEST, "PATCH");
}

if (RTEST(data) && !RTEST(multipart)) {
if (action == rb_intern("post")) {
curl_easy_setopt(curl, CURLOPT_POST, 1);
Expand Down Expand Up @@ -563,21 +566,21 @@ static void set_options_from_request(VALUE self, VALUE request) {
"The libcurl version installed doesn't support automatic content negotiation");
#endif
}

url = rb_funcall(request, rb_intern("url"), 0);
if (!RTEST(url)) {
rb_raise(rb_eArgError, "Must provide a URL");
}
curl_easy_setopt(curl, CURLOPT_URL, StringValuePtr(url));

#ifdef CURLPROTO_HTTP
// Security: do not allow Curl to go looking on gopher/SMTP etc.
// Must prevent situations like this:
// https://hackerone.com/reports/115748
curl_easy_setopt(curl, CURLOPT_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
curl_easy_setopt(curl, CURLOPT_REDIR_PROTOCOLS, CURLPROTO_HTTP | CURLPROTO_HTTPS);
#endif

timeout = rb_funcall(request, rb_intern("timeout"), 0);
if (RTEST(timeout)) {
curl_easy_setopt(curl, CURLOPT_TIMEOUT, FIX2INT(timeout));
Expand Down Expand Up @@ -705,6 +708,21 @@ static void set_options_from_request(VALUE self, VALUE request) {
}
}

ssl_cert = rb_funcall(request, rb_intern("ssl_cert"), 0);
if(RTEST(ssl_cert)) {
curl_easy_setopt(curl, CURLOPT_SSLCERT, StringValuePtr(ssl_cert));
}

ssl_cert_type = rb_funcall(request, rb_intern("ssl_cert_type"), 0);
if(RTEST(ssl_cert_type)) {
curl_easy_setopt(curl, CURLOPT_SSLCERTTYPE, StringValuePtr(ssl_cert_type));
}

ssl_key_password = rb_funcall(request, rb_intern("ssl_key_password"), 0);
if(RTEST(ssl_key_password)) {
curl_easy_setopt(curl, CURLOPT_SSLKEYPASSWD, StringValuePtr(ssl_key_password));
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://curl.haxx.se/mail/lib-2005-03/0172.html

found CURLOPT_SSLKEYPASSWD in maillist

}

cacert = rb_funcall(request, rb_intern("cacert"), 0);
if(RTEST(cacert)) {
curl_easy_setopt(curl, CURLOPT_CAINFO, StringValuePtr(cacert));
Expand All @@ -728,7 +746,7 @@ static VALUE create_response(VALUE self, CURL* curl, VALUE header_buffer, VALUE
long code = 0;
long count = 0;
VALUE responseKlass = Qnil;

curl_easy_getinfo(curl, CURLINFO_EFFECTIVE_URL, &effective_url);
args[0] = rb_str_new2(effective_url);

Expand All @@ -741,7 +759,7 @@ static VALUE create_response(VALUE self, CURL* curl, VALUE header_buffer, VALUE
args[3] = header_buffer;
args[4] = body_buffer;
args[5] = rb_funcall(self, rb_intern("default_response_charset"), 0);

responseKlass = rb_funcall(self, rb_intern("response_class"), 0);
return rb_class_new_instance(6, args, responseKlass);
}
Expand Down Expand Up @@ -821,9 +839,9 @@ static VALUE perform_request(VALUE self) {
VALUE header_str = membuffer_to_rb_str(header_buffer);
VALUE body_str = Qnil;
if (!state->download_file) { body_str = membuffer_to_rb_str(body_buffer); }

curl_easy_setopt(curl, CURLOPT_COOKIELIST, "FLUSH"); // Flush cookies to the cookie jar

return create_response(self, curl, header_str, body_str);
} else {
rb_raise(select_error(ret), "%s", state->error_buf);
Expand Down Expand Up @@ -851,7 +869,7 @@ static VALUE cleanup(VALUE self) {
fclose(state->request_body_file);
state->request_body_file = NULL;
}

if (state->post) {
curl_formfree(state->post);
state->post = NULL;
Expand Down Expand Up @@ -922,7 +940,7 @@ static VALUE session_interrupt(VALUE self) {
* default or in +file+ if specified. The `file` must be readable and
* writable. Calling multiple times will add more files.
* FIXME: what does the empty string actually do here?
*
*
* @param [String] file path to the existing cookie file, or nil to store in memory.
* @return self
*/
Expand Down
12 changes: 7 additions & 5 deletions lib/patron/request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,13 @@ def initialize
:ignore_content_length, :multipart, :action, :timeout, :connect_timeout, :dns_cache_timeout,
:max_redirects, :headers, :auth_type, :upload_data, :buffer_size, :cacert,
:ssl_version, :http_version, :automatic_content_encoding, :force_ipv4, :download_byte_limit,
:low_speed_time, :low_speed_limit, :progress_callback
:low_speed_time, :low_speed_limit, :progress_callback, :ssl_cert_type, :ssl_cert, :ssl_key_password
]

WRITER_VARS = [
:url, :username, :password, :file_name, :proxy, :proxy_type, :insecure, :dns_cache_timeout,
:ignore_content_length, :multipart, :cacert, :ssl_version, :http_version, :automatic_content_encoding, :force_ipv4, :download_byte_limit,
:low_speed_time, :low_speed_limit, :progress_callback
:low_speed_time, :low_speed_limit, :progress_callback, :ssl_cert_type, :ssl_cert, :ssl_key_password
]

attr_reader *READER_VARS
Expand Down Expand Up @@ -109,7 +109,7 @@ def connect_timeout=(new_timeout)

@connect_timeout = new_timeout.to_i
end

# Sets the maximum number of redirects that are going to be followed.
#
# @param new_max_redirects[Integer] The number of redirects to follow, or `-1` for unlimited redirects.
Expand Down Expand Up @@ -179,7 +179,8 @@ def eql?(request)
def marshal_dump
[ @url, @username, @password, @file_name, @proxy, @proxy_type, @insecure,
@ignore_content_length, @multipart, @action, @timeout, @connect_timeout,
@max_redirects, @headers, @auth_type, @upload_data, @buffer_size, @cacert ]
@max_redirects, @headers, @auth_type, @upload_data, @buffer_size, @cacert,
@ssl_cert_type, @ssl_cert, @ssl_key_password ]
end

# Reinstates instance variables from a marshaled representation
Expand All @@ -188,7 +189,8 @@ def marshal_dump
def marshal_load(data)
@url, @username, @password, @file_name, @proxy, @proxy_type, @insecure,
@ignore_content_length, @multipart, @action, @timeout, @connect_timeout,
@max_redirects, @headers, @auth_type, @upload_data, @buffer_size, @cacert = data
@max_redirects, @headers, @auth_type, @upload_data, @buffer_size, @cacert,
@ssl_cert_type, @ssl_cert, @ssl_key_password = data
end

end
Expand Down
28 changes: 20 additions & 8 deletions lib/patron/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class Session
# Username for http authentication
# @return [String,nil] the HTTP basic auth username
attr_accessor :username

# Password for http authentication
# @return [String,nil] the HTTP basic auth password
attr_accessor :password
Expand Down Expand Up @@ -67,6 +67,15 @@ class Session
# @return [String] path to the CA file used for certificate verification, or `nil` if CURL default is used
attr_accessor :cacert

# @return [String] path to the SSL Cert file used for certificate verification, or `nil` if CURL default is used
attr_accessor :ssl_cert

# @return [String] the SSL Cert file type, should be one of p12(PKCS12) or pem
attr_accessor :ssl_cert_type

# @return [String] the SSL Key Password
attr_accessor :ssl_key_password

# @return [Boolean] whether Content-Range and Content-Length headers should be ignored
attr_accessor :ignore_content_length

Expand All @@ -81,7 +90,7 @@ class Session
# response does not specify a charset in it's `Content-Type` header already, if it does that charset
# will take precedence.
attr_accessor :default_response_charset

# @return [Boolean] Force curl to use IPv4
attr_accessor :force_ipv4

Expand Down Expand Up @@ -145,7 +154,7 @@ def initialize(args = {}, &block)
def handle_cookies(file_path = nil)
if file_path
path = Pathname(file_path).expand_path

if !File.exists?(file_path) && !File.writable?(path.dirname)
raise ArgumentError, "Can't create file #{path} (permission error)"
elsif File.exists?(file_path) && !File.writable?(file_path)
Expand All @@ -154,12 +163,12 @@ def handle_cookies(file_path = nil)
else
path = nil
end

# Apparently calling this with an empty string sets the cookie file,
# but calling it with a path to a writable file sets that file to be
# the cookie jar (new cookies are written there)
add_cookie_file(path.to_s)

self
end

Expand Down Expand Up @@ -315,7 +324,7 @@ def copy(url, dest, headers = {})
request(:copy, url, headers)
end
# @!endgroup

# @!group Basic API methods
# Send an HTTP request to the specified `url`.
#
Expand All @@ -329,7 +338,7 @@ def request(action, url, headers, options = {})
req = build_request(action, url, headers, options)
handle_request(req)
end

# Returns the class that will be used to build a Response
# from a Curl call.
#
Expand All @@ -344,7 +353,7 @@ def request(action, url, headers, options = {})
def response_class
::Patron::Response
end

# Builds a request object that can be used by ++handle_request++
# Note that internally, ++handle_request++ uses instance variables of
# the Request object, and not it's public methods.
Expand Down Expand Up @@ -378,6 +387,9 @@ def build_request(action, url, headers, options = {})
req.ssl_version = options.fetch :ssl_version, self.ssl_version
req.http_version = options.fetch :http_version, self.http_version
req.cacert = options.fetch :cacert, self.cacert
req.ssl_cert_type = options.fetch :ssl_cert_type, self.ssl_cert_type
req.ssl_cert = options.fetch :ssl_cert, self.ssl_cert
req.ssl_key_password = options.fetch :ssl_key_password, self.ssl_key_password
req.ignore_content_length = options.fetch :ignore_content_length, self.ignore_content_length
req.buffer_size = options.fetch :buffer_size, self.buffer_size
req.download_byte_limit = options.fetch :download_byte_limit, self.download_byte_limit
Expand Down
2 changes: 1 addition & 1 deletion lib/patron/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module Patron
VERSION = "0.12.1"
VERSION = "0.13.0"
end
Binary file added spec/certs/keystore.p12
Binary file not shown.
17 changes: 13 additions & 4 deletions spec/session_ssl_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
require 'fileutils'

describe Patron::Session do

before(:each) do
@session = Patron::Session.new
@session.base_url = "https://localhost:9043"
Expand Down Expand Up @@ -34,7 +33,7 @@
tmpfile = "/tmp/patron_test.yaml"
@session.get_file "/test2", tmpfile
File.unlink(tmpfile)

# and this one segfaults
pid = fork do
tmpfile = "/tmp/patron_test.yaml"
Expand All @@ -44,11 +43,11 @@
expect(body.request_method).to be == "GET"
FileUtils.rm tmpfile
end

exit_pid, status = Process.wait2(pid)
expect(status.exitstatus).to be_zero
end

it "should download correctly(md5 ok) with get_file" do
tmpfile = "/tmp/picture"
response = @session.get_file "/picture", tmpfile
Expand Down Expand Up @@ -258,6 +257,16 @@
expect(body.request_method).to be == "GET"
end

it "should work when ssl_cert is supplied" do
@session.insecure = nil
@session.ssl_cert = 'spec/certs/keystore.p12'
@sessions.ssl_cert_type = "p12"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sessions should probably be @session? And if it isn't does this test pass?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my bad

@session.ssl_key_password = "pkcs12"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the password is set incorrectly, what is the exception that is going to be raised? Do we need a separate exception type for this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, will add a spec for that case

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curl -v --cert ./keystore.p12 --pass pkcs13 https://google.com                                                                                                                                           * Rebuilt URL to: https://google.com/
*   Trying 74.125.68.113...
* TCP_NODELAY set
* Connected to google.com (74.125.68.113) port 443 (#0)
* WARNING: SSL: Certificate type not set, assuming PKCS#12 format.
* SSL: Incorrect password for the certificate "./keystore.p12" and its private key.
* Closing connection 0

trying to connect with wrong password, seems like failed at SSL level

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, would be useful to check for a proper exception in that case on the Patron end

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, it's kind of busy in those days, i will catch and update this PR asap

response = @session.get("/test")
body = YAML::load(response.body)
expect(body.request_method).to be == "GET"
end

it "should work with different SSL versions" do
['SSLv3','TLSv1'].each do |version|
@session.ssl_version = version
Expand Down