starekrow / lockbox

Encrypted storage with built-in key management facilities

MIT License

95 stars 6 forks source link

Closed KJLJon closed 6 years ago

KJLJon commented 6 years ago

Issue #5 and #11

starekrow commented 6 years ago

Nice! There are a couple of changes I'd like to see, though, that touch a lot of those pieces.

I think the salt belongs with the data, not the key. This has the advantage of removing all that salt handling from the Vault API and allows CyptoKey to just generate a random salt for each encrypted item. My understanding is that there's no security advantage to keeping the salt secret.
Also, I was thinking that we should probably switch the default cipher to AES-128-CFB with this key version bump. There should no issue with weak IVs, and it's a less problematic mode (padding or not, etc).

KJLJon commented 6 years ago

I'm working on storing the salt inside the vault. I am hoping to have this fixed before the weekend :)

starekrow commented 6 years ago

Wouldn't it be easier to just add the salt to the output of lock()?

KJLJon commented 6 years ago

@starekrow great recommendation. I think I was thinking about it too hard.

Your recommendation cleans it up a lot, and is much easier to implement :smile:

Anyways, I have added another commit to this PR, and changed it to use the salt in lock() and unlock()

KJLJon commented 6 years ago

i think #31 should be implemented with the key version bump and before the tagged 1.0.0 version

starekrow commented 6 years ago

31 is independent of key version - the key version should only affect the operation of CryptoKey, so implementation details in Secret are out of its scope.

The PR looks good, with one nitpick: getNumericVerison is a typo.

KJLJon commented 6 years ago

typo fixed.

and yea I even knew that the key version was independent of Secret, idk what I was thinking when I commented :smile: