Fix userns-remap option when username & UID match#42004
Conversation
There was a problem hiding this comment.
Actually wondering why we're looking up both; perhaps instead we should return early (try lookup sub-uids by UID -> if error, or no results -> lookup by Name -> repeat for sub-gids).
That said; ISTR there was some discussion about wether "user-name" or "user-id" takes precedence (user-name should probably not be able to take precedence (i.e., a user named 0 should not be able to "trump" root), but linux disagreed on that (some discussion on #21436, opencontainers/runc#695, and opencontainers/runc@0a5293f (opencontainers/runc#708))
There was a problem hiding this comment.
Did a quick try at what it could look like (feedback welcome) earlier;
// NewIdentityMapping takes a requested username and
// using the data from /etc/sub{uid,gid} ranges, creates the
// proper uid and gid remapping ranges for that user/group pair
func NewIdentityMapping(name string) (*IdentityMapping, error) {
usr, err := LookupUser(name)
if err != nil {
return nil, fmt.Errorf("Could not get user for username %s: %v", name, err)
}
subuidRanges, err := lookupSubUIDRanges(usr)
if err != nil {
return nil, err
}
subgidRanges, err := lookupSubGIDRanges(usr)
if err != nil {
return nil, err
}
return &IdentityMapping{
uids: subuidRanges,
gids: subgidRanges,
}, nil
}
func lookupSubUIDRanges(usr user.User) ([]IDMap, error) {
r, err := parseSubuid(strconv.Itoa(usr.Uid))
if err != nil {
return nil, err
}
if len(r) == 0 {
r, err = parseSubuid(usr.Name)
if err != nil {
return nil, err
}
}
if len(r) == 0 {
return nil, errors.Errorf("no subuid ranges found for user %q", usr.Name)
}
return createIDMap(r), nil
}
func lookupSubGIDRanges(usr user.User) ([]IDMap, error) {
r, err := parseSubgid(strconv.Itoa(usr.Uid))
if err != nil {
return nil, err
}
if len(r) == 0 {
r, err = parseSubgid(usr.Name)
if err != nil {
return nil, err
}
}
if len(r) == 0 {
return nil, errors.Errorf("no subgid ranges found for user %q", usr.Name)
}
return createIDMap(r), nil
}There was a problem hiding this comment.
I think it makes sense for UIDs to take precedence, as 99% of the time a numerical value is going to be a UID rather than a username.
I was thinking originally to implement separate functions, I wasn't sure if it was too much boilerplate but semantically it looks better to me.
Do you want me to update the commit with your implementation?
There was a problem hiding this comment.
Yes, there's definitely some boilerplating; I initially kept it inline, and, well, it was still boilerplating, so I extracted it to separate functions.
Definitely feel free to copy my code (check if I didn't make a mistake, as it was a quick write up 😅)
|
@thaJeztah Updated, PTAL. |
Signed-off-by: Grant Millar <[email protected]>
|
Ping @thaJeztah, are we able to merge this and get it into the next release? I hate to push but we really need this to upgrade our production environment. 😇 |
cpuguy83
left a comment
There was a problem hiding this comment.
@tiborvass mentioned we should have a test for this.
Signed-off-by: Grant Millar [email protected]
- What I did
Fixed #42003 by only matching UID or username.
- How I did it
Check
subuidRangesWithUIDfirst thensubuidRangesWithUserName.- How to verify it
echo "10426:x:10426:10426::/nonexistent:/bin/false" >> /etc/passwdecho "10426:x:10426:" >> /etc/groupecho "10426:10426000:10000" >> /etc/subuidecho "10426:10426000:10000" >> /etc/subgiddockerd --userns-remap=10426 &docker run hello-world- Description for the changelog
Fix userns-remap option when username & UID match
- A picture of a cute animal (not mandatory but encouraged)
