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

rootless: optional support for generating config with subuid map #1692

Closed
wants to merge 1 commit into from
Closed

rootless: optional support for generating config with subuid map #1692

wants to merge 1 commit into from

Conversation

AkihiroSuda
Copy link
Member

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

runc spec --rootless --rootless-subuid generates a config with multiple uidMappings and gidMappings. (See #1529)

@AkihiroSuda AkihiroSuda mentioned this pull request Jan 15, 2018
Merged
@AkihiroSuda AkihiroSuda force-pushed the rootless-map-subuid branch 2 times, most recently from 298d6cd to 5774f21 Compare January 15, 2018 07:35
@AkihiroSuda AkihiroSuda mentioned this pull request Jan 15, 2018
Closed
@AkihiroSuda
Copy link
Member Author

updated to support execution within userns

@AkihiroSuda AkihiroSuda mentioned this pull request Jan 16, 2018
Merged
@cyphar
Copy link
Member

cyphar commented Jan 16, 2018 •
edited
Loading

I'm not a huge fan of this, though I might still be convinced. runc spec --rootless was already a bit too much in terms of trying to provide more configuration with regards to config generation (something users should do with oci-runtime-tools). But since this sources information from /etc/subuid it might be a different story.

However, there is another problem, which is that in many cases users shouldn't be mapping all of their allocated subuids/subgids for each container. They should be using independent sets of uids and gids (this is something that Docker gets very, very wrong -- though there are technical reasons why they made the compromise -- but we shouldn't be repeating that mistake). And example of this done more correctly is rkt or LXC. With that in mind, I'm not sure that you could automatically decide what the best sub-range is of a user's /etc/sub{uid,gid} sets...

@cyphar cyphar self-assigned this Feb 4, 2018
@cyphar cyphar assigned cyphar and unassigned cyphar Feb 23, 2018
@AkihiroSuda
Copy link
Member Author

How about adding UID/GID range fields to RootlessOpts?
If empty entire spaces will be used.

cc @jessfraz

@jessfraz
Copy link
Contributor

I'm not sure the best approach here to be honest, but not making the same mistakes as docker sounds good haha

@cyphar
Copy link
Member

cyphar commented Feb 26, 2018

Just to note that LXC also does the right thing here -- they allocate sub-sections of the available /etc/sub{uid,gid} ranges. But I'm not sure whether they deal with collisions by storing stuff on-disk -- which would be nice to avoid. I'll ping @brauner elsewhere and see how they do this or if they have any tips.

AkihiroSuda
AkihiroSuda commented Feb 27, 2018
View reviewed changes
libcontainer/specconv/example.go Outdated
spec.Linux.GIDMappings = append(spec.Linux.GIDMappings,
specs.LinuxIDMapping{
HostID: uint32(subgid.SubID),
ContainerID: uint32(uNextContainerID),
Copy link
Member Author

Choose a reason for hiding this comment

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

typo: will fix immediately

Copy link
Member Author

Choose a reason for hiding this comment

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

done (cc @jessfraz )

@AkihiroSuda AkihiroSuda force-pushed the rootless-map-subuid branch 2 times, most recently from e835e87 to aa80caf Compare February 27, 2018 07:51
@cyphar cyphar mentioned this pull request Mar 4, 2018
Open
@AkihiroSuda AkihiroSuda mentioned this pull request Apr 2, 2018
Closed
@AkihiroSuda AkihiroSuda force-pushed the rootless-map-subuid branch 2 times, most recently from c4ab5c9 to b3069aa Compare June 9, 2018 14:46
@AkihiroSuda
Copy link
Member Author

Removed CLI and added godoc ,as this seems controversial, although already used in img and rootless BuildKit.

@AkihiroSuda
rootless: optional support for generating config with subuid map
898ae8c 
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@AkihiroSuda
Copy link
Member Author

rebased

@AkihiroSuda
Copy link
Member Author

Any thought?

AkihiroSuda
AkihiroSuda commented Jun 28, 2018
View reviewed changes
libcontainer/specconv/example.go
//
// When running in userns, MapAllSubIDs is ignored and
// /proc/self/[ug]id_map entries are used.
MapAllSubIDs bool
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to add more options such as KeepNetworkNamespace as well after this PR gets merged.

AkihiroSuda
AkihiroSuda commented Jul 4, 2018
View reviewed changes
libcontainer/specconv/example.go
//
// MapAllSubIDs requires newuidmap(1) and newgidmap(1) with suid bit.
//
// When running in userns, MapAllSubIDs is ignored and
Copy link
Member Author

Choose a reason for hiding this comment

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

As this is very confusing, I'm temporary closing this PR.

Please also see #1837

@AkihiroSuda AkihiroSuda closed this Jul 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@cyphar cyphar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@AkihiroSuda @cyphar @jessfraz