Support for Regular XPC services #10

amomchilov · 2021-11-08T14:54:43Z

IDK what the correct nomenclature is, but PR adds support for "regular" (non-Mach) XPC services.

Works great, locally.

I extracted out XPCBase(Client|Server) classes to do most of the work. Perhaps it should be a protocol instead. Though I think it's correct for the client and server types to remain classes, even if they don't end up using inheritance.

Implemented the design @jakaplan described here: #10 (review)

WDYT? @jakaplan

TODOs before merging

Update docs

Sources/SecureXPC/XPCBaseClient.swift

Sources/SecureXPC/XPCMachClient.swift

Sources/SecureXPC/XPCServiceClient.swift

Sources/SecureXPC/XPCServiceServer.swift

Sources/SecureXPC/XPCBaseClient.swift

jakaplan

Exciting to see this gain more functionality!

I think this API design is absolutely workable, but because the distinction between an XPC Service and a "service" which vends a Mach service is frankly confusing I think the following might result in more developers doing the right thing on their first try. It'd also give us more flexibility to change the API in the future without it being a breaking change.

Publicly expose only XPCClient and XPCServer which lack public initializers
XPCServer has three static functions which all return an XPCServer
- forXPCService() which is equivalent to the XPCServiceService.server in this PR
- forBlessedHelperTool() as it currently exists in XPCMachServer
- forMachService(withName: String, clientRequirements: [SecRequirement]) which is equivalent to the current XPCMachServer(machServiceName: String, clientRequirements: [SecRequirement]) initializer
XPCClient has two static functions which both return XPCClient:
- forXPCService(withName: String) which is equivalent to the initializer for the new XPCServiceClient in this PR
- forMachService(withName: String) which is equivalent to the existing initializer for XPCMachClient

In practice they could return subclasses like the ones that exist in this diff, but that's an implementation detail and we could change that any time in the future without resulting in an incompatible change. This flexibility is also a reason I like this approach.

Thoughts?

Sources/SecureXPC/XPCBaseClient.swift

Sources/SecureXPC/XPCBaseServer.swift

Sources/SecureXPC/XPCMachClient.swift

Sources/SecureXPC/XPCMachServer.swift

Sources/SecureXPC/XPCServiceServer.swift

jakaplan · 2021-11-09T11:02:58Z

Sources/SecureXPC/XPCServiceServer.swift

