Multiplatform - Native #162

luca992 · 2023-02-10T16:14:58Z

Added my native implementation targeting all tier 1-3 native targets.

Personally I don't agree with having a NATIVE_TARGETS_ENABLED toggle. But up to you.

Not sure why detekt fails for Pbkdf2Sha512 in the nonJvmMain due to Pbkdf2Sha512.F:

> Task :detektAll FAILED
/Users/lucaspinazzola/Projects/kotlin-bip39/bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/Pbkdf2Sha512.kt:54:17: Function names should match the pattern: [a-z][a-zA-Z0-9]* [FunctionNaming]

But succeeds in commonMain. I renamed both to Pbkdf2Sha512.f for consistency

Edit: linuxArm64 target wasn't added due to okio not supporting it.

ccjernigan

Thanks for the effort that went into creating this PR. The changes are clear, and it is nice to have various native targets. I appreciate the care that went into generating secure random data in platform-specific ways.

I've looked at this from the general Kotlin/Gradle code review, clarity, maintainability, etc. perspective, and there are some questions inline. Another member of our team will be looking at this from a security perspective, and will chime in over the next week or so.

From my perspective, the main thing that we need to figure out is how we can reduce any third party dependency (e.g. Okio). It's great that we don't introduce dependencies in the Android targets, and I hope we can build on that.

I added a followup issue for CI jobs for other platforms, as I see that as complementary but out of scope for this initial PR. Relatedly, we may want to explicitly disable native release publishing until we get the CI jobs properly set up (we'll leave the publishing on for snapshots). That would mean adding
ORG_GRADLE_PROJECT_NATIVE_ENABLED: false
here.

ccjernigan · 2023-02-10T18:10:03Z

bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/Pbkdf2Sha512.kt

+            bU[s.size + j] = (i shr (24 - 8 * j)).toByte()
+        }
+
+        val uXor = bU.toByteString().hmacSha512(key).toByteArray()


What options do we have for hmacSha512 that doesn't rely on Okio?

#162 (comment)

ccjernigan · 2023-02-10T18:11:08Z

bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/bip39/ToSha256.kt

+
+import okio.ByteString.Companion.toByteString
+
+internal actual fun ByteArray.toSha256(): ByteArray = this.toByteString().sha256().toByteArray()


What options to we have to implement sha256 without Okio (or other third party dependencies)?

#162 (comment)

ccjernigan · 2023-02-10T18:16:49Z

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

-    internal var cachedList = WordList()
+
+    @Suppress("VARIABLE_IN_SINGLETON_WITHOUT_THREAD_LOCAL")
+    var cachedList = WordList()


Could we make this a val, which might help avoid the need for a @Suppress?

that would break the actual purpose of it being a var:

var cachedList = WordList() fun getCachedWords(languageCode: String): List<String> { if (cachedList.languageCode != languageCode) { cachedList = WordList(languageCode) } return cachedList.words }

... I think I removed internal by accident. I'll fix that. However it could also be made private. It's not used anywhere else.

also the suppress isn't technically needed. But without it, it will print a bunch of info level logs while building saying:
With old Native GC, variable in singleton without @ThreadLocal can't be changed after initialization

Which is just annoying considering kotlin uses the new GC by default now

Thanks—reviewing just on GitHub hides some of the important context!

bip39-lib/build.gradle.kts

bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/PBEKeySpecCommon.kt

bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/Pbkdf2Sha512.kt

luca992 · 2023-02-11T04:45:48Z

these are okios implementations. I guess they could be copied. I don't necessarily agree with the no dependencies rule you have, as java's standard library is pretty massive (essentially they already bundle things that would be a dependency in like any other language).

Plus rolling your own crypto is something I avoid; I guess if you have the team to maintain it that works. But at that point honestly I would make a new library or module and import it into this project. That way other people can at least benefit from the code if you plan on maintaining it. If every project made everything self contained it would lead to tons of code duplication. I do get you want to keep it as concise as possible though.

hashben228 · 2023-02-16T19:40:35Z

bip39-lib/src/mingwMain/kotlin/cash/z/ecc/android/random/SecureRandom.kt

