-
-
Notifications
You must be signed in to change notification settings - Fork 362
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 custom RAW format conversion tool support #929
base: master
Are you sure you want to change the base?
Changes from 13 commits
a720468
ea56250
9141661
bd05169
e726258
5516c1e
8e81d86
de0b902
31b836b
4363047
76d94f1
4ba309f
f10694d
9c98767
ef75e3f
c0eea6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,9 @@ ENV PHOTOVIEW_LISTEN_PORT 80 | |
|
||
ENV PHOTOVIEW_SERVE_UI 1 | ||
ENV PHOTOVIEW_UI_PATH /ui | ||
ENV PHOTOVIEW_RAW_PROCESSING_EXECUTABLE="darktable-cli" | ||
ENV PHOTOVIEW_RAW_PROCESSING_EXECUTABLE_CHECK="--version" | ||
ENV PHOTOVIEW_RAW_PROCESSING_ARGS="{{ .InputPath }} {{ .OutputPath }} --core --conf plugins/imageio/format/jpeg/quality={{ .Quality }} --configdir /tmp/photoview-convert" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this lead to a command injection attack, by crafting a malicious environment variable? And if so is it a problem? Just wanted to mention it to hear what you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @viktorstrate, This is a great catch! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this issue is within the scope of consideration. The prerequisite for attacking through custom environment variables is that the attacker has obtained the permissions of the machine. If the attacker has obtained the permissions of the machine, then the security issue does not require us to consider it. In addition, regarding the docker image, the program and environment variables used for RAW conversion have been pre-installed in the official image. If the program or the value can be artificially modified, the attacker still needs to obtain the permissions of the machine. From what perspective, this should not be an issue that ordinary applications need to consider. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An attacker might get access to the docker container and put there a new malicious executable, then manipulate with env variables to replace the RAW processing one, so that the malicious executable is called instead. In this case, there is no need to have access to the host. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If an attacker can modify environment variables and restart photoview, he also has permission to directly execute the malicious program. In addition, environment variables can be changed, so even under existing circumstances, the default RAW converter and video converter will still be replaced without requiring root permission. Just modify the PATH environment variable There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I really like this argument, this PR would not make Photoview any less secure because of this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe, we need to prepare an AppArmor profile, which will at least forbid the execution of any apps from non-standard locations, or allow the execution of only allowed binaries - I need to think about this more... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
BTW, if we provide a way to register any external RAW processing app, do we want to provide the same approach for video-converter integration as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the first message I don't think it's something that we need to do unless anyone raises a concern. Ultimatly the attack vector is minimised in a docker environment and on a bare metal install I'd expect people to manage it as they see fit based on there use case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If you refer to the "do we want to provide the same approach for video-converter integration as well?" message, sure, I agree that this might be a new PR. My question was just from a product functionality perspective: do you see the inconsistency between the flexibility of the RAW processing app integration and video / thumbnails converters, possibly some other workers? |
||
|
||
EXPOSE 80 | ||
|
||
|
Large diffs are not rendered by default.
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.
As we add the support of ImageMagick, it would be great to enhance this by adding the
else
with ImageMagick installation. Also, it makes sense to move just created 3ENV
lines to thethen
branch of thisif
, setting the same 3 variables in theelse
to the values, compatible with the ImageMagickThere 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.
ImageMagick
is not in conflict withDarktable
, we can keep it the way it is, i.e.Darktable
is installed by default, and other possible handlers are advanced usage. Because I can't be sure which environmentsDarktable
can't be installed andImageMagick
can, in the current issues, there is no known problem of not being able to handle RAW formats because darktable can't be installed!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.
Actually, as you can see from the mentioned
if
, Darktable doesn't support (and cannot be installed on) ARM-based systems, some right now Photoview, deployed on ARM doesn't support RAW formats at all.I cannot test it, as I don't have such a system, but as far as I know, ImageMagick supports ARM and can be installed there - that is why I've proposed this change: to finally extend the RAW support to the ARM platform as well.
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.
@kkovaletp I think support for arm based images and using raw can be added at a later date under another issue so we can get this piece in. I think OP has said in the past he doesn't use docker, so unless he's willing to do it we should make another issue for it so someone with more expertise can implement it correctly.
@fierceX It looks like you've pulled in a load of extra commits somehow, bad rebase maybe? when you fix those up I'll give a proper review but a quick glance through it looks good. The only thing missing looks like defaults when running without docker...
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.
@jordy2254 I merged in commits that were already merged into master, and now how do I cancel those commits in order to prevent my changes from conflicting with some existing changes?
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.
@fierceX general process would be;
git pull
git rebase master
(you can do-i
if you'd like to change commitsorgit merge master
will do the jobIf you need more support feel free to reach out.
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.
@jordy2254 I pushed a new commit and it should now work fine, the change file contains only the changes needed for this PR。Other than that, do I need to adjust anything?
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.
@fierceX sorry for the late reply commit history and file changes look fine now thanks :)
I'm not on the computer for the next few days but spotted one minor thing I've commented on.
By any chance have you looked at the test failures on your branch yet? Do you know what's up with them?