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

Give useful error message on SSL errors, e.g. self-signed certificate #1191

Open
borisyankov opened this issue Sep 19, 2017 · 26 comments
Open
Labels
a-onboarding Everything you would do when first joining a realm. a-TLS TLS/SSL certificates and config; "Cannot connect to server" errors upstream: RN Issues related to an issue in React Native

Comments

@borisyankov
Copy link
Contributor

borisyankov commented Sep 19, 2017

If a user tries to connect to a server that uses a self-signing certificate be able to detect that and give a reasonable error (instead of generic one)

React Native does not allow us to differentiate or to override this. We do not want to patch it.

In RealmScreen.js if const serverSettings = await getServerSettings(realm); fails, call a native function that tries to connect to the same URL but ignoring SSL.
If successful, then the server is using a self-signed certificate. Show a specific error detailing this.

The task can be worked by two people, one doing the Android work and another doing the iOS code.

@gnprice gnprice changed the title Detect servers with self-signing certificates Give useful error message on SSL errors, e.g. self-signed certificate Mar 22, 2018
@gnprice
Copy link
Member

gnprice commented Mar 22, 2018

The current behavior seems to be that we give the error message "Network request failed". This doesn't give much idea that there might be an issue with the certificate -- instead it just looks like the app is broken. We regularly get people reporting this as an issue in the app; here's one today.

If we can just distinguish between a network failure (like the server doesn't exist, or our network connection is down) vs. an SSL error (like a self-signed, otherwise untrusted, or invalid certificate), that'd allow us to do much better. E.g., for the SSL error we can say something like "This server can't provide a secure connection (help)", with "help" a link to our SSL docs.

@gnprice
Copy link
Member

gnprice commented Mar 22, 2018

Alternatively, if it really is super hard to distinguish "the server or network is down" from "the server is on the network but SSL is busted", @timabbott points out that we could make the situation a lot better even without that distinction: we just need to make the error message more verbose, explaining that it could be this issue and again linking to docs.

@borisyankov
Copy link
Contributor Author

I will edit this issue and will suggest it as a potential task for GSoC candidates.

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 26, 2018

I can work on this.

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 26, 2018

@zulipbot claim

@zulipbot
Copy link
Member

Hello @ebouJ, it looks like you've currently claimed 1 issue in this repository. We encourage new contributors to focus their efforts on at most 1 issue at a time, so please complete your work on your other claimed issues before trying to claim this issue again.

We look forward to your valuable contributions!

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 26, 2018

Currently, this is what I have so far. I just need to know what a self-signing certificate is

  `tryRealm = async () => {   


 const { realm } = this.state;
  

   this.setState({
   realm,
   progress: true,
   error: undefined,
});

const { actions } = this.props;

try {
  const serverSettings = await getServerSettings(realm);
  actions.realmAdd(realm);
  actions.navigateToAuth(serverSettings);
  Keyboard.dismiss();
} catch (err) {
  this.classifyError();
} finally {
  this.setState({ progress: false });
}
};`

and in ClassifyError function, this is what I have

`  classifyError = async () => {
let err = '';
const isConnected = await NetInfo.isConnected.fetch();
if (!isConnected) {
  // network connection is down
  err = 'Network connection is down';
} else {
  // two cases either the server doesn't exist or SSL problem
  // I don't know much about SSL
  // but if I realized that it's an SSL issue set the error message to that
  // otherwise the server doesn't exist
  err = 'Cannot connect to the server';
}

this.setState({ error: err });
};`

@borisyankov You suggested this "call a native function that tries to connect to the same URL but ignoring SSL" can you guide me more on this so that I can complete the function and send a PR.

@borisyankov
Copy link
Contributor Author

@ebouJ read the description of the task carefully. You are missing key points of it!

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 26, 2018

Should I bridege and write a native function to ignore SSL when connecting to a URL? @borisyankov

@borisyankov
Copy link
Contributor Author

Yes!

@akashnimare
Copy link
Member

I have set up this server for testing self-signed certs. If anyone wants then feel free to use it. PM me for the certificate.

@borisyankov
Copy link
Contributor Author

@ebouJ this is a cool opportunity to test if your code works. Let us know if you need help.

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 27, 2018

See the output of using his self-signed certificate. I also tried a couple of them yesterday from https://badssl.com. @borisyankov
29634720_1975114692530318_250454613_o

@borisyankov
Copy link
Contributor Author

Oh, https://badssl.com/ is great!
Can you distinguish between https://self-signed.badssl.com/ and any other? That is what we care about.

@ebouJ
Copy link
Collaborator

ebouJ commented Mar 28, 2018

I did that for my first implementation but had to change it because of the approach I took. For now, all SSL related issues are classified as the same error. What I did before was to classify them based on the error message I get if I try to connect. Each of the SSL problems returns a unique error message but I feel like it was hardcoded and if the error change it's going to break the code. So I can try to look into it more and see if I can come up with a more solid solution, if not I will get directions from you. @borisyankov

@gnprice
Copy link
Member

gnprice commented Apr 2, 2018

Nice!

I don't think it's necessary, or even helpful, to give different error messages for different kinds of SSL errors. For most users that will be more confusing than helpful and not at all actionable; and for the server admin, they have better tools to investigate the SSL issue if they want to.

Like I said above, if we can just distinguish between

  • a network failure (like the server doesn't exist, or our network connection is down)
  • an SSL error (like a self-signed, otherwise untrusted, or invalid certificate)

then we'll be in good shape.

@ebouJ
Copy link
Collaborator

ebouJ commented Apr 3, 2018

@gnprice the current pull request is doing exactly that. The first thing I did was to check if the user has active internet connection, if not then that's the problem. Otherwise it's either SSL or the server doesn't exist. I then try to connect using the native function and ignoring all the SSL trust issues if the connection was successful then I gave a generic error message like "This server can't provide a secure connection" otherwise I assume the server doesn't exist. The PR isn't complete yet because I have to write native module for IOS, I only know swift so I want to look into Objective C and complete the module. For now, it's only working for Android.

@gnprice
Copy link
Member

gnprice commented Apr 11, 2018

the current pull request is doing exactly that

Yep, understood. (We're talking about #2145 -- linking that just because it took me a minute to find just now.) There was a comment above suggesting that we needed to make a different distinction, so I wanted to make clear that that's not needed.

@gnprice gnprice added the a-onboarding Everything you would do when first joining a realm. label May 24, 2018
@zulipbot
Copy link
Member

Hello @zulip/server-onboarding members, this issue was labeled with the "area: onboarding" label, so you may want to check it out!

@anemes
Copy link

anemes commented Dec 5, 2018

hello, just rolling out a self-hosted self-signed zulip server and wondering if there is a solution to allow the android app to connect?

@colin-zhou
Copy link

I meet the same problem, ios app cannot connect the self-hosted and self-signed zulip server, could this be supported in the future?

@danielecr

This comment has been minimized.

@timabbott
Copy link
Member

Check out https://zulip.readthedocs.io/en/latest/production/ssl-certificates.html#troubleshooting; it's likely that your SSL certificate isn't installed correctly (e.g. you haven't provided a proper certificate chain to nginx).

@danielecr

This comment has been minimized.

@danielecr

This comment has been minimized.

@danielecr

This comment has been minimized.

@gnprice gnprice added the a-TLS TLS/SSL certificates and config; "Cannot connect to server" errors label Aug 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a-onboarding Everything you would do when first joining a realm. a-TLS TLS/SSL certificates and config; "Cannot connect to server" errors upstream: RN Issues related to an issue in React Native
Projects
None yet
Development

No branches or pull requests

9 participants