Skip to content

unixcreds: use euid instead of uid#14

Merged
stevvooe merged 1 commit intocontainerd:masterfrom
AkihiroSuda:avoid-os-user
Dec 1, 2017
Merged

unixcreds: use euid instead of uid#14
stevvooe merged 1 commit intocontainerd:masterfrom
AkihiroSuda:avoid-os-user

Conversation

@AkihiroSuda
Copy link
Copy Markdown
Member

@AkihiroSuda AkihiroSuda commented Dec 1, 2017 •
edited
Loading

This commit also eliminates call for os/user.Current(),
which segfaults when glibc is statically linkedin.
(moby/moby#29478) (EDIT: the segfault issue is already handled in #13)

Signed-off-by: Akihiro Suda [email protected]

@AkihiroSuda AkihiroSuda mentioned this pull request Dec 1, 2017
Merged
@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

I've already patched the issue with #13.

What is the benefit of using euid over uid? The shim seems to have always used uid (really, just root).

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

What is the benefit of using euid over uid? The shim seems to have always used uid (really, just root).

IIUC ttrpc can be used from other programs.

Also, rather than checking the credential by ourselves, shouldn't we use sendmsg(2) the ucred structure as a SCM_CREDENTIALS cmsg?

Example: https://github.com/lxc/cgmanager/blob/8f599b54c8021f37c1eeb7394a30b9e5fd0870f9/access_checks.c#L187

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

@AkihiroSuda I reproduced what was already in the shim in the 1.0 for months. The documentation on what is the correct thing to use here is thin, so I don't know.

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

For example, when a suid bit is set to a ttrpc server program, the EUID of the program corresponds to the binary file owner.
In such a case, I expect EUID to be validated as the credential.

cc @estesp PTAL?

@AkihiroSuda AkihiroSuda force-pushed the avoid-os-user branch 2 times, most recently from a8df331 to 89e62cb Compare December 1, 2017 07:00
@crosbymichael
Copy link
Copy Markdown
Member

crosbymichael commented Dec 1, 2017

LGTM

Although i don't think we need to change the function name for this change

@estesp
Copy link
Copy Markdown
Member

estesp commented Dec 1, 2017

Agree with @crosbymichael that the function name was fine; using euid definitely seems correct to cover the setuid case.

@mlaventure
Copy link
Copy Markdown

mlaventure commented Dec 1, 2017

Likewise, no need to be too verbose with the name, the intent stays the same.

And agreed, EUID makes sense here

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

reverted the function name

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

test failure seems unrelated

--- FAIL: TestClientEOF (0.00s)
	server_test.go:326: accept unix @TestClientEOF: use of closed network connection

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

restarted CI and now green

stevvooe
stevvooe reviewed Dec 1, 2017
View reviewed changes
Comment thread unixcreds.go Outdated
//
// This is useful when using abstract sockets that are accessible by all users.
//
// This function validates the *effective* UID/GID rather than the real UID/GID.
Copy link
Copy Markdown
Member

@stevvooe stevvooe Dec 1, 2017

Choose a reason for hiding this comment

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

Move this description to UnixSocketRequireUidGid and point to it here instead.

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

@AkihiroSuda Needs another rebase.

@AkihiroSuda
Copy link
Copy Markdown
Member Author

AkihiroSuda commented Dec 1, 2017

done

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

@AkihiroSuda Sigh. Again, pls. Sorry :(

@stevvooe
Copy link
Copy Markdown
Member

stevvooe commented Dec 1, 2017

LGTM

@stevvooe stevvooe merged commit 76e6834 into containerd:master Dec 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@stevvooe stevvooe stevvooe left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@AkihiroSuda @stevvooe @crosbymichael @estesp @mlaventure