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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: data_portability_export email download #5342

Draft
wants to merge 3 commits into
base: master
from

Conversation

@aitorlb
Copy link
Contributor

commented Sep 18, 2019

馃帺 What? Why?

Trying to download "My data" through the link received via mail (you can apply for it on Participant Settings --> My Data --> Download the data/ Request data) I get an Error:500

We were saving the data portability files locally but that is incompatible with PaaS with an ephemereal filesystem. To fix that we have added support for storing the files in a cloud storage service, but then this results in the files being in a public directory, so we have considered protecting them with a password.

These guidelines have been given to us:

Finally, adding a password protected zip could be cracked https://security.stackexchange.com/questions/17774/how-to-recover-a-lost-zip-file-password so if this is the solution it should at least have multiple mitigations:

  1. that the zip filename is not predictable (ie not user IDs but random strings)
  2. that the zip password is not predictable (random long strings)
  3. that the zip file gets deleted after some time (like 5 days).

Current approach:

  • The zip file is stored in /public/uploads/data-portability/

  • The name of the zip file is a random string (using SecureRandom::urlsafe_base64), i. e.

    gFTZFKqTPP6-OQ6GU9XoMQ.zip

  • The zip file contains a password-protected zip file with the user data (using 7-Zip) named:

    data-portability.7z

  • The password is a random string (using SecureRandom::urlsafe_base64), i. e.

    4Q_FtJ9lfx6S6cVDAxT5mA

  • An email is send to the user with a link to download their data (the url contains the filename) and the password to open it.

  • The file is available on the server for 5 days.

馃搶 Related Issues

馃搵 Subtasks

  • Add CHANGELOG entry
  • Add tests

馃摲 Screenshots (optional)

data_port_email

@aitorlb aitorlb force-pushed the CodiTramuntana:fix/data_portability_download branch from 77f50fd to f6bbb7b Oct 3, 2019
@aitorlb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 7, 2019

@andreslucena is the workflow described acceptable?

@andreslucena

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

The zip file contains a password-protected zip file

Why do we need this?

Also people normally expects that zip files has an extension of .zip, I'd prefer that instead of .7z

@microstudi

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

The zip file contains a password-protected zip file

Why do we need this?

In the event of someone getting access to the public file, they would still need the password, isn't it @aitorlb
However, I think that if someone has access to that link is because they had access to the received email, so they'll have the password anyway.
In that case I think it would be enough to just create a md5 (or similar) named file, no encryption and deleted after a period (also the email should inform about this deletion).

@aitorlb

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2019

Thank you both for the feedback.

@andreslucena,

The zip file contains a password-protected zip file

Why do we need this?

We don't. But without extracting the data of the password-protected files, the files themselves inside the zip can be broken; it seemed right to protect the whole content as a unit which prevents messing with the individual files.

Also, this approach made it really easy to add encryption since I did not have to modify the existing code, just wrap the returned zip in another zip.

Also people normally expects that zip files has an extension of .zip, I'd prefer that instead of .7z

I would prefer that too, but the gem currently used for zipping files has rather weak encryption capabilites, and 7-Zip supports encryption with AES-256 algorithm, which is better, as far as I understand.

@microstudi

In the event of someone getting access to the public file, they would still need the password, isn't it

As I hinted above, the file could be opened and its insides browsed but the contained files could not be opened/extracted without password.

However, I think that if someone has access to that link is because they had access to the received email, so they'll have the password anyway.
In that case I think it would be enough to just create a md5 (or similar) named file, no encryption and deleted after a period (also the email should inform about this deletion).

As far as not using encryption, I though the whole point of it was protecting the user data from people with access to the server where the data would be stored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.