-
-
Notifications
You must be signed in to change notification settings - Fork 32
Added AES encryption/decryption using native crypto #15
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
Merged
Merged
Changes from 40 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
95b5a89
add native AES file encryption
cbaker6 7ae60e4
removed update of package.json
cbaker6 e507825
update package.json
cbaker6 dad1e7c
update node in travis
cbaker6 d01aa1f
update node in travis
cbaker6 e6c8f9b
trying different verion of key and iv hash
cbaker6 f9893de
downgrading node to match minimal parse-server
cbaker6 74c7c9e
removed commented code
cbaker6 9af50d3
add random iv for each file instead of using constant. Also removed e…
cbaker6 db8a1a8
remove unneccesary files
cbaker6 c9f9db8
Use AES 256 GCM to detect file tampering
cbaker6 11c6f3c
remove codecov from package.json
cbaker6 9f4fd11
add repo field to get rid of npm install warning
cbaker6 fb7517f
Fix options
cbaker6 92013a4
switch secretKey to fileKey
cbaker6 6f2752e
switch secretKey to fileKey
cbaker6 d2fce74
added the ability to rotate fileKeys
cbaker6 b42d683
add syntax highlighting to readme
cbaker6 0d25c2c
bump version
cbaker6 f633f70
attempt to fix coverage
cbaker6 e07ec58
update testcase title
cbaker6 7f1784c
clean up unused vars
cbaker6 8939921
add directions for multiple instances of parse-server
cbaker6 75d6b8a
update readme
cbaker6 9ac405f
update file names in readme
cbaker6 0c4bf0d
add testcase for rotating key from oldKey to noKey leaving all files …
cbaker6 47c87e7
Add notice about previous versions of parse-server
cbaker6 07061b7
Update README.md
cbaker6 bd19045
Update README.md
cbaker6 51c2b51
Update README.md
cbaker6 01fdf22
Update README.md
cbaker6 4517087
make createFile and getFile use streams instead of putting whole file…
cbaker6 b68681c
Merge branch 'master' of https://github.com/netreconlab/parse-server-…
cbaker6 9646246
don't read file into memory while deleting
cbaker6 39f96d2
clean up code
cbaker6 fbcabf4
make more consistant with GridFS adapter
cbaker6 1d3ca24
fixed formatting
cbaker6 4c344ee
Update .travis.yml
cbaker6 a20a7a2
Remove unnecessary testcase
cbaker6 b649dde
Update secureFiles.spec.js
cbaker6 2390e59
add directions for dev server to readme
cbaker6 6b175fb
Revert version
cbaker6 5beb618
Update package.json
cbaker6 490dc75
Update .travis.yml
cbaker6 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"reporter": [ | ||
"lcov", | ||
"text-summary" | ||
], | ||
"exclude": [ | ||
"**/spec/**" | ||
] | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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 convoThere was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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:
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:
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?
There was a problem hiding this comment.
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 thenewKey
.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?
There was a problem hiding this comment.
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.