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

Connection failed loop if hostname doesn't match certificate #652

Open
Ghosthree3 opened this issue Dec 24, 2023 · 5 comments
Open

Connection failed loop if hostname doesn't match certificate #652

Ghosthree3 opened this issue Dec 24, 2023 · 5 comments

Comments

@Ghosthree3
Copy link

Describe the bug
If the server is configured to use TLS but upon connecting the hostname does not match the certificate (or perhaps if the certificate has expired as well, haven't tested), an error will be printed indicating as such every 3-5 seconds as Syncplay attempts to reconnect.

To Reproduce
Set up working TLS support, then change from using the valid hostname in the client settings to either another or to the direct IP address of the server.

Expected behavior
As Syncplay is supposed to support Opportunistic TLS I would expect that upon failing to establish a working TLS connection, even if TLS is supposed to be supported by the server, a non TLS fallback connection would be established instead, exactly as if TLS support was not included in the client.

Version and platform:

  • OS: Both Windows and Linux
  • Syncplay version and build type: Syncplay 1.7.1
  • Media player and version: mpv
@daniel-123
Copy link
Contributor

I think what you describe is not a bug. It's in fact necessary feature for TLS to work in its confidentiality aspect.

What you described as expected behavior would be to allow a zero-effort man-in-the-middle attack to be executed against the connection by forcing a fallback.

If the server you are connecting to is using a certificate for different hostname than it actually has, that's a problem for the server administrator to resolve.

@Ghosthree3
Copy link
Author

As long as an appropriate message is printed denoting that the connection is NOT using TLS I don't really see the issue, given that if a user simply lacks a TLS dependency like python-service-identity they can connect to the server using any address or hostname without error and begin leaking an entire chatroom over plain text anyway.
If TLS was meant to be enforced and not opportunistic I would agree with you, or if the user was somehow fooled into thinking they did have a TLS connection when in fact they did not.

However, I do understand your point and would understand if this were closed as not a bug. The scenario is pretty niche, and I agree that the better option is to get users and administrators to configure their configs correctly.

@Et0h
Copy link
Contributor

Et0h commented Dec 27, 2023

I agree that the present behaviour isn't really a bug, as it doesn't go against documented behaviour. Just changing it to connect with plaintext seems like a step backwards, and making user option just for this edge case seems unnecessary. Even if implemented, it would only solve the problem for people using newer versions of Syncplay, and the host would still need to fix their mismatch issue. A change in new versions of Syncplay could make things work for this edge case because it would mean that those on newer versions of Syncplay would not understand the issue faced by those on older versions and it would disincentive the host from fixing an issue that harmed backwards compatibility.

If there was going to be a change, it would want to be a change more generally to how Syncplay more generally handled plaintext connections.

If the present behaviour were changed I'm not sure if it would be better for it to be:

  1. require SSL (with exception list) by default; or
  2. allow plaintext but error on invalid SSL by default, but allow SSL to be required and allow hosts where plaintext is always allowed.

The advance of option 1 is that it would encourage security, but the advantage of option 2 is that it would not scare people disproportionately for non-SSL servers.

There's also the issue that if a user makes a big deal of requiring SSL then that still doesn't guarantee that all users on the connection have SSL. If we raise expectations about SSL, then there would need a way for warning people about if there is someone connected to the server who doesn't have SSL to avoid a false sense of privacy.

All of this is a lot of complexity, and would result in a lot of text that would need to be converted into multiple languages.

I'm not really sure it is worth it when the vast majority of Syncplay users use the public servers (that support SSL) and use Windows (that supports SSL). There's a lot of design decisions that would need to be consulted upon and made, and a lot of testing that would need to be done on the changes.

Even if there is consensus for how to change the current behaviour someone would have to volunteer to implement it. I only connect to one server and that server has SSL enabled so it's not really a feature I would use, and as such it is not a good one for me to developer or maintain. Also, I don't really have much time for non-critical features at the moment in any case.

@Ghosthree3
Copy link
Author

There's also the issue that if a user makes a big deal of requiring SSL then that still doesn't guarantee that all users on the connection have SSL. If we raise expectations about SSL, then there would need a way for warning people about if there is someone connected to the server who doesn't have SSL to avoid a false sense of privacy.

Having an "enforce TLS" toggle for the server, which if disabled resulted in users having a green tick or red cross by their name showing their connection status would be a nice way of handling this (could add tooltips saying "using TLS/not using TLS").

Personally I would always prefer enforcing TLS. But it wasn't enforced, and has resulted in a peculiar situation that I should probably have handled at an administration level rather than as a bug report (though I still consider the looping connection failure strange when you can bypass TLS entirely anyway).

I agree that this is a non critical topic, so feel free to shelve or close it.

@Et0h
Copy link
Contributor

Et0h commented Dec 27, 2023

If someone wanted to code an enforce SSL server switch I wouldn't have a problem with that so long as they had a backwards compatible way of telling users why their connection attempt failed.

I don't think having a green tick is necessary but maybe there could be a broken lock icon or something for people not on SSL.

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

No branches or pull requests

3 participants