Feat: keychain rotate passphrase

[zeim839]

This is a rough implementation supporting rotating keychain passphrases. I need some suggestions on how to proceed. I haven't received a reply so I've copied the original conversation here for reference:

async changeKeychainPassword(oldPassword, newPassword){
    if (typeof oldPassword !== 'string' || typeof newPassword !== 'string') {
      throw new Error(`Invalid pass type '${typeof oldPassword}'`);
    }
    if (newPassword.length < 20) {
      throw new Error('pass must be least 20 characters')
    }
    // TODO: Need to decide on how to import/generate opts
    const newDek = newPassword
      ? crypto.pbkdf2(
        newPassword,
        this.opts.dek.salt,
        this.opts.dek.iterationCount,
        this.opts.dek.keyLength,
        this.opts.dek.hash)
      : ''
    const oldDek = privates.get(this).dek
    privates.set(this, { "dek":newDek })
    var keys = await this.listKeys()
    await keys.forEach(async key =>{
      // TODO: Decide how to handle deleting and importing the "self" key. 
          // importKey and removeKey throw an error when handling "self"
      if (key.name != "self"){
        var keyAsPEM = await this._getPrivateKey(key.name)
        await this.removeKey(key.name)
        this.importKey(key.name, keyAsPEM, oldDek)
      }
    })
  }

src/keychain/index.js

What's left is to decide how to handle the "self" key (this.importKey() and this.removeKey() throw an error on "self") and how to handle changing passwords in the context of src/index.js (see here). I'm trying to be cautious/minimal with the changes I make so I'd appreciate some suggestions on how to proceed.

[vasco-santos]

Thanks for the PR @zeim839

I think I addressed all the comments

[zeim839]

@vasco-santos Please take a look at the new changes and let me know what you think. Regarding tests, I've incorporated testing for validation but I'm not exactly sure how to go about the following:

it('can rotate keychain passphrase', async () => {  
    //TODO...
})

[zeim839]

I think I've come up with a possible solution. If its ok with you then everything should be good to go. I also decided to create a new keychain here (instead of using ks) to avoid influencing any other/future tests that dont use the new rotated password.

[vasco-santos]

I did an initial iteration on the PR. The new pm does not seem to be created with the new password. I left inline comments on how I think it would look like.

Also, before submitting the PR try to get the CI green please 🙏🏼 I added small inline suggestion to fix the lint, but I probably did not get to all of them. You should run both npm run lint and npm run test

Thanks @zeim839

[zeim839]

Also, before submitting the PR try to get the CI green please 🙏🏼 I added small inline suggestion to fix the lint, but I probably did not get to all of them. You should run both npm run lint and npm run test

Sorry about that, I shouldve read the contrib guidelines more carefully. Anyway, I've ran lint and fixed any issues that I could trace back to me but I'm not sure If I can even get the CI to green because linting fails regardless of my edits. I've fixed the mistakes from the previous review and wrote the test as described.

However, I keep getting "Error: Cannot read the key, most likely the password is wrong or not a RSA key" here when testing on browser, despite it working fine in test:node. Am I missing/overlooking something?

[vasco-santos]

Sorry about that, I shouldve read the contrib guidelines more carefully. Anyway, I've ran lint and fixed any issues that I could trace back to me but I'm not sure If I can even get the CI to green because linting fails regardless of my edits. I've fixed the mistakes from the previous review and wrote the test as described.

No problem, thanks for looking into it. I already submitted a suggestion fixing the lint. I will try to replicate the browser issue and get back to you

[vasco-santos]

Thanks for the new iteration. This is mostly done, I tried the changes I suggested below and I got everything to work.

Moreover, I added a lint skip for the error you had, as the linter was complaining for the ternary condition in the constant in the test, which is fine. When you add the suggestion, please check if the linting is still fine, given that the suggestion spaces could break the spacing.

Also, you will need to pull the changes done in the commit that fixed the linting
when you add the suggestions

[zeim839]

@vasco-santos Thanks for the help, I wasn't aware of that. I've removed the redundancy and everything works as expected now.

[vasco-santos]

LGTM!
Thanks for landing this @zeim839

[vasco-santos]

Released in 0.31.6

[zeim839]

@vasco-santos Thanks for all the help!