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.

Sign up for GitHub

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

Jump to bottom

replace 777 by 775 and change file owner to seluser #1963

Closed
wants to merge 2 commits into from
Closed

replace 777 by 775 and change file owner to seluser #1963

wants to merge 2 commits into from

Conversation

eravion
Copy link

@eravion eravion commented Oct 2, 2023 •
edited
Loading

Thanks for contributing to the Docker-Selenium project!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines, applied for this repository.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@CLAassistant
Copy link

CLAassistant commented Oct 2, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@eravion eravion closed this Oct 2, 2023
@eravion eravion reopened this Oct 2, 2023
@diemol
Copy link
Member

diemol commented Oct 3, 2023

Can you explain why this is needed?

@eravion
Copy link
Author

eravion commented Oct 4, 2023

Hello @diemol ,
We have security check on docker folder rights, 777 are not allowed by our security dept..
This change fix this issue.
One border effect is managed by #1964

@eravion eravion mentioned this pull request Oct 4, 2023
Closed
8 tasks
@VietND96
Copy link
Member

Hi @eravion, may I know your security dept checks this compliance and reports it by which tool?

@eravion
Copy link
Author

eravion commented Nov 17, 2023

Hello @VietND96
Internal tool, sorry cannot share more.

@luisfcorreia
Copy link
Contributor

This change seems logical to me.
Must investigate if there are any side effects

@eravion
Copy link
Author

eravion commented Nov 22, 2023

thanks @luisfcorreia
Side effect identify during my test fixed by : #1964

diemol
diemol requested changes Nov 23, 2023
View reviewed changes
Copy link
Member

@diemol diemol left a comment

Choose a reason for hiding this comment

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

Can you please check the comments and update your PR with the changes from trunk?

Base/Dockerfile
Comment on lines +64 to +65
# change right for /opt/bin*.sh file
RUN chmod 775 /opt/bin/*.sh
Copy link
Member

Choose a reason for hiding this comment

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

Please do the COPY with chmod as mentioned in the Docker documentation https://docs.docker.com/engine/reference/builder/#copy

Base/Dockerfile
@@ -72,7 +74,10 @@ COPY supervisord.conf /etc
#==========
RUN mkdir -p /opt/selenium /opt/selenium/assets /var/run/supervisor /var/log/supervisor \
&& touch /opt/selenium/config.toml \
&& chmod -R 777 /opt/selenium /opt/selenium/assets /var/run/supervisor /var/log/supervisor /etc/passwd \
# && chown seluser:seluser /var/run/supervisor /var/log/supervisor /opt/selenium /etc/passwd \
Copy link
Member

Choose a reason for hiding this comment

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

Why are you keeping this commented line?

Base/Dockerfile
@@ -72,7 +74,10 @@ COPY supervisord.conf /etc
#==========
RUN mkdir -p /opt/selenium /opt/selenium/assets /var/run/supervisor /var/log/supervisor \
&& touch /opt/selenium/config.toml \
&& chmod -R 777 /opt/selenium /opt/selenium/assets /var/run/supervisor /var/log/supervisor /etc/passwd \
# && chown seluser:seluser /var/run/supervisor /var/log/supervisor /opt/selenium /etc/passwd \
&& chown seluser:seluser /var/run/supervisor /var/log/supervisor /opt/selenium \
Copy link
Member

Choose a reason for hiding this comment

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

Why did you remove /etc/passwd?

@VietND96
Copy link
Member

Hi @eravion, 777 removal is already considered via PR #2056.
A new image tag will be released soon for your scan.
Thank you!

@VietND96
Copy link
Member

Please use the new image tag 4.16.1-20231212 for your evaluation.
I will close the PR, feel free to continue the conversation around this feat #2056 or reopen this PR if something else you want to add. Thanks!

@VietND96 VietND96 closed this Dec 12, 2023
@VietND96 VietND96 mentioned this pull request Dec 20, 2023
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@diemol diemol diemol requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@eravion @CLAassistant @diemol @VietND96 @luisfcorreia