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

Plugin encrypt #385

Merged
merged 19 commits into from Apr 20, 2020
Merged

Plugin encrypt #385

merged 19 commits into from Apr 20, 2020

Conversation

dbw9580
Copy link
Contributor

@dbw9580 dbw9580 commented Mar 30, 2020

This plugin adds support for password-protected galleries. Images are encrypted using a password during the build, and then decrypted in browser when they are accessed by viewers if they can provide the correct password.

Summaries:

  • Pure Javascript implementaion thanks to Web Crypto, no server application or database is needed;
  • Images are encrypted with AES-128-GCM, keys are generated using PBKDF2 with SHA1;
  • Support incremental builds;
  • decrypt.js is tested to work with all 3 themes bundled with Sigal, on Chrome and Firefox;

@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Mar 30, 2020

Demo: https://dbw9580.github.io/sigal-encrypt-demo/
The password is password.

@saimn
Copy link
Owner

@saimn saimn commented Apr 1, 2020

Looks cool, thanks!
The code seems good, and Ihave not much to say about the encryption part, maybe just a few questions.
I wonder if we could simplify a bit your implementation by adding some signals in sigal's code, for instance when writing an image, which could be used to write the encrypted version without having to check if it was already encrypted or not. It would also be useful to have a way to inject the HTML code in the header when writing HTML files, I think templates have an extra_head tag (or similar) so if there was a signal before writing HTML fies which you could use to inject your code it would also simplify things I guess.

sigal/plugins/encrypt/encrypt.py Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
tests/sample/sigal.conf.py Outdated Show resolved Hide resolved
@saimn
Copy link
Owner

@saimn saimn commented Apr 1, 2020

Do you think you could add some tests, building the gallery with the example settings and checking that images are indeed encrypted?

@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Apr 1, 2020

simplify a bit your implementation by adding some signals in sigal's code

Definitely a good idea. I'll work on that.

add some tests, building the gallery with the example settings and checking that images are indeed encrypted

So I was trying to create some tests, but not sure about how fine granularity it is suitable for such a plugin. So basically what the test should look like is first build the sample gallery, encrypt several pictures and decrypt them in-place to see if decryption works out with no error, right? As for the javascript parts of code, I am not sure how to test them properly.

setup.cfg Outdated Show resolved Hide resolved
@saimn
Copy link
Owner

@saimn saimn commented Apr 2, 2020

So I was trying to create some tests, but not sure about how fine granularity it is suitable for such a plugin. So basically what the test should look like is first build the sample gallery, encrypt several pictures and decrypt them in-place to see if decryption works out with no error, right?

Yes, something like this. Testing that some images are indeed encrypted, and maybe that they can be descrypted with the Python decrypt code that you included ?
There is an example of how to build the test gallery and modify the settings here:

def test_plugins(settings, tmpdir, disconnect_signals):
settings['destination'] = str(tmpdir)
if "sigal.plugins.nomedia" not in settings["plugins"]:
settings['plugins'] += ["sigal.plugins.nomedia"]
if "sigal.plugins.media_page" not in settings["plugins"]:
settings['plugins'] += ["sigal.plugins.media_page"]
init_plugins(settings)
gal = Gallery(settings)
gal.build()

As for the javascript parts of code, I am not sure how to test them properly.

Yeah that's doable but it's a lot of work so I never went into this.

@codecov
Copy link

@codecov codecov bot commented Apr 4, 2020

Codecov Report

Merging #385 into master will decrease coverage by 1.20%.
The diff coverage is 78.07%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #385      +/-   ##
==========================================
- Coverage   87.49%   86.28%   -1.21%     
==========================================
  Files          19       22       +3     
  Lines        1503     1750     +247     
==========================================
+ Hits         1315     1510     +195     
- Misses        188      240      +52     
Impacted Files Coverage Δ
sigal/plugins/zip_gallery.py 90.24% <ø> (ø)
sigal/plugins/encrypt/endec.py 70.00% <70.00%> (ø)
sigal/plugins/encrypt/encrypt.py 80.24% <80.24%> (ø)
sigal/plugins/encrypt/__init__.py 100.00% <100.00%> (ø)
sigal/signals.py 100.00% <100.00%> (ø)
sigal/writer.py 83.87% <100.00%> (+0.82%) ⬆️
sigal/image.py 89.81% <0.00%> (-2.46%) ⬇️
... and 1 more

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 972f0f1...a3b0876. Read the comment docs.

@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Apr 6, 2020

I rewrote most of the frontend code with Service Worker. The decryption process is now (almost) transparent to the gallery app and the user. Image srcs are no longer replaced with blob: urls, which means many browser extensions that rely on these links to work (e.g. image bulk downloaders, thumbnail previewers) can now function properly. Also original images can be opened in a separate tab without showing an error (which was impossible with the previous approach that based on hijacking image load events).

Speaking of original images, I find that they are handled differently than the resized ones and the thumbnails, in terms of how they end up where they belong in the output directory.

