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

Add error message if SAN doesn't match hostname

Merged
merged 4 commits into from May 17, 2020

Conversation

kidburglar
Copy link
Contributor

@kidburglar kidburglar commented Aug 17, 2019

When I was trying to make syncplay working with a self-signed certificate I had a reconnection loop.

The issue was related to the SAN (Subject Alternative Name) that was not correct in my certificate.
I think it can be usefull to detect this error.

@Et0h Et0h requested review from albertosottile and Et0h Oct 6, 2019
Et0h
Et0h approved these changes Oct 6, 2019
Copy link
Contributor

@Et0h Et0h left a comment

The code looks fine, although I note it does not include stubs for other languages which would be a potential improvement. Thanks for your work on this.

Copy link
Member

@albertosottile albertosottile left a comment

Thank you for this contribution. I'd also say this is ok, but I would still like to have stubs for all the other supported languages.

@kidburglar
Copy link
Contributor Author

kidburglar commented Oct 6, 2019

Hello @albertosottile @Et0h the stubs is to copy/paste the translation in the other langages ?
If it's that I can to it and provide an update so it can be merged.

@Et0h
Copy link
Contributor

Et0h commented Oct 6, 2019

Hello @albertosottile @Et0h the stubs is to copy/paste the translation in the other langages ?
If it's that I can to it and provide an update so it can be merged.

You copy it over and then add '# TODO: Translate' as comment. Then translators can just look for the TODOs to find out what needs translating.

@daniel-123
Copy link
Contributor

daniel-123 commented Oct 6, 2019

I'm not against this change, but if there is a more specific and user friendly message about TLS failure it should be worded in way that's more understandable - since I don't expect most people (or even most power users) to know X.509 field names and their purposes by heart.

Not to reinvent the wheel I'd think wording more similar to what browsers use to be more appropriate. For example Firefox in such situation will show the following in advanced description of problem:

Websites prove their identity via certificates. Firefox does not trust this site because it uses a certificate that is not valid for wrong.host.badssl.com. The certificate is only valid for the following names: *.badssl.com, badssl.com

@kidburglar
Copy link
Contributor Author

kidburglar commented Oct 6, 2019

@daniel-123

I'm open to any wording, at first it was to avoid an generic error when it can be more precise.
But if someone can tell me which text I need to use I will do the replacement (or if someone want change it directly in the PR it's good for me also)

@Et0h
Copy link
Contributor

Et0h commented May 14, 2020

@kidburglar If you want to propose wording and update your pull request soon in line with @daniel-123's comments then this can hopefully be included in the release candidate of Syncplay v1.6.5.

@kidburglar
Copy link
Contributor Author

kidburglar commented May 15, 2020

Hello @Et0h

Like I said before to @daniel-123 I'm open to any wording. There is just need to tell me which wording using and I will make the changes (if it's faster to make the changes directly without that I make it, it's good for me too).

@daniel-123
Copy link
Contributor

daniel-123 commented May 15, 2020

I would go with minimal modification to the previously mentioned message, so:
Syncplay does not trust this server because it uses a certificate that is not valid for its hostname.

Ideally it would spell out both the current hostname it sees and what's valid in certificate it received, but I guess that would be more complexity than this is worth.

@kidburglar
Copy link
Contributor Author

kidburglar commented May 16, 2020

Hello @daniel-123 @Et0h I have change the message and I add it to the other languages with a " # TODO: Translate" comment.

Remove "startTLS-server-certificate-invalid-DNS-ID" that I wrongly copy pasted
@Et0h Et0h merged commit fbd474c into Syncplay:master May 17, 2020
1 check passed
@kidburglar kidburglar deleted the add-missmatch-san-error branch May 17, 2020
albertosottile pushed a commit to albertosottile/syncplay that referenced this issue Sep 30, 2020
* Add error message if SAN doesn't match hostname

* Add a better message for the error startTLS-server-certificate-invalid-DNS-ID and add the strings to the other languages
albertosottile pushed a commit to albertosottile/syncplay that referenced this issue Sep 30, 2020
* Add error message if SAN doesn't match hostname

* Add a better message for the error startTLS-server-certificate-invalid-DNS-ID and add the strings to the other languages
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

Successfully merging this pull request may close these issues.

None yet

4 participants