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

Migrate Crypto Dependencies to Swift Crypto #382

Open
0xTim opened this issue Jul 20, 2022 · 6 comments
Open

Migrate Crypto Dependencies to Swift Crypto #382

0xTim opened this issue Jul 20, 2022 · 6 comments

Comments

@0xTim
Copy link

0xTim commented Jul 20, 2022

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:

  • It's another C dependency that needs to be updated and maintained by the NIO team, not to mention the scripts that go with it etc
  • It's another library that calls C code and all the potential issues that using C code brings
  • Building multiple copies of BoringSSL is wasteful and leads to a detrimental experience for users already fighting with potentially long compile times

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

@Lukasa
Copy link
Contributor

Lukasa commented Jul 20, 2022

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 X.509 API is a truly bad idea, as the implementation is not long for this world, and the performance and correctness characteristics leave much to be desired.

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.

@0xTim
Copy link
Author

0xTim commented Jul 21, 2022

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 _CryptoExtras to allow us to remove the extra copies we need to vend (like in MySQLNIO or JWTKit) and I'm expecting that NIOSSL would be right at the end of the list. We (Vapor) consider this an important enough piece of work to improve the experience for our users that we're willing to do the implementations. I get that it's not always a simple ask however! We'll need to discuss practicalities over the coming months.

Regarding the x509 APIs - do you have more information on what changes are coming? I'm not suggesting that any BoringSSL code would leak in the SwiftCrypto API, it should just be an implementation detail that could be replaced by OpenSSL or even a pure Swift implementation (in a perfect world where APIs never leaked implementation details!).

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!)

@Lukasa
Copy link
Contributor

Lukasa commented Jul 22, 2022

Regarding the x509 APIs - do you have more information on what changes are coming?

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.

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.

I don't think I communicated my position clearly enough: the existence of Security.framework's APIs is not an issue on Apple platforms, but adding another API that is less capable most certainly is. In particular, having a separate API that supports trust verification vended by Swift Crypto risks cognitive burden on adopters. Worse still, by building that API on top of Boring's X.509 layer, this new API will be the awkward combination of both widely available and worse than the system-provided API on Apple platforms. That would turn the API into an attractive nuisance, encouraging users to use an API that gives worse results than the platform native validator. This has already happened in a number of language ecosystems, such as the Python ecosystem, and those ecosystems have had real trouble unwinding this decision.

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:

  1. SSLCertificate, including some of its currently internal surface area
  2. SSLPrivateKey, including some of its currently internal surface area.
  3. ByteBufferBIO, currently entirely internal (!) and which necessarily includes a NIO dependency (!!)
  4. SSLConnection, currently entirely internal (!)
  5. SSLContext's entire internal surface area

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: ByteBufferBIO in particular is a good example, though I'm sure there are others. ByteBufferBIO today is the gluing together of a BoringSSL concept (BIO) and a NIO concept (ByteBuffer). Crypto cannot possibly have a NIO dependency so it becomes necessary to construct a generic BIO-buffer-wrapper-protocol that ByteBufferBIO would need to refine to avoid simply exposing the entire C layer to NIOSSL. Defining and implementing both the protocol and the wrapper type will become quite a complex unit of work, and the whole thing is unavoidably public API.

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.

@0xTim
Copy link
Author

0xTim commented Jul 24, 2022

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).

The existing BoringSSL X.509 library is going to go away altogether.

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

@0xTim
Copy link
Author

0xTim commented Aug 7, 2023

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)

@Lukasa
Copy link
Contributor

Lukasa commented Aug 7, 2023

The X.509 layer can be replaced. The actual TLS implementation cannot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants