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

Update Dockerfile #618

Merged
merged 1 commit into from
Sep 5, 2021
Merged

Update Dockerfile #618

merged 1 commit into from
Sep 5, 2021

Conversation

juliaszone
Copy link
Contributor

Running programs as root that don't require it is a big security risk. Docker is only containerization and not virtualization.
Instead of using the user "nobody" a custom user may be created, but as shadowsocks-rust only uses networking and only reads the config file I see it as unnecessary.
If the server is intended to bind on a privileged port (<1024) Docker's port mapping may be used or a program such as nftables or setcap may be used.

Running programs as root that don't require it is a big security risk. Docker is only containerization and not virtualization.
Instead of using the user "nobody" a custom user may be created, but as shadowsocks-rust only uses networking and only reads the config file I see it as unnecessary.
If the server is intended to bind on a privileged port (<1024) Docker's port mapping may be used or a program such as nftables or setcap may be used.
Copy link
Contributor

@kallydev kallydev left a comment

Choose a reason for hiding this comment

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

Yes, this is in line with the principle of least privilege, thank you. This modification has passed my test, it can now be merged. @zonyitoo

@zonyitoo zonyitoo merged commit 806ad48 into shadowsocks:master Sep 5, 2021
@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 5, 2021

@kallydev The latest builds were failed somehow.

@juliaszone
Copy link
Contributor Author

@zonyitoo I tried the Dockerfile on my Server (linux, amd64) before creating the PR and it appeared to have worked.
docker build -t sstesting . --build-arg BUILDPLATFORM=linux --build-arg TARGETARCH=amd64 has returned
Successfully built 7521edc99fdb
Successfully tagged sstesting:latest
and when running it and then checking netstat -tupnl inside the container:
tcp 0 0 127.0.0.1:8388 0.0.0.0:* LISTEN 1/ssserver
whoami also returned nobody.

@zonyitoo
Copy link
Collaborator

zonyitoo commented Sep 5, 2021

@kallydev
Copy link
Contributor

kallydev commented Sep 5, 2021

https://github.com/shadowsocks/shadowsocks-rust/actions/runs/1203348443

Check this Action.

The reason for the error is that Docker tried to build the image based on a commit that does not exist in the master branch. The hash value of that commit is a0d91b5.

This is probably because there was a forced push while GitHub Actions was building, and now we just need to rerun workflow.

@kallydev
Copy link
Contributor

kallydev commented Sep 5, 2021

This is probably because there was a forced push while GitHub Actions was building, and now we just need to rerun workflow.

Rerun the workflow doesn't seem to solve the problem, because it has been bound to the corresponding commit.

If necessary, you can fork a local backup branch from master, then force master back to 58381e5 and remove v1.12.0-alpha.2 tag from remote.

Finally, merge the backup branch to the local master, and repush the tag and commits to trigger the correct workflow.

atkdef pushed a commit to atkdef/shadowsocks-rust that referenced this pull request Sep 24, 2021
Running programs as root that don't require it is a big security risk. Docker is only containerization and not virtualization.
Instead of using the user "nobody" a custom user may be created, but as shadowsocks-rust only uses networking and only reads the config file I see it as unnecessary.
If the server is intended to bind on a privileged port (<1024) Docker's port mapping may be used or a program such as nftables or setcap may be used.
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

3 participants