cpp-best-practices / gui_starter_template

A template CMake project to get you started with C++ and tooling
The Unlicense
2.5k stars 447 forks source link

WIP Feature/add non root user in dev container #200

Closed Jason5480 closed 2 years ago

Jason5480 commented 2 years ago

Make docker dev container safer to use by logging to a non-root user as described here https://aka.ms/vscode-remote/containers/non-root . Clean up some defaulted arguments. Install cmake_format in the container

codecov-commenter commented 2 years ago

Codecov Report

Merging #200 (2f23178) into main (687c3d2) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #200   +/-   ##
=======================================
  Coverage   80.00%   80.00%           
=======================================
  Files           3        3           
  Lines          30       30           
=======================================
  Hits           24       24           
  Misses          6        6           
Flag Coverage Δ
Linux ?
Windows 80.00% <ø> (ø)
macOS ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 687c3d2...2f23178. Read the comment docs.

Jason5480 commented 2 years ago

Unfortunately, this causes some low level conan package installation to fail since they are dependent on system packages and somehow the installation does not work for non-root users... @ddalcino did you face that also in the past during the very first implementation? CONAN_SYSREQUIRES_MODE=enabled doesn't seem to work for regular users.

Jason5480 commented 2 years ago

Unfortunately, this causes some low level conan package installation to fail since they are dependent on system packages and somehow the installation does not work for non-root users... @ddalcino did you face that also in the past during the very first implementation? CONAN_SYSREQUIRES_MODE=enabled doesn't seem to work for regular users.

The errors are not caused by the user changes of this branch. I went in the main branch and a new dependency was needed (pkg-config) when I added that, I got the following error

image

I will create a separate issue for that

lefticus commented 2 years ago

@Jason5480 I'm currently trying to reorganize things into a light-weight "boilerplate" project and this "starter_project"

Could I trouble you to move this PR over to https://github.com/cpp-best-practices/cpp_boilerplate_project, since that is the new baseline project, and we'll then merge the changes back into the starter_project?

ddalcino commented 2 years ago

Unfortunately, this causes some low level conan package installation to fail since they are dependent on system packages and somehow the installation does not work for non-root users... @ddalcino did you face that also in the past during the very first implementation? CONAN_SYSREQUIRES_MODE=enabled doesn't seem to work for regular users.

CONAN_SYSREQUIRES_MODE=enabled means that the Conan process does not need superuser privileges, because it already has them. Many Conan packages have dependencies that must be installed via the system package manager, and typically, sudo is required for this. Without CONAN_SYSREQUIRES_MODE=enabled, you will need to apt-get install sudo. I don't know how to make this work without superuser privileges, unless you stick to packages that don't need it. That means no GUI libraries like SDL.

You could try going through each package's dependencies manually, and installing them yourself; I've done this in the past, and gotten SDL to build entirely without superuser privileges. It was ridiculous, and I'll never do it again. I do not recommend this approach.

lefticus commented 2 years ago

Unfortunately, this causes some low level conan package installation to fail since they are dependent on system packages and somehow the installation does not work for non-root users... @ddalcino did you face that also in the past during the very first implementation? CONAN_SYSREQUIRES_MODE=enabled doesn't seem to work for regular users.

CONAN_SYSREQUIRES_MODE=enabled means that the Conan process does not need superuser privileges, because it already has them. Many Conan packages have dependencies that must be installed via the system package manager, and typically, sudo is required for this. Without CONAN_SYSREQUIRES_MODE=enabled, you will need to apt-get install sudo. I don't know how to make this work without superuser privileges, unless you stick to packages that don't need it. That means no GUI libraries like SDL.

You could try going through each package's dependencies manually, and installing them yourself; I've done this in the past, and gotten SDL to build entirely without superuser privileges. It was ridiculous, and I'll never do it again. I do not recommend this approach.

Thank's @ddalcino for your insight - I really know nothing about container usage in this way

Jason5480 commented 2 years ago

@Jason5480 I'm currently trying to reorganize things into a light-weight "boilerplate" project and this "starter_project"

Could I trouble you to move this PR over to https://github.com/cpp-best-practices/cpp_boilerplate_project, since that is the new baseline project, and we'll then merge the changes back into the starter_project?

I created that one https://github.com/cpp-best-practices/cpp_boilerplate_project/pull/8 where I added some missing format tools into the container. @ddalcino I will reconsider the non-root user solution and I will try to find a solution to work with conan (if any). Using root user in containers is not accepted in many companies for security reasons...

Jason5480 commented 2 years ago

@Jason5480 I'm currently trying to reorganize things into a light-weight "boilerplate" project and this "starter_project"

Could I trouble you to move this PR over to https://github.com/cpp-best-practices/cpp_boilerplate_project, since that is the new baseline project, and we'll then merge the changes back into the starter_project?

I created that one https://github.com/cpp-best-practices/cpp_boilerplate_project/pull/8 where I added some missing format tools to the container. @ddalcino I will reconsider the non-root user solution and I will try to find a solution to work with conan (if any). Using root user in containers in not accepted in many companies for security reasons

Jason5480 commented 2 years ago

Unfortunately, this causes some low level conan package installation to fail since they are dependent on system packages and somehow the installation does not work for non-root users... @ddalcino did you face that also in the past during the very first implementation? CONAN_SYSREQUIRES_MODE=enabled doesn't seem to work for regular users.

CONAN_SYSREQUIRES_MODE=enabled means that the Conan process does not need superuser privileges, because it already has them. Many Conan packages have dependencies that must be installed via the system package manager, and typically, sudo is required for this. Without CONAN_SYSREQUIRES_MODE=enabled, you will need to apt-get install sudo. I don't know how to make this work without superuser privileges, unless you stick to packages that don't need it. That means no GUI libraries like SDL.

You could try going through each package's dependencies manually, and installing them yourself; I've done this in the past, and gotten SDL to build entirely without superuser privileges. It was ridiculous, and I'll never do it again. I do not recommend this approach.

Do you know any command to check if the current user is a superuser and set those variables accordingly?

ddalcino commented 2 years ago

Do you know any command to check if the current user is a superuser and set those variables accordingly?

You can try checking $EUID == 0, or google it. Personally, I think I would prefer an option that I can set (possibly an ARG) that controls whether or not this runs in superuser mode or not, and set the default to non-superuser. Projects that use SDL etc. would have to either use the option, change the default, or install the dependency outside of Conan.

Thanks for looking into this PR, this is a serious security issue. I hated writing the original in this way, but I didn’t see an alternative.

ddalcino commented 2 years ago

CONAN_SYSREQUIRES_MODE=enabled means that the Conan process does not need superuser privileges, because it already has them. Many Conan packages have dependencies that must be installed via the system package manager, and typically, sudo is required for this. Without CONAN_SYSREQUIRES_MODE=enabled, you will need to apt-get install sudo. I don't know how to make this work without superuser privileges, unless you stick to packages that don't need it. That means no GUI libraries like SDL.

Whoops, looks like I got this backwards. I was thinking of CONAN_SYSREQUIRES_SUDO; see https://docs.conan.io/en/latest/reference/env_vars.html?highlight=conan_sysrequires_mode#conan-sysrequires-sudo. CONAN_SYSREQUIRES_MODE is documented here: https://docs.conan.io/en/latest/reference/env_vars.html?highlight=conan_sysrequires_mode#conan-sysrequires-mode

If I had read the comments I left in the Dockerfile, I would not have made this mistake.