-
-
Notifications
You must be signed in to change notification settings - Fork 360
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?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #929 +/- ##
==========================================
- Coverage 57.34% 57.24% -0.10%
==========================================
Files 196 196
Lines 15607 15634 +27
Branches 533 533
==========================================
Hits 8950 8950
- Misses 6408 6435 +27
Partials 249 249
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@viktorstrate @kkovaletp @jordy2254 |
args := []string{ | ||
worker.path, | ||
inputPath, | ||
outputPath, | ||
fmt.Sprintf("%d", jpegQuality), | ||
} |
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 don't think that all custom programs necessarily use this format, it would be nice to have a string where the paths and quality could be inserted into, something like this:
/path/to/program
-i %input_path% -o %output_path% -q %jpeg_quality%
From the product logic perspective, we have another PR to include support for the ImageMagick tool. I think that this way it could be also integrated (as well as some other image converters), so I like this flexible way more, but I'd like to get @jordy2254 opinion from the product's code perspective and the application architecture perspective: we have to make sure that this code is robust and will work well for other similar tools without any degradation. |
I really like this @fierceX great work! What's your thoughts on the below? Replacing both darktable and imagemagik with a generic RawProcessor. This will then take arguments in a gotemplate as @viktorstrate said and then we can set everything in program I think we'd need 3 to keep the current format; in the case of your other PR this would be; Doing so should give us the flexibility to encode with whatever we want (Given they don't need additional values from the code) and leave it up to the user. We can if desired set preconfigured examples do demonstrate the differnt types we've come accross as examples. This approach should also let us execute scripts aswell with full path so that we open up other avenues of processing. |
Sounds like a good idea @jordy2254. One thing to consider is backwards compatibility. It would be nice if the old setup would still work. Maybe by having a set of predefined applications like for Darktable, imagemagick etc. with manual opt-out environment variables. |
@jordy2254 I think that makes more sense, so I change this PR to that mode and then undo the other PR and change it to default to using |
@fierceX yep default to darktable, some documentation on the other 2 programs you've implemented in this and the other PR for reference would be great once this is merged; here is the repo for the documentation site https://github.com/photoview/photoview.github.io There's several ways to achieve the default values I'll leave it to you what you wan't to implement below is a few of my potential suggestions.
If you'd like any further help feel free to reach out on discord, or comment back here :) |
…RL (photoview#874) * fix UI tests expecting localhost:3000 as base URL * fix: avoid prefixing UI domain when serving media URLs
…nails from videos (photoview#917)
Video playback starts faster in browsers.
* Don't stop scanning album on media fail * Update api/scanner/scanner_album.go accepting suggestion from Jordan Co-authored-by: Jordan Hellier <13520761+jordy2254@users.noreply.github.com> --------- Co-authored-by: Konstantin Koval <kkb@ukr.net> Co-authored-by: Jordan Hellier <13520761+jordy2254@users.noreply.github.com>
* Remove login page non-empty password requirement * Remove required property from password register
Add a "no_face_detection" build tag to disable face detection when building. This is useful when installing the face detection dependencies is undesirable and cuts down build times (e.g. on a Raspberry Pi).
* chore: upgrade api dependencies to latest compatible version * chore: update ui dependencies first pass * chore: migrate ui dependency to latests Migrates i18next-parser from 6.x -> 8.x per documentation * chore: add missing tools file * chore: regenerate files * chore: fix docker build from incorrect depedency bookworms latest stable version of libheif-dev is 1.15.1
@jordy2254 I added three environment variables and set the value to |
Dockerfile
Outdated
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.
RUN if [ "${TARGETPLATFORM}" = "linux/amd64" ] || [ "${TARGETPLATFORM}" = "linux/arm64" ]; then \
apt install -y darktable; fi
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 3 ENV
lines to the then
branch of this if
, setting the same 3 variables in the else
to the values, compatible with the ImageMagick
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.
ImageMagick
is not in conflict with Darktable
, 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 environments Darktable
can't be installed and ImageMagick
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.
There is no known problem of not being able to handle RAW formats because Darktable can't be installed!
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;
- sync your fork's master branch with photoview
- switch the master branch on your dev machine do a
git pull
- switch back to your feature branch
- this is where things can differ personally I use the first
git rebase master
(you can do-i
if you'd like to change commitsor- OR
git merge master
will do the job
- Fix the conflicts you get from either method. a rebase -i will let you manually drop those stray commits you seemed to of picked up somehow.
If 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?
@fierceX
|
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
@viktorstrate, This is a great catch!
Yes, from a security point of view, this is a critical vulnerability, as we cannot know in advance which executable should be called by the Photoview and rely on the external config (environment variable in this case), which can be modified.
To mitigate this we need to statically resolve the path to the executable at the build time and accept later only the commandline parameters from the variables. This also means that we will need to preinstall all supported executables inside the same image and define paths to them in Dockerfile providing the ability to choose an executable from the predefined list in the scanner options on the Settings page. A similar approach might work for a directly hosted app, where all paths should be provided in some form at the build time, so they are available on the Settings later. If a user wants to add a custom executable (a self-developed one or just something, not tested by us and not supported from the project perspective) in the case of deployment with docker, a local rebuild of the Photoview image will be required.
An alternative way would be to define the AppArmor profile for the Photoview image and allow only a predefined list of executables to be executed inside the container. However, this is more difficult to maintain and would require strong Linux admin skills for a user in the case of direct deployment to a host instead of using docker (which, I believe, is a stop-factor for us, as our target audience is not only Linux gurus but also regular users), while doesn't provide much more flexibility, as we just shift the place of executables hardcoding from the code to the AppArmor profile, so to add a custom executable, the profile should be changed, which requires an image rebuild 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.
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 comment
The 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.
If Photoview is deployed on the host directly, this is the same system, so an attacker has access to the host, as a 1st step.
But in both cases, there are different levels of access: to put a file to the home folder and modify an env variable you don't need admin permissions. The photoview
user can do that. While to modify an existing executable in the system (owned by root), which is registered in the Photoview app, an attacker should become an admin 1st.
Please see the PR #863, where I implemented many security enhancements for our docker image, so now the Photoview app is executed there under non-root user, which has write access to his home folder and mounted app cache (which by default is also inside the home folder). All executables remain owned by root and cannot be modified without root permissions.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
and video converter
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?
How does the scanner create thumbnails for non-RAW images: does it use the same RAW-processing app integration? if not, probably, we need to change that as well? I assume that there is a low probability that a user wants to use a non-default app to process RAW files while keeping the standard one for regular images.
@viktorstrate, @jordy2254, @fierceX, what do you think?
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.
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.
For the latter message In my opinion out of scope for this piece of work. lets keep things in scope and make additional issues/PRs for them as needed to try and keep risk to a minimum and follow CI/CD better.
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.
For the latter message In my opinion out of scope for this piece of work
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?
This is a good approach to give the user an opportunity to redefine what a 3rd-party app should process the incoming media. I'd like to have it available for all types of workers at the final stage (after all corresponding PRs are merged)
I added a custom RAW format conversion function specified by environment variables, for the reasons listed below:
#928