Bug 233295 – More webpushd architecture work

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

More webpushd architecture work

[Brady Eidson]

Created attachment 444609 [details]
EWS v1

Besides missing ChangeLogs, I know this'll need more work. Need EWS to tell me how much more.

[Brady Eidson]

Created attachment 444623 [details]
EWS v2

[Brady Eidson]

Created attachment 444628 [details]
EWS v3

[Brady Eidson]

Created attachment 444631 [details]
EWS v4

[Brady Eidson]

Created attachment 444641 [details]
EWS v5

[Brady Eidson]

Going to mark for review even while I work on resolving the api-mac failure

[Alex Christensen]

Comment on attachment 444641 [details]
EWS v5

View in context: https://bugs.webkit.org/attachment.cgi?id=444641&action=review

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:45
> +        auto cfData = m_networkSession.networkProcess().sourceApplicationAuditData();

You can probably do this from sourceApplicationAuditToken to be slightly more efficient.

> Source/WebKit/Shared/Cocoa/CodeSigning.mm:78
> +    auto pair = codeSigningIdentifierAndPlatformBinaryStatus(token);

This doesn't need to call SecTaskGetCodeSignStatus.

> Source/WebKit/webpushd/PushClientConnection.mm:42
> +void ClientConnection::setAuditTokenData(const Vector<uint8_t>& tokenData)

This is a little strange.  Isn't xpc_connection_get_audit_token enough?

> Source/WebKit/webpushd/PushClientConnection.mm:72
> +        m_hostHasPushEntitlement = WTF::hasEntitlement(*m_hostAppAuditToken, "com.apple.private.webkit.webpush");

This is typically done at the time a connection is being received, then terminating the connection if it doesn't have an entitlement.  That way you avoid bugs where you forgot to check the entitlement.  If you have a connection, then it is entitled.

> Source/WebKit/webpushd/WebPushDaemon.h:56
> +    void echoTwice(ClientConnection*, const String&, CompletionHandler<void(const String&)>&& replySender);

We can probably remove this at some point.

> Source/WebKit/webpushd/WebPushDaemon.h:77
> +    HashMap<xpc_connection_t, std::unique_ptr<ClientConnection>> m_connectionMap;

It's a little strange that the connection is both in the key and the value.

[Brady Eidson]

