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

Why https module doesn't make a check for certificate revocation? #16338

Closed
astafev opened this Issue Oct 20, 2017 · 12 comments

Comments

Projects
None yet
4 participants
@astafev
Copy link

astafev commented Oct 20, 2017

Hello, didn't find any information and find it weird that https by default doesn't make a check for certificate revocation.
And in fact there's no normal way to do so using standard libraries. IMHO, this check is equally important as any other check, such as trusted CA or expiration date.

P.S. I'm testing on https://revoked.badssl.com, I'd expect that https (tls) module would throw an exception as it does for https://self-signed.badssl.com, for example.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Oct 20, 2017

And in fact there's no normal way to do so using standard libraries.

There is, look for the .crl option in the tls documentation. You can also enable OCSP stapling.

@astafev

This comment has been minimized.

Copy link

astafev commented Oct 20, 2017

as I understood from the documentataion, crl option accepts an already downloaded crl, which is impossible when we connect to a host that we do not know much about before connection. I'm talking about situation when we use https as a client libarary, so enabling OCSP Stapling won't work as well.

And, not less important IMO, I think that it should be enabled by default. Basic idea is that when I make an http request to some host, I expect that it's certificate would be checked. Now the situation is that some checks are performed (certificate path, expiration date), some are not.

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Oct 20, 2017

I'm a little confused as to how you propose this would be done if not with a downloaded CRL? As far as I know that's literally what Chrome does, it updates it's CRL from all the different sources whenever it auto-updates itself.

@astafev

This comment has been minimized.

Copy link

astafev commented Oct 20, 2017

@apapirovski didn't look into Chrome implementation, if it does it this way, at least it should do updates every few hours. What I suggest is to make OCSP request (and CRL, not sure), if possible, enable it by default.

Just to clarify my point: open https://revoked.badssl.com/ in any browsers - it will show an error. https doesn't show any error/warning.

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Oct 20, 2017

This is just IMO but the default use case for the client side implementation isn't to query a bunch of random, unpredictable hosts. It would make no sense for the default to opt you into the whole mess of retrieving an updated list of revoked certificates and managing such a list.

If part of your use case is dealing with random hosts and revoked certificates then it seems like that is something that you need to implement. Node does include the building blocks for users to implement this functionality which is IMHO enough.

To me this sort of mimics the discussion around CRLs within curl. https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=745837

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Oct 20, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Oct 20, 2017

CRLs and OCSP, plain and stapled, have enough practical concerns that turning them on by default is going to lead to widespread fallout. A good overview: https://www.maikel.pro/blog/current-state-certificate-revocation-crls-ocsp/

Bundling CRLs with Node.js isn't useful because they would be out-of-date almost immediately (and before anyone asks, download-and-cache is not an option - too many failure modes.)

@astafev

This comment has been minimized.

Copy link

astafev commented Oct 20, 2017

ok, I understand your practical concerns about default enabling. Even though some tools (Firefox?) as I unederstood implemented it. But what about option like enableTheStrictiestSecurityChecks? And 2 more things:

  • somehow nodejs developers are very bold in enforcing security, for example many cyphers that still work in web are disabled in tls module, making the revocation check would be in line :) ;
  • if not enable the check, it should be at least somehow emphasized in the docs (like caution: we do not check revocation, because of performance reasons and some others, see link), it was a surprise for me.

Again, IMO, this check is at least equally important as other checks of certificates and the fact that it's techically challenging is not a reason to ignore the problem.

@apapirovski not random hosts, just hosts, known hosts also can suffer from compromised certificates. Now I can't even think of possible normal crl option use case, it must be: we should know CA of the host's certificate, then make a request to retrieve CRL, then use it as value in the option.

@apapirovski

This comment has been minimized.

Copy link
Member

apapirovski commented Oct 20, 2017

@astafev There's a reason literally nobody out there offers this out of the box, including curl which is used for all sorts of things. Please read this article to understand the problem in more detail https://scotthelme.co.uk/revocation-is-broken/

If you have an elegant solution that even the browser vendors haven't been able to come up with then I'm sure node would consider a PR.

@sam-github

This comment has been minimized.

Copy link
Member

sam-github commented Oct 20, 2017

Have you checked for an npm module that does CRL revocation checks? Using CRLs, if it makes sense in a particular environment, can be done in the callback to check cert identity.

@astafev

This comment has been minimized.

Copy link

astafev commented Oct 20, 2017

why do you guys make me read similar articles? about problems that I knew about before starting this thread...

So, you're saying that as nobody implements it in client libraries, than it's ok. I guess it's fair, even though a bit dissapointing.

@sam-github yeah, I found ocsp library that can do the work, thanks.

@bnoordhuis

This comment has been minimized.

Copy link
Member

bnoordhuis commented Oct 20, 2017

about problems that I knew about before starting this thread...

If you knew about those problems, then why did you say it was "weird" it's not enabled by default? Logic error, does not compute.

I'll go ahead and close this out, I think your question has been answered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment