-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add fileKey rotation to GridFSBucketAdapter #6768
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6768 +/- ##
==========================================
- Coverage 93.88% 93.84% -0.04%
==========================================
Files 169 169
Lines 12353 12403 +50
==========================================
+ Hits 11597 11639 +42
- Misses 756 764 +8
Continue to review full report at Codecov.
|
…nd the fileKey was appended to links. This key is now an encryption key
src/Controllers/FilesController.js
Outdated
if (filename.indexOf('tfss-') === 0) { | ||
fileObject['url'] = | ||
'http://files.parsetfss.com/' + | ||
config.fileKey + |
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.
Removed "fileKey" from being appended to urls. I assumed legacy parse.com used the fileKey for encryption, didn't realize is was only appended. I made the modification to the legacy test of removing the fileKey from the links there also.
Now "fileKey" is repurposed as an encryption key
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 do you have any thoughts on this?
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 am not sure. I'd prefer to have a new parameter for the encryption as this change will be a breaking change and may affect a lot of apps using it not only changing their files urls but also encrypting their files (and not being able to decrypt the current ones)
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.
Sorry for my last comment. I've just checked the code again and fileKey is only being used here in this piece of code. I believe this current code is actually useless now and can be replaced to only
fileObject['url'] = this.adapter.getFileLocation(config, filename);
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 only afraid that some developers may have fileKey in their config since parse.com migration and in this case they will have their files being encrypted (maybe without noticing that).
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.
Using a new parameter would definitely prevent an accidental usage from people who have the old fileKey set. I think there was an old issue someone opened up where I mentioned for them to remove fileKey from their configuration. I was trying to save an old parameter, but it may not be the best solution here
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.
Added and option for encryptionKey
and used that instead of fileKey
. Let me know if this works and I can make the change in the docs.
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 reverted this particular file since I’m no longer using fileKey
Danger run resulted in 1 fail and 1 markdown; to find out more, see the checks page. Generated by 🚫 dangerJS |
@davimacedo @dplewis I updated the comments here to reflect deploying a development server for key rotation like the FS adapter. You should also check the review comment I left as I removed some code from |
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.
LGTM!
Adds the ability to rotate fileKeys for:
PARSE_SERVER_FILE_KEY
orfileKey
. Instead pass your key directly toFSFilesAdapter
. parse-server versions > 4.2.0 can pass inPARSE_SERVER_FILE_KEY
orfileKey
.If you have no files on your parse-server currently, but plan on adding them, simply add a
PARSE_SERVER_FILE_KEY
. There's no need to rotateKeys now, but you may want to do it in the futurePeriodically you may want to rotate your fileKey for security reasons. When this is the case, it is recommended to start up a development parse-server (or a separate process from your main process) that has the same configuration as your production server. On the development server, initialize the file adapter with the new key and do the following in your
index.js
:Pass the
newKey
as env varPARSE_SERVER_FILE_KEY
, pass in--fileKey "newKey"
in the CL, or initialize ParseServer withfileKey="newKey"
.An example using
GridFSBucketAdapter
:An example using
FileSystemAdapter
:Files were previously unencrypted and you want to encrypt them with a fileKey
You probably want to back up your unencrypted files before doing this the first time.
Files were previously encrypted with
oldKey
and you want to encrypt withnewKey
The same process as above, but pass in your
oldKey
torotateFileKey()
.Files were previously encrypted with
oldKey
and you want to decrypt all filesDifferent from the previous process, don't initialize your fileAdapter with a
fileKey
. Pass in youroldKey
torotateFileKey()
.For
GridFSBucketAdapter
, omit the key:For
FileSystemAdapter
, don't pass thefileKey
option:Only rotate a select list of files that were previously encrypted with
oldKey
and you want to encrypt withnewKey
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 youroldKey
along with the array offileNames
torotateFileKey()
.