Add Git Backend

[robert-cronin]

This PR will add a way to serve the vaults over http as git repos by exposing them via a git upload pack (stitched together from isomorphic-git/http-receiver). All it needs is an address (ip and port) to listen on (which is auto-generated). The other side is pulling vaults, this also needs an address (ip and port) which is provided by the peer discovery mechanism being implemented concurrently as a part of PR #40

Vault store is also implemented to keep track of which pubKeys have access to which vaults. This is also a convenience to pass into GitServer to retrieve the specific vault that git needs in order to use it's EFS instance to serve the vault from it's upperDir.

Fixes #31
This also in part solves #16, in terms of automatic commits. However, there is still the issue of sensible commit messages when committing more than 1 file (i.e. changing more than one secret at a time).

Fixes #22
Fixes #16
Fixes #32

[CMCDragonkai]

To be precise this enables "pull" behaviour. But not "push" behaviour. Eventually we need to apply capability theory to the connections between different Polykeys and vaults. That is what PK agents are allowed to pull from my vault, and what PK agents are allowed to push to my vault.

However in doing so, we then have to deal with merge conflicts. There are ways to resolve this automatically however.

[CMCDragonkai]

Vault store is also implemented to keep track of which pubKeys have access to which vaults. This is also a convenience to pass into GitServer to retrieve the specific vault that git needs in order to use it's EFS instance to serve the vault from it's upperDir.

I originally thought that the above could be implemented through hardlinks or something else. A pub key may relate to multiple vaults and indicate some sort of relationship. These relationships are conceptualized as "capabilities".

[CMCDragonkai]

Automatic commit messages are for convenience, we don't expect users of Polykey to understand Git. So we have to "specialize" the behaviour of git for secrets management. I'm sure they will appreciate it.

[CMCDragonkai]

I'm thinking everything under GitServer/ should just be under git/. I suspect there's a lot more to managing the git stuff than just the serving of git. I suppose if we maintain directory names to be capitalized, then Git/ is fine too. But I've actually stopped doing that for my other projects. I know I did that for js-virtualfs, but we can leave that as so. So lower case directory names is fine. Upper case file names for actual class files. Lower case file names for non-classes. Also this depends on what the default export is. For some files, the default export is a class, but it has ancillary exports, and upper case name is fine here. In other cases, a file exports lots of little things, or the default export is a function or an object/dictionary/array. Then lowercase name.

[CMCDragonkai]

A quick review of this PR. I have a feeling that the git portion of it has a lot of complexity, and it would be better to have that in a separate library, or at least extensively tested. It also has a bunch of code smells. And I'm not sure if alot of that code was just derived from some upstream library.

[CMCDragonkai]

Needs rebase on top of master.

And most importantly lots more tests for this git stuff.

[CMCDragonkai]

Surely we don't have roll our own tls. This us very common.

…

On 16 June 2020 17:23:44 GMT+10:00, Robbie Cronin ***@***.***> wrote: @robert-cronin commented on this pull request. > + return true + } + + const service = (<string>params.service).replace(/^git-/, '') + if (services.indexOf(service) < 0) { + res.statusCode = 405 + res.end('service not available') + return true + } + + const repoName = this.parseGitName(m[1]) + + // check if the repo is authenticated + const type = this.getType(service) + const headers = req.headers + const { username, password } = {username: 'foo', password: 's'} yeah these were just stubs, I am yet to dive deep into mutual TLS authentication. I will begin looking into PKIjs for a self signed certificate. I think its good if we can add it as a part of this PR. Then later if we want to give users the ability to sign with their own internal CA, then we can do that. -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #43 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[robert-cronin]

Surely we don't have roll our own tls. This us very common.

Not sure what you mean by rolling our own TLS, we can use whatever HTTPS library is provided by the environment polykey is deployed in and the server certificate.pem and key.pem we generate.

I've also found PKI.js is very complicated but I've found something much easier in node-forge it comes with the ability to use just pure JavaScript or node if we're in that environment. Perhaps we can use this instead? PKIjs examples can extend to 900 lines of code to generate a certificate.

If you're talking about the self-signing part, this isn't intended on being a long term solution, I was planning on looking into the Web of trust concepts for peers acting as CAs.

Another thing is, I'm not entirely sure yet how to incorporate keybase keys into this whole thing. They are PGP blocks so technically we can extract the RSA keys out of them to use in the X.509 certificates but I don't see any way to do this in JavaScript as yet. Currently I am generating a new RSA keypair for the X.509 certificates.

[CMCDragonkai]

PKI is an additional feature that is rather complicated, I want to have a discussion about this before going ahead. Any web of trust should right now be focusing on PGP only.

[CMCDragonkai]

Some preliminary review, mostly focusing on the overabundance of dependencies that shouldn't really be there. For a strong crypto app, the less dependencies the better. It becomes easier to audit, and make sure we are not bringing any unknown vulnerabilities.

[robert-cronin]

This PR is ready for review. Among the changes are a reduction of external dependencies, creating local methods for any minimally used packages and even removing some that weren't even used. Additionally, the domain modelling of the whole lib has been strengthened with the creation of a manager for each domain that polykey oversees. The polykey class now is much more like a convenience class that sets everything up for you than anything that manages Vaults/Keys/Peers. The latter have been delegated to their respective Manager classes.

[CMCDragonkai]

I want to merge this so I can review the code in its entirety (including nix-shell and execution) and then meetup later to discuss some finer details. But I reckon, you need to add more comments and function documentation (mainly on the the algorithmic details and data flow logic). Anything that is more likely to change and not stabalise you can leave it not commented, but must be documented in the MR or issue, or new issues should be created to address TODO elements.

I haven't got through all of it yet. But this is review number 2!

But it's looking alot more cleaner and structured! :D

[CMCDragonkai]

We should have `pk` and `polykey` as the main cli aliases. Gui app can be `pk gui` or something. Although we also need a libexec command for starting the daemon. Like `pk agent`. Will discuss this with you soon. Suggest starting a spec somewhere in Polykey issue of all the command workflows.

…

On 29 June 2020 13:07:12 GMT+10:00, Robbie Cronin ***@***.***> wrote: @robert-cronin commented on this pull request. > @@ -5,7 +5,7 @@ import inquirer from 'inquirer' import Configstore from 'configstore' import PolyKey from '../lib/Polykey' import cliProgress from 'cli-progress' -import KeyManager from ***@***.***/KeyManager' +import KeyManager from ***@***.***/keys/KeyManager' import { actionRunner, pkLogger, PKMessageType } from './polykey' I've added in a new key value pair for polykey cli. The `bin/pk.js` file was generated by hand, I am not sure how to get it to be generated by webpack. I can set it as an entry and then have it compiled but that would compile the rest of the code into that file as well since webpack follows `require` imports. Perhaps we should also add another key for our alias `pk`. I've added it in for now, otherwise we can make that something that happens with our desktop app installation process. -- You are receiving this because your review was requested. Reply to this email directly or view it on GitHub: #43 (comment)

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[robert-cronin]

Captains log, star-date 73962.7.
I have also added a fix for #22 to the PR. This is explained at the bottom of the linked issue.

[CMCDragonkai]

Time to merge this!

We'll move on from the git problem. And now start on the IPC architecture and start to draft out how Electron, CLI, Agent, OS platforms and nativescript will all work together.

New issues for IPC required, @robert-cronin please link them at the end of this PR. And just squash/clean the commits.

[robert-cronin]

Yes time to merge indeed!

I've added two issues (#51 and #50) from our discussion yesterday that I will also create a new branch and PR for.