+actual class SecureRandom {
+    actual fun nextBytes(bytes: ByteArray) {
+        val result = bytes.usePinned {
+            @Suppress("UNCHECKED_CAST")


What errors occur if this @Suppress is removed? Are there consequences to making the value nullable instead of suppressing the unchecked cast?

> Task :bip39-lib:compileKotlinMingwX64 FAILED e: warnings found and -Werror specified w: file:///Users/lucaspinazzola/Projects/kotlin-bip39/bip39-lib/src/mingwMain/kotlin/cash/z/ecc/android/random/SecureRandom.kt:17:33 Unchecked cast: CPointer<ByteVar /* = ByteVarOf<Byte> */> to CPointer<UByteVar /* = UByteVarOf<UByte> */> Execution failed for task ':bip39-lib:compileKotlinMingwX64'. > Compilation finished with errors

it's supposed to be a UByte not a Byte for mingw. The suppress isn't for null-ablity

update to gradle 8.0.1 and updates locks using procedure in `dependency.md`

results failing native tests

luca992 · 2023-02-21T06:52:27Z

Turns out the native tests weren't running at all. Fixed that by adding and applying the kotest plugin.

And that led me to find out there was an issue with the require statement in the native nextBytes implementation. Had to change the check: check(result < 0) -> check(result >= 0)

Also, I updated gradle to 8.0.1

luca992 · 2023-02-21T06:53:38Z

I think I've fixed everything that needs to be fixed. Up to you guys on how you want to handle hmacSha512 and sha256

ccjernigan

Thanks for these enhancements. I have a few minor comments inline, and I'm about to trigger the CI build.

gradle/wrapper/gradle-wrapper.properties

ccjernigan · 2023-02-21T18:38:26Z

bip39-lib/src/nonJvmMain/kotlin/cash/z/ecc/android/crypto/Pbkdf2Sha512.kt

-            }
-            return ByteArray(dkLenBytes).apply {
-                baos.read(this)
+        val dkLenBytes = dkLen / 8


Thanks for rolling back use of Okio here. This minimizes the surface area of using third parties, and will make it easier to swap out Okio with something else in the future since its now just the hashing functions.

ccjernigan · 2023-02-21T18:45:43Z

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

-    internal var cachedList = WordList()
+
+    @Suppress("VARIABLE_IN_SINGLETON_WITHOUT_THREAD_LOCAL")
+    var cachedList = WordList()


Thanks—reviewing just on GitHub hides some of the important context!

luca992 · 2023-02-22T04:18:50Z

found another option for the hashing:
https://github.com/korlibs/korge/tree/main/krypto

ccjernigan · 2023-03-01T12:22:28Z

@luca992 Thanks for your patience. We are still having internal security auditing discussions, and we hope to wrap those up soon. As a preview, this where I think we will land:

Merge this PR as-is (I might ask to tweak the readme slightly because it's explicit about no dependencies which doesn't hold true with this PR). This PR moves our multiplatform efforts forward in a positive way in the codebase, while native deployments are still initially disabled.
Create a followup issue to swap out Okio. We're leaning towards copying the necessary hashing functions from Okio. Then we can own and audit those as our own implementation going forward.
Enable deployment of native targets on CI

No action items yet, just keeping you in the loop as we're making decisions internally.

ccjernigan · 2023-05-11T18:08:47Z

@luca992 Although it took a while, I got internal approval for the plan discussed above #162 (comment)

Once we resolve the conflicts, then I can merge this PR 🎉

https://pinterest.github.io/ktlint/0.49.0/rules/experimental/#string-template-indent

luca992 · 2023-05-11T21:04:12Z

@ccjernigan I updated kotlin and all dependencies. If you don't want that lmk and I'll revert it

ccjernigan · 2023-05-12T14:48:56Z

@ccjernigan I updated kotlin and all dependencies. If you don't want that lmk and I'll revert it

We don't mind updating dependencies, although I would recommend a separate PR for the dependencies. When I merge this PR, it will be squashed into a single commit. For dependencies, Kotlin 1.8.21 is good to go, and we've updated our other repos as well. (We're sticking with Gradle 7.6 until we resolve an issue with the Rust integration in another project though).

We still have merge conflicts with this PR—I think the WordList changes from the main branch need to be brought in which incorporates a significant performance improvement.

ccjernigan

Thank you for the effort that went into this PR, as well as your patience during our review process. This brings some great enhancements for the project and the ecosystem.

ccjernigan · 2023-05-18T12:46:01Z

I created a new PR that incorporated these changes while resolving the remaining conflict. These changes have been merged. See #188

luca992 · 2023-05-18T13:49:55Z

Np. Now I'll make that last pr for JS 👌

luca992 force-pushed the multiplatform-native branch 2 times, most recently from ea7f8e8 to 6b7090c Compare February 10, 2023 16:52

add native implementation

fd6021f

luca992 force-pushed the multiplatform-native branch from 6b7090c to fd6021f Compare February 10, 2023 16:54

ccjernigan mentioned this pull request Feb 10, 2023

Add CI jobs for Apple and Windows targets #163

Open

ccjernigan reviewed Feb 10, 2023

View reviewed changes

hashben228 reviewed Feb 16, 2023

View reviewed changes

luca992 added 7 commits February 20, 2023 23:22

configure native source sets only when enableNative==true

004f0b1

refactor PBEKeySpecCommon for more idiomatic kotlin

4e9a5f9

PBEKeySpecCommon: make salt not nullable

8667b13

fix to run native tests by adding kotest plugin. Update gradle and locks

c190eb8

update to gradle 8.0.1 and updates locks using procedure in `dependency.md`

fix nextBytes implementation for native. result should be >= 0

97149a0

results failing native tests

revert to internal as it was before

285ce7b

remove buffer use by allocating an array of appropriate size copying in

2a8812a

remove implementation documentation from expect object Pbkdf2Sha512

f31a79b

ccjernigan reviewed Feb 21, 2023

View reviewed changes

revert to gradle 7.6

19ad498

hashben228 approved these changes Feb 27, 2023

View reviewed changes

luca992 added 2 commits May 11, 2023 17:01

update to kotlin 1.8.21 and all other dependencies to latest

9497e81

conform WordList multiline string to new ktlint rule

6b829e8

https://pinterest.github.io/ktlint/0.49.0/rules/experimental/#string-template-indent

Merge branch 'main' into multiplatform-native

c615f0e

hashben228 approved these changes May 16, 2023

View reviewed changes

ccjernigan approved these changes May 18, 2023

View reviewed changes

ccjernigan mentioned this pull request May 18, 2023

Add native implementation #188

Merged

9 tasks

ccjernigan closed this May 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiplatform - Native #162

Multiplatform - Native #162

luca992 commented Feb 10, 2023 •

edited

Loading

ccjernigan left a comment

ccjernigan Feb 10, 2023

luca992 Feb 11, 2023

ccjernigan Feb 10, 2023

luca992 Feb 11, 2023

ccjernigan Feb 10, 2023

luca992 Feb 21, 2023

luca992 Feb 21, 2023 •

edited

Loading

ccjernigan Feb 21, 2023

luca992 commented Feb 11, 2023 •

edited

Loading

hashben228 Feb 16, 2023

luca992 Feb 21, 2023

luca992 Feb 21, 2023

luca992 commented Feb 21, 2023 •

edited

Loading

luca992 commented Feb 21, 2023

ccjernigan left a comment

ccjernigan Feb 21, 2023

ccjernigan Feb 21, 2023

luca992 commented Feb 22, 2023 •

edited

Loading

ccjernigan commented Mar 1, 2023

ccjernigan commented May 11, 2023

luca992 commented May 11, 2023

ccjernigan commented May 12, 2023

ccjernigan left a comment

ccjernigan commented May 18, 2023

luca992 commented May 18, 2023


		import okio.ByteString.Companion.toByteString

		internal actual fun ByteArray.toSha256(): ByteArray = this.toByteString().sha256().toByteArray()

Multiplatform - Native #162

Multiplatform - Native #162

Conversation

luca992 commented Feb 10, 2023 • edited Loading

ccjernigan left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 commented Feb 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 commented Feb 21, 2023 • edited Loading

luca992 commented Feb 21, 2023

ccjernigan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luca992 commented Feb 22, 2023 • edited Loading

ccjernigan commented Mar 1, 2023

ccjernigan commented May 11, 2023

luca992 commented May 11, 2023

ccjernigan commented May 12, 2023

ccjernigan left a comment

Choose a reason for hiding this comment

ccjernigan commented May 18, 2023

luca992 commented May 18, 2023

luca992 commented Feb 10, 2023 •

edited

Loading

luca992 Feb 21, 2023 •

edited

Loading

luca992 commented Feb 11, 2023 •

edited

Loading

luca992 commented Feb 21, 2023 •

edited

Loading

luca992 commented Feb 22, 2023 •

edited

Loading