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

unixcreds: use euid instead of uid #14

Merged
merged 1 commit into from
Dec 1, 2017

Conversation

AkihiroSuda
Copy link
Member

@AkihiroSuda AkihiroSuda commented Dec 1, 2017

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 suda.akihiro@lab.ntt.co.jp

@stevvooe
Copy link
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
Member Author

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
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
Member Author

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
Member

LGTM

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

@estesp
Copy link
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

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

And agreed, EUID makes sense here

@AkihiroSuda
Copy link
Member Author

reverted the function name

@AkihiroSuda
Copy link
Member Author

test failure seems unrelated

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

@AkihiroSuda
Copy link
Member Author

restarted CI and now green

unixcreds.go Outdated
// UnixCredentialsFunc that will validate incoming unix connections against the
// current credentials.
//
// 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
Member

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
Member

stevvooe commented Dec 1, 2017

@AkihiroSuda Needs another rebase.

@AkihiroSuda
Copy link
Member Author

done

@stevvooe
Copy link
Member

stevvooe commented Dec 1, 2017

@AkihiroSuda Sigh. Again, pls. Sorry :(

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@stevvooe
Copy link
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants