expose .peer_cert on Response

[machty]

Closes #633

There are use cases for accessing the X509
certificate used by the server; this commit
exposes this certificate via Response#peer_cert,
which uses the same method name as peer_cert
on the Net::HTTP class:

https://ruby-doc.org/stdlib-2.4.2/libdoc/net/http/rdoc/Net/HTTP.html#method-i-peer_cert

[machty]

My last commit messed this up for everything... my first commit was working on half of the Ruby versions (the more recent ones)... seems there may be some issues with the way WebMock works on older rubies. Would appreciate any ideas / advice from anyone who's had to fight this kind of stuff in the past.

[machty]

OK I think I got it! I'm already using this branch in our codebase to monitor/report certificates that are expiring within a month and it works great.

[machty]

@jnunemaker This is ready for review when you have a second :)

[machty]

@TheSmartnik do you have a second to take a look at this? We've using it to great effect for a few months now.

[TheSmartnik]

@machty Hi! Thank you for contributing to httparty. I'm sorry it took so long to review your pr.

I feel like exposure of peer_cert in Response object is in the wrong abstraction layer. It belongs to connection not to a response. It would somewhat justifiable to put it in a response, if it was often used/needed, but unfortunately it's not.

However, you're right that we need some way to access peer_cert. I'm just not sure how yet. Maybe by exposing connection or something like that. I'll write back as soon as I think of a solution. If you have any ideas, would love to hear them.

[machty]

@TheSmartnik exposing the connection object seems fine to me. One weird thing about that (which is true of my PR as well) is with following redirects: if we expose Response#connection or Response#peer_cert, it's ambiguous which one you'll get in the case of redirects. My PR just exposes / stashes the first peer_cert it can find (i.e. it'll stash the peer cert from the first connection in a series of redirects that uses SSL).

I guess for my use case, I'd want to know if any of the peer_certs in a series of redirects is expiring soon, so I guess I'd want access to all the connections involved. It just feels perhaps a bit heavy to stash an array of connections on the Response, but perhaps not?

[TheSmartnik]

@machty thanks for suggestions.

It just feels perhaps a bit heavy to stash an array of connections

Run a few tests. Difference turned out to be insignificant.

However, it appears that it wouldn't work anyway. Due to implementation of Net::HTTP.peer_cert, certificate doesn't get prefetched and it's impossible to get it once connection is closed.

The only other solution I can think of is to rename FragmentWithResponse and update it to include connection
https://github.com/jnunemaker/httparty/blob/master/lib/httparty/request.rb#L151

So retrievement of peer_cert would look like this.

peer_cert = nil
HTTParty.get(url, stream_body: true) do |response|
   peer_cert ||= response.connection.peer_cert
end

The only problem with this is that passing a block to httparty makes it to stream response. On the other hand it will allow much greater flexibility. You can check whether it's a redirect and save multiple certificates etc.

[machty]

@TheSmartnik I'm not familiar enough with the internals to know if something like this exists, but it seems like it'd be nice and less disruptive (relative to requiring stream/block form) if a Response body included an array of all the intermediate redirects/requests that were part of a Response, and whatever this "mini-Request" objects were would provide the peer_cert if present.

Alternatively, I just realized it might be a nice use case to be able to do additional validation on the certificate before allowing the download / request to continue; I don't know if this is possible at the Net::HTTP level, but it might be nice if you could provide HTTParty an on_cert_received hook/proc which you could use either to stash the peer cert for your own needs/reporting, or raise an error to stop the request.

What do you think?

[TheSmartnik]

@machty

array of all the intermediate redirects/requests that were part of a Response, and whatever this "mini-Request" objects were would provide the peer_cert if present.

Thanks. Idea is generally good, but I don't think it fits nicely here.
Creating some response/connection decorator that would eagerly get peer_cert, when few actually need it, doesn't seem like a good idea. It's more complex, memory consuming and less flexible. I think a better way would be to give a developer tools to access/manipulate connections how he wants to.

might be nice if you could provide HTTParty an on_cert_received hook/proc which you could use either to stash the peer cert for your own needs/reporting, or raise an error to stop the request.

HTTParty already has lots of little known and used options. I don't think we need any more, to be honest. Sorry 😞

[machty]

@TheSmartnik

Thanks for the feedback.

The only problem with this is that passing a block to httparty makes it to stream response

I'm looking at this code and unless I'm misunderstanding, I think you'd want to omit the stream_body: true option, right?

So, to modify your example:

peer_cert = nil
resp = HTTParty.get(url) do |response|
   peer_cert ||= response.connection.peer_cert
end

# resp.body contains the response body; 
# if we provided stream_body: true, I believe it would be nil

Maybe I'm wrong.

Either way, I'm OK with your proposed API of exposing the connection object when using block mode. Would you be OK if I made these changes to my PR?

[TheSmartnik]

I'm looking at this code and unless I'm misunderstanding, I think you'd want to omit the stream_body: true option, right?

Basically, the stream_body just removes body from request. I thought in case above you would just care about certificates and not request, so I've added the option. You can omit it, if it's confusing.

Would you be OK if I made these changes to my PR?

Would be awesome, thanks! Also, could you please add an example to examples as well.

[machty]

Closing in favor of #648