-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
request 2.65.0 getPeerCertificate()
always returns null
on node 4.2.1, works on 0.12.7
#3545
Comments
Do you also see it when you use the https or tls module directly? request is a third-party library. |
WFM with master, by the way:
|
Thanks @bnoordhuis! Yep tls is fine here. I'd thought it was a node issue since the request version is the same, but given that the tls module itself is fine it looks like this is a request issue? |
Moving to request: request/request#1867 |
After some digging through the request source code, I don't believe the issue is with request. In node 4.2.1, after the response connection is closed (once the In node 0.12.x, calling getPeerCertificate even after the connection has closed returns the peer certificate. This seem to be a regression between node 0.12 and 4.x (or maybe an intentional change that isn't clearly documented), and is especially noticeable with the request library, because the request callback is always called after the connection has been closed. One workaround is to grab the response before the close event is fired. You can do this like so:
|
I can confirm that var c = require('tls').connect(443, 'github.com', function() { this.destroy() });
c.once('close', function() { console.log(this.getPeerCertificate()) }); /cc @indutny - No opinion on whether it's a good idea to bring back the old behavior. I'm not sure from looking at the code it that's feasible without negating the memory savings. |
@bnoordhuis this is rather unfortunate and fortunate at the same time. This gave us a pretty solid cut off in RSS usage. I think we could introduce an option to keep the Will this work for you, @mikemaccana ? |
I would lean towards just documenting it. I don't think we need more complexity in the tls module and the current behavior doesn't strike me as unreasonable: Connection closed? Then so is the TLS metadata. It's unfortunate this slipped through but it's not as if the old behavior was documented or even intentional (witness the paucity of regression tests for this feature.) It was just an accident of the implementation. |
Sounds about right @bnoordhuis . Is anyone interested in contributing this? ;) |
I'll write something for it a bit later. |
Ditto. As long as the correct approach is documented (eg, holding connection open it seems) in happy. |
I'm, ahem. |
Documented in eff8c3e. |
This seems to be a regression between node 4.2.1 and node 0.1.2:
First node 0.12.7:
Then node 4.2.1:
The text was updated successfully, but these errors were encountered: