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

react native support #819

Open
wants to merge 23 commits into
base: master
from

Conversation

@sirpy
Copy link
Contributor

sirpy commented Sep 17, 2019

using isomorphic-webcrypto which uses @peculiar/webcrypto for node.
it also uses msrcrypto for environments without webcrypto (ie react native)
and also webcrypto-liner and asmcrypto under the hood to add missing support to pbkdf2
some SEA methods required to change syntax
example react-native app on expo that uses this branch with some SEA tests
https://expo.io/@sirpy/gun-sea-test
expo code here:
https://github.com/GoodDollar/gun-expo-react-native

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Sep 17, 2019

@mhelander can you review this?
i've also added a test for a failing conversion case, so SEA currently doesn't pass unit tests.

@amark i've change package.json browser: to brower.js which exports Gun+SEA since expo.io didnt support the syntax of importing sea.js from 'gun/sea'.
so we can revert this change or check possible usage of browser.ios.js and browser.android.js for different platforms

sirpy added 2 commits Sep 17, 2019
sea.js Show resolved Hide resolved
sea.js Outdated
@@ -168,24 +171,31 @@

if(SEA.window){
api.crypto = window.crypto || window.msCrypto;
if(!api.crypto) {
api.crypto = require('isomorphic-webcrypto');
}

This comment has been minimized.

Copy link
@mhelander

mhelander Sep 18, 2019

Collaborator

I might have combined statement here:

api.crypto = window.crypto || window.msCrypto || require('isomorphic-webcrypto');

This comment has been minimized.

Copy link
@sirpy

sirpy Sep 20, 2019

Author Contributor

yeah I originall tried that, but I think expo has issues with this syntax.
I will try again.

sea.js Outdated Show resolved Hide resolved
sea.js Show resolved Hide resolved
sea.js Outdated Show resolved Hide resolved
sea.js Show resolved Hide resolved
sea.js Outdated Show resolved Hide resolved
@@ -563,7 +582,7 @@
bufiv = shim.Buffer.from(json.iv, opt.encode || 'base64');
bufct = shim.Buffer.from(json.ct, opt.encode || 'base64');
var ct = await aeskey(key, buf, opt).then((aes) => (/*shim.ossl ||*/ shim.subtle).decrypt({ // Keeping aesKey scope as private as possible...
name: opt.name || 'AES-GCM', iv: new Uint8Array(bufiv)
name: opt.name || 'AES-GCM', iv: new Uint8Array(bufiv), tagLength: 128

This comment has been minimized.

Copy link
@mhelander

mhelander Sep 18, 2019

Collaborator

Should this magic constant 128 became from SEA constants/module props?

This comment has been minimized.

Copy link
@sirpy

sirpy Sep 21, 2019

Author Contributor

dono, It didnt work in react-native (ie msrcrypto lib) so i looked at their examples and it was used there 128.

sea.js Outdated Show resolved Hide resolved
sea/aeskey.js Outdated Show resolved Hide resolved
sea/buffer.js Show resolved Hide resolved
sea/decrypt.js Show resolved Hide resolved
sea/secret.js Show resolved Hide resolved
sea/settings.js Show resolved Hide resolved
sea/shim.js Show resolved Hide resolved
sea/shim.js Outdated Show resolved Hide resolved
sirpy added 3 commits Nov 4, 2019
update to master
@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 4, 2019

@amark how about merging this PR?

@skiqh

This comment has been minimized.

Copy link

skiqh commented Nov 4, 2019

So is decrypt(encrypt("4e2")) === 400 desireable by design? Seems strange to me. If I encrypt a string, I want a string back, no?

The source of the current (falsy) behaviour is in

var msg = (typeof data == 'string')? data : JSON.stringify(data);

That typeof check leaves actual strings as is before ecryption, but stringifies all non-strings. However, the decrypt function uses JSON.parse indiscriminately for all data, so effectively, the type info is lost in the process.

So currently, "4e2" is saved as "4e2" (a string containing 4e2) instead of "\"4e2\"" (a string containing the string "4e2"). The former will be JSON-parsed to 4e2 (aka 400, a number), the latter will be parsed as "4e2" (the string with the chars 4, e, 2).

So I'd recommend not changing the tests, but changing the behaviour of the encrypt method to always use JSON.stringify regardless of input type.

@amark

This comment has been minimized.

Copy link
Owner

amark commented Nov 4, 2019

@skiqh that is a bug, correct.

Oh thanks for me reminding me about this PR. Not sure how to go about the merge tho, maybe @mmalmi can help me?

We avoided stringifying strings because JSON produces really bad nested escapes that we had to fix last time. I wonder if we can case detect this without losing performance?

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 5, 2019

@skiqh as you can see i've capitalized the test and called it DOESNT DECRYPT SCIENTIFIC NOTATION. i wanted it pass the build.

@amark what issues do you see with merging?

@amark

This comment has been minimized.

Copy link
Owner

amark commented Nov 5, 2019

@sirpy it looks great!

Just need to also confirm it works in Browser & NodeJS etc. especially with our recent change to @peculiar/webcrypto from ossl, I thought isomorphic was IE & React only?

Have you already seen it working there as well?

Then a few of the other changes I want to verify it doesn't break any existing old data or shared keys on NAB or other apps, etc.

And (this is not necessary for merge) @mhelander already shimmed Buffer in SEA, was there something not working with it? That we can keep the internal one, versus an external dep? Will pull regardless, cause can always refactor it in later.

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 5, 2019

I want iris-lib to work both on node & browser as well as RN. I guess I would need to do
import {Gun, SEA} from 'gun/browser.ios' in iris-lib then? Is there any smarter way?

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 6, 2019

@amark I dont recall exactly why, but something needed buffer, I think maybe some circular dependency that couldnt use SEA buffer shim.

isomorphic uses peculiar webcrypto inside, it uses multiple crypto packages in order to make it work on node,web,react-native.

nothing in the data structures or crypto has changed in this PR only minor syntax changes, all tests are passing.

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 6, 2019

@mmalmi on react native you should be importing like previously
import {Gun, SEA} from 'gun'
it is supposed to import, hopefully from browser.ios/android.js automatically
on web you import as usuall import Gun from 'gun' and it will import it from browser.js

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 6, 2019

I would like to use the same iris-lib version on node & RN, so there can be only one version of the import statement. Maybe I could do import Gun from 'gun/gun' and import SEA from 'gun/sea'?

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 6, 2019

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 6, 2019

Another thing: In browser I'm getting

sea.js:7 Uncaught ReferenceError: global is not defined
    at sea.js:7
    at sea.js:1330
@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 6, 2019

Another thing: In browser I'm getting

sea.js:7 Uncaught ReferenceError: global is not defined
    at sea.js:7
    at sea.js:1330

can you share your complete import code?

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 7, 2019

iris-angular:

<script src="../bower_components/gun/gun.js"></script>
<script src="../bower_components/gun/sea.js"></script>
@amark

This comment has been minimized.

Copy link
Owner

amark commented Nov 13, 2019

is this my fault? I don't use global except typeof which is safe.

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 14, 2019

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 14, 2019

This? a2a7ef9#diff-834a891c1d9e580bf6e061f63d911855

Yeah i guess this requires fixing.
I just remembered that SEA ./buffer requires atob which isnt available in react-native and atob requires buffer, so that's why we need to import external buffer

@amark

This comment has been minimized.

Copy link
Owner

amark commented Nov 15, 2019

So you're saying React Native does NOT have Buffer or atob/btoa?

If it has Buffer, then we can use that to do atob/btoa.

If it does not have Buffer, then @mhelander 's included-inside SEA shim for Buffer should work if atob/btoa available.

If it has neither, oye! Then...

I presume the Cryptography shim would have it tho, yes? So maybe we can pull it from there instead?

sirpy added 2 commits Nov 21, 2019
@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 21, 2019

@amark @mhelander
I've updated the Buffer import, please test now

@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 27, 2019

@amark can you check this out now?

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 28, 2019

In browser:

sea.js:52 Uncaught ReferenceError: require is not defined
    at sea.js:52
    at sea.js:12
    at sea.js:58
    at sea.js:1330

sea.js:51-53:

    if(typeof Buffer === "undefined") {
      root.Buffer = require("buffer").Buffer
    }
@sirpy

This comment has been minimized.

Copy link
Contributor Author

sirpy commented Nov 29, 2019

@mhelander ok how about now:)

@mhelander

This comment has been minimized.

Copy link
Collaborator

mhelander commented on b2709e2 Nov 29, 2019

This to my understanding works under NodeJS control. How well it plays in browser and/or transpilers, dunno.

@mmalmi

This comment has been minimized.

Copy link
Collaborator

mmalmi commented Nov 29, 2019

Seems to work in browser now 👍 Didn't re-test in react native yet.

@amark

This comment has been minimized.

Copy link
Owner

amark commented Dec 8, 2019

OK, thanks if it is working in browser, @mmalmi and I'm guessing still works in native @sirpy. I'd like to clone this this week and see if I can finally merge, I apologize it has taken so long, and this next week is pretty busy for me, so no promises. I certainly have been pointing everybody to this thread and recommending sirpy's work, so I'm super grateful for the contribution. :)

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