Implementation of peer credentials for Unix sockets #75148

joechrisellis · 2020-08-04T14:57:44Z

The code in ucred.rs is based on the work done in PR 13 in the tokio-uds repository on GitHub.

This commit is effectively a port to the stdlib, so credit to Martin Habovštiak (@Kixunil) and contributors for the meat of this work. 🥇

Happy to make changes as needed. 🙂

rust-highfive · 2020-08-04T14:57:50Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @withoutboats (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Kixunil · 2020-08-04T17:31:59Z

Awesome, thanks for the credit and for your additional work on getting this to std! Looking forward to another opportunity when I can use it. :)

Kixunil · 2020-08-04T17:42:06Z

library/std/src/sys/unix/ext/ucred.rs

+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)]
+pub struct UCred {
+    pub uid: uid_t,
+    pub gid: gid_t,


something_t types are not used in std often (at all?), is this a good idea?

Personally I'd love seeing use of newtypes as done in nix, but not sure if that should go to std.

I did a quick grep of the codebase and I can see a few places where these are used... I'll leave it as it is for now! If you would strongly like this to change feel free to follow up. 🙂

Kixunil · 2020-08-04T17:44:36Z

library/std/src/sys/unix/ext/ucred.rs

+        let mut ucred_size = ucred_size as u32;
+
+        unsafe {
+            let mut ucred: ucred = MaybeUninit::uninit().assume_init();


I don't think assume_init is correct here. I believe raw ptr should be obtained from MaybeUninit, then passed to getsockopt and only after it succeeds calling assume_init

Done -- I opted against using MemUninit in the end, it doesn't seem necessary.

joechrisellis · 2020-08-05T13:43:19Z

library/std/src/sys/unix/ext/ucred.rs

+                &mut ucred_size,
+            );
+
+            if ret == 0 && ucred_size as usize == mem::size_of::<ucred>() {


Fixed. 😄

Kixunil · 2020-08-04T17:46:13Z

library/std/src/sys/unix/ext/ucred.rs

+    pub fn peer_cred(socket: &UnixStream) -> io::Result<UCred> {
+        unsafe {
+            // Create `cred` and attempt to populate it.
+            let mut cred: UCred = MaybeUninit::uninit().assume_init();


Same issue with assume_init here

See above. 😄

Kixunil · 2020-08-05T12:58:51Z

library/std/src/sys/unix/ext/ucred.rs

+        assert!(ucred_size <= u32::max_value() as usize);
+
+        let mut ucred_size = ucred_size as u32;
+        let mut ucred: ucred = ucred { pid: 1, uid: 1, gid: 1 };


Oooh, nice defensive programming using 1 instead of 0! ❤️

library/std/src/sys/unix/ext/ucred.rs

ionut-arm · 2020-08-05T19:31:07Z

library/std/src/sys/unix/ext/ucred.rs

+pub struct UCred {
+    pub uid: uid_t,
+    pub gid: gid_t,
+}


Out of curiosity - why not return the process ID too, since that's returned by the syscall?

PID is only available on Linux, it's not available on BSD.

Is it possible to include it (conditionally) on Linux?

Technically yes. It'd require change of the API due to coding standard in std (mainly using extension traits for additional platforms, IDK why such standard exists).

Ah, fair enough, I was hoping for a small, clean change :) nevermind then

Could we make the PID an Option<pid_t>? A quick search reveals that it is in fact supported on quite a few BSD platforms (e.g. FreeBSD).

Done -- however, my Google-fu is failing me here. I can't find anything about retrieving the PID from a domain socket on FreeBSD. The closest I can find is getpeereid which accepts a UID and GID only. So at the moment, the optional PID is set to None for BSD, and Some(pid) on Linux.

It seems to have been added relatively recently: https://svnweb.freebsd.org/base?view=revision&revision=348419

This authenticator uses Unix peer credentials for authentication. Unix peer credentials provide direct access to the (effective) uid/gid on the other end of a domain socket connect, without cooperation between the endpoints. This means that we can determine the uid/gid of the connecting process, and therefore infer the username of the user that owns said process. This authenticator: - grabs the (uid, gid) pair of the connecting process. - determine the username of the owner of the connecting process based on the uid. - creates an `ApplicationName` based on the username. Note that this patch depends on the following PR being merged: rust-lang/rust#75148 At the time of writing, this PR is currently under review and is not merged into the Rust stdlib. This patch therefore will not build with the current a stable/nightly compiler. Signed-off-by: Joe Ellis <joe.ellis@arm.com>

This authenticator uses Unix peer credentials for authentication. Unix peer credentials provide direct access to the (effective) uid/gid on the other end of a domain socket connect, without cooperation between the endpoints. This means that we can determine the uid/gid of the connecting process, and therefore infer the username of the user that owns said process. This authenticator: - grabs the (uid, gid) pair of the connecting process. - grabs the self-declared uid sent in the authentication request. - verifies that authentication is successful by checking that the self-declared uid in the authentication request is equal to the actual uid from the peer credentials. - if authentication was successful, creates an `ApplicationName` based on the uid. The authenticator is hidden behind the Cargo feature `uds-authenticator`. Note that this patch depends on the following PR being merged: rust-lang/rust#75148 At the time of writing, this PR is currently under review and is not merged into the Rust stdlib. This patch therefore will not build with the current a stable/nightly compiler. Signed-off-by: Joe Ellis <joe.ellis@arm.com>

This authenticator uses peer credentials for authentication. The specific type of peer credentials in mind at the moment are Unix peer credentials, but this can be extended in the future. Unix peer credentials provide direct access to the (effective) uid/gid on the other end of a domain socket connect, without cooperation between the endpoints. This means that we can trivially determine the uid/gid of the connecting process, which we can then use for authentication. This authenticator: - grabs the (uid, gid) pair of the connecting process. - grabs the self-declared uid sent in the authentication request. - verifies that authentication is successful by checking that the self-declared uid in the authentication request is equal to the actual uid from the peer credentials. - if authentication was successful, creates an `ApplicationName` based on the uid. The authenticator is hidden behind the Cargo feature `peer-credentials-authenticator`. Note that gid is currently unused by the authenticator. Also note that this patch depends on the following PR being merged: rust-lang/rust#75148 At the time of writing, this PR is currently under review and is not merged into the Rust stdlib. This patch therefore will not build with the current a stable/nightly compiler. Signed-off-by: Joe Ellis <joe.ellis@arm.com>

joechrisellis · 2020-08-19T08:22:53Z

Hi all! Just gently pinging this PR for review. 🙂

Dylan-DPC-zz · 2020-09-05T00:30:06Z

@joechrisellis thanks but would advise not to ping for reviewers. The triage team currently uses the last time a PR was updated as a parameter to filter which pull requests need to be checked, which means posting a message that doesn't contribute to the PR can delay the process further. In future, if anyone reading this wants to grab attention on a PR that's sitting idle for a while, you can contact the triage-team on zulip and we will take care of it

Dylan-DPC-zz · 2020-09-05T00:30:41Z

r? @Amanieu

Amanieu · 2020-09-08T12:06:06Z

library/std/src/sys/unix/ext/ucred.rs

+    pub uid: uid_t,
+    pub gid: gid_t,
+    // pid field is an option because it is not supported on some platforms.
+    pub pid: Option<pid_t>,


Could we get doc comments for each of the public fields? In particular the Option<pid_t> should be explained in the docs.

Amanieu · 2020-09-08T12:14:39Z

@bors r+

bors · 2020-09-08T12:14:41Z

📌 Commit 80860181445f14e6f305c687b2056f11ebc4629a has been approved by Amanieu

bors · 2020-09-15T12:35:32Z

📌 Commit 68ff495 has been approved by Amanieu

bors · 2020-09-15T17:06:03Z

⌛ Testing commit 68ff495 with merge a874956...

bors · 2020-09-15T19:04:33Z

☀️ Test successful - checks-actions, checks-azure
Approved by: Amanieu
Pushing a874956 to master...

This authenticator uses peer credentials for authentication. The specific type of peer credentials in mind at the moment are Unix peer credentials, but this can be extended in the future. Unix peer credentials provide direct access to the (effective) uid/gid on the other end of a domain socket connect, without cooperation between the endpoints. This means that we can trivially determine the uid/gid of the connecting process, which we can then use for authentication. This authenticator: - grabs the (uid, gid) pair of the connecting process. - grabs the self-declared uid sent in the authentication request. - verifies that authentication is successful by checking that the self-declared uid in the authentication request is equal to the actual uid from the peer credentials. - if authentication was successful, creates an `ApplicationName` based on the uid. The authenticator is hidden behind the Cargo feature `peer-credentials-authenticator`. Note that gid is currently unused by the authenticator. Also note that this patch depends on the following PR being merged: rust-lang/rust#75148 At the time of writing, this PR is currently under review and is not merged into the Rust stdlib. This patch therefore will not build with the current a stable/nightly compiler. Signed-off-by: Joe Ellis <joe.ellis@arm.com>

The code introduced in this patch has been cherry-picked from the following PR: rust-lang/rust#75148 At the time of writing (16/09/20), this patch is in the nightly Rust channel. To avoid needing to use the nightly compiler to build Parsec, this patch includes the change from the standard library to allow us to use this feature 'early'. Once the feature hits the stable branch, it should be safe to revert this commit with `git revert`. Signed-off-by: Joe Ellis <joe.ellis@arm.com>