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

NSS crypto backend for SHA-256 digest #891

Merged
merged 2 commits into from Apr 11, 2019

Conversation

Projects
None yet
5 participants
@eoger
Copy link
Contributor

commented Apr 2, 2019

Notes

  • We introduce the new crypto crate in support/crypto, and replace 2 calls from FxAClient from ring to our new crate for SHA-256 digest.
  • On iOS we link dynamically against NSS.
  • On every other platform we dlopen against either geckoview's NSS or our own provided libs.
  • On Android for unit testing (since there's no GV), we bundle the relevant NSS libs in the forUnitTests JAR artifact.
  • Use the NSS system installed lib for local building/testing (see TC/CircleCI changes) so we don't have to mess with ld_library_path on our dev machines.
  • We cross-compile NSS for every single platform, except one: Linux-> MacOS. I lost hope and gave up. Thankfully it's only used in unit tests so we can fix it later (we still download the relevant libs from the NSS project CI).
  • There's a plugin implemented with kotlin-dsl to handle the new nativeLibs extension: it makes sure the libs (nss and friends) we specify are included in the artifacts we produce. I have high hopes that the new Android megazord spec will make it easier.
  • We need to kill the iOS non-megazords projects for this to work. (tracked in #932).
  • We rename the push crypto crate to push_crypto.

New dependencies

Libloading is a new dependency. The code is pretty simple and can be audited in a reasonable time, while providing good wrappers around dlopen and friends. It is vendored in Firefox and MPL-2 compatible (ISC is similar to BSD/MIT).

@eoger eoger added the WIP label Apr 2, 2019

@eoger eoger force-pushed the nss-crypto branch 12 times, most recently from 0f7c3e2 to ef181e3 Apr 2, 2019

@eoger eoger force-pushed the nss-crypto branch 16 times, most recently from 4c06b34 to a44e1af Apr 4, 2019

@eoger eoger force-pushed the nss-crypto branch 10 times, most recently from 6da0a85 to 30a1a57 Apr 9, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2019

Added documentation for nss_sys and the Kotlin plugin

@thomcc
Copy link
Contributor

left a comment

Wow, very nice. Phenominal work on the build system parts. I have a few nits, but that's it.

Oh, do we need to document the need to install anything on any system?

Show resolved Hide resolved components/support/crypto/Cargo.toml Outdated
) => {
#[cfg(not(target_os = "ios"))]
lazy_static::lazy_static! {
pub static ref $i: libloading::Symbol<'static, unsafe extern fn($($arg: $argty),*) -> $ret> = {

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 10, 2019

Contributor

One lock (from lazy_static) per symbol isn't ideal, but ok.

// We check it at runtime using `NSS_VersionCheck`.
pub const COMPATIBLE_NSS_VERSION: &str = "3.26";

// Code adapted from https://stackoverflow.com/a/35591693. I am not this kind of smart.

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 10, 2019

Contributor

Is there a reason you can't just do?

    ($(unsafe fn $i:ident($($arg:ident: $argty:ty),*) -> $ret:ty;)*) => {$(
        #[cfg(not(target_os = "ios"))]
        lazy_static::lazy_static! {
            pub static ref $i: libloading::Symbol<'static, unsafe extern fn($($arg: $argty),*) -> $ret> = {
                unsafe {
                    LIBNSS3.get(stringify!($i).as_bytes()).expect(stringify!(Could not get $i handle))
                }
            };
        }
        #[cfg(target_os = "ios")]
        extern "C" {
            pub fn $i($($arg: $argty),*) -> $ret;
        }
    )*};

This will avoid us needing to expand the recursion limit if we add more than 128 functions.

This comment has been minimized.

Copy link
@eoger

eoger Apr 11, 2019

Author Contributor

I assume that would mean that we only support functions that return in that case right? We might as well keep the flexibility (and I'm not quite sure we'll hit 128 functions anyway).

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 11, 2019

Contributor

You should be able to use $(-> $ret:ty)? for that, I think

This comment has been minimized.

Copy link
@eoger

eoger Apr 15, 2019

Author Contributor

By the way this worked, thanks!

Show resolved Hide resolved components/support/crypto/nss_sys/src/lib.rs Outdated
Show resolved Hide resolved libs/build-all.sh Outdated
NSS_SRC_PATH=$(abspath "$NSS")

# Remove once NSS 3.44 is out (see bug 1540205 for context).
echo '\

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 10, 2019

Contributor

Oh my god this file is a uh. Jeez.

@@ -2,9 +2,6 @@

set -euvx

abspath () { case "$1" in /*)printf "%s\\n" "$1";; *)printf "%s\\n" "$PWD/$1";; esac; }

This comment has been minimized.

Copy link
@thomcc

thomcc Apr 10, 2019

Contributor

Thanks for killing this too.

@thomcc

thomcc approved these changes Apr 10, 2019

@eoger eoger changed the title [POC] NSS crypto backend NSS crypto backend for SHA-256 digest Apr 11, 2019

@eoger eoger added review and removed WIP in progress labels Apr 11, 2019

@eoger

This comment has been minimized.

Copy link
Contributor Author

commented Apr 11, 2019

Note to myself: this needs changelog entries.

@rfk

rfk approved these changes Apr 11, 2019

Copy link
Member

left a comment

I really like the way this has come together! We've got ongoing conversations about the build setup but can iterate on that over time.

Show resolved Hide resolved buildSrc/build.gradle.kts Outdated
Show resolved Hide resolved buildSrc/src/main/java/NativeLibsPlugin.kt Outdated
"android/x86_64",
"desktop/linux-x86-64",
"desktop/darwin",
"desktop/win32-x86-64"

This comment has been minimized.

Copy link
@rfk

rfk Apr 11, 2019

Member

Might we ever need to add items to this list in the future? If so, is there somewhere we should put a reminder to tell our future selves about it?

Show resolved Hide resolved components/fxa-client/android/build.gradle Outdated
Show resolved Hide resolved components/support/crypto/Cargo.toml Outdated
Show resolved Hide resolved components/support/crypto/nss_sys/README.md Outdated
}

pub fn get_nss() -> (PathBuf, PathBuf) {
let nss_dir = env("NSS_DIR").expect("To build for iOS, NSS_DIR must be set!");

This comment has been minimized.

Copy link
@rfk

rfk Apr 11, 2019

Member

Nice, this is much simpler than the previous version :-)

@@ -96,9 +113,11 @@ afterEvaluate {
}
def buildType = "${variant.buildType.name.capitalize()}"
tasks["generate${productFlavor}${buildType}Assets"].dependsOn(tasks["cargoBuild"])
// Don't depend on copyNativeLibs here as NSS libs are shipped with GeckoView.

This comment has been minimized.

Copy link
@rfk

rfk Apr 11, 2019

Member

Thank you for noting this explicitly for future readers!

Show resolved Hide resolved megazords/lockbox/android/build.gradle Outdated

@eoger eoger force-pushed the nss-crypto branch 3 times, most recently from a1f5749 to a560ea1 Apr 11, 2019

@eoger eoger added the 🔑 NSS label Apr 11, 2019

eoger added some commits Mar 26, 2019

@eoger eoger force-pushed the nss-crypto branch from a560ea1 to 817711f Apr 11, 2019

@eoger eoger merged commit dd3f6e6 into master Apr 11, 2019

9 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
ci/circleci: Carthage build Your tests passed on CircleCI!
Details
ci/circleci: Check Rust dependencies Your tests passed on CircleCI!
Details
ci/circleci: Check Rust formatting Your tests passed on CircleCI!
Details
ci/circleci: Lint Rust with clippy Your tests passed on CircleCI!
Details
ci/circleci: Rust benchmarks Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - beta Your tests passed on CircleCI!
Details
ci/circleci: Rust tests - stable Your tests passed on CircleCI!
Details
ci/circleci: Sync integration tests Your tests passed on CircleCI!
Details

@eoger eoger deleted the nss-crypto branch Apr 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.