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

Fix crash on expired certificates #37

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

alubbe
Copy link

@alubbe alubbe commented Dec 10, 2019

I've run into an error that's crashed our monitoring service. In short, when trying to check an https service which runs an expired certificate, nodejs doesn't create socket.ssl (at least in node v13), and thus oscp proceeds to throw an error. In our case, we're using the https module with ocsp's Agent, which produces the following error:

node_modules/ocsp/lib/ocsp/agent.js:70
  var cert = socket.ssl.getPeerCertificate(true);
                        ^

TypeError: Cannot read property 'getPeerCertificate' of null
    at Agent.handleOCSPResponse (node_modules/ocsp/lib/ocsp/agent.js:70:25)
    at TLSSocket.<anonymous> (node_modules/ocsp/lib/ocsp/agent.js:52:17)
    at TLSSocket.emit (events.js:215:7)
    at TLSSocket._finishInit (_tls_wrap.js:794:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:608:12)

This can be fixed by moving the relevant code into the try/catch statement that already exists in Agent.prototype.handleOCSPResponse, which then allows https to display the actual error properly:

Error: certificate has expired
    at TLSSocket.onConnectSecure (_tls_wrap.js:1321:34)
    at TLSSocket.emit (events.js:215:7)
    at TLSSocket._finishInit (_tls_wrap.js:794:8)
    at TLSWrap.ssl.onhandshakedone (_tls_wrap.js:608:12)

@alubbe alubbe force-pushed the fix-crash-on-expired-certificates branch 2 times, most recently from 6a399b3 to 3281524 Compare December 10, 2019 09:25
@alubbe
Copy link
Author

alubbe commented Dec 10, 2019

Travis reports one failing test, but I don't think it should block this PR because it also fails on master:

 1 failing

  1) OCSP Stapling Provider .check() should validate google.com:

     Uncaught Error: Bad OCSP response status: unauthorized

@alubbe alubbe force-pushed the fix-crash-on-expired-certificates branch from 439ea0a to dbb51d8 Compare February 13, 2020 06:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant