dart-lang / sdk

The Dart SDK, including the VM, JS and Wasm compilers, analysis, core libraries, and more.
https://dart.dev
BSD 3-Clause "New" or "Revised" License
10.24k stars 1.57k forks source link

intent to implement: dart:crypto #50290

Closed devoncarew closed 1 week ago

devoncarew commented 2 years ago

This is an issue to raise awareness that we're considering adding a new library to the Dart SDK - dart:crypto. This would be to provide basic cryptographic APIs to Dart. Some details:

For a preview of what the Dart API might look like, see https://github.com/google/webcrypto.dart.

Are you certain you'll ship this?

No, but we're actively considering it. We'll keep this issue updated as plans firm up.

What's the timeframe for shipping it?

We don't have a concrete timeframe; it likely wouldn't happen sooner then ~6 months from now.

Have you considered shipping it as a package instead of as a dart: library?

Yes. Shipping it as a package is currently challenging as we'd need to ship native code with the package, and, we'd need to coordinate the version of BoringSSL used by the package with the version already shipped in the VM. Additionally, crypto is likely a fundamental enough thing to a platform that it's worth having it as a dart: library.

a-siva commented 2 years ago

As discussed in the presentation done on dart:crypto we need to also account for the case that boringssl might not be linked in with the VM or Flutter engine when cupertino_http or cronet_http packages are used in an application. We would need an architecture that enables plugging in a different implementation similar to how we now have pluggablity for the http stack.

bivens-dev commented 2 years ago

Just out of curiosity here but would there be any consideration towards wrapping something like Tink as a way to surface

  1. Known good crypto primitives that are vetted by experts
  2. Exposing them in such a way that they are designed to be difficult to misuse

AFAIK Tink is actually built on top of BoringSSL itself but brings that added safety and development experience to the problem working with crypto primitives which as they mention in their readme can sometimes leave developers feel like they are juggling chainsaws 😂

devoncarew commented 2 years ago

As discussed in the presentation done on dart:crypto we need to also account for the case that boringssl might not be linked in with the VM or Flutter engine when cupertino_http or cronet_http packages are used in an application. We would need an architecture that enables plugging in a different implementation similar to how we now have pluggablity for the http stack.

I don't think we'll end up with a pluggable implementation (though I do remember that being discussed). I think that we'll rely on eventual tree-shaking for libraries using native code via FFI; so, if user code did not end up using the dart:io implementation of SSL (they either weren't using it or had plugged in their own networking stack), and were not using dart:crypto, then BoringSSL could be tree-shaken away. cc @dcharkes

jonasfj commented 2 years ago

I think high-level abstractions like tink might be better provided by a package on pub.dev; if we only offered such APIs I think it would limit the compatibility with external/older systems. And such libraries can hopefully be written on top of dart:crypto.

The proposal here is to offer the primitives available to JS in browsers through window.crypto. It's still unclear if we should ship the keyUsages/exportable capability logic and AES-KW from web cryptography. That's not part of the current prototype. Feel free to provide feedback on the repository: https://github.com/google/webcrypto.dart

The benefit is that we get a single library that works across all platforms (including web and native).

a-siva commented 2 years ago

I don't think we'll end up with a pluggable implementation (though I do remember that being discussed). I think that we'll rely on eventual tree-shaking for libraries using native code via FFI; so, if user code did not end up using the dart:io implementation of SSL (they either weren't using it or had plugged in their own networking stack), and were not using dart:crypto, then BoringSSL could be tree-shaken away. cc @dcharkes

This brings in a new dependency of boringssl in the VM through dart:crypto and I wanted to avoid that. Application size is a very big factor for flutter apps and we are constantly looking for ways to reduce this size and by adding this dependency we would be increasing the size (especially in cases where the native implementation is being used).

dcharkes commented 2 years ago

I think that we'll rely on eventual tree-shaking for libraries using native code via FFI; so, if user code did not end up using the dart:io implementation of SSL (they either weren't using it or had plugged in their own networking stack), and were not using dart:crypto, then BoringSSL could be tree-shaken away. cc @dcharkes

That would be ideal indeed. Please note that tree-shaking away native code comes in various flavours with various engineering challenges:

  1. A dynamic library bound to in a package: library is not referred at all, we don't include it. Should be relatively easy to implement with the work that would build on top https://github.com/dart-lang/sdk/issues/49803 for supporting native libaries in dart run/dart compile/flutter build.
  2. A static library bound to in a package: library and only a subset symbols are used. We can use the native linker for the linking step which would remove unreachable native code. This will only work with assembly snapshots, because our elf snapshots are already a dynamic library which is too late in the native compilation pipeline to link static libraries. This means both a native linker and native compiler have to be available on the users' system (we could ship it with the SDK). Currently, we rely heavily on the ELF snapshots for various OSes. So we would either need to figure out how to generate ELF static libraries instead of ELF dynamic libraries (af93ebcf4cb55ae5f0f39a183ad2d42ca13ae51f) in the VMs ELF-writer, or make assembly snapshots work on all our backends.
  3. A static library bound to in a dart: library. This requires all logic of (2), but in addition that we change our dartaotruntime to a static library rather than an executable and run the native compiler to link an assembly-aot-snapshot with the dartaotruntime. Then the native linker would remove all native code in the dartaotruntime that is not referred.

cc @mraleph @mkustermann

If we would be able to get rid of boringssl in the VM and only have it through dart: packages, tree-shaking would be easier with approach (1). However, if boringssl is in the dartaotruntime, we include it twice in an app.

