-
Notifications
You must be signed in to change notification settings - Fork 74
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
base: master
Are you sure you want to change the base?
Conversation
|
||
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)); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really impressive, thanks! Good to merge but I am curious what the failure mode is with a non-happy path. Is there a clear exception message propagated via the curl abort?
@session.insecure = nil | ||
@session.ssl_cert = 'spec/certs/keystore.p12' | ||
@sessions.ssl_cert_type = "p12" | ||
@session.ssl_key_password = "pkcs12" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
it "should work when ssl_cert is supplied" do | ||
@session.insecure = nil | ||
@session.ssl_cert = 'spec/certs/keystore.p12' | ||
@sessions.ssl_cert_type = "p12" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, my bad
@lenage Sorry it's been a while - but could you maybe take another look at the feedback? |
Is this project pretty much unmaintained now? |
I'm looking at PRs regularly here. On this PR there was feedback which was left unadressed. Also I do think that using a self-signed cert does merit setting up the test Puma with that cert to verify whether connecting works correctly and it is possible to do a GET. |
I was also looking at the fact that there hasn't been a release since 2014. |
Added
ssl_cert
,ssl_cert_type
andssl_key_password
to support self-signed PKCS12 cert.see cURL docs for those options:
https://curl.haxx.se/libcurl/c/CURLOPT_SSLCERT.html
https://curl.haxx.se/libcurl/c/CURLOPT_SSLCERTTYPE.html
I just wrote one spec, and it passed, if i missed something, please let me know.