Add intial tests for XPCEncoder

[amomchilov]

What it says on the tin.

[amomchilov]

@trilemma-dev what do you think?

[jakaplan]

Thanks for putting this together. Will review; may take me a few days to get through all of it. FYI I'm in the NZDT timezone.

[jakaplan]

I read through the PR and this brought up several important points that are better discussed at a high level before getting into any specifics on the details of the code:

• XPCEncoder and XPCDecoder aren't intended to be a general purpose encoder or decoder respectively, which is in part why they're not part of the public API. They only exist for the purpose of serializing/deserializing Codable instances across an XPC connection.
  • So for example while XPC has a unique concept of a "data" type, Codable does not. The Swift type Data knows how encode and decode itself (I believe it chooses to use an unkeyed container with UInt8 elements, but that's its own hidden implementation detail). This is equally the case for UUID and Date. All of these Swift types are Codable and therefore can be used with SecureXPC without issue and without any special casing needed.
  • File descriptors and shared memory are intentionally not supported as they are concepts incompatible with Codable which is designed for indefinite serialization and cross-device transfer. As such choosing Codable for SecureXPC had tradeoffs because XPC does not have the same requirements — it can represent live resources on a single device. However, I decided the tradeoff was worth it because of Swift's excellent built in support for Codable (particularly the compiler automatically generating conformance in many cases). If there was a desire to support file descriptors and shared memory, I can envision ways to add them in the future, although it would mean expanding the API quite considerably.
• Currently SecureXPC makes no guarantees that the internal serialization format it uses will remain stable between releases. This is something I do want to figure out prior to a 1.0.0 release as I believe it should be considered part of the SemVer criteria. So for example a guarantee could exist that all 1.x clients can talk to all 1.x servers, and vice versa. In its current 0.x form that's not a guarantee.
  • The unit tests you've written are asserting specific ways of encoding Swift values into XPC values, which is not currently a guarantee of this framework. Once a serialization format is stable, having such a set of tests would be quite valuable to ensure that the format does not change except intentionally between major versions.
  • What is a guarantee of the current API (but lacks unit tests to do so) is that a Codable instance sent through the API can be decoded upon receipt such that equality is preserved. So unit tests which invoke both the encoder and decoder together could validate this. As is quite evident I have not written such unit tests, but this was what I had been loosely envisioning. Would you be open to writing tests at this granularity of unit?
• Your PR made very clear that decoupling the actual encoding and decoding from the "packaging" format which includes the route information would improve testability. I can make that change relatively soon.

[amomchilov]

Some thoughts, in (kind of) reverse order.

Your PR made very clear that decoupling the actual encoding and decoding from the "packaging" format which includes the route information would improve testability. I can make that change relatively soon.

That's an untested code path, so I'm afraid to make that change. If you make the change, I suppose you use whatever SecureXPC-using project that you have to test it :D

What is a guarantee of the current API (but lacks unit tests to do so) is that a Codable instance sent through the API can be decoded upon receipt such that equality is preserved. So unit tests which invoke both the encoder and decoder together could validate this. As is quite evident I have not written such unit tests, but this was what I had been loosely envisioning. Would you be open to writing tests at this granularity of unit?

Yes, absolutely! :)

I suppose I should have communicated my higher level intentions for the test suite. I think it should consist of:

1. A test of the encoder, in isolation (this test).

   The unit tests you've written are asserting specific ways of encoding Swift values into XPC values, which is not currently a guarantee of this framework. Once a serialization format is stable, having such a set of tests would be quite valuable to ensure that the format does not change except intentionally between major versions.

   I think updating these tests is easy enough that it's worth checking them in early. It'll at least notify us when the serialization format changes (which is acceptable pre-1.0.0, but we'd still want to know about it).

2. A test of the decoder, in isolation, for the same reason.

3. A round-trip test, like what you were saying. Strictly speaking, this isn't necessary when the encoder and decoder tests are good enough and consistent with each other (the expected inputs of one are the expected outputs on the other). However, in the past I've found that adding an round-trip test suite as an extra step useful, because it guarantees that consistency rather than hoping for it.

4. An integration test that launches a test XPC service and round-trips messages through it. Though IDK how to package an XPC service in a Swift package.

5. Other stuff...

Part of my motivation here was that I wanted to locally refactor to see if I can make the *Container classes write directly into xpc arrays/dictionaries, without the intermediate Swift array/dicts, to see how much of a performance lift that might have.

---

So for example while XPC has a unique concept of a "data" type, Codable does not. The Swift type Data knows how encode and decode itself (I believe it chooses to use an unkeyed container with UInt8 elements, but that's its own hidden implementation detail). This is equally the case for UUID and Date. All of these Swift types are Codable and therefore can be used with SecureXPC without issue and without any special casing needed.

I think this is easy enough to do that it might be worth doing.

In particular, I've noticed that Data is really inefficient to encode. As it is today, it bloats sizes by 8x. It tries to encode itself as a [UInt8], but each byte gets widened into an 8 byte int64 in order to be passed to xpc_int64_create. Luckily, there is not per-number object allocation overhead there (that would have been brutal for my use-case haha), because the only possible int64 values here would be 0-255, all of which fit into the tagged pointer representation.

You can get a rough of idea of what that might look like, here: amomchilov/SecureXPC@add-intial-tests-for-XPCEncoder...Alex/new-scalar-support . Of course, I only changed the encoder half, and the decoder would need to match.

---

File descriptors and shared memory are intentionally not supported as they are concepts incompatible with Codable which is designed for indefinite serialization and cross-device transfer. As such choosing Codable for SecureXPC had tradeoffs because XPC does not have the same requirements — it can represent live resources on a single device. However, I decided the tradeoff was worth it because of Swift's excellent built in support for Codable (particularly the compiler automatically generating conformance in many cases). If there was a desire to support file descriptors and shared memory, I can envision ways to add them in the future, although it would mean expanding the API quite considerably.

I don't have an immediate need for passing FDs or shared memory regions in my program yet, but my app does need to pass an xpc_endpoint_t. My main app is sandboxed, so the only way it can use SMBless is by delegating that work to an unsandboxed XPC helper service. Once the installation is done, it opens the XPC connection to the privileged helper service, and passes the endpoint back to the main app.

This is the approach that's recommended in Apple's EvenBetterAuthorizationSample, so I think supporting xpc_endpoint_t is pretty crucial. So at a minimum, at least some level of special-casing needs to happen.

[jakaplan]

That's an untested code path, so I'm afraid to make that change. If you make the change, I suppose you use whatever SecureXPC-using project that you have to test it :D

I've made this change and committed it. It should now be trivial to directly test the encoder or decoder independent of the rest of the system.

I'll reply to the rest of your comments tomorrow as it's a bit after 2AM here (got a bit carried away making this refactor)!

[amomchilov]

(got a bit carried away making this refactor)!

Haha I sure know that feeling! Good night!

[amomchilov]

Dang it, I regret force-pushing that. Oh well, doesn't matter, I got rid of the __payload unwrapping in the encode test helper.

🧹 super clean!

[jakaplan]

Thanks for providing the additional context. I'm very welcoming of contributions to this framework, just wanted to make sure there was a shared understanding of how and why things work in this space. In particular, I didn't want you to have a mistaken understanding that by committing this test of tests that the on wire protocol would remain stable (pre-1.0.0 that is).

1. A test of the encoder, in isolation (this test).

Sounds good.

2. A test of the decoder, in isolation, for the same reason.

Makes sense.

3. A round-trip test, like what you were saying. Strictly speaking, this isn't necessary when the encoder and decoder tests are good enough and consistent with each other (the expected inputs of one are the expected outputs on the other). However, in the past I've found that adding an round-trip test suite as an extra step useful, because it guarantees that consistency rather than hoping for it.

I believe this is necessary in order to test that arbitrary Codable instances can be properly transferred. Because it's the internal implementation detail of each Codable conforming type and not the SecureXPC framework, I don't think it'd be a good idea to assert the specific manner in which such types encode and decode themselves. (If we were ever to provide optimized encoding and decoding for some types, that's excluded from this statement.)

4. An integration test that launches a test XPC service and round-trips messages through it. Though IDK how to package an XPC service in a Swift package.

I haven't fully thought this through, so not actually sure if this will work but I think this ought to be feasible using a Launch Agent. They run as the user, not root, and so I'm hoping it's possible to package these up.

Just to be extra explicit, this framework is for XPC Mach Service communication, which is not what XPC Services use (yeah it's stupidly confusing how Apple overloaded terms here).

5. Other stuff...

At the very least, I want to add testing for routes. If there was ever a regression here it'd likely be very disruptive.

Part of my motivation here was that I wanted to locally refactor to see if I can make the *Container classes write directly into xpc arrays/dictionaries, without the intermediate Swift array/dicts, to see how much of a performance lift that might have.

Intriguing. It's not obvious to me that's possible (at least in a useful way). Much of the encoding does not occur at the time a call is made to the encoder, but instead at the "end" of the process when XPCEncoder calls encodedValue() on the XPCEncoderImpl instance it creates. To see an example of how this works, take look at XPCKeyedEncodingContainer's superEncoder() function which is required to return an Encoder. The returned encoder itself is set as the intermediate value in the container's values dictionary and then its actual encoding is deferred until encodedValue() is called on the container. This process can occur arbitrarily many layers deep. It's completely possible there's a different design pattern which can be used here that I haven't considered.

---

[M]y app does need to pass an xpc_endpoint_t. My main app is sandboxed, so the only way it can use SMBless is by delegating that work to an unsandboxed XPC helper service. Once the installation is done, it opens the XPC connection to the privileged helper service, and passes the endpoint back to the main app.

This is the approach that's recommended in Apple's EvenBetterAuthorizationSample, so I think supporting xpc_endpoint_t is pretty crucial. So at a minimum, at least some level of special-casing needs to happen.

Oof, okay this is going to be rather complicated. Will need a bunch of thought. If you weren't already aware, the term "sandbox" used by EvenBetterAuthorizationSample does not refer to same thing as the sandboxing macOS uses today. The sample is just that old :)

---

Will begin reviewing your PR again in light of the above.

[jakaplan]

Thanks! This looks like a really good test of the basics.

If your intention is to use this set of tests in order to make changes to XPCEncoder then the remaining portion is encoding classes which need to encode their super classes. This along with nested unkeyed contains and keyed containers makes up a lot of the complexity to how XPCEncoder is designed. That's because they require deferred encoding of the actual values because at the time they need to return their containers or encoders what will actually need to be encoded isn't known.

See XPCUnkeyedEncodingContainer's superEncoder() function and XPCKeyedEncodingContainer's superEncoder() and superEncoder(forKey key: K) functions for the super class encoding.

[jakaplan]

I don't believe this function is needed if the one below it which takes [T?] also exists?

[amomchilov]

There was a reason I did this, but I don't remember it, haha. I'll look into it.

[amomchilov]

I'm going to dedupe all comments. When we decide on an outcome from the main thread, i'll follow up and fix them all

[jakaplan]

This approach seems pretty convenient for containers of all the same types and no nesting (so arrays or dictionaries with all of the same scalar elements/values), but I could see it being rather cumbersome to use when testing deeply nested types or varying types. Considering there's a finite number of types that the Encoder protocol (and its related protocols it "vends"), it may be more convenient to create an extension for each of those types and then have them encode themselves. (Although maybe this would be tricky for nil?) That way mixed type arrays/dictionaries will just encode themselves and deeply nested types should be pretty trivial to support via mutual recursion of their elements/values (when applicable).

This feedback is not intended to mean I'd like you to change your approach for this PR, but something to consider in the future.

[jakaplan]

Huh, but it works properly when nested in an array or dictionary?

[amomchilov]

🤷

[jakaplan]

Hmm, is there any value in doing this for all of these different types? It seems like what this is actually testing is Swift's implementation of Optional?

[amomchilov]

In effect; kind of. But that's not something that I knew from black box testing this (I wrote as much of this as possible without biasing myself by looking at the implementation)

[amomchilov]

Reply to first comment

Just to be extra explicit, this framework is for XPC Mach Service communication, which is not what XPC Services use (yeah it's stupidly confusing how Apple overloaded terms here).

Uhhhh I didn't know this. What's the difference?

At the very least, I want to add testing for routes. If there was ever a regression here it'd likely be very disruptive.

Indeed, I was just scratching the surface

This process can occur arbitrarily many layers deep. It's completely possible there's a different design pattern which can be used here that I haven't considered.

I didn't quite understand this, but I'll come back to it.

If you weren't already aware, the term "sandbox" used by EvenBetterAuthorizationSample does not refer to same thing as the sandboxing macOS uses today. The sample is just that old :)

I wasn't aware. Could you link to something that explains the difference?

then the remaining portion is encoding classes which need to encode their super classes.

Got any ideas on how to test that? Just make a sample class hierarchy, and encode/decode it?

See XPCUnkeyedEncodingContainer's superEncoder() function and XPCKeyedEncodingContainer's superEncoder() and superEncoder(forKey key: K) functions for the super class encoding.

Will-do!

[jakaplan]

Reply to first comment

Just to be extra explicit, this framework is for XPC Mach Service communication, which is not what XPC Services use (yeah it's stupidly confusing how Apple overloaded terms here).

Uhhhh I didn't know this. What's the difference?

XPC Services use "plain" XPC for lack of a better term. In the C API, this corresponds to an app using the xpc_connection_create(_:_:) function to initiate a connection. When this is called launchd searches within the calling app's bundle for a service bundle whose CFBundleIdentifier value matches the name specified in the xpc_connection_create(_:_:) call, then launches that XPC service (if it's not already running).

The way this communication works means it's inherently secure because all communication is scoped by launchd to bundles within the outermost app bundle. To quote Apple directly, "Further, an XPC service is private, and is available only to the main application that contains it."

In contrast XPC for a Mach service uses the xpc_connection_create_mach_service(_:_:_:) function. Mach services can be communicated with by many processes across the system including arbitrary apps running as standard users (although there are restrictions placed on sandboxed apps). This communication by default has no security at all and has led to numerous CVEs opened against Mach services which run as root (here's a detailed write up about one). LaunchDaemons also have this issue and to a lesser extent LaunchAgents may as well (although they don't have a privileged escalation risk).

This is SecureXPC's reason for existing — to make it dead simple to secure these Mach service connections. Hence the intro part of the README calling out, "This framework is ideal for communicating with helper tools installed via SMJobBless."

This process can occur arbitrarily many layers deep. It's completely possible there's a different design pattern which can be used here that I haven't considered.

I didn't quite understand this, but I'll come back to it.

No worries, happy to discuss in-depth in the Discussions section.

If you weren't already aware, the term "sandbox" used by EvenBetterAuthorizationSample does not refer to same thing as the sandboxing macOS uses today. The sample is just that old :)

I wasn't aware. Could you link to something that explains the difference?

Apologies, what I wrote was misleading bordering on just incorrect. The sandbox is the same, but what's new since then is non-Mac App Store apps need to use the hardened runtime (in order to be notarized) and that interacts with the sandbox. By default this results in inheriting entitlements. This Apple Developer thread explains what's going on. I'm no expert in this space, but happy to discuss further in the Discussions section.

then the remaining portion is encoding classes which need to encode their super classes.

Got any ideas on how to test that? Just make a sample class hierarchy, and encode/decode it?

Yeah so long as the super class(es) encode (at least some of) their own properties that should do it.

[amomchilov]

This is SecureXPC's reason for existing — to make it dead simple to secure these Mach service connections. Hence the intro part of the README calling out, "This framework is ideal for communicating with helper tools installed via SMJobBless."

But as far as I can tell, nothing stops you from using the major parts (defining routes, serializing values) with non-Mach XPC services, right? This library just doesn't have a wrapper around xpc_connection_create (yet?).

My main draw to it was the Codable support, removing my need to rewrite all my structs into NSObject subclasses haha.

[amomchilov]

I've followed up on all the PR comments and made the appropriate changes.

• I've fixed the formatting in some places
• I've removed the comments about the string copying; we know it's safe for sure now
• I've added leastNonZero and leastNormal magnitudes in the float tests
• I've removed Data, Date and UUID related tests. I'll add them back in future PRs.

I haven't added any tests for structs/classes. I would like to follow up on that and add it into a new PR.

How do you feel about merging this work as it is, so far?

[jakaplan]

But as far as I can tell, nothing stops you from using the major parts (defining routes, serializing values) with non-Mach XPC services, right? This library just doesn't have a wrapper around xpc_connection_create (yet?).

My main draw to it was the Codable support, removing my need to rewrite all my structs into NSObject subclasses haha.

I'm glad you've found the Codable support appealing, it was actually a pretty late addition (although done before I split it out into its own true framework and open sourced it on Github). Not sure if this makes sense, but Codable wasn't the motivating reason behind this framework (if it was I probably would've called it something like CodableXPC); validating the security of the connection was.

As far as I know there's nothing stopping the use of serialization and routes with "plain" XPC communication, although I've never tried to use it that way. Overall I imagine it's net simpler: no security considerations and the on wire protocol should be able to change without being a breaking change because an XPC Service always runs from inside the app bundle itself. There are some changes I've been considering for routes which don't make sense for XPC Services (they have to do with versioning), but in all likelihood they could be done in an optional way and just not be applicable for this type. To the extent there's some complexity here it's about how to keep the API surface simple and minimize developers using the wrong type of connection by mistake.

[jakaplan]

Looks good, thanks for writing this and incorporating feedback!