Multiplatform #14

luca992 · 2022-04-04T08:12:47Z

This isn't done yet, I still have to clean it up.

But, I ported the project over to using the multiplatform plugin. Added js and native(mac, ios) targets. Updated all dependencies and swapped use of moshi for kotlinx serialization

All targets seem to be working. I used okio's multiplatform sha and hmac sha implementations to replace the use of java specific ones.

The only thing I'm sure unsure about is switching from java Locale to https://github.com/fluidsonic/fluid-locale . I think it accomplishes the same thing?

Todo: get non-jvm tests running. They use kotest. I can't get the tests to run on non-jvm for some reason

use RELEASE_SIGNING_ENABLED=false More info: https://github.com/vanniktech/gradle-maven-publish-plugin#signing

ccjernigan · 2022-04-20T15:25:53Z

@luca992 Thank you for the pull request. Multiplatform is an important feature that we're also keeping an eye on internally, so the effort here is appreciated!

The prior maintainer of this project left Zcash a few months ago, and I've just started as the new maintainer about 2 weeks ago. With the transition, the notification of your draft PR was missed. I'll be looking at reviewing and providing feedback to this PR very soon.

luca992 · 2022-04-20T16:03:25Z

I'm probably going to just move the test json to a string... if that's cool. I'm using moko-resources to load the file in a multiplatform way. But it appears that moko-resources doesn't support resources in the /commonTest/resources folder atm.

added some moko-resources related warning workarounds

ccjernigan

Thanks for this pull request—I appreciate all the effort that went into these great suggested changes here! You might have noticed that we're also taking some steps towards being more multiplatform ready in our other Android projects, so this is something that I think is important.

To speed up the process of incorporating these changes, I'm going to recommend that we break this down into multiple steps as a series of PRs. That will help create a secure and robust multiplatform implementation as quickly as possible.

This is the rough roadmap I have in mind:

Apply Multiplatform Kotlin Plugin: Modernize the build scripts and refactor to be an enabler for multiplatform, but only with a JVM target. Code lives in jvmMain/jvmTest. I've created Modernize build #32 that addresses this.
Multiplatform tests: Port tests to be multiplatform, by dropping use of a JSON resource file. Move tests to commonTest source directory. Tests still only run on JVM, because no other targets added yet. This should be a quick extraction from this existing PR, once I get PR Modernize build #32 merged in the next few days.
Refactor for JVM-only multiplatform: Move most code into commonMain, add expect classes and methods, and add actual classes/methods in the jvmMain source directory. To library consumers, this should behave exactly as before but it sets up the ability to then add additional multiplatform implementations with minimal refactoring. This should also be a somewhat quick extraction from this PR.
Add additional platforms one-by-one (e.g. JS, native). This will be the slowest step, as we'll have to do security reviews of the platform-specific implementations. As a general policy, we want to avoid third party libraries so this will require some careful consideration on what we do for the SHA256 implementation.

I have added some comments inline. I don't necessarily expect to see another commit to this PR addressing those unless you'd like the feedback and iteration. We won't merge this PR but we can quickly merge independent PRs for steps 1, 2, and 3, and then we'll have to slow down a bit for 4.

Do you have a particular non-JVM platform that's especially important to you first? Knowing that can also help focus our discussion around 4 as well.

lib/src/commonMain/kotlin/cash/z/ecc/android/bip39/Mnemonics.kt

lib/karma.config.d/moko-resources-generated.js

ccjernigan · 2022-04-19T14:45:42Z

lib/gradle.properties

@@ -1,7 +1,7 @@
 # Publishing : Required
 GROUP=cash.z.ecc.android
 POM_ARTIFACT_ID=kotlin-bip39
-VERSION_NAME=1.0.2
+VERSION_NAME=1.0.2-SNAPSHOT


Just an FYI I think we'll end up moving towards 2.0.0 to support multiplatform, as there are a few breaking API changes to make.

yeah, didn't mean to commit that. Just needed a unique version for local haha

gradle.properties

lib/src/commonMain/kotlin/cash/z/ecc/android/bip39/Mnemonics.kt

ccjernigan · 2022-04-21T18:46:16Z

lib/src/commonTest/kotlin/cash/z/ecc/android/bip39/BIP-0039-test-values.kt

@@ -1,3 +1,6 @@
+package cash.z.ecc.android.bip39
+
+const val BIP0039TestValues = """


Moving this from a file resource is a good improvement.

Honestly, I don't think it needs to be a json string though. This test data could easily be a list of data class instances so that we don't even have to use Kotlinx serialization for the JSON.

ccjernigan · 2022-04-21T18:51:19Z

lib/src/nonJvmMain/kotlin/cash/z/ecc/android/random/SecureRandom.kt

+
+actual class SecureRandom {
+
+    actual fun nextBytes(bytes: ByteArray){


Until we have a secure source of randomness for each supported platform, this should block merging this PR.

It looks like KotlinJS for browser has a Crypto API that it can use. I think last time I checked, having a different implementation between Node and Browser was painful, so we might need to think about a solution there.

Thoughts on native?

js would be easy enough as you said. would probably have to use cinterop with these system apis: https://libsodium.gitbook.io/doc/generating_random_data ... or...

If you're not opposed to adding another dependency:
https://github.com/ionspin/kotlin-multiplatform-libsodium
would work for a secure random generator

or if you had a c library in mind to use with cinterop, checkout how I integrated building this c library libaes_siv for each target in my build.gradle for my project secretk

We don't want to bring in third party dependencies. cinterop with what's available on a native platform is reasonable, as that's effectively what we're doing with SecureRandom from the Java side.

Apple already has API’s for generating secure random data, and the existing native implementation done by Luca is probably sufficient to not need the Apple-specific API.

The hold up on merging this pull request is that it needs to be divided into smaller PRs that we can review. As one large PR, it will take a very long time to get through our review process since multiple security experts in different platforms need to be involved.

@ccjernigan I already published my own fork to my own repo so this really isn't my top priority atm. But I might be able to break this pr up as you requested soon. Has anything changed with how you want it done since: #14 (comment) ?

I saw your version got published a while back, and I'm glad you were able to leverage our project to help your own efforts move forward 🎉.

What I wrote before is still pretty accurate. A few minor notes since then:

We had another contributor move the tests from jvmTest to commonTest, so that part is done already.

I'd suggest starting with the JVM-only multiplatform refactor. It should effectively be a no-op, and I should be able to review/merge that very quickly.

For the native/JS targets, it might be faster to split that into a PR for each (one for native, one for JS) because then our team can review each independently.

JVM-only multiplatform refactor done #158

Thanks—I'm doing the review for the JVM-only changes ASAP.

There's a new JetBrains blog post relevant for which native targets we choose to configure: https://blog.jetbrains.com/kotlin/2023/02/update-regarding-kotlin-native-targets/

lib/src/jvmMain/kotlin/cash/z/ecc/android/random/SecureRandom.kt

lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/PBEKeySpec.kt

luca992 · 2022-04-21T21:03:27Z

Do you have a particular non-JVM platform that's especially important to you first? Knowing that can also help focus our discussion around 4 as well.

iOS is most important. Then probably macOS and linux. But if there's no limiting dependencies, personally I would just target everything, it's not any extra work.

luca992 · 2022-04-26T00:23:35Z

I'm confused. If I wanted to help contribute to your roadmap, should I make a new pr off of main. Or refactor this PR off of main?

java secure random for jvm /dev/urandom for native crypto.getRandomValues for js

MessageDigest for jvm okio for others

…able all native targets

Pbkdf2Sha512 jvm uses javax crypto as before Pbkdf2Sha512 non-jvm uses okio

luca992 · 2022-04-26T08:09:52Z

Think I fixed the majority of your concerns @ccjernigan take a look

…es are accepted KHash PR: komputing/KHash#23 kotlin-bip39: Electric-Coin-Company/kotlin-bip39#14

ccjernigan · 2022-04-26T20:59:34Z

I'm confused. If I wanted to help contribute to your roadmap, should I make a new pr off of main. Or refactor this PR off of main?

My suggestion would be:

Create a new PR off of main (since we just finished fixing up our build scripts). We do have a few additional minor changes pending review that you can see open, but they shouldn't create too many merge conflicts.
Split your original PR into multiple steps. I can quickly review and merge a single or two PRs with changes for these two items described originally:

Multiplatform tests: Port tests to be multiplatform, by dropping use of a JSON resource file. Move tests to commonTest source directory. Tests still only run on JVM, because no other targets added yet. This should be a quick extraction from this existing PR, once I get PR Modernize build #32 merged in the next few days.

Refactor for JVM-only multiplatform: Move most code into commonMain, add expect classes and methods, and add actual classes/methods in the jvmMain source directory. To library consumers, this should behave exactly as before but it sets up the ability to then add additional multiplatform implementations with minimal refactoring. This should also be a somewhat quick extraction from this PR.

For native and JS targets, the SecureRandom implementation is looking good. We'll still have to come up with a replacement for Okio that doesn't rely on a third party library. We'll also have to get a member of our DevSecOps team to participate in the review of the JS and Native platform code for the hashing and SecureRandom code. This will take more time.

I'm suggesting splitting up the work so that we can quickly merge the things that are low risk (multiplatform tests and a JVM-only multiplatform friendly refactoring) versus the JS and Native implementations for the security-related stuff.

luca992 and others added 7 commits April 3, 2022 00:19

update kotlin and dependencies

e7a3614

Begin port to multiplatform

f54c96c

Fixes. All tests passing jvm. update dokka

f86dbe9

Finish multiplatform implementation and make tests multiplatform

c4daa90

Todo: get non-jvm tests running. They use kotest. I can't get the tests to run on non-jvm for some reason

fix accidental infinite recursion. fixes non-jvm tests

5a3a5f0

update to 1.6.20

850ddc3

fix signing issue when publishing

cdc7084

use RELEASE_SIGNING_ENABLED=false More info: https://github.com/vanniktech/gradle-maven-publish-plugin#signing

luca992 added 3 commits April 21, 2022 14:22

update to 1.6.21 add iosSimulatorArm64 target

245f8e7

added some moko-resources related warning workarounds

Merge remote-tracking branch 'luca992/master'

8a5fb52

remove moko-resources, move test values to const string in commonTest

13716d9

ccjernigan self-requested a review April 21, 2022 19:09

ccjernigan reviewed Apr 21, 2022

View reviewed changes

remove now unused moko-resources generated webpack config

1072c46

luca992 added 7 commits April 26, 2022 01:00

match other zcash projects' gradle jvmargs

c4160de

revert to using CharArray for passphrase param in toSeed

9f9a40f

use CPRNGs for all targets

132da15

java secure random for jvm /dev/urandom for native crypto.getRandomValues for js

use expect actual for toSha256

a758860

MessageDigest for jvm okio for others

remove fluidsonic.locale dependency

6f2def9

add linuxX64 and all darwin targets. and gradle prop to enable or dis…

fc77c85

…able all native targets

remove okio from jvmMain. make Pbkdf2Sha512 expected

f8c4be2

Pbkdf2Sha512 jvm uses javax crypto as before Pbkdf2Sha512 non-jvm uses okio

add mingX64

b1ffe2f

luca992 added a commit to eqoty-labs/secretk that referenced this pull request Apr 26, 2022

temporarily include repository until multiplatform PRs for dependenci…

076d0d4

…es are accepted KHash PR: komputing/KHash#23 kotlin-bip39: Electric-Coin-Company/kotlin-bip39#14

luca992 mentioned this pull request Apr 29, 2022

Migrate to official kotlin-bip39 after mpp support eqoty-labs/secretk#4

Open

burnoo mentioned this pull request Sep 28, 2022

Move tests from jvmTest to commonTest #108

Merged

11 tasks

luca992 added 2 commits February 7, 2023 20:13

Merge branch 'main'

9537bdf

fix webpack config for js tests

af1269d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiplatform #14

Multiplatform #14

luca992 commented Apr 4, 2022

ccjernigan commented Apr 20, 2022

luca992 commented Apr 20, 2022

ccjernigan left a comment •

edited

Loading

ccjernigan Apr 19, 2022

luca992 Apr 21, 2022

ccjernigan Apr 21, 2022

ccjernigan Apr 21, 2022

luca992 Apr 21, 2022 •

edited

Loading

luca992 Apr 21, 2022 •

edited

Loading

luca992 Apr 21, 2022 •

edited

Loading

ccjernigan Apr 25, 2022

ccjernigan Feb 7, 2023

luca992 Feb 7, 2023 •

edited

Loading

ccjernigan Feb 7, 2023

luca992 Feb 8, 2023

ccjernigan Feb 8, 2023

luca992 commented Apr 21, 2022 •

edited

Loading

luca992 commented Apr 26, 2022

luca992 commented Apr 26, 2022

ccjernigan commented Apr 26, 2022

		@@ -1,3 +1,6 @@
		package cash.z.ecc.android.bip39

		const val BIP0039TestValues = """


		actual class SecureRandom {

		actual fun nextBytes(bytes: ByteArray){

Multiplatform #14

Are you sure you want to change the base?

Multiplatform #14

Conversation

luca992 commented Apr 4, 2022

ccjernigan commented Apr 20, 2022

luca992 commented Apr 20, 2022

ccjernigan left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

luca992 Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

luca992 Apr 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 commented Apr 21, 2022 • edited Loading

luca992 commented Apr 26, 2022

luca992 commented Apr 26, 2022

ccjernigan commented Apr 26, 2022

ccjernigan left a comment •

edited

Loading

luca992 Apr 21, 2022 •

edited

Loading

luca992 Apr 21, 2022 •

edited

Loading

luca992 Apr 21, 2022 •

edited

Loading

luca992 Feb 7, 2023 •

edited

Loading

luca992 commented Apr 21, 2022 •

edited

Loading