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

SSL Pinning support #129

Merged
merged 4 commits into from
Jul 20, 2024
Merged

SSL Pinning support #129

merged 4 commits into from
Jul 20, 2024

Conversation

enahum
Copy link
Contributor

@enahum enahum commented Jul 3, 2024

Summary

As part of some of the requirements by XXXXX we needed to add support for SSL Pinning when building your own app.

The way is being built is a combination of the app and the network-client library, the app will hold the certificate assets while the network-library will use these certs to verify the server trust.

The cert files should have either a .crt or .cer extension while the name of the file must be the domain name.

The reason we support two files extensions per domain is to be able to build the app taking into account server cert rotation and minimizing the amount of app distribution through updates in order to validate the certs.

This SSL Pinning certificates should be included in the app at build time.

Ticket Link

https://mattermost.atlassian.net/browse/MM-59055

@enahum enahum added the 2: Dev Review Requires review by a core committer label Jul 3, 2024
Copy link
Contributor

@larkox larkox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a couple of questions that I think are not blocking.

@rahimrahman
Copy link

@enahum Same deal with this PR, I am removing myself as a reviewer. Thanks for including me to observe and VERY slowly understanding all the changes that are happening here.

@rahimrahman rahimrahman removed their request for review July 8, 2024 21:18
@enahum
Copy link
Contributor Author

enahum commented Jul 8, 2024

Sure @rahimrahman I can explain at some point in the near future about all this

@enahum enahum requested a review from cpoile July 10, 2024 13:24
Copy link

@esarafianou esarafianou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly done with the review. One comment for my understanding

ios/Delegates/ApiClientSessionDelegate.swift Show resolved Hide resolved
ios/Adapters/BearerAuthenticationAdapter.swift Outdated Show resolved Hide resolved
ios/SessionManager.swift Outdated Show resolved Hide resolved
ios/SessionManager.swift Outdated Show resolved Hide resolved
ios/SessionManager.swift Outdated Show resolved Hide resolved
ios/WebSocket/WebSocketEngine.swift Show resolved Hide resolved
@enahum enahum requested a review from enzowritescode July 20, 2024 01:08
@enahum enahum added 3: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Jul 20, 2024
@enahum enahum merged commit 1e5ec20 into master Jul 20, 2024
@enahum enahum deleted the pinning branch July 20, 2024 11:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants