-
Notifications
You must be signed in to change notification settings - Fork 142
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
Migrate Crypto Dependencies to Swift Crypto #382
Comments
While I think the ask here is reasonable, as a practical matter it causes substantial trouble. The API surface that Crypto would need to add is huge, and it is currently entirely internal to this module. Requiring API stability and evolution processes for this surface is a substantial increase, likely more than doubling the existing API surface. Additionally, it forces Crypto to host APIs that sit very awkwardly with CryptoKit, violating a layering distinction that exists on Apple platforms with CryptoKit vs Security.framework. Finally, it bakes in APIs that we actively do not want. Exposing certificates via BoringSSL’s The TL;DR here is doing this in a way that doesn’t make bullet 1 worse by adding a huge new swathe of API surface that we have to maintain is a huge investment of time and energy. While I agree that it’s important, I think it’s not the most impactful thing we can do with our time to support the Swift on Server ecosystem at this time. |
So to provide some context to this request: This came out of SSWG discussions. It's worth noting that one of the SSWG goal's this year is focusing on build times and the multiple copies of BoringSSL is part of this. This is part of a wider range of proposals for adding new APIs to Regarding the Regarding the Security.framework and CryptoKit distinctions - to be clear we're proposing that the new APIs would be added to CryptoExtras as that's the purpose of it. If that is also going to be problematic then we should have a discussion as from my point of view, the server ecosystem should not have the recommended Crypto library held back due to issues on Apple-only platforms that are not a concern. (As ever with the medium of text and GitHub issues, please don't take any perceived criticisms as meant as anything other than constructive! I completely appreciate that this is a big ask that would require resources to be allocated in an ever competing space and it's not always that simple. We are grateful for all the work that allows Vapor to build a great framework for its users!) |
The existing BoringSSL X.509 library is going to go away altogether. The Google folks don't use it (they use a number of internal solutions), and most other BoringSSL users don't use it either because of its well established correctness and performance problems. I am deeply, deeply nervous about us building an API that wraps this underlying implementation and hoping to transfer it to another implementation in future: this seems likely to cause an enormous set of problems.
I don't think I communicated my position clearly enough: the existence of None of this is the main problem I have though, these are just complications with reasonably straightforward engineering compromises that can be solved with sufficient application of time and energy. The main problem is that we have to expose a vast API surface area from BoringSSL's libssl for general consumption, and that API has to be supported. To clarify this, I quickly checked: we call 160 unique BoringSSL functions, none of which are exposed from CryptoKit today in any form. Conceptually wrapping these into types, we end up needing to expose:
All of this internal surface area that becomes public becomes a maintenance burden and partially frozen in amber. It gets extremely hard to evolve that layer due to being public, so bad decisions become things we're stuck with. Additionally, some types require producing a whole new abstraction: I want to stress that I'm not saying we shouldn't do this. What I'm saying is that it's a sizeable unit of work, and that the cost of that work is not just the original authoring of the code. Importantly, though, NIOSSL is the only customer for any of this code. No-one else is using BoringSSL for this purpose in the Swift ecosystem that I am aware of. This unavoidably pushes us to a tricky place where we're trying to write a general public interface for a single customer, paying all the costs of generality without reaping any of its benefits. In my view we would get substantially more benefit from removing the need for NIOSSL to have any BoringSSL code in it at all, or by finding ways to distribute precompiled binaries, than we do by adding this API surface. |
This would definitely be the ideal solution, I agree. I totally understand the point about exposing a ton of types and APIs that are only used by NIOSSL. (Although there is an argument that if anyone else wants to write their own SSL library they need to roll their own Crypto etc).
So I'm assuming at that point, NIOSSL will need to implement another solution anyway if the functions simply aren't available in BoringSSL? So that might be a good point to see what can become pure Swift, or NIOSSL-only code etc and go from there |
Bumping this as we're working on JWTKit - how much of the current behaviour can be replaced with Swift Certificates and ASN1? (I know they need to hit 1.0 before considering it etc) |
The X.509 layer can be replaced. The actual TLS implementation cannot. |
Going to rename this to focus on the removal of BoringSSL to make it clear migrating different parts to certificates and Crypto and any other libraries with the goal of not needing to build another vended copy of BoringSSL |
Thanks Tim! |
Swift Crypto is the canonical crypto library for the Swift Ecosystem. Current SwiftNIOSSL vends it's own copy of BoringSSL for the cryptography functions it needs. This can cause issues in two ways:
Migrating NIOSSL to use Swift Crypto as a dependency would solve or improve all these issues. There may be some work to add missing APIs to Swift Crypto, but if they're required for NIOSSL to work, that should be enough justification for the Swift ecosystems recommended crypto library.
The number one complaint on the Vapor forums is memory usage when compiling/linking and build times. This would be a great start in addressing that
The text was updated successfully, but these errors were encountered: