search

msupply-foundation / open-msupply

Open mSupply represents our most recent advancement in the Logistics Management Information System (LMIS), expanding on more than two decades of development inherited from the well-established legacy of the original mSupply.
https://msupply.foundation/open-msupply/
Other
21 stars 13 forks source link

Problem in openmSupply desktop with certs #3395

Open nishaDangol-Sussol opened 7 months ago

nishaDangol-Sussol commented 7 months ago

What went wrong? 😲

Problem in openmSupply desktop with certs ## Expected behaviour 🤔 If you have correct certs used, you must not see such error? PS: it works fine with browser, problem's only with oms-desktop exe ## How to Reproduce 🔨 Steps to reproduce the behaviour: 1. Get oms server running. Have the correct certs file as well 2. Now open oms-desktop exe 3. You'll see the server in the available list. Click it 4. See error SCR-20240322-idj It was working good for v1.6, so wonder if this is a problem with v1.7-rc3; need to investigate/check further! ## Your environment 🌱
clemens-msupply commented 7 months ago

I just tried it out by installing:

Could you please clarify what exactly you were doing? e.g. what do you mean by "Have the correct certs file as well" do you run omSupply as a public server with a public url? (@mark-prins is the omSupply desktop client supposed to be able to connect to such a server?)

mark-prins commented 7 months ago

Looking at the error - the problem would be that the cert is for a domain name (xxx.msupply.org perhaps?) but.. if you connect using desktop and IP discovery, then it connects using the IP ( as per the error above ).

It's a pretty unique scenario! I don't see clients being able to connect a desktop client to a public server in this way. Two possible workarounds:

  1. we are only checking
      error != 'net::ERR_CERT_INVALID' &&
      error != 'net::ERR_CERT_AUTHORITY_INVALID'

as potential SSL errors to allow through, in electron. Expand the list to include ERR_CERT_COMMON_NAME_INVALID

  1. Add the ability to enter a random server address / url for connecting to. The user could then enter the valid domain name.

I'm thinking that we do both. What do others think?

raviSussol commented 7 months ago

I was thinking a different idea that we should do a http connection (instead of https connection) to the available connection servers since ssl connection can't be established in the same sub-net IPs for different servers anyway. So showing available IPs with https and trying to do a ssl connection wouldn't make sense IMO?

Besides, having an option 2 (ability to add random server address/url) would be a good idea or we'll be needing that in the future at some point anyway I think. Specially, if the server is being hosted in the different cloud machine.

mark-prins commented 7 months ago

Thanks Ravi - was hoping that someone would see this issue! the server will be running on SSL though so we cannot connect via HTTP

clemens-msupply commented 7 months ago

We want ssl in any case, you don't know in which network open mSupply will run, e.g. if the local network is also publicly available as a wifi spot. To connect to self-signed certs we do our own certificate pinning, i.e. we assume there is no attacker while setting up the system, and that we are getting the correct pub cert from the server when connecting the very first time. We then remember this cert and fail if the cert has changed, e.g. this could be because there is an attacker...

Will have a look if ERR_CERT_COMMON_NAME_INVALID opens up other things as well...

As I understand there are two issues:

clemens-msupply commented 7 months ago

Is this actually a realistic scenario at the moment?

I am a bit reluctant to accept any ERR_CERT_COMMON_NAME_INVALID, especially if we might want to support using the electron app as a web browser in the future. Then it might be useful to have this check in but think both features should be implemented at the same time to get it right...

mark-prins commented 7 months ago

Yes, I'm with you @clemens-msupply. We could look at supplying the domain name in the discovery packet, though I don't think we have the domain on the server, so that would require extra config.

Simplest would be to allow specifying the server to connect to, and that fits a couple of use-cases now, so I think is helpful. This is a pretty niche case, unlikely to be a problem for normal users

clemens-msupply commented 5 months ago

I don't think this is a pressing issue, but more of a nice to have. So I will remove it from the v2 milestone. Think this is mostly a problem for testers? @nisha-dangol does this need to be addressed soon?

nishaDangol-Sussol commented 5 months ago

I don't think this is a pressing issue, but more of a nice to have. So I will remove it from the v2 milestone. Think this is mostly a problem for testers? @nisha-dangol does this need to be addressed soon?

I haven't seen this original issue recently in v2.0.0-rc5, but have been seeing another issue on fresh installation of standalone mostly: https://github.com/msupply-foundation/open-msupply/assets/59193490/c6843cf2-b1f5-47b6-9b0a-deb21aca8bcd

Since on installation of standalone, it automatically starts the server and desktop-client (without giving me any time to change the updated certs manually), this error pops up continuosly. Anything that can be done for this instead?