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

Decision Record - Defer support of SQLite.swift 0.13.0+ #1943

Closed
calvincestari opened this issue Sep 18, 2021 · 5 comments
Closed

Decision Record - Defer support of SQLite.swift 0.13.0+ #1943

calvincestari opened this issue Sep 18, 2021 · 5 comments
Assignees
Labels
decision-record A log of a decision made and all surrounding context.

Comments

@calvincestari
Copy link
Member

calvincestari commented Sep 18, 2021

Status

  • Proposed - Sep 17, 2021
  • Accepted - Sep 20, 2021
  • Closed - Nov 17, 2021

Summary

SQLite.swift 0.13.0 was recently released and one of the changes included is an increase of the macOS minimum deployment target to 10.15 - diff.

PR #1925 began the work of bumping our own macOS minimum deployment target to 10.15, enabling us to support SQLite.swift 0.13.0. Dropping macOS 10.14 results in a number of warnings about deprecated APIs within the Apple Security.framework API. Note that the code in question is in the Starscream source code that we absorbed in PR #1906.

/apollo-ios/Sources/ApolloWebSocket/DefaultImplementation/SSLSecurity.swift:161:13: warning: 'SecTrustEvaluate' was deprecated in macOS 10.15: renamed to 'SecTrustEvaluateWithError(_:_:)'
/apollo-ios/Sources/ApolloWebSocket/DefaultImplementation/WebSocketStream.swift:101:23: warning: 'SSLSetEnabledCiphers' was deprecated in macOS 10.15: No longer supported. Use Network.framework.
/apollo-ios/Sources/ApolloWebSocket/DefaultImplementation/WebSocketStream.swift:197:7: warning: 'SSLGetPeerDomainNameLength' was deprecated in macOS 10.15: No longer supported. Use Network.framework.
/apollo-ios/Sources/ApolloWebSocket/DefaultImplementation/WebSocketStream.swift:201:9: warning: 'SSLGetPeerDomainName' was deprecated in macOS 10.15: No longer supported. Use Network.framework.

Replacing SecTrustEvaluate with SecTrustEvaluateWithError(_:_:) is easy enough - c0523d0.

The other "No longer supported." warnings are a bit more complicated:

  • macOS 10.15 increased the security requirements for trusted certificates - announcement. With this increased scrutiny at the OS level it may no longer be required for our code to verify the domain name in order to trust a certificate. There are no tests of the Starscream source code to validate this logic.
  • The Network framework appears to have an equivalent of SSLSetEnabledCiphers in sec_protocol_options_append_tls_ciphersuite but the number of supported ciphers is dramatically reduced (SSLSettings vs. tls_ciphersuite_t). There are no tests of the Starscream source code to validate this logic.

We may be able to make the above changes and get the two frameworks working together but would have to invest in building a robust test suite for these changes; Starscream's test suite is not enough to rely on.

I've come to the conclusion that the effort to fix the warnings is non-trivial and without an immediate compelling reason to support a minimum of macOS 10.15 we should defer the work. Recently we have discussed excluding any networking stack changes from release 1.0. This decision would work in favour of that new timeline; including this change in release 2.0 would provide ~6 months post macOS 12/iOS 15 release, during which many apollo-ios users will hopefully drop their own support of macOS 10.14 and iOS 12.

Decision

Defer dropping support for macOS 10.14 and iOS 12 until release 2.0. This will very likely require writing our own WebSocket implementation using Apple's native WebSocket classes.

Consequences

Applications that have both apollo-ios and SQLite.swift as dependencies would be limited to SQLite.swift release 0.12.x and below (< 0.13.0). This is because we cannot support the required macOS minimum deployment target of 10.15 without fixing the deprecated API warnings.

@calvincestari calvincestari added the decision-record A log of a decision made and all surrounding context. label Sep 18, 2021
@designatednerd
Copy link
Contributor

How many of these problems would go away if we dropped iOS 12 support and used URLSessionWebSocket instead of our modified version of Starscream?

(note: I don't mean this to propose doing so immediately, just to gauge how much work we'd save when we eventually do it)

@calvincestari
Copy link
Member Author

How many of these problems would go away if we dropped iOS 12 support and used URLSessionWebSocket instead of our modified version of Starscream?

The Security APIs in question were deprecated in both macOS 10.15 and iOS 13 so doing the work for one, or both is the same. Rewriting the WebSocket module using a modern networking API (URLSessionWebSocket) is exactly what I'm proposing, and I believe it is the correct solution.

(note: I don't mean this to propose doing so immediately, just to gauge how much work we'd save when we eventually do it)

Rewriting the WebSocket module in v2.0 aligns with the larger networking overhaul (Apollo Link, async/await, etc.) and given our current objective (Swift codegen), frankly the timing is better. As for how much work we'd save; I think fixing the rest of the deprecations is non-trivial and we'd still be rewriting the WebSocket module later too.

@calvincestari calvincestari self-assigned this Sep 19, 2021
@calvincestari
Copy link
Member Author

Marking this as accepted after agreement within the team.

@calvincestari
Copy link
Member Author

calvincestari commented Nov 3, 2021

The was a PR recently (#2015) that was impacted by this decision. In debugging that there are a couple observations that should be noted.

  • Xcode doesn't give any warnings when building apollo-ios and there is a version mismatch, i.e.: apollo-ios requiring macOS 10.14 and SQLite.swift requiring macOS 10.15. This kinda makes sense given the context of building the framework because you're not actually setting a deployment target, you're just stating minimums. To be honest though I would expect Xcode to at least warn me about it.
  • I verified this with a Cocoapod project and as expected when executing pod install there is a warning about the version mismatch. So it's really only consumers of apollo-ios that are going to bear the brunt of this on macOS targets.
  • If the project that depends on apollo-ios only builds for iOS you would never be impacted by this and can fork to update the minimums/dependencies as needed.

How do we resolve this:

  • Currently this is slated to be included in apollo-ios 2.0 with the Networking Stack overhaul; we will likely bump the minimum deployment targets for macOS and iOS to 10.15 and 13.0 respectively.
  • Have differing macOS minimum deployment targets for ApolloWebSocket and the rest of the apollo-ios targets. This is possible in Xcode but not in the dependency managers where there is only a single platform version specification for the whole dependency. It's also not a very good idea to diverge versions like this.
  • At the moment it seems the best solution is for consumers to fork and make their as-needed changes to move to SQLite.swift 0.13.0. We'll catch up with that in apollo-ios 2.0.

@apollographql apollographql deleted a comment from WALLOUD Nov 4, 2021
@apollographql apollographql deleted a comment from WALLOUD Nov 4, 2021
@calvincestari
Copy link
Member Author

With the release of SQLite.swift 0.13.1 the minimum macOS deployment target has been reverted 10.10 which means we can update our dependency of SQLite.swift without the impacts outlined in this decision log.

#2015 does the update and that closes out this decision log.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
decision-record A log of a decision made and all surrounding context.
Projects
None yet
Development

No branches or pull requests

3 participants