Add Unix peer credentials authenticator #214

joechrisellis · 2020-08-06T11:15:29Z

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:

https://github.com/rust-lang/rust/pull/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-06T11:15:51Z

Note: CI failure is expected since this patch depends on a feature that is not yet in the stdlib.

paulhowardarm

Great to see this appearing! I have put in a few comments for consideration.

paulhowardarm · 2020-08-06T12:02:51Z

src/authenticators/uds_authenticator/mod.rs

+//!
+//! The `UDSAuthenticator` uses peer credentials from Unix domain sockets to perform
+//! authentication. As such, it uses the UID/GID to grab a username for the owner of the connecting
+//! process. This is used as the application name.


Do we gain anything by converting to a user name as the application name, rather than just using the stringified UID itself as the application ID string? This seems to have additional dependencies and operations that could conceivably fail. I'm also slightly fearful about converting to usernames given that the environment may be containerised, which could have implications in terms of how the UIDs convert to names in the transition between containers.

Do we gain anything by converting to a user name as the application name, rather than just using the stringified UID itself as the application ID string?

I think the only thing we gain is for it to look nicer in the mappings folder and in logs like "New request received from application name \"{}\"". So mostly for esthetics/debugging. Also, if we add any operation which takes an application name as parameter, it might be easier to specify the user name rather than the UID.

I guess I supposed that every UID must have an username mapping to it but if that is not true in some environments, I agree using the plain UID would be better.

That's a good point.

I can't think of any reason why the stringified UID wouldn't work just fine. The only comment that I have is that the username is perhaps more 'human-friendly', but I don't think these strings will ever necessarily be exposed anyways.

Happy to change this. 🙂

paulhowardarm · 2020-08-06T12:04:50Z

src/authenticators/mod.rs

@@ -12,6 +12,7 @@
 //! Currently only a simple Direct Authenticator component is implemented.

 pub mod direct_authenticator;
+pub mod uds_authenticator;


Is it useful to perhaps wrap this authenticator in a Rust/Cargo feature so that it can be selected for at build time? (Actually, looking at the rest of the change, perhaps you've already done that? But there's an indication that it's only temporary. I actually think there is value in having this be a selectable thing even longer term, because it's useful for people to be able to compile down a Parsec executable with only the precise pieces that their deployment needs. Providers and authenticators are the kinds of things that we might want to snip out, because they can be heavyweight, especially in terms of the dependencies that get dragged in with them).

Yes, good idea. Will do this. 👍

paulhowardarm · 2020-08-06T12:06:23Z

src/authenticators/mod.rs

@@ -12,6 +12,7 @@
 //! Currently only a simple Direct Authenticator component is implemented.


This comment is outdated by the change.

Ah, well spotted, thanks!

paulhowardarm · 2020-08-06T12:09:26Z

src/authenticators/uds_authenticator/mod.rs

@@ -0,0 +1,129 @@
+// Copyright 2019 Contributors to the Parsec project.


I guess this should be 2020 now.

ionut-arm · 2020-08-06T13:32:12Z

src/authenticators/uds_authenticator/mod.rs

+    fn authenticate(
+        &self,
+        _auth: &RequestAuth,
+        meta: Option<ConnectionMetadata>,
+    ) -> Result<ApplicationName> {
+        let meta = match meta {
+            Some(meta) => meta,
+            None => {
+                error!("Authenticator did not receive any metadata; cannot authenticate.");
+                return Err(ResponseStatus::AuthenticationError);
+            }
+        };
+
+        let (uid, _gid) = match meta {
+            ConnectionMetadata::PeerCredentials{uid, gid} => (uid, gid),
+            // TODO: add wildcard pattern when `ConnectionMetadata` has more possibilities.
+        };
+
+        Ok(ApplicationName(uid.to_string()))
+    }


I thought the authentication was meant to verify that the RequestAuth contains a string with that UID, just as a "sanity" check

Ah, I wasn't aware. Will fix that. 👍

Agreed, I think it should. I didn't follow through every line here and I thought there was some Rusty magic going on. My opinion is that this system should be fully explicit with a well-defined authentication type and expected header contents. The calling process should "declare" its UID in the auth header, and the service should use the peer credential to check that it matches.

Yeah, it's definitely a fair point. This is now implemented. 🙂

paulhowardarm · 2020-08-10T15:48:09Z

src/authenticators/mod.rs


 pub mod direct_authenticator;

+#[cfg(feature = "uds-authenticator")]
+pub mod uds_authenticator;


