Skip to content
This repository has been archived by the owner on May 22, 2021. It is now read-only.

Gcm #106

Merged
merged 12 commits into from Jul 10, 2017
Merged

Gcm #106

merged 12 commits into from Jul 10, 2017

Conversation

abhinadduri
Copy link
Collaborator

No description provided.

@pdehaan
Copy link
Collaborator

pdehaan commented Jun 22, 2017

Fixes #86?

@ekr
Copy link

ekr commented Jun 29, 2017

Can you sketch out what's going on with the crypto, divorced from the code?

It looks like you're using a fraction of the pathname as a "salt" which you then call the IV? What properties do you expect that to have?

@abhinadduri
Copy link
Collaborator Author

@ekr The iv is also used as the file id, which is what we use to retrieve the file in the server-side code. The salt is not a parameter sent to the server (it follows the hash), and is used to encrypt/decrypt files. The aad is a separate field sent to the server by the uploader, for the sake of additional security.

All of these values are random Uint8's created by the window.crypto module.

The uploader uses the iv, salt, and aad to encrypt a file. The server sends the encrypted file to a receiver, who uses the iv and salt from the copy/pasted url, and the aad it gets from the server to decrypt the file in the browser.

@ekr
Copy link

ekr commented Jul 6, 2017

@abhinadduri I'm not really following. Do you have a wire protocol specification I can look at?

@@ -136,20 +137,24 @@ app.post('/delete/:id', (req, res) => {
});

app.post('/upload/:id', (req, res, next) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We no longer need the :id parameter in the upload url. new_id should be created at this level instead of in storage.js. It'd also be better to create meta.delete up here too. Ideally storage.js should only be concerned with saving and fetching the data from persistent storage and leaving the logic of what the data is and how it's created to something else (this function).

Copy link
Contributor

@dannycoates dannycoates left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few nits


class FileReceiver extends EventEmitter {
constructor() {
super();
this.salt = strToIv(location.pathname.slice(10, -1));
this.salt = hexToArray(location.pathname.slice(10, -1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iv is coming from X-File-Metadata now so we shouldn't need this anymore.

secretKey,
file.plaintext
)
.catch(err => console.log('Error with encrypting.')),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to rethrow

const uuid = crypto.randomBytes(10).toString('hex');

redis_client.hmset([id, 'filename', filename, 'delete', uuid]);
meta.id = id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is meta.id being used for? We probably don't need the old id that used to be iv anymore. If so we can remove it as a function parameter too

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants