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

grpc client is insecure by default but doesn't advertise it in any way. #47

Open
jschaul opened this issue Mar 4, 2021 · 5 comments
Open

Comments

@jschaul
Copy link

jschaul commented Mar 4, 2021

Code in https://github.com/haskell-grpc-native/http2-grpc-haskell/blob/master/http2-client-grpc/src/Network/GRPC/Client/Helpers.hs#L94-L108 is overriding the TLS library's default certificate validation when establishing a TLS connection by accepting any certificate, by setting TLS.onServerCertificate = \_ _ _ _ -> return []

Quoting the relevant pieces of documentation here:

onServerCertificate :: OnServerCertificate

Used by the client to validate the server certificate. The default implementation calls validateDefault which validates according to the default hooks and checks provided by Data.X509.Validation. This can be replaced with a custom validation function using different settings.
-> IO [FailedReason] | the return failed reasons (empty list is no failure)

This default behaviour is unacceptable in any real world use case scenario. At the very least, either the function names should indicate an insecure creation of a grpc client; or haddocks and documentation added to warn users about this.

The project README is also lacking any words of warning.

@ProofOfKeags
Copy link
Collaborator

What behavior needs to be modified in order for it to be secure? I can try to document this but I'm not sure I understand the issue.

@jschaul
Copy link
Author

jschaul commented Jun 1, 2021

When a browser (e.g. Firefox or Chrome) opens a website (e.g. github.com), then it uses a "secure-by-default" approach. This means, it will prefer HTTPS over HTTP, and when using HTTPS, it will prefer certain TLS ciphers over others, and it will validate that the TLS certificates used on the website are valid (have not expired, have been signed by a trusted root certificate). When something is not in order according to its built-in secure-by-default behaviour, it will show a big warning and not load the website in question. See for example the pages on https://badssl.com/ where you can see a range of those problems that your browser will warn you about. You, the users of your browser, will be to some extend protected because the software you use has good defaults.

Now, that is not the case with this grpc client. grpcClientConfigSimple not only simply uses the defaults of the underlying TLS library, but it actively turns off any kind of even basic TLS certificate validation. So when using the grpcClientConfigSimple function with TLS, I will get no indication if the grpc server I connect to has an invalid TLS certificate. No trust store is checked, expiration is ignored, ciphers are not considered, etc.

In general, using TLS means there is some encryption at the transport layer. This is however only useful if that encryption can A) not be easily broken (e.g. by using weak ciphers) and B) there is a check made to ensure the entity you wish to send some payload to is actually the entity you expect - and not a man-in-the-middle attacker.

The current grpc client creation behaviour is unexpected and downright dangerous behaviour: Unassuming users of this library can create software which will be used by real people who expect a certain amount of default security, which is currently absent.

I can understand that for testing purposes you may wish to have a function that is insecure. But that should not be the only function, and it should clearly state that it is insecure.

As it stands, not only am I as the user of haskell-grpc-native mislead when using the library with TLS, as I assume some basic validation (That's the regular expectation I have come to expect when using tools on the internet these days) which has been turned off with TLS.onServerCertificate = \_ _ _ _ -> return [], but in addition I do not have any alternative function from this library that I can use which gives me TLS connection that has some basic checks performed.

Could you please:

  • document that the default behaviour is insecure (at a minimum)
  • rename grpcClientConfigSimple to grpcClientConfigInsecure and add haddocks stating that no TLS checks are performed (this would be appreciated!)
  • if you have the time, add another function which does not turn off TLS checks, or even allows a user of the library to hook their own TLS configuration into it when creating a grpc client.

Does that make more sense now?

@ProofOfKeags
Copy link
Collaborator

Perfect. Thank you for the detailed request. I have just been given co-maintainership of the project. I will not have the chance to address this for the next week or so but hope to be able to get back to it after June 10th.

@drownbes
Copy link

drownbes commented Oct 5, 2021

@ProofOfKeags What is the story behind this suppression function? As I can see Network.TLS have default implementation https://hackage.haskell.org/package/x509-validation-1.6.11/docs/src/Data.X509.Validation.html#validateDefault . Isn't it good enough? Can we just use it?

@ProofOfKeags
Copy link
Collaborator

I don't see why not at first glance. As long as the way we add it in doesn't stop us from writing a custom one or swapping out another value of that type. The main non-default use case is adding additional RootCA certificates to the set that is extracted from the system.

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

No branches or pull requests

3 participants