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

feat: move to shared components #666

Merged
merged 44 commits into from
Jun 23, 2023
Merged

feat: move to shared components #666

merged 44 commits into from
Jun 23, 2023

Conversation

TimoGlastra
Copy link
Contributor

Summary of Changes

Updates Bifold to use the new shared component libraries (anoncreds-rs, aries-askar, indy-vdr) and updates AFJ to 0.4.0.

Draft for now until we add the migration script handling and some last released in AFJ have been made

karimStekelenburg and others added 5 commits March 2, 2023 20:54
Signed-off-by: Karim Stekelenburg <karim@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra requested a review from a team as a code owner March 17, 2023 13:24
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra TimoGlastra marked this pull request as draft March 17, 2023 14:44
core/App/contexts/auth.tsx Outdated Show resolved Hide resolved
core/App/contexts/auth.tsx Outdated Show resolved Hide resolved
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
Signed-off-by: Timo Glastra <timo@animo.id>
@TimoGlastra
Copy link
Contributor Author

Why isn't it migrated?

I didn't take it into account. It's not something I've seen before, and the logic wasn't fully known to me.

though it was just a normal wallet with a PIN and if the user PIN unlocks it then we know the PIN is valid

correct

Or maybe I should ask what makes it custom?

Having a separate wallet to verify the pin. In our wallets we always just tried to open the wallet and if we get a WalletInvalidKeyError (or something like that, forgot the name), we show to the user the pin is incorrect.

Is this a trivial fix are are we in trouble 💀 .

It's quite trivial. We have to migrate the custom pin database in addition to the normal wallet database

@jleach
Copy link
Contributor

jleach commented Jun 7, 2023

@TimoGlastra Sorry to belabour the point, but, our PIN or Biometric has derived from it is stored in the device keychain. Not in the Wallet. Maybe we just need to do something in the keychain to "upgrade" it?

@TimoGlastra
Copy link
Contributor Author

@jleach
Copy link
Contributor

jleach commented Jun 8, 2023

@jleach see this code: https://github.com/hyperledger/aries-mobile-agent-react-native/blob/feat/upgrading-to-AFJ-040/packages/legacy/core/App/contexts/auth.tsx#L96

It is also there in main, but using indy sdk in askar.

Indeed, our approach may seem unusual at first, but rest assured, there's a solid reason behind it. Different assurance levels (I think its a NIST standard) exist for wallets. For instance, when a wallet key is secured behind biometrics, it is considered high-assurance. This is because it takes advantage of the device's secure enclave - a dedicated region in the processor designed to safeguard data. If we were to store the derived key in the keychain outside of biometrics that would drop the wallets assurance level. What we do to make everything legit is store the salt in the keychain, derive the key on the fly with argon2(PIN, salt, {}), then, ensure that the derived key is authentic by using it to unlock the wallet.

@jleach
Copy link
Contributor

jleach commented Jun 8, 2023

@TimoGlastra I think I have a fix for this. I need to clean it up and test some more but I'll add i back to this PR when ready.

@TimoGlastra
Copy link
Contributor Author

@jleach left a comment on your PR, but otherwise LGTM

Co-authored-by: Bryce McMath <32586431+bryce-mcmath@users.noreply.github.com>
Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
@TimoGlastra
Copy link
Contributor Author

@jleach I just added 8a28f19 (#666), which should fix the issue I meant.

I don't have a device at hand to test, could you take a look?

@jleach
Copy link
Contributor

jleach commented Jun 13, 2023

@jleach I just added 8a28f19 (#666), which should fix the issue I meant.

I don't have a device at hand to test, could you take a look?

I think it will work but I still think there is some confusion. We only have one wallet so we just need state tracked in one place because it can get upgraded in either place, but not both. Print the wallet ID and there will only be one in the app walletId:

If I print out the wallet ID which is the same in both places I get this from auth.tsx L97.

 LOG  Auth wallet ID =  walletId

And this from Splash.txs L215:

 LOG  Splash wallet ID = walletId

Because they have the same ID they're the same wallet. I think the comment in auth.txs is confusing because it says a "cutom wallet" bit its not - same ID means same wallet. No?

And yes, we should have used a better wallet ID that that 🫤

@TimoGlastra
Copy link
Contributor Author

Ah thanks for clarifying. I messed this up then 😅

However we need to be careful here, as this will also initialize the agent now, which means there will also be an auto update of storage, for which all modules must be enabled (so the minimal agent that is created in the migration misses some modules that need migration)

So we need to initialize the full agent, or we need to run just the askar migration script on the wallet.

@jleach
Copy link
Contributor

jleach commented Jun 14, 2023

@TimoGlastra Should we back-out your last changes and add any missing modules or leave 'em? I think we're pretty close to being done. Also, I had a look at the shared code to migrate and it does not seem to care about modules or parameters.

@TimoGlastra
Copy link
Contributor Author

I was more talking on the JS side of things and the agent auto updating storage. It seems that the migration script doesn't call intialize on the agent and this we should be fine.

I think we should set https://github.com/hyperledger/aries-mobile-agent-react-native/blob/feat/upgrading-to-AFJ-040/packages/legacy/core/App/utils/migration.ts#L29 to false, as that can lead to issues if behavioir in afj were to change.

But otherwise we can revert my change and we should be good to go.

Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
@jleach jleach force-pushed the feat/upgrading-to-AFJ-040 branch from d5fcd11 to 83e5daf Compare June 15, 2023 22:17
@jleach
Copy link
Contributor

jleach commented Jun 15, 2023

@TimoGlastra Done. I reverted your change, made the additional change you suggested, and merged main in to resolve conflicts. Tested quite a bit today, especially upgrading from AFJ 0.3.3 to 0.4.0 and it seems solid. We'll merge into main in the next day or so. Nice work!

@TimoGlastra
Copy link
Contributor Author

Thanks @jleach -- that is great to hear. Super excited about this :)

@TimoGlastra TimoGlastra marked this pull request as ready for review June 22, 2023 09:24
Signed-off-by: Jason C. Leach <jason.leach@fullboar.ca>
@jleach jleach enabled auto-merge (squash) June 23, 2023 19:49
Copy link
Contributor

@bryce-mcmath bryce-mcmath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work everyone

@jleach jleach merged commit b53e115 into main Jun 23, 2023
@jleach jleach deleted the feat/upgrading-to-AFJ-040 branch June 23, 2023 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants