Uses https endpoint and root cert verification #14
Conversation
|
Hey @notahat, any chance of getting this merged? My info is that HTTP is going away in about a month. |
|
Given plain text is being deprecated (and is just plain wrong), +1 to this. Could it please be merged and a gem released? |
|
+1 |
1 similar comment
|
+1 |
| require 'pagerduty/version' | ||
|
|
||
| RootCA = '/etc/ssl/certs' |
There was a problem hiding this comment.
Could we put this inside Pagerduty instead of exposing a global constant?
|
Aside from comment, LGTM. +1 |
|
+1 |
|
I moved the rootca path moved to where it is used in determining if strict verification is necessary. I also tested this branch in our monitoring app and was successful in triggering an PD incident. Is this worthy of a version bump so we can be sure that the https code is being brought in? |
|
👍 |
| if (File.directory?(rootca) && http.use_ssl?) | ||
| http.ca_path = rootca | ||
| http.verify_mode = OpenSSL::SSL::VERIFY_PEER | ||
| http.verify_depth = 5 |
There was a problem hiding this comment.
I'm not sure about this setting. What happens when the certificate chain is longer than five?
What if we don't set it? Can we live with verifying potentially infinite certificate chains?
If we must set it, are we sure 5 a good number?
There was a problem hiding this comment.
The openssl default is 9
https://www.openssl.org/docs/ssl/SSL_CTX_set_verify.html
|
+1 |
1 similar comment
|
👍 |
Uses https endpoint and root cert verification
|
This: |
|
Can you change your code so that you only set the http.ca_path when there's a problem with the current default for your platform? Eg: |
|
@jamesbouressa You may want to add onto a new issue about this problem. I opened #15. |
Pagerduty appears to be disabling HTTP requests to the API. This PR adds HTTPS.