(In reply to Alex Christensen from comment #7)
> Comment on attachment 444641 [details]
> EWS v5
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=444641&action=review
> 
> > Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:45
> > +        auto cfData = m_networkSession.networkProcess().sourceApplicationAuditData();
> 
> You can probably do this from sourceApplicationAuditToken to be slightly
> more efficient.

Yes.

> 
> > Source/WebKit/Shared/Cocoa/CodeSigning.mm:78
> > +    auto pair = codeSigningIdentifierAndPlatformBinaryStatus(token);
> 
> This doesn't need to call SecTaskGetCodeSignStatus.

I was trying to reuse the existing abstractions, but will open it up a bit more.

> 
> > Source/WebKit/webpushd/PushClientConnection.mm:42
> > +void ClientConnection::setAuditTokenData(const Vector<uint8_t>& tokenData)
> 
> This is a little strange.  Isn't xpc_connection_get_audit_token enough?

xpc_connection_get_audit_token gets the audit token of the remote process.

For example, in webpushd, it gets the audit token of com.apple.WebKit.Networking

We need the remote-remote audit token. E.g. if com.apple.WebKit.Networking is running on behalf of Safari, we need Safari's audit token.

This would've been made much clearer if I named these methods more precisely.

> 
> > Source/WebKit/webpushd/PushClientConnection.mm:72
> > +        m_hostHasPushEntitlement = WTF::hasEntitlement(*m_hostAppAuditToken, "com.apple.private.webkit.webpush");
> 
> This is typically done at the time a connection is being received, then
> terminating the connection if it doesn't have an entitlement.  That way you
> avoid bugs where you forgot to check the entitlement.  If you have a
> connection, then it is entitled.

Once again, see above: This isn't the remote connection... this is the host app *of* the remote connection.

So it cannot be done at connection time.

> 
> > Source/WebKit/webpushd/WebPushDaemon.h:56
> > +    void echoTwice(ClientConnection*, const String&, CompletionHandler<void(const String&)>&& replySender);
> 
> We can probably remove this at some point.

Sure. Will do separately.

> 
> > Source/WebKit/webpushd/WebPushDaemon.h:77
> > +    HashMap<xpc_connection_t, std::unique_ptr<ClientConnection>> m_connectionMap;
> 
> It's a little strange that the connection is both in the key and the value.

I have no alternative suggestions though.

[Brady Eidson]

Created attachment 444760 [details]
Updated for review feedback, still building locally to explore Mac API test failure

[Alex Christensen]

In DaemonConnectionSet I made the map a map from the connection to a collection of metadata that doesn't contain another reference to the connection.  That's my alternative suggestion

[Brady Eidson]

(In reply to Alex Christensen from comment #10)
> In DaemonConnectionSet I made the map a map from the connection to a
> collection of metadata that doesn't contain another reference to the
> connection.  That's my alternative suggestion

I'm not clear on how that's better - It's still a map of xpc_connection_t to information about those connections.

For future reference: ClientConnection WILL have more data and behaviors than just what this patch introduces, very soon.

It will be supremely useful for things like "This xpc connection dropped - Find it's resulting WebPushD::ClientConnection object and have it clean up outstanding activities"

[Brady Eidson]

I figured out the Mac API test issue, but need to finish a full Mac build to resolve. Will do soon. This patch still reviewable.

[Alex Christensen]

Comment on attachment 444760 [details]
Updated for review feedback, still building locally to explore Mac API test failure

View in context: https://bugs.webkit.org/attachment.cgi?id=444760&action=review

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:39
>  NetworkNotificationManager::NetworkNotificationManager(NetworkSession& networkSession, const String& webPushMachServiceName)

nit: mach service names can be a CString because they are always ASCII.

> Source/WebKit/NetworkProcess/Notifications/NetworkNotificationManager.cpp:53
> +        sendMessage<WebPushD::MessageType::SetHostAppAuditToken>(auditTokenData);

Sending the first message will start the daemon if it hasn't been started, so we probably want to lazily send this to prevent unnecessary daemon starting.

> Source/WebKit/webpushd/PushClientConnection.h:44
> +    String hostAppCodeSigningIdentifier();

nit: can this return a const String& ?

[Brady Eidson]

Created attachment 444780 [details]
More for EWS while I explore Mac API thingy

[Brady Eidson]

Can't reproduce the mac-api test failure locally. My suspicion is internal vs non-internal SDK. (related to entitlements or audit tokens somehow)

Building on a public SDK now.

[Brady Eidson]

Created attachment 444787 [details]
PFL v1

The Mac-api-test failure was because the source application audit token wasn't making its way through.
This was because USE_SOURCE_APPLICATION_AUDIT_DATA was true for Mac only in the internal SDK... but it seems to work great in the public SDK as well.

Enabling that and giving it a test run.

[Brady Eidson]

(In reply to Brady Eidson from comment #16)
> Created attachment 444787 [details]
> PFL v1
> 
> The Mac-api-test failure was because the source application audit token
> wasn't making its way through.
> This was because USE_SOURCE_APPLICATION_AUDIT_DATA was true for Mac only in
> the internal SDK... but it seems to work great in the public SDK as well.
> 
> Enabling that and giving it a test run.

And it's obvious that we're not ready for all the related fallout from that.

Doing something more limited.

[Brady Eidson]

Created attachment 444838 [details]
PFL v2

[Brady Eidson]

Created attachment 444844 [details]
PFL v3

[EWS]

Committed r286075 (244462@main): <https://commits.webkit.org/244462@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 444844 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85618026>