WIP Implementation of peer credentials. #13

Kixunil · 2017-06-15T22:22:25Z

This implements peer credentials for UnixStream. Closes #12.

This is WIP now. It contains only Linux implementation (macOS coming soon). I published it now so I can have it reviewed to avoid huge rewrite.

I decided to name the method peer_cred instead of get_peer_cred to be consistent with other methods (e.g. peer_addr).

The implementation is based on one I found in PostgreSQL.
Relevant SO question

I think this might be added to std and then forward it but I think it'd be nice to use this crate as a testing ground.

Unresolved questions:

Bikeshed names (I'm fairly confident about at least method name but I'm open to other suggestions.)
Should I implement for Solaris too? I have nothing to test it on.
Tests? (I don't see any tests in this crate but that doesn't mean there shouldn't be.)
Is documentation sufficient?
Someone else should probably review the unsafe code (it should be just syscall though, nothing complicated)
~~Should I define #[repr(C)] struct UCred manually or should it be somehow auto-generated? I didn't find it in libc. (Maybe PR against `libc?)~~ I found it. Fixed in e0da3a9.
Is this method applicable for other Unix sockets? (E.g. datagram.)

…acOS.

Kixunil · 2017-06-15T23:18:57Z

Note: I've created a simple test to play with it.

Kixunil · 2017-06-16T19:15:11Z

Stupid mistake. :D Anyway, I've tested it on macOS and it works! 🎉

Could you comment on the code and help with resolving the questions, please?

alexcrichton · 2017-06-19T14:37:18Z

src/ucred.rs

+        unsafe {
+            let raw_fd = sock.as_raw_fd();
+
+            let mut cred: super::UCred = mem::uninitialized();


Can this use zeroed? Seems like there's no reason to use the unsafer version here?

It could be. But I see no point. My reasoning is that the code is already unsafe so I see no point in zeroing it. However, If you have good reason to do it, I'd probably go with explicit safe version (pid: 0, uid: 0, gid: 0).

Let's use zeroed here.

Why? Is that some kind of standard?

Because I see no reason to use uninitialized, please change it to zeroed.

I meant zeroed instead of ucred { pid: 0, uid: 0, gid: 0 }.

I guess that syscall only overwrites it, so the reason would be performance. I agree it's negligible, so I accept zeroig. I'm just interested to know whether there is some difference between using zeroed and zeroing "manually".

Nah those are the same. With libc I tend to use zeroed because there's often random hidden fields or fields used for padding, but either way is fine in this case.

Ah, OK. I prefer "safe" version in this case.

Can this be udpated to using UCred { ... }?

alexcrichton · 2017-06-19T14:37:32Z

src/lib.rs

@@ -279,6 +282,12 @@ impl UnixStream {
        self.io.get_ref().peer_addr()
    }

+    /// Returns effective credentials of the process which called `connect` or `socketpair`.
+    #[cfg(any(target_os = "linux", target_os = "macos"))]
+    pub fn peer_cred(&self) -> io::Result<UCred> {


Could this be added via an extension trait if it's platform-specific?

I was thinking about it too. I'm not sure if Rust runs on any platform that has UnixStream but no way of getting peer credentials. Solaris seems to have different implementation, but I have no machine to test it.

If I wanted to use a trait how to name it? LinuxAndMacPeerCredentials seems long to me. I wouldn't separate them provided there can be a common trait for both. (And I'm actually currently working on code that should be Linux/macOS.) On the other hand, Linux has possibility to provide pid too.

Do you think all other unices have an implementation of this function? If that's the case then this should just remove the #[cfg].

Digging into the topic more I found that:

On Linux, getsockopt should be used

BSD families (including macOS) use getpeereid

Solaris uses getpeerucred

I know of no other unix used in the wild with Rust.

So maybe we can just remove #[cfg]. However, it'd be nice to implement this for Solaris too. But as I said, I have nothing to test it on.

Also, macos cfg will have to be changed to #[cfg(any(target_os = "macos", target_os = "ios", target_os = "freebsd"))]

What do you think?

This either needs to be ungated to cause compile failures on other platforms to necessitate an implementation, or it needs an extension trait. I don't mind which, but there's no middle between those tw.

OK, I'll go with ungated. Gating pushes the compilation error to consumer, so there'd still be the error. If you have some explanation about why compile error in this crate is better than at consumer, I'm interested in learning it. (I personally don't mind using the one you choose.)

If we don't have a cfg here then we're claiming it's available on all unices, both in the code and in the documentation. At that point we need an assertion as such, which is to attempt to compile the code on all platforms.

alexcrichton · 2017-06-19T14:38:46Z

src/ucred.rs

+    use UnixStream;
+    use std::os::unix::io::AsRawFd;
+
+    use libc::ucred as UCred;


Can this avoid being renamed? It confused me when considering the outer one as well.

Well, I dislike having lower case types. So maybe LinuxUCred? Or RawUCred? CUCred?

Let's stick to the name already given to it by libc, ucred

Kixunil · 2017-06-20T19:03:12Z

I've updated the code as you requested. I tested it on Linux and it works. Will test on macOS tomorrow.

alexcrichton · 2017-06-21T14:31:39Z

Mind adding a test in this PR? That way it can be tested on Travis

Kixunil · 2017-06-21T16:33:58Z

OK, do you think something like this would be fine? Possibly checking uid against geteuid() (same with gid).

alexcrichton · 2017-06-22T14:22:00Z

Sure!

Kixunil · 2017-06-22T19:36:19Z

🎉

Anything else you would like? Do you think the documentation is sufficient?

alexcrichton · 2017-06-22T19:45:55Z

Nah looks good, thanks!

Kixunil · 2017-06-22T21:26:46Z

I'm happy I helped. I've opened rust-lang/rust#42839 now.

Kixunil added 4 commits June 16, 2017 00:03

Linux implementation of peer credentials.

7270717

Use struct ucred from libc crate instead of our own.

e0da3a9

Dropped support for getting PID, since there's no way to do this on m…

49c09e8

…acOS.

Untested implementation of peer_cred for macOS.

23941f9

Fix for macOS pid -> gid.

c25564d

alexcrichton reviewed Jun 19, 2017

View reviewed changes

Implemented requested changes. Fixed cfg for non-Linux unices.

1efd6b3

Added test for peer credentials.

db78129

alexcrichton merged commit a11335c into tokio-rs:master Jun 22, 2017

Kixunil deleted the ucred branch June 22, 2017 21:11

Kixunil mentioned this pull request Jun 22, 2017

Peer credentials on Unix socket rust-lang/rust#42839

Open

Kixunil mentioned this pull request Jul 11, 2017

Add UserCredentials struct as the result of PeerCredentials sockopt nix-rust/nix#608

Closed

zeenix mentioned this pull request Dec 14, 2019

Peer credentials on Unix socket async-rs/async-std#624

Open

joechrisellis mentioned this pull request Aug 4, 2020

Implementation of peer credentials for Unix sockets rust-lang/rust#75148

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Implementation of peer credentials. #13

WIP Implementation of peer credentials. #13

Kixunil commented Jun 15, 2017 •

edited

Loading

Kixunil commented Jun 15, 2017

Kixunil commented Jun 16, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 19, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 20, 2017

alexcrichton Jun 20, 2017

Kixunil Jun 20, 2017

alexcrichton Jun 20, 2017

Kixunil Jun 20, 2017

alexcrichton Jun 21, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 19, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 20, 2017

alexcrichton Jun 20, 2017

Kixunil Jun 20, 2017

alexcrichton Jun 20, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 19, 2017

alexcrichton Jun 19, 2017

Kixunil Jun 20, 2017

Kixunil commented Jun 20, 2017

alexcrichton commented Jun 21, 2017

Kixunil commented Jun 21, 2017

alexcrichton commented Jun 22, 2017

Kixunil commented Jun 22, 2017

alexcrichton commented Jun 22, 2017

Kixunil commented Jun 22, 2017

WIP Implementation of peer credentials. #13

WIP Implementation of peer credentials. #13

Conversation

Kixunil commented Jun 15, 2017 • edited Loading

Kixunil commented Jun 15, 2017

Kixunil commented Jun 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kixunil commented Jun 20, 2017

alexcrichton commented Jun 21, 2017

Kixunil commented Jun 21, 2017

alexcrichton commented Jun 22, 2017

Kixunil commented Jun 22, 2017

alexcrichton commented Jun 22, 2017

Kixunil commented Jun 22, 2017

Kixunil commented Jun 15, 2017 •

edited

Loading