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

crun: write setgroups=deny when mapping a single uid/gid #1089

Merged
merged 1 commit into from
Dec 8, 2022

Conversation

maleadt
Copy link
Contributor

@maleadt maleadt commented Dec 7, 2022

Closes #1088

cc @giuseppe

@maleadt maleadt force-pushed the setgroups_deny branch 2 times, most recently from 919d207 to 32a2a7c Compare December 7, 2022 10:46
src/libcrun/linux.c Show resolved Hide resolved
src/libcrun/linux.c Show resolved Hide resolved
@@ -2868,6 +2887,7 @@ libcrun_set_usernamespace (libcrun_container_t *container, pid_t pid, libcrun_er
}

single_mapping = format_mount_mapping (container->container_uid, container->host_uid, 1, &single_mapping_len, true);

Copy link
Member

Choose a reason for hiding this comment

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

spurious change

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

src/libcrun/linux.c Show resolved Hide resolved
Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM comment https://github.com/containers/crun/pull/1089/files#r1042260747 is still unresolved though

@maleadt
Copy link
Contributor Author

maleadt commented Dec 7, 2022

Ah, I replied on that comment in the wrong place: I included the whitespace change to align the gid case with the uid case, where there is an additional newline:

crun/src/libcrun/linux.c

Lines 2836 to 2838 in 2700598

single_mapping = format_mount_mapping (container->container_gid, container->host_gid, 1, &single_mapping_len, true);
ret = write_file (gid_map_file, single_mapping, single_mapping_len, err);

crun/src/libcrun/linux.c

Lines 2870 to 2871 in 2700598

single_mapping = format_mount_mapping (container->container_uid, container->host_uid, 1, &single_mapping_len, true);
ret = write_file (uid_map_file, single_mapping, single_mapping_len, err);

... but I can just remove that if you prefer.

@giuseppe
Copy link
Member

giuseppe commented Dec 7, 2022

you need to run make clang-format

@maleadt
Copy link
Contributor Author

maleadt commented Dec 7, 2022

Done.

@giuseppe
Copy link
Member

giuseppe commented Dec 7, 2022

@lsm5 fedora-rawhide-mockbuild is failing

@giuseppe
Copy link
Member

giuseppe commented Dec 7, 2022

nit, if you are going to re-push, could you make the first line shorter in the commit message (e.g. crun: write setgroups=deny when mapping a single uid/gid and if you'd like add more details in the following lines)

@giuseppe
Copy link
Member

giuseppe commented Dec 7, 2022

tests failures do not depend on this PR. It seems github moved to cgroup v2, opened a PR: #1090

@giuseppe
Copy link
Member

giuseppe commented Dec 8, 2022

could you please rebase on top of main?

Signed-off-by: Tim Besard <tim.besard@gmail.com>
@maleadt
Copy link
Contributor Author

maleadt commented Dec 8, 2022

Sure; done.

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

@giuseppe giuseppe merged commit eaa116b into containers:main Dec 8, 2022
@maleadt maleadt deleted the setgroups_deny branch December 8, 2022 21:06
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.

rootless on kernel 5.4: write to gid_map fails, requires setgroups=deny
3 participants