-
Notifications
You must be signed in to change notification settings - Fork 9
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
refactor: add encrypted detailed files #840
base: master
Are you sure you want to change the base?
Conversation
587c3ef
to
399ae75
Compare
17e13d5
to
3e60d65
Compare
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.
Tested the patch and the decryption/encryption/git workflow, it works great 😄 A few question, nits and remarks and we're good to go 👍
https://github.com/Scalingo/apt-buildpack.git | ||
https://github.com/Scalingo/ssh-private-key-buildpack.git | ||
https://github.com/Scalingo/python-buildpack | ||
https://github.com/MTES-MCT/ecobalyse-buildpack-run.git |
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.
Nit: does the .builpacks format accept comments? would be nice adding a line about why each is used
Also: is the ssh-private-key-buildpack one still necessary?
npm run encrypt $ECOBALYSE_DATA_DIR/data/textile/processes_impacts.json dist/processes_impacts_textile.json.enc | ||
npm run encrypt $ECOBALYSE_DATA_DIR/data/food/processes_impacts.json dist/processes_impacts_food.json.enc | ||
npm run encrypt $ECOBALYSE_DATA_DIR/data/object/processes_impacts.json dist/processes_impacts_object.json.enc | ||
npm run encrypt public/data/textile/processes_impacts.json dist/processes_impacts_textile.json.enc | ||
npm run encrypt public/data/food/processes_impacts.json dist/processes_impacts_food.json.enc | ||
npm run encrypt public/data/object/processes_impacts.json dist/processes_impacts_object.json.enc |
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 have something in my eye 🥲
@@ -73,6 +72,14 @@ Pour initialiser la base de données (attention, toutes les données présentes, | |||
|
|||
## Développement | |||
|
|||
### Déchiffrage des fichiers | |||
|
|||
Certains fichiers d’impacts détaillés nécessitent de configurer [`transcrypt`](https://github.com/elasticdog/transcrypt) pour les lire en local. Assurez-vous d’installer les dépendances listées sur [la page dédiée](https://github.com/elasticdog/transcrypt/blob/main/INSTALL.md) puis lancez la commande suivante : |
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.
transcrypt requirement & installation should rather be mentioned in the "Socle technique et prérequis" at the top of the file. Also, see my other comment about why we do need this while we have a local version of it at hand in the repo?
|
||
Certains fichiers d’impacts détaillés nécessitent de configurer [`transcrypt`](https://github.com/elasticdog/transcrypt) pour les lire en local. Assurez-vous d’installer les dépendances listées sur [la page dédiée](https://github.com/elasticdog/transcrypt/blob/main/INSTALL.md) puis lancez la commande suivante : | ||
|
||
./bin/transcrypt -c aes-256-cbc -p 'your_password' |
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.
Shouldn't we rather use en env var for that? Also, shouldn't this be part of the npm commands, eg. npm postinstall
and friends, so that npm test
works directly?
U2FsdGVkX1//F0iscfNH9mDXUnWSWY+xSNGaGIYLBn8m3W5izFdRJYY/LGWbadrY | ||
8UcLLenA5hLa6X5f8nEJ4/JMoXuqRFBgcyUjkxT0z/oV7sQEWNa+lIMzDstWaqN0 | ||
oTXnyJBDgvqFfaA7v6kl+VIAdcr4t6M8vedGX84p32zAjI+YjLypW4ARx6KfMzsp | ||
66z/gLzXB8JTTnEptmt5wS3YJG05Kj7Fh1e/wqQMoLSQlLhsbyxUpumQwl10y5u1 | ||
GDVjEyrgBFldA4gMHDibYBdYxZtr51B9TbOdG/JalSzUzufLOxA0JmEp3tcXgYZE |
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.
Here we're losing the ability the possibility to have diffs entirely on github… may not be part of this patch but I could totally see some utility able to generate a JSON diff with the previous version of the file, at least locally…
npm run encrypt $PUBLIC_GIT_CLONE_DIR/public/data/object/processes_impacts.json $PUBLIC_GIT_CLONE_DIR/dist/processes_impacts_object.json.enc | ||
|
||
# Objects are not present in old versions | ||
if [[ -f "$PUBLIC_GIT_CLONE_DIR/public/data/object/processes_impacts.json" ]]; then | ||
npm run encrypt $PUBLIC_GIT_CLONE_DIR/public/data/object/processes_impacts.json $PUBLIC_GIT_CLONE_DIR/dist/processes_impacts_object.json.enc | ||
fi |
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.
Oh my, this means we'll have to maintain old versions using hacks and bash scripts forever… we should start discussing deprecating and dropping support for older versions (maybe file a card for that)
#!/usr/bin/env bash | ||
set -euo pipefail | ||
|
||
# | ||
# transcrypt - https://github.com/elasticdog/transcrypt | ||
# | ||
# A script to configure transparent encryption of sensitive files stored in | ||
# a Git repository. It utilizes OpenSSL's symmetric cipher routines and follows | ||
# the gitattributes(5) man page regarding the use of filters. | ||
# |
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.
This script is way to large and complex to review for me, and I suppose this is just a local versioning of the transcrypt library in this repo, because Scalingo does not allow installing it as a dependency for some reason. So as we have a local version of it in the repo, why do we still need to install transcrypt
from package managers locally on devs machines? Can't we just always use this one?
🔧 Problem
In order not to make public the Ecoinvent data containing detailed processes impacts, this data is stored in a private repository on Github https://github.com/MTES-MCT/ecobalyse-private, so it is not included with the source code of the Ecobalyse application, even though it is necessary for its proper functioning. This frequently leads to technical problems when putting features into production, as it is very complicated to keep data synchronized between the two repositories.
🍰 Solution
Re-store detailed processes in the main repository, but encrypt them using git encryption with the help of https://github.com/elasticdog/transcrypt
🚨 Points to watch/comments
The warning
*** WARNING : deprecated key derivation used.
on the ci is ok for now, see elasticdog/transcrypt#169As Scalingo doesn't give access to the git repo I need to git clone it when deploying to Scalingo. The I can run
transcrypt
to decrypt the files and copy them over.We don't synchronize with
ecobalyse-private
anymore. To add new detailed files they will just need to be added to a commit in the main repo. You can see the diffs locally but not on Github anymore as the files are encrypted.🏝️ How to test
Depending on your OS, install
trancrypt
dependencies listed on https://github.com/elasticdog/transcrypt/blob/main/INSTALL.md. Get thetrancrypt
key in https://vaultwarden.incubateur.net/. If you don't have access to the Vault, you should ask for one.You can check that files are encrypted by running:
In should give you some cryptic content.
Then init your repo with
transcrypt
using the following command (you will need to do it only once for all):The processes should then be decrypted and you should be able to read them directly with:
Try to change some detailed files and check that you can commit the changes in this branch without any problem (you can change the objects one).