+	private override init() {}
+
+	public override func start() -> Never {
+		xpc_main { connection in


What happens when this is called not from an XPC Service? Considering it's quite confusing which type of XPC to use, I expect people will screw this up. Would be good if we could throw an error when this is used wrong.

Good question. I couldn't find any way to check if a program is executing as an XPC service. Do you know of any way to do that?

Searching around a bit I couldn't find a publicly documented way to determine this; however, I think what we want is actually the opposite and that is partially possible enough to be helpful.

It should be possible to determine in the running code is not an XPC Service. Namely we can check if the CFBundlePackageType Info.plist entry is present and equal to "XPC!". If that's not the case, it's not an XPC Service. (We could go one step further and find the bundle location and see if it's where it should be within the containing app bundle, but not sure that's worth the extra complexity.) When it's not an XPC Service, throwing an error with a useful debug message would be quite helpful.

Great find!

What happens if someone removes this key/value pair from their service's bundle? I'm hoping that the XPC service won't work at all, to prompt them to fix it.

Yeah I have to imagine the XPC Service wouldn't work, but I haven't personally tried it. The documentation says this key/value pair is required:

XPC requires you to specify a number of special key-value pairs in the Info.plist file within the service helper’s bundle. These keys are listed below.

I'm opting to use CFBundleGetPackageInfo for this, because it's public API (rather than relying on the info.plist directly. I doubt the format will change, but why bother risking it).

Here's what that might look like:

extension Bundle { func getPackageInfo() -> (packageType: String?, pacakgeCreator: String?) { var packageTypeI = UInt32() var packageCreatorI = UInt32() CFBundleGetPackageInfo(CFBundleGetMainBundle(), &packageTypeI, &packageCreatorI) packageTypeI = packageTypeI.bigEndian packageCreatorI = packageCreatorI.bigEndian func uint32ToString(_ input: UInt32) -> String? { if input == 0 { return nil } var input = input return String(data: Data(bytes: &input, count: 4), encoding: .utf8) } return (uint32ToString(packageTypeI), uint32ToString(packageCreatorI)) } func isXPCService() -> Bool { self.getPackageInfo().packageType == "XPC!" } }

In case you were curious, the actual implementation of it is totally bizarre.

Using a direct API to access the information is sensible; also reduce the amount of "magic" constants used. This looks great!

I'm not that surprised the implementation for this is so gnarly, that's what a couple decades worth of backwards compatibility and workarounds often end up looking like 😬

If this really only makes sense to be used by the XPCServiceServer, seems preferable to not have it be an extension of Bundle. This is a reflection of my own architectural bias which is to always default to limited access/scope and only increase it once the need arises; but I don't insist that's the case so long as this remains framework internal.

When this actually make it into the code, would be good to have a comment referencing where the "XPC!" string comes from; namely: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingXPCServices.html#//apple_ref/doc/uid/10000172i-SW6-SW6

Nit: pacakgeCreator -> packageCreator

seems preferable to not have it be an extension of Bundle

Ah, I thought it seemed quite fitting. It's what I would do in my own project, but IDK if the rules change once you're writing a library that others integrate, because:

so long as this remains framework internal.

I've been putting off testing/researching this: Is there UB if two modules define the same internal extensions on the same type? I'll look into it.

When this actually make it into the code, would be good to have a comment referencing where the "XPC!" string comes from; namely: https://developer.apple.com/library/archive/documentation/MacOSX/Conceptual/BPSystemStartup/Chapters/CreatingXPCServices.html#//apple_ref/doc/uid/10000172i-SW6-SW6

Great call.

amomchilov · 2021-11-09T13:57:47Z

Exciting to see this gain more functionality!

I think this API design is absolutely workable, but because the distinction between an XPC Service and a "service" which vends a Mach service is frankly confusing I think the following might result in more developers doing the right thing on their first try. It'd also give us more flexibility to change the API in the future without it being a breaking change.
* Publicly expose only `XPCClient` and `XPCServer` which lack public initializers

* `XPCServer` has three static functions which all return an `XPCServer`
  
  * `forXPCService()` which is equivalent to the `XPCServiceService.server` in this PR
  * `forBlessedHelperTool()` as it currently exists in `XPCMachServer`
  * `forMachService(withName: String, clientRequirements: [SecRequirement])` which is equivalent to the current `XPCMachServer(machServiceName: String, clientRequirements: [SecRequirement])` initializer

* `XPCClient` has two static functions which both return `XPCClient`:
  
  * `forXPCService(withName: String)` which is equivalent to the initializer for the new `XPCServiceClient` in this PR
  * `forMachService(withName: String)` which is equivalent to the existing initializer for `XPCMachClient`
In practice they could return subclasses like the ones that exist in this diff, but that's an implementation detail and we could change that any time in the future without resulting in an incompatible change. This flexibility is also a reason I like this approach.

Thoughts?

I love it! I'll play around with it in the coming days.

Only nit: I think with is mildly discouraged in parameter labels. I think forXPCService(named:) reads nicer.

jakaplan · 2021-11-10T00:04:47Z

I love it! I'll play around with it in the coming days.

Awesome. I haven't thought it all through, but I'm also hopeful this architecture will extend nicely for passing connections between processes.

Only nit: I think with is mildly discouraged in parameter labels. I think ``forXPCService(named:)` reads nicer.

Good to know. named does read well.

amomchilov · 2021-11-10T00:16:23Z

will extend nicely for passing connections between processes.

Spoilers! 🤫

(I'm working on this next)

I think the class hierarchy actually might need to be re-jigged to "secure" vs "insecure" (client/server), instead of "mach service" vs "xpc service".

This is because from what I can tell, there's no way to know what kind of service is modelled by a xpc_endpoint_t. Even once you hydrate it back into an xpc_connection_t, I still don't know of a way to distinguish which kind of service it's connected to.

jakaplan · 2021-11-10T00:45:06Z

I think the class hierarchy actually might need to be re-jigged to "secure" vs "insecure" (client/server), instead of "mach service" vs "xpc service".

This is because from what I can tell, there's no way to know what kind of service is modelled by a xpc_endpoint_t. Even once you hydrate it back into an xpc_connection_t, I still don't know of a way to distinguish which kind of service it's connected to.

This is only relevant for the client right? Since the client doesn't play a role in the security, does this distinction need to exist? I could envision under the hood there's three subclasses of XPCClient:

Mach: creates a new mach connection
XPC Service: creates a new "regular" connection
Transferred (not sure the best name for this): creates connection by hydrating xpc_endpoint_t

And I'm imagining there wouldn't be a way for a user to directly. create a "transferred" client but it's instead somehow returned from a call to the server (hand wave on specifically how that happens).

amomchilov · 2021-11-10T01:10:03Z

Since the client doesn't play a role in the security, does this distinction need to exist?

I'm not sure. In principle, XPC allows an "inverse" connection to be opened, where the service can initiate message sends to the app (rather than just replying). Is that something we'd ever want? (I have no need for it in my app, that I can envision)

There needs to be 3 client classes

Yeah, that makes perfect sense. They each override createConnection to use the appropriate xpc_connection_create API.

And I'm imagining there wouldn't be a way for a user to directly. create a "transferred" client but it's instead somehow returned from a call to the server (hand wave on specifically how that happens).

Meh, if they have an xpc_endpoint_t, I don't see why we shouldn't allow them be able to create our client from it.

amomchilov · 2021-11-10T02:06:24Z

@jakaplan I'm pretty with how this is shaping up. I think it's ready for review and merge.

I updated the documentation to fix the broken references, but I didn't document this XPC service capability much. Could I leave that for you to put in? I don't know how you would like to organize that information. (Should it all reside on XPCClient and XPCService?)

jakaplan · 2021-11-10T08:57:11Z

In principle, XPC allows an "inverse" connection to be opened, where the service can initiate message sends to the app (rather than just replying). Is that something we'd ever want? (I have no need for it in my app, that I can envision)

Not sure if it should be part of the v1 as it certainly adds quite a bit of complexity. It's on my radar as I'm going to need a Launch Agent to regularly update an app with information. However, to start with I might just approach this with polling and that approach very well may turn out to be good enough for my purposes. What I'm working on has no high performance/low latency needs at all.

Meh, if they have an xpc_endpoint_t, I don't see why we shouldn't allow them be able to create our client from it.

I feel rather strongly this not be the case. I don't want SecureXPC to feel like a wrapper of the C API. I want it to feel like the way to use XPC with Swift (well I originally envisioned it as the way to use Swift with XPC Mach services, while this PR has made a great expansion on that). As a point of reference, the Objective-C XPC API does not offer any way to access the C API even though I very much expect it uses it under the hood (although I could be wrong).

jakaplan

This is looking really close! The only major thing remaining is getting consistent naming for the XPCServer static factory functions.

Sources/SecureXPC/Client/XPCMachClient.swift

jakaplan · 2021-11-10T09:17:07Z

Sources/SecureXPC/Server/XPCServer.swift

+	///                                   SecCSFlags(),
+	///                                   &requirement) == errSecSuccess,
+	///   let requirement = requirement {
+	///    let server = XPCMachServer(machServiceName: "com.example.service",


This is now out of date. I can address this when updating documentation after this has been merged.

Sources/SecureXPC/Server/XPCServer.swift

Sources/SecureXPC/Server/XPCServiceServer.swift

jakaplan · 2021-11-10T09:24:20Z

Sources/SecureXPC/Server/XPCMachServer.swift

+		}
+	}
+
+	internal static func _forBlessedHelperTool() throws -> XPCMachServer {


I understand why the the leading _ is here, but it seems rather un-Swift like? Maybe call it createForBlessedHelperTool()? I'm not horribly opposed to the underscore, it just looks odd to me.

I needed some arbitrary distinction to make it not clash with the inherited forBlessedHelperTool().

Alternatively, I could make the parent as class (instead of static, which is like final class), but I didn't want that to be exposed to public API

jakaplan · 2021-11-10T09:38:00Z

I updated the documentation to fix the broken references, but I didn't document this XPC service capability much. Could I leave that for you to put in? I don't know how you would like to organize that information. (Should it all reside on XPCClient and XPCService?)

Yup, happy to do so.

I updated the broken references, but I didn't do a full on write-up the XPC service support introduced in this PR

amomchilov · 2021-11-10T14:49:16Z

Hey Jake, I've incorporated your feedback.

Here's the outstanding stuff before merge. Feel free to edit this PR to your personal taste:

Decide between (comment):
- forThisBlessedHelperTool() and forThisXPCService(); OR
- forBlessedHelperTool() and forXPCService()
Solve the naming problem around _forBlessedHelperTool()

Post merge:

Update the docs

Sources/SecureXPC/Server/XPCServer.swift

jakaplan · 2021-11-11T09:51:51Z

Hey Jake, I've incorporated your feedback.

FYI my name is Josh. The "a" is for my middle name because "jkaplan" was already taken :/

Here's the outstanding stuff before merge. Feel free to edit this PR to your personal taste:

Went with what you had, didn't modify the PR.

Post merge:

Update the docs

Will start working on this. May hold off on updating the README.md until a new release is tagged.

Lightly adapted from #10 (comment)

amomchilov commented Nov 8, 2021

View reviewed changes

jakaplan reviewed Nov 9, 2021

View reviewed changes

amomchilov force-pushed the regular-xpc-service-support branch from 02b43c8 to 5ec91ff Compare November 10, 2021 01:28

amomchilov requested a review from jakaplan November 10, 2021 01:28

amomchilov mentioned this pull request Nov 10, 2021

Documentation issues #11

Closed

3 tasks

amomchilov force-pushed the regular-xpc-service-support branch from 5ec91ff to ffacf35 Compare November 10, 2021 02:04

amomchilov marked this pull request as ready for review November 10, 2021 02:06

jakaplan requested changes Nov 10, 2021

View reviewed changes

amomchilov added 2 commits November 10, 2021 09:30

Rename to XPCMachServer to XPCServer

dd77a09

Lower XPCMachServer-specific API to a subclass

712b2a8

amomchilov force-pushed the regular-xpc-service-support branch from ffacf35 to be51397 Compare November 10, 2021 14:31

amomchilov added 5 commits November 10, 2021 09:43

Hide XPCMachServer and introduce public factories

ed63630

Rename to XPCMachClient to XPCClient

fee444e

Lower XPCMachClient-specific API to a subclass

53cf067

Add client/server for regular XPC Services

6ff5e61

Improve documentation

5015753

I updated the broken references, but I didn't do a full on write-up the XPC service support introduced in this PR

amomchilov force-pushed the regular-xpc-service-support branch from be51397 to 5015753 Compare November 10, 2021 14:46

amomchilov requested a review from jakaplan November 10, 2021 14:46

jakaplan approved these changes Nov 11, 2021

View reviewed changes

Sources/SecureXPC/Server/XPCServer.swift Show resolved Hide resolved

jakaplan merged commit f0b152b into trilemma-dev:main Nov 11, 2021

jakaplan pushed a commit that referenced this pull request Nov 12, 2021

Throws error when caller is not an XPC Service

36ed189

Lightly adapted from #10 (comment)

amomchilov deleted the regular-xpc-service-support branch November 12, 2021 14:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for Regular XPC services #10

Support for Regular XPC services #10

amomchilov commented Nov 8, 2021 •

edited

Loading

jakaplan left a comment •

edited

Loading

jakaplan Nov 9, 2021

amomchilov Nov 9, 2021

jakaplan Nov 10, 2021

amomchilov Nov 10, 2021 •

edited

Loading

jakaplan Nov 10, 2021 •

edited

Loading

amomchilov Nov 10, 2021

jakaplan Nov 10, 2021

amomchilov Nov 10, 2021 •

edited

Loading

amomchilov commented Nov 9, 2021 •

edited

Loading

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021

amomchilov commented Nov 10, 2021

jakaplan commented Nov 10, 2021

jakaplan left a comment •

edited

Loading

jakaplan Nov 10, 2021

jakaplan Nov 10, 2021

amomchilov Nov 10, 2021

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021 •

edited by jakaplan

Loading

jakaplan commented Nov 11, 2021 •

edited

Loading

Support for Regular XPC services #10

Support for Regular XPC services #10

Conversation

amomchilov commented Nov 8, 2021 • edited Loading

jakaplan left a comment • edited Loading

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

amomchilov Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

jakaplan Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Nov 10, 2021 • edited Loading

Choose a reason for hiding this comment

amomchilov commented Nov 9, 2021 • edited Loading

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021

amomchilov commented Nov 10, 2021

jakaplan commented Nov 10, 2021

jakaplan left a comment • edited Loading

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

jakaplan commented Nov 10, 2021

amomchilov commented Nov 10, 2021 • edited by jakaplan Loading

jakaplan commented Nov 11, 2021 • edited Loading

amomchilov commented Nov 8, 2021 •

edited

Loading

jakaplan left a comment •

edited

Loading

amomchilov Nov 10, 2021 •

edited

Loading

jakaplan Nov 10, 2021 •

edited

Loading

amomchilov Nov 10, 2021 •

edited

Loading

amomchilov commented Nov 9, 2021 •

edited

Loading

jakaplan left a comment •

edited

Loading

amomchilov commented Nov 10, 2021 •

edited by jakaplan

Loading

jakaplan commented Nov 11, 2021 •

edited

Loading