feat(socket source): Add inode number & peer creds to events #17705

KJTsanaktsidis · 2023-06-18T06:02:45Z

This is the implementation of a couple of new pieces of data on Unix socket source, as discussed in #17671.

inode_key: If inode_key is set on the socket configuration, then we will look up the inode & device number for the socket, and attach a structure like {"ino": 123 "dev": 456} at the configured path. This is done through a call to File::metadata (which essentially calls fstat(2)) and should work on all unixy-platforms.

On a Unix stream socket, the combination of these two numbers uniquely identifies a particular connection, in the same way that a host & port number uniquely identifies a TCP connection. This can be used to group events by connection in reduce transforms, for instance. On a datagram socket, every event will have the same inode number (because one single socket in the server accepts messages from all the clients), so it's probably not especially useful, but I added it anyway for symmetry's sake. It does at least uniquely identify the socket source instance in this case.

credentials_key: If credentials_key is set on a socket, then we will use a call to Tokio's ::peer_creds() on the socket to look up the UID, GID, and PID of the remote process. This only works for a stream socket, and it isn't guaranteed to be present on every event even then. For example, if a process connects to the socket, then passes the socket to a child process, and then exits, before Vector gets a chance to call ::peer_creds(), then at least on macOS (and probably elsewhere too), the pid isn't returned. So this data should probably be considered "best effort".

On datagram sockets, setting credentials_key will just emit the value as null always. It should in theory be possible to implement this for datagram sockets on most unix-alikes by looking at SCM_CREDS/SCM_CREDENTIALS type ancillary data messages, but Rust std, and also Tokio's mio, doesn't have any mechanism for receiving ancillary data. There's an open issue for adding this to std here - rust-lang/rust#76915. I guess nothing stops this from being implemented in Tokio's mio before that, but in any case I didn't really need this feature.

Finally, I noticed an unrelated bug. If you bind a client-side unix socket before connecting it to Vector, Vector is supposed to take the bound name of the client socket and put it in the host field. When doing this, it actually truncates the name by one character, at least on macOS. You can test this by setting up a Unix socket source and connecting to it with a bound socket in socat, like this: socat STDIN UNIX:./vector_socket.sock,bind=./my_socket_name.

Actually, Vector will report the hostname as "./my_socket_nam" in that case. I believe this is because this code in mio is expecting the sockaddr to be zero-terminated, but on macOS, it seems not to be: https://github.com/tokio-rs/mio/blob/c2f79f6969cccb6dc0452a3f360757d9e01290ce/src/sys/unix/uds/socketaddr.rs#L47

In any case, I haven't got around to reducing this down to a testcase and sending a patch to tokio yet, but perhaps it's something we should do.

This struct at the moment just contains the peer_name field, but it provides a place to add other kind of metadata on the Socket source later. This is a pure refactor, everything should work precicely how it did before.

This was an inconsistency between unix stream & datagram. The datagram support adds peer_path as "(unnamed)" if it's not available (i.e. the peer didn't bind their socket), but the stream one does not set peer_path at all in that case. It seems like there is little need for the inconsistency, so make them consistent.

This way, we can only actually collect metadata and make syscalls for things that configuration has indicated is wanted.

When this value is set, we will call fstat(2) on connected sockets and include the inode and device number in the output events. This can be used to uniquely identify a given connection on a socket.

This will get the peer UID/GID/PID of the connecting process and enrich the output events with it.

bits-bot · 2023-06-18T06:02:49Z

All committers have signed the CLA.

netlify · 2023-06-18T06:02:49Z

✅ Deploy Preview for vector-project ready!

Name	Link
🔨 Latest commit	`1131db0`
🔍 Latest deploy log	https://app.netlify.com/sites/vector-project/deploys/648ea54bc30ca50007d48705
😎 Deploy Preview	https://deploy-preview-17705--vector-project.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify · 2023-06-18T06:02:54Z

✅ Deploy Preview for vrl-playground canceled.

Name	Link
🔨 Latest commit	`1131db0`
🔍 Latest deploy log	https://app.netlify.com/sites/vrl-playground/deploys/648ea54bb0c8f00008b0c3e3

KJTsanaktsidis · 2023-06-18T06:34:54Z

Had a bit of a question about how to document this - Ideally I'd like to put an example of what the credentials & inode objects look like under "Output Data > Logs > Line > Fields" in the socket documentation, but the key in which it appears is user-settable and defaults to "" (so as to not collect it). Any thoughts on how to resolve this?

neuronull · 2023-06-26T16:08:33Z

👋 hi @KJTsanaktsidis , thanks for this contribution. I will take a look at this within the next couple days.

tobz

Overall, there's a lot going on here.

Briefly reading through this, I think at a high level, credentials needs to be scrapped. Given how much extra baggage is involved, it doesn't feel worth it when users will have to be mindful of a ton of edge cases and the data may not even be available.

Once that's been dropped, we can review the rest of the PR.

neuronull

I started to review this but will stop at this point in anticipation of changes wrt to the suggestion to remove the credentials.

neuronull · 2023-06-28T20:51:00Z

website/cue/reference/components/sources/base/socket.cue

+			If set, on a stream socket, output events will contain information about
+			the UID/GID/PID of the process which connected to the socket. Note that
+			this information is subject to some race conditions and is not available
+			in all circumstances:
+
+			* If a process exits after connecting, and passes the socket on to a different
+			  process, (e.g. a child), the pid might now refer to a different process
+			  (or, on some platforms, might not be returned at all)
+			* If a process connects to this socket source, writes data, and immediately
+			  disconnects, it's possible Vector won't query the peer credentials before
+			  the other process disconnects; in this case, the data won't be available
+			  on some platforms.


It looks like the cue docs (generated), are out of sync.

make check-component-docs can be run to validate the cue docs are correct (if you run it right now you will see the check will fail) , this is also run as part of CI.

make generate-component-docs , will generate the changes to the cue files for you. Then you can commit those.

I just noticed this isn't documented well in our docs, but I'll make sure we update those.

neuronull · 2023-06-28T21:20:33Z

src/sources/util/unix.rs

+#[derive(Copy, Clone, Debug)]
+pub struct SocketInode {
+    pub dev: u64,
+    pub ino: u64,
+}
+
+impl Into<Value> for SocketInode {
+    fn into(self) -> Value {
+        let mut map = BTreeMap::new();
+        // really want "as i64" here - actually despite claiming to return
+        // a u64, the value we get from fstat(2) for dev on some platforms
+        // is the bit pattern for -1. So, reinterpret the "u64" as signed.
+        map.insert("dev".to_string(), Value::Integer(self.dev as i64));
+        map.insert("ino".to_string(), Value::Integer(self.ino as i64));
+        Value::Object(map)
+    }
+}


I'm not seeing a ton of value in having a struct + impl for this logic, when the socket_inode in UnixSocketMetadata could just be an Option<Value> , and the place where this is constructed could directly instantiate a Value with the data, and not have to do the map insert() operations.

neuronull · 2023-06-28T21:33:02Z

src/sources/util/unix.rs

+pub struct UnixSocketMetadata {
+    /// The peer address of the socket, as returned from getpeername(2). This
+    /// will usually not be set (unless the connecting peer has explicitly
+    /// bound their socket to a path).
+    pub peer_path: Option<String>,
+
+    /// The inode/device of the socket, as collected from fstat(2). This only
+    /// makes sense for stream sockets, not datagram ones.
+    pub socket_inode: Option<SocketInode>,
+
+    /// The peer credentials of the process which connected to the socket,
+    /// reported by getsockopt(2) SO_PEERCRED/LOCAL_PEERCRED/etc. This may not
+    /// be the same as the process currently writing to the socket, because
+    /// the socket could have been passed down to a child process or sent
+    /// to a different process.
+    pub peer_creds: Option<SocketCredentials>,
+}


Taking @tobz 's comment (#17705 (review)) into consideration, if the credentials part is removed, we probably don't need to encapsulate the peer_path and inode in this struct, which would also simplify other parts of these changes.

neuronull · 2023-06-28T21:38:59Z

src/sources/util/unix.rs

