search

mvdan / bitw

Minimalist BitWarden client
BSD 3-Clause "New" or "Revised" License
169 stars 15 forks source link

Add organization support #44

Closed quexten closed 1 year ago

quexten commented 1 year ago

Adds basic support for organizations. I tested only dump so far, but also implemented support for serve. Fixes #19

Technical details

Each user profile has a private key. It is pkcs#8 encoded RSA. This key is encrypted with the accounts symmetric master key. The organizations stored in Profile.Organizations each have an encrypted symmetric master key, with which the organization ciphers are encrypted (or technically an encryption key as the first 32 bytes, and a mac key as the last 32 bytes).

The procedure is now as follows: With the master key, decrypt the private key. Save the private key in the secretCache. For each organization, decrypt the encrypted organization symmetric master key using the private key, and store them in a map in the secretCache.

To decrypt a cipher, the code then has to retreive the keys from the secret cache.

I used decryptWith instead of secrets.decrypt, since the cipherstring does not know it is encrypted with the organizations key, and thus secrets.decrypt uses the user's own keys.

In bitwarden's TypeScript clients, they actually pass the orgId in the decrypt call, which is how they know which keys to retrieve, should we do that here too? https://github.com/bitwarden/clients/blob/fbd0d41b518445502b7d09d15ad27c9daa44acee/libs/common/src/models/domain/enc-string.ts#L142-L161

quexten commented 1 year ago

As for test cases for this, as I understand the free official Bitwarden service offers 1 free organization with 2 users / 2 collections. So for the tests, we would just add an organization to the test accounts, and check whether a shared credential is visible in the output of dump / serve.

Any thoughts on this?

mvdan commented 1 year ago

What you say about testing sounds right. I've emailed you the test account secrets so you can run the tests and update the accounts to increase test coverage, e.g. by adding an org.

Note that CI is red, by the way, and it seems like a regression.

mvdan commented 1 year ago

Also, please rebase when you update the PR.

quexten commented 1 year ago

Thank you for the credentials for the test accounts. I have now cleaned up the API a bit (secrets.decrypt, secrets.decryptStr now have an optional parameter "orgID". This is similar to how it is done in the official Bitwarden typescript implementation).

Also, I have fixed the test run.

Furthermore, I have added a test organization to the test accounts, and have changed the dump script to look for an organization-owned credential.

On that note, the dump script also contained test vectors for Secure Note / Card Data and failed because of this, however the dump command does not even contain code for outputting these, so I'm guessing the test was outdated here. I have removed these.

Edit: Ok seems the latest commit actually broke the tests again. I'll investigate.

quexten commented 1 year ago

Ok, tests pass now.

quexten commented 1 year ago

I should note that I updated the data in "data-notfa.json" with the current data of the test account, including the organization data and one cipher. There are a lot of other changes between the JSON files, which is mostly due to equivalent domains not being stored in the JSON anymore.

Amolith commented 1 year ago

@mvdan any ETA on when this might be merged?