I should have noticed this before, but I'm not sure why this is called UDS authenticator? Assuming UDS stands for Unix Domain Socket, that's the name of the transport rather than the method of authentication (which is UID). You could have all sorts of different authenticators on a UDS transport, so calling this a UDS authenticator doesn't quite characterise it accurately.

Does PeerCredentialAuthenticator make more sense? I can imagine we'd be able to get peer credentials for other IPC mechanisms as well and we could apply the same logic for authentication there

My bad; I made the wrong assumption that using a Unix domain socket as a transport meant that we would always want to use peer credentials for authentication. I agree with @ionut-arm, PeerCredentialsAuthenticator seems like a good name here. 🙂

hug-dev

Thanks for adding it! Probably better to wait for the specs on the book and the PR in Rust to be merged before merging this one but thought of some early comments!

hug-dev · 2020-09-08T17:41:34Z

Cargo.toml

@@ -70,3 +70,4 @@ pkcs11-provider = ["pkcs11", "picky-asn1-der", "picky-asn1", "picky-asn1-x509",
 tpm-provider = ["tss-esapi", "picky-asn1-der", "picky-asn1", "picky-asn1-x509"]
 all-providers = ["tpm-provider", "pkcs11-provider", "mbed-crypto-provider"]
 docs = ["pkcs11-provider", "tpm-provider", "tss-esapi/docs", "mbed-crypto-provider"]
+peer-credentials-authenticator = []


See my related comment, if it is agreed, I would personally prefer all occurences of "peer credentials" to be renamed "Unix peer credentials".

hug-dev · 2020-09-08T17:47:32Z

src/front/domain_socket.rs

@@ -202,11 +203,10 @@ impl Listen for DomainSocketListener {
                    format_error!("Failed to set stream as blocking", err);
                    None
                } else {
+                    let metadata = stream.metadata();


Is the GetMetadata trait needed here if we are already in Unix specific code? As in, could you just replace the implementation of that trait on UnixStream here?

Just pinging this question as I think it is still valid

We could -- this was more just thinking for the future. Will revert this, apologies for the lag on responding to this comment. 😄

hug-dev · 2020-09-08T17:48:13Z

src/front/domain_socket.rs

+    fn metadata(&self) -> Option<ConnectionMetadata> {
+        let ucred = self
+            .peer_cred()
+            .expect("Failed to get peer credentials for Unix domain socket connection.");


I would prefer returning an error instead of panicking. In our case, you can log something and return None!

hug-dev · 2020-09-09T14:02:41Z

src/authenticators/unix_peer_credentials_authenticator/mod.rs

+
+        let boxed_slice = expected_uid_bytes.into_boxed_slice();
+        let boxed_array: Box<[u8; 4]> = boxed_slice.try_into().unwrap();
+        let expected_uid = u32::from_le_bytes(*boxed_array);


I think you can directly use the TryInto from the slice to the owned type and use the failed case as a way to check that the auth buffer was not 4 bytes (so even checks if not empty 😃 ):

let expected_uid = auth.buffer.expose_secret(); let expected_uid: [u8; 4] = expected_uid_bytes.try_into().ok_or_else(|| { error!( "UID in authentication request is not the right size (expected: {}, got: {}).", EXPECTED_UID_SIZE_BYTES, expected_uid_bytes.len() ); return Err(ResponseStatus::AuthenticationError); })?; let expected_uid = u32::from_le_bytes(expected_uid);

Ooh, that's much nicer. 😄

hug-dev

Good idea of having that code directly here 👌

hug-dev · 2020-09-16T12:55:31Z

src/authenticators/unix_peer_credentials_authenticator/mod.rs

+        if expected_uid_bytes.is_empty() {
+            error!("Expected UID in authentication request, but it is empty.");
+            return Err(ResponseStatus::AuthenticationError);
+        }


Small nit but I think that the empty case will also be checked in the following try_into (as the buffer will not be 4 bytes).
I would also say that you can use the hardcoded 4 in the following code instead of EXPECTED_UID_SIZE_BYTES as this size os probably not something meant to change often.

Small nit but I think that the empty case will also be checked in the following try_into (as the buffer will not be 4 bytes).

Nice catch, done.

I would also say that you can use the hardcoded 4 in the following code instead of EXPECTED_UID_SIZE_BYTES as this size os probably not something meant to change often.

I don't know -- I kind of like this. It gives some obvious context to the magic number; when the user reads it, it is clear that the UID is supposed to be 4 bytes long. If that variable didn't exist it would be as clear, I don't think. 🙂

I don't know -- I kind of like this. It gives some obvious context to the magic number; when the user reads it, it is clear that the UID is supposed to be 4 bytes long. If that variable didn't exist it would be as clear, I don't think. 🙂

Yes, it's true that this is more readable!

hug-dev · 2020-09-16T13:33:12Z

src/authenticators/unix_peer_credentials_authenticator/mod.rs

+        // secret when using Unix peer credentials authentication with Unix domain sockets.
+
+        // Create two connected sockets.
+        let (sock_a, _sock_b) = UnixStream::pair().unwrap();


Pretty good idea to use that for unit tests!

hug-dev · 2020-09-16T13:34:25Z

src/front/domain_socket.rs

@@ -202,11 +203,10 @@ impl Listen for DomainSocketListener {
                    format_error!("Failed to set stream as blocking", err);
                    None
                } else {
+                    let metadata = stream.metadata();


Just pinging this question as I think it is still valid

hug-dev · 2020-09-16T13:36:11Z

src/front/domain_socket.rs

+//
+// The code below has been cherry-picked from the following PR:
+//
+//     https://github.com/rust-lang/rust/pull/75148


Could you also please add the stabilizing issue rust-lang/rust#42839?

hug-dev

Thanks for applying the changes, it looks pretty good to me 👌

src/front/listener.rs

src/authenticators/unix_peer_credentials_authenticator/mod.rs

ionut-arm · 2020-09-21T13:26:45Z

src/authenticators/unix_peer_credentials_authenticator/mod.rs

+
+        let (uid, _gid) = match meta {
+            ConnectionMetadata::UnixPeerCredentials { uid, gid } => (uid, gid),
+            // TODO: add wildcard pattern when `ConnectionMetadata` has more possibilities.


Can you add the wildcard pattern and "disable" the clippy warning? Or is it something the compiler doesn't like?

We can -- done.

Will be required for testing the Unix peer credentials authenticator. 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>

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>

ionut-arm

Thank you! 💯

hug-dev linked an issue Aug 6, 2020 that may be closed by this pull request

Implement an authenticator based on the domain socket peer credential #200

Closed

hug-dev mentioned this pull request Aug 6, 2020

Implement an authenticator based on the domain socket peer credential #200

Closed

paulhowardarm reviewed Aug 6, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch from 9a65b40 to f783921 Compare August 6, 2020 13:25

ionut-arm reviewed Aug 6, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch from f783921 to 8b4bfb6 Compare August 6, 2020 14:10

paulhowardarm reviewed Aug 10, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch from 8b4bfb6 to 179675e Compare August 11, 2020 08:57

joechrisellis force-pushed the uds_authenticator branch 5 times, most recently from 393aaab to f193c43 Compare September 8, 2020 14:19

hug-dev reviewed Sep 8, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch from f193c43 to 11030c7 Compare September 9, 2020 11:39

hug-dev reviewed Sep 9, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch 4 times, most recently from ba9529c to bcd8de5 Compare September 16, 2020 10:11

joechrisellis mentioned this pull request Sep 16, 2020

Nightly features CI #244

Closed

joechrisellis force-pushed the uds_authenticator branch 4 times, most recently from 908e428 to 7298258 Compare September 16, 2020 11:33

hug-dev reviewed Sep 16, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch from 7298258 to 6ea9a16 Compare September 17, 2020 08:30

hug-dev approved these changes Sep 17, 2020

View reviewed changes

ionut-arm reviewed Sep 21, 2020

View reviewed changes

joechrisellis force-pushed the uds_authenticator branch 2 times, most recently from fc8744d to 90b8f76 Compare September 21, 2020 14:49

Joe Ellis added 3 commits September 21, 2020 16:24

Add rand crate as a dev-dependency

d56700c

Will be required for testing the Unix peer credentials authenticator. Signed-off-by: Joe Ellis <joe.ellis@arm.com>

joechrisellis force-pushed the uds_authenticator branch from 90b8f76 to 0b48895 Compare September 21, 2020 15:25

ionut-arm approved these changes Sep 21, 2020

View reviewed changes

hug-dev merged commit 2a57277 into parallaxsecond:master Sep 22, 2020

		@@ -12,6 +12,7 @@
		//! Currently only a simple Direct Authenticator component is implemented.

		@@ -0,0 +1,129 @@
		// Copyright 2019 Contributors to the Parsec project.

Add Unix peer credentials authenticator #214

Add Unix peer credentials authenticator #214

Conversation

joechrisellis commented Aug 6, 2020

joechrisellis commented Aug 6, 2020

paulhowardarm left a 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

hug-dev left a 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

hug-dev left a 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

hug-dev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ionut-arm left a comment

Choose a reason for hiding this comment