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

[feature/tls-diff] TLS certificate comparison #872

Merged
merged 6 commits into from
Feb 2, 2021
Merged

Conversation

felix-schwarz
Copy link
Contributor

@felix-schwarz felix-schwarz commented Jan 19, 2021

Description

When logging into an account and experiencing a different certificate that does not fulfill the rules for automatic acceptance as replacement, the issue it brings up now shows the differences between the two certificates to allow an informed decision by the user.

Screenshots (if appropriate):

Certificate diff Certificate diff (chain segment) Diff of next certificate in the chain
Bildschirmfoto 2021-01-19 um 15 25 56 Bildschirmfoto 2021-01-19 um 15 26 08 Bildschirmfoto 2021-01-19 um 15 30 38

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

- ConnectionIssueViewController:
	- compare against certificate stored in the bookmark where available
	- remove text-based rendering via CertificateViewController (and remove the class), replace it with the standard certificate view controller
- adaptions to new ThemeCertificateViewController initialization where needed
- update SDK
@CLAassistant
Copy link

CLAassistant commented Jan 19, 2021

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hosy
❌ felix-schwarz
You have signed the CLA already but the status is still pending? Let us recheck it.

@felix-schwarz felix-schwarz added this to the 11.5.0-Current milestone Jan 19, 2021
@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

any way to build a server in which this feature can be tested?

CC @michaelstingl

@michaelstingl
Copy link
Contributor

any way to build a server in which this feature can be tested?

Valid cert ==> invalid cert

first connect to real server with SSL (demo.owncloud.org), then connect through mitmproxy with hacky-hacky cert. cert diff should show you plenty of differences

Valid cert ==> other valid cert

Setup server with Traefik and let's encrypt (Cloudflare DNS only), then enable Cloudflare SSL proxy in front of it…
(maybe too mich effort for this tiny feature)

@jesmrec
Copy link
Contributor

jesmrec commented Jan 26, 2021

i will try that. I was trying to avoid much extra effort setting up environments, but there will be room for learning.

@felix-schwarz
Copy link
Contributor Author

@jesmrec If you haven't tested this in an other way, yet: there is a quick guide on how to test this here in the SDK repo: https://github.com/owncloud/ios-sdk/blob/master/doc/TESTING.md#simulating-certificate-changes

The files used in that example are next to it and can be found here.

@jesmrec
Copy link
Contributor

jesmrec commented Jan 28, 2021

@jesmrec If you haven't tested this in an other way, yet: there is a quick guide on how to test this here in the SDK repo: https://github.com/owncloud/ios-sdk/blob/master/doc/TESTING.md#simulating-certificate-changes

The files used in that example are next to it and can be found here.

Thanks a lot!! that was totally helpful!!

It works fine from my side. Information correctly shown in different themes, orientations, devices and fields.

Photo-2021-01-28-13-20-05_1404 Photo-2021-01-28-13-21-09_1405 Photo-2021-01-28-13-21-15_1406 Photo-2021-01-28-13-21-21_1407

just two considerations from my side:

  • The option Show/Hide works only in case the difference exists. If certificate is shown for the first time or revoked, option Show/Hide does nothing. Nothing bad nor blocker.

  • Current branch points to an sdk commit that does not build. I've tested agains latest one 3a73b20. Just for you to take in account when merging.

@hosy
Copy link
Collaborator

hosy commented Feb 1, 2021

@jesmrec I commit the latest SDK commit into this branch. Not sure if this solves the problem, but you can have a look again. If this issue still exists, we can move forward after adding a new issue for that problem and we can solve this in 11.5.1. But you are right, UI elements without function should be hidden.

@jesmrec
Copy link
Contributor

jesmrec commented Feb 1, 2021

@jesmrec I commit the latest SDK commit into this branch. Not sure if this solves the problem, but you can have a look again. If this issue still exists, we can move forward after adding a new issue for that problem and we can solve this in 11.5.1. But you are right, UI elements without function should be hidden.

SDK correctly updated. The UX issue is not a blocker, just a minor detail.

approved

@jesmrec jesmrec added the Approved by QA Approved by QA label Feb 1, 2021
@felix-schwarz
Copy link
Contributor Author

@jesmrec Good catch. I made a change so that the Show ± button only appears if there are any differences to be shown.

@jesmrec
Copy link
Contributor

jesmrec commented Feb 2, 2021

@jesmrec Good catch. I made a change so that the Show ± button only appears if there are any differences to be shown.

works perfect! now it's more than approved.

@hosy hosy merged commit 41bc25f into milestone/11.5 Feb 2, 2021
@delete-merged-branch delete-merged-branch bot deleted the feature/tls-diff branch February 2, 2021 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved by QA Approved by QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants