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

Support OCI standard dockerfile #2961

Merged
merged 7 commits into from May 14, 2024
Merged

Conversation

SwirtaB
Copy link
Contributor

@SwirtaB SwirtaB commented Feb 26, 2024

I have slightly changed the Dockerfile to better comply with the OCI standard so that other container engines can be used (e.g., podman).

Modifications:

  1. Allow to specify USERNAME of user inside container via --build-arg
  2. Add custom GID for user inside container
  3. Create user without sudo permissions, enforcing fully rootless operation (inside container)
  4. Delete SHELL command which is specific to Docker and update scripts to sh (only pytroch installation required changes)
  5. Minor optimizations (reduced number of layers by aggregating commands)

This image can be successfully built and run with podman fully rootless, without any additional build parameters. I do not have ability to test it against docker build engine I would be grateful if one could test it. If you find this PR worth integrating I can update documentation to address deploying container in fully rootless mode.

@Zunhammer
Copy link
Contributor

Hi,
thanks for the proposes changes. I can confirm that docker build works as well as running the container. However, I disagree to remove sudo as some people might want to install additional software inside the container for testing or development which requires elevated rights (e.g. apt) and without rebuilding the image all the time. IMO this is used more as a dev container so I would like to keep sudo here.

Best

@SwirtaB
Copy link
Contributor Author

SwirtaB commented Feb 27, 2024

Thanks for the answer. I can bring back the ability to run sudo by the user inside the container. That said, I think that it would be beneficial (from a security standpoint) to drop skipping asking for password and allow the end user to provide its own (for example, via --build-arg) with some default clearly stated in doc if someone decided not to use this functionality. What do you think about it?
I will have some time later this week to come up with a proper solution, and I will update this PR accordingly. Should I change this PR to draft or leave it as it is?

@SwirtaB
Copy link
Contributor Author

SwirtaB commented Feb 28, 2024

@Zunhammer
Hi,
I have two propositions to address your issue:

  1. Force the end user to change the default password when the container is created (using the standard passwd command), done via the CMD command in Dockerfile.
  2. Add non root user to sudoers and allow them to use apt install/remove/update.

The first option is quite robust, since end users have the ability to enforce security with a strong password and use sudo without restrictions. Also, by doing it via CMD, one can override this procedure and run the image as executable (as stated in doc).

The second option strives to achieve fine-grained control over user privileges, limiting them to only the necessary ones (installing, removing and updating packages from the system repo). Unfortunately, this is more prone to errors on the maintainer's side, and I think that the first one is better due to its simplicity.

What do you think?

@Zunhammer
Copy link
Contributor

Hi SwirtaB,

thanks for evaluating and sharing the options. Personally, I think it's bothering to change the password on each container start which is why I would prefer the second option.

However, the main purpose (IMO) of this container is to use it for development. So, I would keep it simple and mainly easy to use at this point. People who want to use the container in areas relevant for security should adopt the Dockerfile anyway.

Looking foward to get your feedback. Would be nice to have other opinions as well :)

Best regards

@SwirtaB
Copy link
Contributor Author

SwirtaB commented Mar 2, 2024

Personally, I am a strong advocate for enforcing good security practices, especially in FOSS. Nerfstudio has a lot of dependencies that are outside nerfstudio team control and can be exploited, e.g., in supply chain attacks. Ruling out such a scenario without taking any action just because it is unlikely is questionable, in my opinion. That said, I completely understand that this project isn't production-grade software but is provided as is (though with good intentions ;) ), and the end user is responsible for preventing any unwanted behaviors.

Personally, I think that changing the password on the first container run isn't that much of a problem, assuming the container is stopped and not deleted after exiting (no --rm flag). But I do understand that one can be against that behavior. So I propose three actions:

  1. Add back unrestricted sudo.
  2. Clearly state in the documentation that this can be exploited, so that the end user is properly informed.
  3. Keep in Dockerfile commented lines that will enforce security inside the container. That way, any user who wishes to develop in a stricter environment can easily build an image without sudo permissions.

In summary, container workflow stays the same (sudo permissions), users are informed about potential dangers and can decide for themselves, and users who want to enforce best practices can do so by commenting clearly marked parts of Dockerfile and building a custom image.

I can make changes in coming week, so that there will be actual Dockerfile to look at. Many thanks for your feedback and pointing out too strict approach from my side.

That is quite the long one :)

@SwirtaB
Copy link
Contributor Author

SwirtaB commented Mar 4, 2024

I have updated this PR. I brought back sudo and added ability for user to run apt-get update/upgrade/install/remove without typing password.

@SwirtaB
Copy link
Contributor Author

SwirtaB commented Mar 28, 2024

@Zunhammer any thoughts?

@jb-ye
Copy link
Collaborator

jb-ye commented Apr 24, 2024

@Zunhammer Could you follow up this PR? Does the current state look good to you?

Custom rules for sudo group
Comented lines for easy image modification
Copy link
Contributor

@Zunhammer Zunhammer left a comment

Choose a reason for hiding this comment

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

Hey @SwirtaB

sorry for the late reply. To me it looks good.

@SwirtaB
Copy link
Contributor Author

SwirtaB commented May 13, 2024

No worries; I am glad I could contribute to this project.

@Zunhammer Zunhammer enabled auto-merge (squash) May 13, 2024 19:47
@jb-ye jb-ye disabled auto-merge May 14, 2024 14:40
@jb-ye jb-ye enabled auto-merge (squash) May 14, 2024 18:59
@jb-ye jb-ye merged commit dd811f5 into nerfstudio-project:main May 14, 2024
2 checks passed
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