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

feat: ATS certificate pinning narrow down on iOS #593

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

alexruzenhack
Copy link
Contributor

@alexruzenhack alexruzenhack commented Nov 1, 2024

Motivation

The default network security provided by ATS on iOS is broad, and this PR narrows it down to anchor and pinned certificates.

Description

React Native makes use of the iOS network framework in its own way. A possible entry point to hook network events and intercept a connection lay on the RCTHTTPRequestHandler class, which is a category of NSURLSessionDataDelegate, therefore responsible to intercept request events and add logic over it. The React Native implementation of the data delegate doesn't implement the authentication challenge and relies on the default behavior provided by ATS. This fact turns our job easier because we don't need to worry about likely side effects.

Acceptance Criteria

  • Extends RCTHTTPRequestHandler to handle the authentication challenge event on session scope
  • Validates anchor certificate prior the hostname filter
  • Filter connections to allow only pinned domains using the ATS settings
  • Rely on session identity cache provided by ATS to improve performance on requests
  • Conduct certificate evaluation for the allowed domains using ATS
  • Cancel the connection if the TLS handshake fail to match the anchor certificate
  • Cancel the connection if the host domain is not allowed
  • Cancel the connection if the certificate is not trust worthy
  • Crashes the application if the ATS settings is not valid

Note

I'm providing an ATS configuration alongside with Android's network configuration used during the tests, but this should change in a further PR with the proper certificate configuration when the certificates are issued.

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@alexruzenhack alexruzenhack changed the title feat: extends RCTHTTPRequestHandler to handle authentication challenge for hardening feat: ATS certificate pinning narrow down on iOS Nov 1, 2024
@alexruzenhack alexruzenhack removed the request for review from pedroferreira1 January 2, 2025 15:57
@@ -221,7 +221,7 @@ export const PRE_SETTINGS_TESTNET = {
txMiningServiceUrl: TX_MINING_SERVICE_TESTNET_URL,
};

export const NODE_SERVER_MAINNET_URL = 'https://mobile.wallet.hathor.network/v1a/';
export const NODE_SERVER_MAINNET_URL = 'https://mobile-wallet.trust.hathor.network/v1a/';
export const EXPLORER_MAINNET_URL = 'https://explorer.hathor.network/';
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the explorer URLs?

Copy link
Contributor

Choose a reason for hiding this comment

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

And tx mining service

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review (WIP)
Development

Successfully merging this pull request may close these issues.

3 participants