Add Endpoint APIs #23

amomchilov · 2021-11-28T21:13:39Z

This PR builds on top of the code introduced in #21. Do not ship until #21 is merged.

Add a new XPCEndpoint wrapper around xpc_endpoint_t.

Sources/SecureXPC/Client/XPCClient.swift

jakaplan · 2021-11-30T09:35:16Z

Sources/SecureXPC/Client/XPCClient.swift

+    // MARK: Implementation
+
+    internal let serviceName: String?
+    private var connection: xpc_connection_t? = nil

    /// Creates a client which will attempt to send messages to the specified mach service.
    ///
    /// - Parameters:
    ///   - serviceName: The name of the XPC service; no validation is performed on this.


This documentation is getting confusing as it references both mach services and XPC service. Additionally both parameters can be nil but if both are is that actually valid? Although this is internal, some more documentation would be good.

Ah yes, this doc comment is a mess.

Additionally both parameters can be nil but if both are is that actually valid?

Yes.

The serviceName can be nil for anonymous services. For regular (named) XPC services, and Mach services, it needs to be non-nil. Perhaps serviceName should be an abstract property, and the subclasses can store their own name variable (which can be non-nil for mach services.

When creating a client from an XPCEndpoint, the connection is already created, and so the client just uses that. In other cases, connection will be nil, and the client will create a new connection for itself from the service name.

Actually looking at this in more details, looks like there's no need for XPCClient to have a serviceName instance variable. This can purely be an internal implementation detail of the XPCMachClient and XPCServiceClient subclasses and a future XPCAnonymousClient (or whatever it's called) subclasses wouldn't have one.

Similarly there doesn't appear to be any benefit to passing the connection into the initializer as it'll get created (or return an existing one for an XPCEndpoint) as part of the createConnection() function. Is that right?

Actually looking at this in more details, looks like there's no need for XPCClient to have a serviceName instance variable. This can purely be an internal implementation detail of the XPCMachClient and XPCServiceClient subclasses and a future XPCAnonymousClient (or whatever it's called) subclasses wouldn't have one.

Errrr I disagree. NSXPCConnection and the C APIs both take this approach, where connections are just these magical opaque things with very little exposed API. They happen to print their service name (if known) in their description, but they're still annoying to deal with, because e.g. their name isn't visible from the object view in the debugger.

I think having the service name be exposed is easy enough that we should have it. It's just a nice convenience for development. (not that anybody should ever need to like, switch over it, or anything like that)

Similarly there doesn't appear to be any benefit to passing the connection into the initializer as it'll get created (or return an existing one for an XPCEndpoint) as part of the createConnection() function. Is that right?

How will "or return an existing one for an XPCEndpoint" work if the connection isn't passed in?

I still have to update the docs here (and elsewhere, probably), but could yo uplease look over my last comment?

NSXPCConnection and the C APIs both take this approach, where connections are just these magical opaque things with very little exposed API. They happen to print their service name (if known) in their description, but they're still annoying to deal with, because e.g. their name isn't visible from the object view in the debugger.

Hmm, I think we just have a difference of opinion when it comes to API design here and I think Apple took the right approach. As far as I see it, this is why CustomDebugStringConvertible exists. Unless I'm mistaken the value of debugDescription can be visible in the debugger so long as it's not a computed property?

How will "or return an existing one for an XPCEndpoint" work if the connection isn't passed in?

It'd get passed into the initializer of a concrete subclass.

Sources/SecureXPC/Client/XPCClient.swift

jakaplan · 2021-11-30T09:40:03Z

Sources/SecureXPC/Client/XPCMachClient.swift

-	}
+    /// Creates and returns a connection for the Mach service represented by this client.
+    internal override func createConnection() -> xpc_connection_t {
+        xpc_connection_create_mach_service(self.serviceName!, nil, 0)


Having the self.serviceName! strikes me as a bit convoluted / code smell. Seems like this should be a private variable of this class which it takes into its initializer. It'd result in a bit more code, but would be cleaner.

Are you thinking perhaps having out own machServiceName: String and then having an override var serviceName: String? { machServiceName }?

I don't believe there's any need for the super class to have access to serviceName. It appears to never be used. So yes to the first part, but no to the second.

It would be used in debug descriptions of the object, and xpc endpoints (in the future when I add the ability to export a client as an endpoint)

Is there a reason not to take the same approach as XPCServer where each subclass actually implements this and the superclass just has:

public var endpoint: XPCServerEndpoint { fatalError("Abstract Method") }

In regards to debug descriptions, I'd think there'd be advantages to having the subclass do that because each subclass has the most comprehensive set of contextual information.

In regards to debug descriptions, I'd think there'd be advantages to having the subclass do that because each subclass has the most comprehensive set of contextual information.

To clarify, I didn't necessarily mean CustomDebugStringConvertible (although we should implement that, too), but the breakdown of the value when displayed in the debugger. Stored properties are nice because they're shown there directly, without needing po obj or the like.

Sources/SecureXPC/Server/XPCServiceServer.swift

jakaplan · 2021-11-30T09:59:27Z

Sources/SecureXPC/XPCConnectionType.swift

+
+import Foundation
+
+public enum XPCConnectionType {


Since this is such a small enum and only ever used from XPServerEndpoint would it be clearer if it was nested inside?

I think I originally had more methods on it. I'll fold it in.

Actually, it's used in the client and endpoint files, too. I think it should stay standalone. However, the cases could use better names (they shouldn't be client-specific)

jakaplan · 2021-11-30T09:59:41Z

Sources/SecureXPC/XPCServerEndpoint.swift

+
+import Foundation
+
+// TODO: make this codable so it can be sent over XPC.


Indeed, that's important :)

jakaplan · 2021-11-30T10:00:18Z

Sources/SecureXPC/XPCServerEndpoint.swift

+    // Technically, an `xpc_endpoint_t` is sufficient to create a new connection, on its own. However, it's useful to
+    // be able to communicate the kind of connection, and its name, so we also store those, separately.
+    internal let kind: XPCConnectionType
+    internal let serviceName: String?


In what circumstances is it valid for the name to be nil?

iff it's an anonymous service

I think we should structure this as part of the XPCConnectionType enum with an associated value such that when a connection must have a connection name, it has one, and when it can't then it's not possible to specify one. That'll let the compiler ensure we've implemented the invariant correctly. Thoughts?

I’ll play around with it and let you know how it goes!

Oh I remember why I didn’t do this:

it complicates the structure of the way this value gets encoded (IIRC Swift added the ability to auth-synthesize the codable conformance for an enumeration with associated values, but I haven’t played with that yet)

It would make it impossible to build an XPCEndpoint out of a plain xpc_endpoint_t.

Whether the latter should be allowed at all comes down to a matter of design philosophy. My opinion is that we should allow people to access endpoints, connections, audit tokens, etc, if they want to. I don’t think a framework called “SecureXPC” should be afraid of exposing the fact that is uses XPC under the hood (it’s not a secret ;) ). NSXPCConnection was designed as a more opaque wrapper where everything should “just work”, but as you’ve probably seen, that adds a bit too much smoke and mirrors sometimes.

I think having “escape hatches” can be useful, particularly when looking for help. E.g. when working on the anonymous connection code, I ran into a fair number of difficulties that I only managed to solve by “inlining” the abstractions of this framework. This allowed me to more easily compare my work against the existing body of literature that covers the C and ObjC APIs.

In regards to #1, if all of the associated values are Codable then conformance is automatically generated by the compiler. XPCError make use of this; several of its cases have String associated values.

In regards to #2, I consider it an intentional design choice of SecureXPC that it does not expose the C API it uses. I don't have any issue with NSXPCConnection in regards to its abstractions except that it's an Objective-C API which maps very awkwardly to Swift. My view is escape hatches cause many issues and should only be done as a last resort. Fundamentally I don't view SecureXPC as a wrapper of the XPC C API, I view it as its own XPC API which internally makes use of the XPC C API. (This is quite different than the Blessed framework I open sourced which is intentionally a Swift-friendly wrapper of the Authorization Services and Service Management frameworks.) Happy to discuss this in more depth in the Discussions section.

amomchilov

I think I've addressed most (all?) of your feedback. Wanna have another look?

Sources/SecureXPC/Client/XPCClient.swift

amomchilov · 2021-12-04T17:12:17Z

Sources/SecureXPC/Client/XPCClient.swift

+    // MARK: Implementation
+
+    internal let serviceName: String?
+    private var connection: xpc_connection_t? = nil

    /// Creates a client which will attempt to send messages to the specified mach service.
    ///
    /// - Parameters:
    ///   - serviceName: The name of the XPC service; no validation is performed on this.


I still have to update the docs here (and elsewhere, probably), but could yo uplease look over my last comment?

amomchilov · 2021-12-04T17:14:28Z

Sources/SecureXPC/Client/XPCMachClient.swift

-	}
+    /// Creates and returns a connection for the Mach service represented by this client.
+    internal override func createConnection() -> xpc_connection_t {
+        xpc_connection_create_mach_service(self.serviceName!, nil, 0)


In regards to debug descriptions, I'd think there'd be advantages to having the subclass do that because each subclass has the most comprehensive set of contextual information.

To clarify, I didn't necessarily mean CustomDebugStringConvertible (although we should implement that, too), but the breakdown of the value when displayed in the debugger. Stored properties are nice because they're shown there directly, without needing po obj or the like.

amomchilov · 2021-12-04T17:18:29Z

Sources/SecureXPC/Server/XPCMachServer.swift

@@ -193,6 +196,19 @@ internal class XPCMachServer: XPCServer {
 		return accept
 	}

+    public override var endpoint: XPCServerEndpoint {
+        guard let connection = self.serviceListenerConection else {
+            fatalError("You can only create an `endpoint` for an XPCMachServer after starting it with `start()`.")


I've made the message more clear, LMK what you think.

amomchilov · 2021-12-04T17:34:49Z

Sources/SecureXPC/Client/XPCClient.swift

+        xpc_connection_set_event_handler(connection, { (event: xpc_object_t) in
+            fatalError("Is should be impossible for this connection to recieve an event.")
+        })
+        xpc_connection_resume(connection)


FIXME: we shouldn't resume the client here. We should probably defer it until getConnection().

Sources/SecureXPC/Client/XPCClient.swift

jakaplan · 2021-12-05T03:01:51Z

Sources/SecureXPC/Server/XPCMachServer.swift

+        guard let connection = self.serviceListenerConnection else {
+            fatalError("""
+                An XPCServer for a Mach service can only have its `connection` exported to an `endpoint` after
+                a `connection` has been created by calling `start()`.


Remove the backticks since they'll just literally print as backticks?

The concept of a connection isn't part of the public API and I don't believe the fact this is for a Mach service is relevant (is it?) so probably better to say something like:

An XPCServer's endpoint can only be retrieved after start() has been called on it.

(is it?)

I don't think that's something that could be reasonably withheld. This framework doesn't exist in isolation: I'd bet almost everyone looking to use it will be cross-referencing against the body of existing literature on XPC, where the existence of the connection is a core concept.

Try to hide the notion of a connection is also a leaky abstraction. Its existence leaks through the errors (e.g. .connectionInvalidated), and it's explicitly mentioned in docc docs, in several places. In addition, the message latency is a side channel that hints towards the existing of a connection, and its currency status.

I should've been more explicit in my feedback. As written you have connection which to me made it sound like that was a property that was actually exposed to the end user. Users never directly create connections or have access to one so depending on how it's worded it could be confusing and just be easier to say it needs to be started, but I have no issue with a connection being mentioned.

In terms of it being for a Mach service, since all of the servers will need to be started it's probably just not very helpful to add the detail that a server for a Mach service needs to be started. This isn't about withholding anything, just that it doesn't seem relevant and therefore may cause the reader to read something into it that isn't applicable.

Oh I see, that makes sense.

jakaplan · 2021-12-05T03:09:07Z

Sources/SecureXPC/Server/XPCServer.swift

@@ -333,6 +333,14 @@ public class XPCServer {
 	internal func acceptMessage(connection: xpc_connection_t, message: xpc_object_t) -> Bool {
 		fatalError("Abstract Method")
 	}
+
+    public var serviceName: String? {


Looks like XPCMachServer doesn't override this.

Separately is this useful for an API user to know? Excluding anonymous servers there's likely to only ever be one server running, and anonymous servers don't have a service name.

I use it in my logger in my project.

Is logging that any more useful than logging the bundle id?

Nah, but that's kind of an implementation detail. The "service name" is what I'm looking to log, and the bundle ID just happens to be the source for it at the moment.

Sources/SecureXPC/Server/XPCServiceServer.swift

Sources/SecureXPC/XPCServiceDescriptor.swift

It will be `nil` for anonymous connections.

amomchilov · 2021-12-13T23:51:49Z

@jakaplan I think I've addressed almost all the comments. Do you feel comfortable merging as-is and iterating on the remaining stuff? Stale PRs become increasingly hard to work on with time

jakaplan · 2021-12-14T12:07:37Z

Sources/SecureXPC/Server/XPCMachServer.swift

+            fatalError("An XPCServer's endpoint can only be retrieved after start() has been called on it.")
+        }
+
+        let endpoint = xpc_endpoint_create(connection)


I'm assuming this actually works? I don't have any good way to test it, but just realized this is rather unintuitive because the connection being serialized into an endpoint here is the one that accepts new connections, not one which can be used to receive events.

IIRC, it's "smart" and doesn't encode the service listener connection itself. When a connection it created from this, it's able to talk to the mach service.

However, this isn't something I can test right now, either

Ah, what's stopping you from trying this out? I assume it doesn't actually need to be sent across processes in order to be tested (although normally not doing so would be pointless).

How do you think we should proceed? I'm moving to a new house soon, so I won't be able to build out a test bed for this any time soon. I see two options:

Yolo ship it, follow up on it later (with an issue to track it)

Fatal-error this code path to say that it's untested, and treat it as effectively not-yet-implemented.

Let's go with option 2 for now. I'd volunteer to try testing it out, but between end of year busyness at work and heading off on holiday soon I just won't have time before early January. Can revisit it then.

Opened #33 , and added ab4e612

Sources/SecureXPC/Server/XPCServiceServer.swift

jakaplan · 2021-12-14T12:15:19Z

@jakaplan I think I've addressed almost all the comments. Do you feel comfortable merging as-is and iterating on the remaining stuff? Stale PRs become increasingly hard to work on with time

Yes, pending an answer to whether the endpoint for XPCMachServer actually works. The other two comments I left were quite minor.

This was referenced Nov 28, 2021

Add support for anonymous connections #24

Merged

Add real XPC integration tests #22

Closed

jakaplan requested changes Nov 30, 2021

View reviewed changes

jakaplan mentioned this pull request Dec 1, 2021

Persistent client connections #21

Merged

amomchilov force-pushed the add-endpoint-APIs branch 2 times, most recently from 2480d40 to 75cd7be Compare December 4, 2021 17:19

amomchilov requested a review from jakaplan December 4, 2021 17:20

amomchilov commented Dec 4, 2021

View reviewed changes

jakaplan requested changes Dec 5, 2021

View reviewed changes

amomchilov added 2 commits December 12, 2021 14:46

Add XPCServiceDescriptor enum

0f011c3

Introduce XPCServerEndpoint

69aad32

amomchilov force-pushed the add-endpoint-APIs branch from 75cd7be to 20f75a9 Compare December 12, 2021 19:46

Make XPCClient.serviceName optional

d85530c

It will be `nil` for anonymous connections.

amomchilov force-pushed the add-endpoint-APIs branch 3 times, most recently from 7170cf8 to bc4f6eb Compare December 13, 2021 23:50

amomchilov requested a review from jakaplan December 13, 2021 23:50

jakaplan reviewed Dec 14, 2021

View reviewed changes

Sources/SecureXPC/Server/XPCServiceServer.swift Outdated Show resolved Hide resolved

jakaplan reviewed Dec 14, 2021

View reviewed changes

Sources/SecureXPC/Server/XPCServiceServer.swift Outdated Show resolved Hide resolved

amomchilov added 2 commits December 14, 2021 18:33

Add ability to export the endpoint of a server

cc67884

Add ability to create clients from an endpoint

7884d7b

amomchilov force-pushed the add-endpoint-APIs branch from bc4f6eb to 7884d7b Compare December 14, 2021 23:33

amomchilov mentioned this pull request Dec 14, 2021

Verify that exporting a connection for a Mach service works correctly #33

Closed

Temporarily disable endpoints for Mach services

ab4e612

amomchilov requested a review from jakaplan December 14, 2021 23:58

jakaplan approved these changes Dec 15, 2021

View reviewed changes

jakaplan merged commit c8a669c into trilemma-dev:main Dec 15, 2021


		import Foundation

		// TODO: make this codable so it can be sent over XPC.

Add Endpoint APIs #23

Add Endpoint APIs #23

Conversation

amomchilov commented Nov 28, 2021 • 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

Choose a reason for hiding this comment

jakaplan Dec 5, 2021 • 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

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

amomchilov Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

jakaplan Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

amomchilov 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

amomchilov commented Dec 13, 2021

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

amomchilov Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

jakaplan commented Dec 14, 2021

amomchilov commented Nov 28, 2021 •

edited

Loading

jakaplan Dec 5, 2021 •

edited

Loading

amomchilov Dec 1, 2021 •

edited

Loading

jakaplan Dec 2, 2021 •

edited

Loading

amomchilov Dec 14, 2021 •

edited

Loading