Skip to content
This repository has been archived by the owner on Oct 7, 2024. It is now read-only.

Provide method for unlocking keyring by decrypted key #147

Closed
wants to merge 29 commits into from

Conversation

darkwing
Copy link
Contributor

@darkwing darkwing commented Aug 11, 2022

Currently just an outline for for unlocking by decrypted key for MV3

@darkwing darkwing requested a review from a team as a code owner August 11, 2022 22:08
@darkwing darkwing marked this pull request as draft August 11, 2022 22:08
@darkwing darkwing changed the title WIP: Provide method for unlocking keyring by decrypted key Provide method for unlocking keyring by decrypted key Aug 16, 2022
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Show resolved Hide resolved
test/index.js Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
test/index.js Outdated Show resolved Hide resolved
async submitEncryptedKey(encryptedKey) {
this.keyrings = await this.unlockKeyrings(undefined, encryptedKey);
this.setUnlocked();
this.fullUpdate();
Copy link

Choose a reason for hiding this comment

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

I don't know what this part is doing, assuming some signaling that's not relevant to the security or lock/unlock process.

index.js Outdated Show resolved Hide resolved
index.js Outdated

// MV3: Since there's a new salt, we need to generate a new encrypted key
// for use in the
this.encryptedKey = await this._generateEncryptedKey(password, salt);
Copy link

Choose a reason for hiding this comment

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

We're overriding the encryptedKey field before going through the (asynchronous) process of preparing the new vault.
That leaves a possibility (because of bad timing or an error) for the key in this.encryptedKey to not be a key that can open the current vault.

It's not a big deal, worst case scenario it fails and forces the user to supply password again. Might be annoying if happening right after they supplied the password already.

Keeping the key in local variable and setting it in the same synchronous execution as the call to store the new vault would eliminate that gap.

And while we're at it - where/when is the key being saved to survive a restart? Will this flow save it too early? Why is it not saved in the same synchronous execution as the vault it unlocks?

@darkwing darkwing marked this pull request as ready for review September 22, 2022 21:05
@darkwing darkwing dismissed a stale review via b950ee6 September 26, 2022 16:35
index.js Outdated

// MV3: Generates the encrypted key
async _generateEncryptionKey(password, salt) {
return toHex(sha256(utf8ToBytes(password + salt)));
Copy link
Member

Choose a reason for hiding this comment

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

Why are we using ethereum-cryptography/sha256 here instead of the encryptor?

Copy link
Contributor

Choose a reason for hiding this comment

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

@darkwing I know this code came from the branch I pushed up, but you might have missed this comment in our DM with Zb:

I think the shim could be used for stubbing random salt with predictable values and asserting for it.
Also, getting a dependency instead using a feature in the browser is not only a detriment to maintainability but also performance 🤫

So we should go back to your old implementation that uses the native crypto api directly. But as per Mark's comment, we can probably just use the encryptor itself

Copy link
Contributor

@danjm danjm Sep 27, 2022

Choose a reason for hiding this comment

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

to be more specific, you could use encryptor.keyFromPassword(password, salt)

Copy link
Contributor

@danjm danjm Sep 27, 2022

Choose a reason for hiding this comment

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

But, we might want to consider how we can more fully leverage browser passworder and push more logic down to that... I'll comment more on that before the end of the day

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried doing the following:

async _generateEncryptionKey(password, salt) {
    //return toHex(sha256(utf8ToBytes(password + salt)));
    const key = await encryptor.keyFromPassword(password, salt);
    const exportedKey = await window.crypto.subtle.exportKey('jwk', key);
    const keyString = new Uint8Array(exportedKey).toString();

    console.log("key is: ", key, exportedKey);
    console.log("keystring is: ", keyString);
    return key;
  }

Result being:

AttemptedKeyExport

The string form of extracted key is empty. I don't think this helps us.

Copy link
Member

Choose a reason for hiding this comment

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

It's not totally clear what you're trying to do in that snippet, or how it relates to this thread. But that is failing because you're asking for the key to be exported as a JWT, which is an object. Then you're passing the object into the Uint8Array constructor, which isn't meant to accept that sort of object as input. It tries to iterate over it but it's not iterable, giving you an empty arraybuffer, thus the empty string.

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants