Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added AES encryption/decryption using native crypto #15

Merged
merged 44 commits into from
Oct 21, 2020
Merged

Added AES encryption/decryption using native crypto #15

merged 44 commits into from
Oct 21, 2020

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented Jun 30, 2020

This PR does the following:

  • Adds AES encryption/decryption to adapter (used server side encryption aes256 #8 as a reference).
  • Adds key rotation to move unencrypted files to encrypted, swap old keys for new ones, or remove file encryption
  • Use createRead/WriteStream for files instead of reading/write whole files in memory (a performance improvement for larger files)
  • Align the FS and GridFS adapters. This should make it easier to copy updates/changes from one adapter to the other as they are essentially doing the same thing, but to different storage mediums.
  • Updates travis to make it work with updated packages

For fileKey can pass in PARSE_SERVER_FILE_KEY (fileKey) from parse-server or supply any key string you want (just be sure to keep it a secret).

Example:

const api = new ParseServer({
  databaseURI: databaseUri || 'mongodb://localhost:27017/dev',
  cloud: process.env.PARSE_SERVER_CLOUD || __dirname + '/cloud/main.js',
  appId: process.env.PARSE_SERVER_APPLICATION_ID || 'myAppId',
  masterKey: process.env.PARSE_SERVER_MASTER_KEY || '', 
  filesAdapter: new FSFilesAdapter({fileKey: process.env.PARSE_SERVER_FILE_KEY}), //Add your file key here. Keep it secret. Note that this is needed for Postgres or anyone using FSFilesAdapter. 
  ...
});

Can rotate keys as well:

Periodically you may want to rotate your fileKey for security reasons. When this is the case. Initialize the file adapter with the new key and do the following in your index.js:

Files were previously unencrypted and you want to encrypt

var FSFilesAdapter = require('@parse/fs-files-adapter');

var fsAdapter = new FSFilesAdapter({
  "filesSubDirectory": "my/files/folder", // optional
  "fileKey": "newKey" //Use the newKey
});

var api = new ParseServer({
	appId: 'my_app',
	masterKey: 'master_key',
	filesAdapter: fsAdapter
})

//This can take awhile depending on how many files and how larger they are. It will attempt to rotate the key of all files in your filesSubDirectory
const {rotated, notRotated} =  await api.filesAdapter.rotateFileKey();
console.log('Files rotated to newKey: ' + rotated);
console.log('Files that couldn't be rotated to newKey: ' + notRotated);

Files were previously encrypted with oldKey and you want to encrypt with newKey

The same process as above, but pass in your oldKey to rotateFileKey().

//This can take awhile depending on how many files and how larger they are. It will attempt to rotate the key of all files in your filesSubDirectory
const {rotated, notRotated} =  await api.filesAdapter.rotateFileKey({oldKey: oldKey});
console.log('Files rotated to newKey: ' + rotated);
console.log('Files that couldn't be rotated to newKey: ' + notRotated);

Only rotate a select list of files that were previously encrypted with oldKey and you want to encrypt with newKey

This is useful if for some reason there errors and some of the files werent rotated and returned in notRotated. The same process as above, but pass in your oldKey along with the array of fileNames to rotateFileKey().

//This can take awhile depending on how many files and how larger they are. It will attempt to rotate the key of all files in your filesSubDirectory
const {rotated, notRotated} =  await api.filesAdapter.rotateFileKey({oldKey: oldKey, fileNames: [fileName1,fileName2]});
console.log('Files rotated to newKey: ' + rotated);
console.log('Files that couldn't be rotated to newKey: ' + notRotated);

@codecov
Copy link

codecov bot commented Jul 1, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@9cd39af). Click here to learn what that means.
The diff coverage is 91.86%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #15   +/-   ##
=========================================
  Coverage          ?   89.31%           
=========================================
  Files             ?        1           
  Lines             ?      131           
  Branches          ?        0           
=========================================
  Hits              ?      117           
  Misses            ?       14           
  Partials          ?        0           
Impacted Files Coverage Δ
index.js 89.31% <91.86%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cd39af...490dc75. Read the comment docs.

console.log('Files that couldn't be rotated to newKey: ' + notRotated);
```

#### Only rotate a select list of files that were previously encrypted with `oldKey` and you want to encrypt with `newKey`
Copy link
Member

Choose a reason for hiding this comment

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

If you only rotate a subset of the files, how will the not rotated ones be served?

Copy link
Contributor Author

@cbaker6 cbaker6 Jul 15, 2020

Choose a reason for hiding this comment

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

You are right, they can't be served with the current key. This is additional functionality for people who have custom setups or if the server crashes/has issues during rotation and you need to rotate the rest of the files.

In common cases, admins should store files for respective apps in individual folders, and rotate all when needed.

Copy link
Member

Choose a reason for hiding this comment

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

I believe that rotating the keys like this can generate a lot of problems in the case there is a large amount of files in the storage. I'd prefer to either have this rotate functionality on a separate module/project or think in some way to have both the old and the new key serving old and new files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure I understand, how does encrypting keys with this module, but having the key rotation on a different module/project help? I think if you encrypt files, that same module should always have a way to decrypt them and leave unencrypted if wanted, which will probably be the main use of the rotate functionality.

Server admins can come up with their own creative ways of maintaining their key, like encrypting the fileKey with a separate key and then rotating the separate key instead of rotating the fileKey encrypting all of the files. They have to maintain two keys, but gets around the large amount of files issues (I think AWS does something similar to this). I'm sure others can come up with a ton of ways, but the ability to decrypt should always be apart of the same module IMO.

Parse-server admins determine when to rotate keys. Another way is they could create create a separate dev parse-server, rotate the keys of a copy of their files directory and then point their production server to the new folder.

Copy link
Member

Choose a reason for hiding this comment

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

I believe this rotate function shouldn't be part of the scope of the adapter and that's why I suggested either a separate project or a separate script. You are suggesting people to use the rotate function in the Parse Server initialization which can generate a lot of problems in the case there is a large amount of files. Take the example below:

var FSFilesAdapter = require('@parse/fs-files-adapter');
var fsAdapter = new FSFilesAdapter({	var fsAdapter = new FSFilesAdapter({
      "filesSubDirectory": "my/files/folder" // optional	  "filesSubDirectory": "my/files/folder", // optional
    });	  "fileKey": "newKey" //Use the newKey
});
var api = new ParseServer({	var api = new ParseServer({
	appId: 'my_app',		appId: 'my_app',
	masterKey: 'master_key',		masterKey: 'master_key',
	filesAdapter: fsAdapter		filesAdapter: fsAdapter
})	})
//This can take awhile depending on how many files and how larger they are. It will attempt to rotate the key of all files in your filesSubDirectory
const {rotated, notRotated} =  await api.filesAdapter.rotateFileKey();
console.log('Files rotated to newKey: ' + rotated);
console.log('Files that couldn't be rotated to newKey: ' + notRotated);
  • Now Imagine that I am running Parse Server in multiple containers. I will have multiple processed running the rotation at the same time.
  • The rotation will consume cpu and memory in these processes which can cause poor performance while it is running.
  • The files not yet rotated will not be correctly served

I still believe that the rotate function is useful though, but we should either solve all these problems (which will be hard to be done) or not suggest it to be used this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dplewis @mtrezza can you both weigh in on this?

The difference in opinion between @davimacedo and I are expressed on the thread and I think there are pros/cons to both approaches. I don’t believe the differences are a showstopper for this PR and I think improvements can be made by others (if I see an improvement, I will make it as well).

On a related note, @davimacedo @dplewis @mtrezza I know there has been recent/past discussions about the parse-server allowing access to all files if you guess the link (which isn’t necessarily easy, but doable). I’m going off memory, but I believe the server has something like getHostFile (I’ll look this up when I get to my computer). I don’t know entirely how this works, but I assume it gets the file whenever someone requests the link and calls the fileAdapters underlying methods to retrieve? If this is the case (or something similar), is possible to use the fileKey to either serve an encrypted or unencrypted file based on the user access (I’m assuming some file always to be present at the link to stay inline with older versions which is why I mention the encrypted file)? I definitely haven’t investigated if this is the easiest and best way, but I wanted to start the convo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davimacedo I know it’s been awhile since we discussed this. If I recall, you didn’t believe the key rotation should be in the adapter itself. I approached from the direction if we provide a way to encrypt, we should provide a way to decrypt (with this adapter as well as the grid adapter on parse-server). Are we able to come to some type of agreement on this? This PR along with the parse-server have been around for a few months and I’m sure my JS skills are not as good as they were when I made the PR’s.

I’m not attempting to force these in, but my assumption is people enable file encryption here will understand what they are doing and take the necessary precautions when rotating keys. People that use the S3 adapter have this benefit because AWS handles it for them https://docs.aws.amazon.com/kms/latest/developerguide/rotate-keys.html

I want to also state that file encryption isn’t enabled by default, so I’m sure many will just be using the refreshed code

Copy link
Member

@davimacedo davimacedo Oct 21, 2020

Choose a reason for hiding this comment

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

Like I told you before, you did a great job here, but the only thing I don't like in this PR is when you suggest something like this:

var FSFilesAdapter = require('@parse/fs-files-adapter');

var fsAdapter = new FSFilesAdapter({
  "filesSubDirectory": "my/files/folder", // optional
  "fileKey": "newKey" //Use the newKey
});

var api = new ParseServer({
	appId: 'my_app',
	masterKey: 'master_key',
	filesAdapter: fsAdapter
})

//This can take awhile depending on how many files and how larger they are. It will attempt to rotate the key of all files in your filesSubDirectory
const {rotated, notRotated} =  await api.filesAdapter.rotateFileKey();
console.log('Files rotated to newKey: ' + rotated);
console.log('Files that couldn't be rotated to newKey: ' + notRotated);

In my vision, the developers should never use the same process they are using for running Parse Server to also rotate the files. That's what this example is suggesting and I don't see it ending well unless the app has just few files.

I believe we can even maintain the key rotation here but it would be more clear and actually more convenient for the developers if it worked via a script that they could run something like:

parse-server-fs-adapter appId masterKey path oldKey newKey

If it is too much effort to be done, I believe we should at least move the rotate method from the adapter to another class in which you would pass appId, masterKey, path, oldKey, newKey (and not an instance of Parse Server) and make it clear in the readme that this operation is not intended to be executed in the same process of Parse Server.

The other maintainers may have a different vision though. @dplewis @mtrezza thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@davimacedo I guess my thinking was similar to your script example, with the belief that the code you mentioned (key rotation code) above would be executed on a development server (not the live server). So the development server is spun up with the newKey, then keys are rotated. If there are additional tests to be done on dev parse-server they can be done as well. After completion, the live server will need to be restarted with the newKey.

Would it help to change my examples to recommend doing key rotation on a dev server? Then mention the part about restarting the live server?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I believe it helps. Anything that makes clear to the developers that they should not run the rotate key in the same process they are running parse server.

index.js Show resolved Hide resolved
@cbaker6
Copy link
Contributor Author

cbaker6 commented Jul 15, 2020

@davimacedo I'll note that I made FS and GridFSBucket look almost exactly the same (testcases are slightly different, but do the samething) so they can be modified in a similar way moving forward since they do the same thing but for different file systems. The original PR was already approved, but the key rotation is here for GridFS. Since you are looking at the code for this PR, it might be easier for your to comment on the changes there as well

@ghost
Copy link

ghost commented Oct 21, 2020

Danger run resulted in 1 fail and 1 markdown; to find out more, see the checks page.

Generated by 🚫 dangerJS

@cbaker6
Copy link
Contributor Author

cbaker6 commented Oct 21, 2020

I'm not sure what "Danger" or "Peril" is but it looks like something that was added to the repo after the initial PR

@davimacedo
Copy link
Member

Yes. This Danger script is broken in all repos.

Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

LGTM!

@davimacedo davimacedo merged commit 44dde17 into parse-community:master Oct 21, 2020
@parse-github-assistant
Copy link

parse-github-assistant bot commented Oct 11, 2022

Thanks for opening this pull request!

  • ❌ Please edit your post and use the provided template when creating a new pull request. This helps everyone to understand your post better and asks for essential information to quicker review the pull request.

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

Successfully merging this pull request may close these issues.

2 participants