They are copied here:

copy(self.src_path, big_path, symlink=s['orig_link'],

while the resized and thumbnails are directly saved from PIL here:

save_image(img, outname, outformat, options=options, autoconvert=True)

and here:

save_image(img, outname, outformat, options=options, autoconvert=True)

This means if I were to use signals to handle the encryption, I would need one signal for the original files, and another for the resized and thumbnails. Which seems a little overkill to me.

Your thoughts?

@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Apr 10, 2020

@saimn I've added the test. I think this PR is ready for review.

Copy link
Owner

@saimn saimn left a comment

Looks very good, thanks for the hard work in this !
I will try to review more carefully in the coming days.

sigal/writer.py Outdated Show resolved Hide resolved
tests/test_encrypt.py Outdated Show resolved Hide resolved
``kdf_iters`` defaults to 10000. Do not specify them in the config file unless
you have good reasons to do so.
- ``encrypt_symlinked_originals``: Force encrypting original images even if they
are symlinked. If you don't know what it means, leave it to ``False``.
Copy link
Owner

@saimn saimn Apr 14, 2020

Choose a reason for hiding this comment

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

I think this should be True by default, and we could maybe even remove this setting (and send a warning that orig_link should not be used in this case). When using this plugin clearly the goal is to protect privacy by encrypting all images including the original ones.

Copy link
Contributor Author

@dbw9580 dbw9580 Apr 14, 2020

Choose a reason for hiding this comment

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

Yes, but we need to also make sure people don't get their original pictures overwritten if they accidentally have both keep_orig and orig_link set to True, and have not made a backup. I think it's better we abort the encryption and send a warning, to make sure the user understands the situation, then it's up to them to decide. It's trivial to restart an aborted gallery build, while to recover the originals by decrypting them is not.

Copy link
Owner

@saimn saimn Apr 18, 2020

Choose a reason for hiding this comment

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

Agreed! So I think you can remove this setting ?

Copy link
Contributor Author

@dbw9580 dbw9580 Apr 18, 2020

Choose a reason for hiding this comment

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

Remove it from the doc or the code?

Copy link
Owner

@saimn saimn Apr 19, 2020

Choose a reason for hiding this comment

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

Remove from the code, as you said above we should abort if keep_orig and orig_link are True, so I don't think we should keep this setting to force encryption. People should just choose between orig_link and using this plugin.

Copy link
Contributor Author

@dbw9580 dbw9580 Apr 19, 2020

Choose a reason for hiding this comment

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

Good point. Changed in a3b0876.

Copy link
Owner

@saimn saimn left a comment

More comments, mostly minor ones but I think the error handling in the Python code could be improved. The Abort exception used everywhere makes it a bit difficult to follow the error cases. You should use standard exceptions when possible (e.g. missing or invalid parameter), and reserve use of an exception specific to the plugin for problems with the encryption. And in particular I think the plugin should always fail the build if something could prevent encyption of the files.

sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/encrypt.py Outdated Show resolved Hide resolved
sigal/plugins/encrypt/endec.py Outdated Show resolved Hide resolved
@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Apr 14, 2020

It occurs to me that the zip_gallery option will be meaningless if this plugin is enabled. The zip archive will contain encrypted images, and the viewers will not be able to decrypt them if they download the archive. We should add a note to the documentation about this.

@dbw9580
Copy link
Contributor Author

@dbw9580 dbw9580 commented Apr 18, 2020

Copy link
Owner

@saimn saimn left a comment

Looks very good, just a small question.

theme_path = os.path.join(settings["destination"], 'static')
copy(os.path.join(ASSETS_PATH, "decrypt.js"), theme_path, symlink=False, rellink=False)
copy(os.path.join(ASSETS_PATH, "keycheck.txt"), theme_path, symlink=False, rellink=False)
copy(os.path.join(ASSETS_PATH, "sw.js"), settings["destination"], symlink=False, rellink=False)
Copy link
Owner

@saimn saimn Apr 20, 2020

Choose a reason for hiding this comment

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

Why sw.js is not in the static directory with the other static files ?

Copy link
Contributor Author

@dbw9580 dbw9580 Apr 20, 2020

Choose a reason for hiding this comment

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

As per Service Worker specification, the worker script has no access to resources outside its directory tree. So putting sw.js in static will break it.

Copy link
Owner

@saimn saimn Apr 20, 2020

Choose a reason for hiding this comment

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

Ok, thanks for the info!

@saimn saimn merged commit 0ba9b10 into saimn:master Apr 20, 2020
0 of 2 checks passed
@saimn
Copy link
Owner

@saimn saimn commented Apr 20, 2020

Great job, thanks @dbw9580 !

@saimn saimn added this to the 2.1 milestone Apr 20, 2020
kontza pushed a commit to kontza/sigal that referenced this issue Aug 28, 2020
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.

None yet

2 participants