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

Create dedicated Docker 'scraper' user #1321

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Create dedicated Docker 'scraper' user #1321

wants to merge 1 commit into from

Conversation

kelson42
Copy link
Collaborator

@kelson42 kelson42 commented Dec 6, 2020

Fixes #995

@kelson42 kelson42 requested a review from rgaudin December 6, 2020 16:58
@kelson42
Copy link
Collaborator Author

kelson42 commented Dec 6, 2020

@rgaudin This is an attempt to make our Docker a bit more secure. But this has for direct consequence that the mounted volumed should be writeable by the new user scraper. Not sure as well about the Redis daemon... which user runs it? Should we change it?

@stale
Copy link

stale bot commented Dec 13, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Dec 13, 2020
@stale stale bot removed the stale label Jun 13, 2022
@codecov
Copy link

codecov bot commented Jun 13, 2022

Codecov Report

Merging #1321 (d945025) into master (ebb737e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1321   +/-   ##
=======================================
  Coverage   69.42%   69.42%           
=======================================
  Files          26       26           
  Lines        2401     2401           
  Branches      469      469           
=======================================
  Hits         1667     1667           
  Misses        569      569           
  Partials      165      165           

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 ebb737e...d945025. Read the comment docs.

Copy link
Member

@rgaudin rgaudin left a comment

Choose a reason for hiding this comment

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

  • most rootless images defaults to user 1001 as this is what a regular distro would create so it has more chances to work out of the box.
  • rootless implies choosing a user so uid and gid might be anything
  • /data is gonna be mounted and there's no guarantee you'll be able to write on it. You may want to check that in entrypoint to provide a quick and clear feedback should permissions not be adequate. I don't know how quick and clear mwoffliner is gonna fail in this case.
  • README may need to be updated to inform about write requirements on mounted volume source.

@stale
Copy link

stale bot commented Jul 10, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Jul 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker user should not be root
2 participants