(2) and (3) are a lot more engineering effort, and require getting alignment on shipping/requiring a native compiler and linker. @mit-mit

At least (2) is where I would want to end up with the Dart eco system and native code. I'm not sure yet about (3).

I do not know if (3) would make signing harder or easier. cc @sstrickl

mkustermann commented 2 years ago

We are generally moving into a world where packages will come with native code and we have to make that use case as easy as possible. If there are hurdles for package:webcrypto we should try to identify them and come up with solutions - because users may run into the same issues (and they cannot add dart:* libraries).

The one thing that is currently not possible - as mentioned above - is to re-use the boringSSL that may be available in the Dart VM / Flutter engine - so there may be an opportunity to save some size, which bears the question: How much code size are we talking about here?

Looking at

=> Blindly adding all of what is in libwebcrypto.so to the flutter engine / dart vm would significantly regress size. => If one would tree-shake libwebcrypto.so to contain only the parts that are needed by dart code using package:webcrypto - it may be quite small - making the need to share with the flutter/dart-vm's version not as important.

jonasfj commented 2 years ago

@mkustermann, I'm actually not sure why libwebcrypto.so probably because just getting it to build was hard enough :see_no_evil:

Logic in package:webcrypto is implemented using dart:ffi which access a table of symbols from BoringSSL. I did an ugly hack where I ensured those symbols retained in the Dart SDK and build the counter app, wich increased app-release.apk size by 2.3kb (see dart-crypto-size.patch, it only aims to retain symbols necessary).

Of course, it's possible I've measured it wrong, or that we'll want other features / tweaks that increase size; in which case we'll have to weigh the pros / cons.

mkustermann commented 2 years ago

As a side note: The following change allows shrinking the 1 MB libwebcrypto.so:

diff --git a/android/CMakeLists.txt b/android/CMakeLists.txt
index 7c8e79b..9768805 100644
--- a/android/CMakeLists.txt
+++ b/android/CMakeLists.txt
@@ -14,6 +14,10 @@

 cmake_minimum_required(VERSION 3.6.0)

+SET(CMAKE_C_FLAGS  "${CMAKE_CXX_FLAGS} -flto")
+SET(CMAKE_CXX_FLAGS  "${CMAKE_CXX_FLAGS} -flto")
+SET(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -flto")
+
 enable_language(ASM)

 # Set as required by android-sources.cmake included below
@@ -40,7 +44,8 @@ add_definitions(-DOPENSSL_SMALL)
 if(ANDROID_ABI STREQUAL "armeabi-v7a")
   set(crypto_sources_ARCH ${crypto_sources_linux_arm})
 elseif ( ANDROID_ABI STREQUAL "arm64-v8a")
-  set(crypto_sources_ARCH ${crypto_sources_linux_aarch64})
+  set(crypto_sources_ARCH)
+  add_definitions(-DOPENSSL_NO_ASM)
 elseif ( ANDROID_ABI STREQUAL "x86")
   set(crypto_sources_ARCH ${crypto_sources_linux_x86})
 elseif ( ANDROID_ABI STREQUAL "x86_64")

it will produce

% unzip -l build/app/outputs/flutter-apk/app-release.apk | grep '[-]v8a/libwebcrypto.so' 
   399224  1981-01-01 01:01   lib/arm64-v8a/libwebcrypto.so

=> ~ 400 kb

When clearing out all symbols referenced (in src/symbols.generated.c), it will produce

% unzip -l build/app/outputs/flutter-apk/app-release.apk | grep '[-]v8a/libwebcrypto.so'
     9920  1981-01-01 01:01   lib/arm64-v8a/libwebcrypto.so

=> ~ 10 kb

So with this simple experiment it seems the base overhead is 10 kb plus any C routines that are actually needed for the Dart code (pay as you go).

Another orthogonal question: I can imagine there's useful crypto algorithms one would expect in a package:crypto that are not needed for TLS connections and may therefore not even be part of BoringSSL. Does the package:webcrypto have additional C libraries it uses for such?

dcharkes commented 2 years ago

When clearing out all symbols referenced (in src/symbols.generated.c)

As discussed with @mkustermann offline, @jonasfj we will need to make the symbols visible and bind to them with @FfiNatives in order for tree-shaking (2) to work. The current indirection in package:webcrypto to avoid name collisions will not support tree-shaking because of the dynamic binding.

jonasfj commented 2 years ago

At least (2) is where I would want to end up with the Dart eco system and native code.

Agreed, this would be lovely.

And shipping crypto as a package would work reasonably, it's a bit unfortunate that apps would include parts of BoringSSL twice, but if we treeshake the parts not used it might be fine.

@mkustermann => ~ 400 kb

Nice, I didn't get -flto to eat much, but digging through logs it was probably because my ndk is too broken to strip symbols :see_no_evil:

arolan commented 2 weeks ago

hello, we are wondering if you have updates on crypto library for dart? do you plan to add it soon? thank you in advance!

jonasfj commented 1 week ago

I think we were unable to reach consensus for a dart:crypto library. There is a lot of reasons to prefer that libraries are built as packages instead. It makes it a lot easier to evolve the API.

And while shipping packages with native binaries is a bit hard today, work is progressing on support for native assets.

In the mean time the API I wanted to propose for a crypto library is available as a Flutter plugin called package:webcrypto on pub.dev. It's still under active development, the API is fairly stable and it works across most platforms.


I'm closing this issue, as I don't think we're going to implement a dart:crypto library.