Encrypted storage

[aesedepece]

A basic implementation of the encrypted storage (see #24, #25, #26)

The beef is in the Vault class invault.ts. It wraps LevelDB (levelup + leveldown) with AES symmetric encryption/decryption.

This PR is not yet to be merged in its current form. It first needs to be rebased because of #32 and potentially #31.

[anler]

I'm ok with this implementation: interface + class but I'll show you this approach that I prefer and imho is more consonant with how the rest of the module is defined (functions that operate on data):

// type alias defining the shape of the options object
type Options = {
  algorithm: string,
  digest: string,
  iterations: number,
  iv: Buffer,
  ivBytes: number,
  keyBytes: number,
  password: string,
  salt: Buffer,
  saltBytes: number
}

export const defaultOptions: Partial<Options> = {
    algorithm: "aes-256-cbc",
    digest: "sha256",
    iterations: 100000,
    ivBytes: 16,
    keyBytes: 32,
    saltBytes: 32
}

export function updateOptions(opts: Options, newOpts: Partial<Options>): Options {
  return Object.assign(opts, newOpts)
}

export function getKey(opts: Options): Buffer {
  const {password, salt, iterations, keyBytes, digest} = opts // <- now these are typechecked to not be null/undefined
  return crypto.pbkdf2Sync(password, salt, iterations, keyBytes, digest)
}

[aesedepece]

I'm just a little afraid that the suggested pattern may be harder to grasp for newcomers and eventual contributors.
Nonetheless, I totally love this approach and at the moment I see no other reason for not adopting app-wide. I'll rewrite it.

[anler]

what happens if this.ivBytes and this.saltBytes is null?

[aesedepece]

By that point (line 56), this.ivBytes and this.saltBytes just can't be null as they are ORed against default values earlier in the constructor. This code is actually safer than it may seem at first sight.

However, I understand that your concern is about the constructor being modified in the future in any way that could break the type safety of this.ivBytes and this.saltBytes, for example if they lose their default values.

[anler]

Here the same, I'm totally ok with it, although I'm more fan on this alternative implementation:

1. Define the interface I'm going to use in the rest of the program to access the key-value store engine, for example (the name of the interface is irrelevant, is just the one that first came to mind):

interface IStore {
  put(key: string, value: any): Promise<boolean>;
  get(key: string): Promise<any>;
  close(): void
}

2. Define a concrete implementation for that interface, in our case it would be a leveldb-backed implementation:

class Vault implements IStore {
  constructor(private name: string, private db: any, private cryptOptions)
  // implementation of methods
  // ....
}

where db is already an active connection to leveldb

3. Define a factory function that is responsible of creating an instance of the concrete implementation for IStore:

function createLevelDbVault(name: string, password: string, path?: string): Promise<Vault> {
  return new Promise(async resolve => {
    // ...
    // This causes the `this.ready` promise to resolve.
    resolve(new LevelDbVault(name, db, cryptOptions))
  })
}

The reason to use a factory function instead of putting that logic inside the class constructor is to avoid having a component with too much responsibility, if for some reason the way the db connection is created is enriched, all we need to do is create a new factory function to accommodate the change.

The advantage of this model is that now, whenever I'm testing a component of the application that needs an IStore value, I can pass one more suitable for testing, e.g. one that keeps the data in-memory.

[aesedepece]

I like your way of thinking...

Same as before, I'll give a try to your suggested pattern. I find it indeed to be quite idiomatic. Forgive me if I got carried away too much by the object-oriented nature of TypeScript 🤣

[anler]

In #32 I added the modulePath option to jest config to be able to write this line as:

import {Vault} from "app/lib/storage/index"

[aesedepece]

This is just because WebStorm doesn't give a ʞɔnɟ for any jest configuration found in package.json, so it keeps flagging everything from the test file up to the project root with a nasty red underline. I'll try to force WebStorm to shut up and leave us alone.

[anler]

crap, that's a pity, I'll see if there's a way of specifying jest config in a way WebStorm likes it

[anler]

There's an issue here that wouldn't exist if we use what I proposed in the previous comment, and the issue is that Jest runs the tests in parallel, if we define another test that uses Vault, the data stored in it would be shared by the tests and that could cause problems

[aesedepece]

Nice point.

But I'm a little unsure about the parallel nature of Jest. The globalVault is already being shared across different tests, yet I never got any race condition, even though the last test depends on the one before being complete (as you can't create a new LevelDB instance before closing the former because of locks).

[aesedepece]

Regardless of any punctualization on wether it's safe or not in its current form, I'm favorable to refactor Vault in such away that it's dettached from the db connection so it can be tested more easily.

[anler]

After testing a bit I think I found the use case where it fails, executing yarn test seems to execute tests sequentially, but if I run yarn jest --watch and I press a (after the first sequential run), it seems those tests are run in parallel. If you duplicate vault.spec.ts file to vault2.spec.ts for example, running all the tests in this scenario will cause an error

[aesedepece]

I wonder if revisions and the current state of these commits will be preserved for reference if I go and edit my commits instead of creating a new one.

It just feel silly to me to have a bunch of overlapping commits merged at once in the same PR. But on the other hand, it would be a pity if we lose the discussion and the reviews, as they could be a valuable source of knowledge in the future.

Once more, I want to stress the importance of documenting the development process as a whole, not only the code but also the problems we face, the tradeoffs we consider and of course the decisions we eventualy take. This policy won't only help the project to be easier to approach and understand by potential contributors but will also make each of us more accountable to the rest of the team and to our future selves.

[aesedepece]

@mmartinbar and @kronolynx would you like to chime in and share some comments on @anler's review? Eight eyes see more than four! 😜

I think the points brought by @anler on his review need to be at least known by everyone in the team. The scope of the discussion transcends the scope of this particular issue indeed. Amongst other topics, there's this dilemma on how much do functions need to be coupled to the data/state they operate upon. The eternal object-oriented vs. functional paradigm fight is knocking at the door!

[mmartinbar]

Promise<any> could be Promise<boolean> as resolve logic just returns true

[anler]

Yes, I thought of the same, actually even boolean is too broad since the function either fails or returns true, so what we really need is a unit type, the closest thing in TypeScript would be void so the end result would be:

export const ensurePath = async (path: string): Promise<void> => {
  return new Promise((resolve, reject) => {
    mkdirp(path, (error: ErrnoException, made: mkdirp.Made) => {
      if (error) {
        reject(error)
      } else {
        resolve()
      }
    })
  })
} 

[mmartinbar]

for clarity, the name of the variable l could be changed to something like len

[mmartinbar]

To make the decrypt function symmetric with the encrypt one, it could just return the Buffer and leave the parse logic (JSON.parse) to the caller.

[aesedepece]

• All the commits in this PR have been refactored to adopt the design patterns we have been discussing.
• The tests are now using a mocked in-memory storage backend.
• Encryption is tested separately now as well.
• Type definitions for 3rd party modules now live in /typings.
• Commit 2fd666a has been added. It upgrades the version of @types/node we are using.
• PR rebased, all tests passing, ready to merge.
• r? @mmartinbar @anler

[anler]

Just a suggestion, these lines are equivalent to:

export default class LevelBackend implements IStorageBackend {
  constructor(private connection: LevelUp) {
  }
  ...
}