+
+
+/// Collects the device & inode number for a socket.
+pub async fn get_socket_inode<T : AsRawFd>(socket: &T) -> Result<SocketInode, Box<dyn std::error::Error>> {


There are quite a lot of clippy formatting errors to resolve on theses changes. Do run make check-clippy , make check-fmt before pushing to the PR to avoid CI failing due to that.

cargo fmt to fix the issues (https://github.com/vectordotdev/vector/blob/master/docs/DEVELOPING.md#bring-your-own-toolbox)

KJTsanaktsidis · 2023-07-12T01:56:42Z

Briefly reading through this, I think at a high level, credentials needs to be scrapped. Given how much extra baggage is involved, it doesn't feel worth it when users will have to be mindful of a ton of edge cases and the data may not even be available.

Yes, I was learning a ton as I went along implementing this, and what I'd hoped would be possible didn't really turn out to be 100% reliable because of those edge-cases....

I'm actually thinking of closing this in favour of binding the client socket to a path instead. It seems that if the socket client does the following sequence of things:

Create a new socket (socket(2))
Makes a randomly-named temp directory
Binds the socket to a name in that temp directory (bind(2))
Connects to Vector (connect(2))

that Vector can still reliably detect that bound name and sets it as host. (n.b. as I mentioned, the name gets truncated by one byte on macOS because of a bug in Tokio, but I don't really care for my use-case). So I think actually doing that might be a better way to go for our use case instead of using the inode numbers.

It's even possible to unlink the socket & temp directory straight after calling connect, and Vector is still able to get the peer socket name (although it does admit the very remote possibility that some other process could take the same name).

jszwedko · 2023-07-18T19:11:27Z

@KJTsanaktsidis ah, yeah, that makes sense if the path satisfies your use case. Should we close this out then?

KJTsanaktsidis · 2023-07-18T22:25:29Z

I think so. The added bonus of binding the client sockets is I get to give them a meaningful name over and above just the inode number so I think that's overall a better solution.

KJTsanaktsidis · 2023-07-18T22:25:49Z

Sorry for wasting your time with this - guess I had to do it to discover it was a bad idea!

jszwedko · 2023-07-18T22:34:11Z

No worries! Appreciate the follow through and discussion on this one!

KJ Tsanaktsidis added 5 commits June 18, 2023 15:36

Refactor "peer_name" support for Unix sockets into a struct

2c600b0

This struct at the moment just contains the peer_name field, but it provides a place to add other kind of metadata on the Socket source later. This is a pure refactor, everything should work precicely how it did before.

Add a mechanism for explicitly specifying what socket metadata is wanted

f7b8ba4

This way, we can only actually collect metadata and make syscalls for things that configuration has indicated is wanted.

Implement inode_path on Unix socket sources

996327f

When this value is set, we will call fstat(2) on connected sockets and include the inode and device number in the output events. This can be used to uniquely identify a given connection on a socket.

Implement credentials_path option for Unix socket sources

4cbde48

This will get the peer UID/GID/PID of the connecting process and enrich the output events with it.

KJTsanaktsidis requested a review from StephenWakely as a code owner June 18, 2023 06:02

KJTsanaktsidis requested a review from a team June 18, 2023 06:02

KJTsanaktsidis requested a review from neuronull as a code owner June 18, 2023 06:02

github-actions bot added the domain: sources Anything related to the Vector's sources label Jun 18, 2023

Spelling fixes

b217d94

KJTsanaktsidis requested a review from a team as a code owner June 18, 2023 06:20

github-actions bot added the domain: external docs Anything related to Vector's external, public documentation label Jun 18, 2023

KJTsanaktsidis force-pushed the ktsanaktsidis/unix_data branch from 93b7850 to a1a2fc5 Compare June 18, 2023 06:25

Add Cue documentation for the inode & credential socket info

1131db0

KJTsanaktsidis force-pushed the ktsanaktsidis/unix_data branch from a1a2fc5 to 1131db0 Compare June 18, 2023 06:33

jszwedko assigned neuronull Jun 26, 2023

tobz reviewed Jun 28, 2023

View reviewed changes

neuronull reviewed Jun 28, 2023

View reviewed changes

neuronull added the meta: awaiting author Pull requests that are awaiting their author. label Jun 28, 2023

KJTsanaktsidis closed this Jul 18, 2023



		/// Collects the device & inode number for a socket.
		pub async fn get_socket_inode<T : AsRawFd>(socket: &T) -> Result<SocketInode, Box<dyn std::error::Error>> {

feat(socket source): Add inode number & peer creds to events #17705

feat(socket source): Add inode number & peer creds to events #17705

Uh oh!

Conversation

KJTsanaktsidis commented Jun 18, 2023

Uh oh!

bits-bot commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for vector-project ready!

Uh oh!

netlify bot commented Jun 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for vrl-playground canceled.

Uh oh!

KJTsanaktsidis commented Jun 18, 2023

Uh oh!

neuronull commented Jun 26, 2023

Uh oh!

tobz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neuronull left a comment

Choose a reason for hiding this comment

Uh oh!

neuronull Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

neuronull Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

neuronull Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

neuronull Jun 28, 2023

Choose a reason for hiding this comment

Uh oh!

KJTsanaktsidis commented Jul 12, 2023

Uh oh!

jszwedko commented Jul 18, 2023

Uh oh!

KJTsanaktsidis commented Jul 18, 2023

Uh oh!

KJTsanaktsidis commented Jul 18, 2023

Uh oh!

jszwedko commented Jul 18, 2023

Uh oh!

Uh oh!

bits-bot commented Jun 18, 2023 •

edited

Loading

netlify bot commented Jun 18, 2023 •

edited

Loading

netlify bot commented Jun 18, 2023 •

edited

Loading

tobz left a comment •

edited

Loading